Conversation
Reviewer's GuideThis PR establishes RFC 0002’s write-tier foundation: can_obj now chooses ORG, explicitly SHARED, or fail-closed platform-shared authorization independently of read scope, with ClientProfile opted into SHARED, E007 enforcing declaration rules, and tests and ADR documentation covering the staged contract. The object-grant/OBJECT arm is intentionally not enabled yet. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="apps/betterangels-backend/common/permissions/checks.py" line_range="267-280" />
<code_context>
+ for model in apps.get_models():
+ if not issubclass(model, OrgScoped):
+ continue
+ tier = model.__dict__.get("write_tier")
+ if tier is None:
+ continue
+ if model.org_via is not None:
+ errors.append(
+ Error(
+ f"{model.__name__}.write_tier = {tier!r} on an org-anchored model.",
+ hint="Org-anchored models derive the ORG write tier from org_via; "
+ "only a platform-shared model (org_via = None) declares a tier.",
+ obj=model,
+ id="permissions.E007",
+ )
+ )
+ elif tier == WRITE_OBJECT:
+ errors.append(
+ Error(
</code_context>
<issue_to_address>
**issue (bug_risk):** E007 accepts arbitrary non-`None` tier strings on platform-shared models. `can_obj` recognizes only `WRITE_SHARED`, so a typo or unsupported value passes `manage.py check` and silently fails closed for every finite org-scoped holder.
**Triggers:** When a platform-shared model declares a tier value other than `WRITE_SHARED` or the reserved `WRITE_OBJECT`.
**Suggested fix:** Reject tier values outside the defined constants with `permissions.E007` before applying the platform-shared and reserved-tier checks.
</issue_to_address>| tier = model.__dict__.get("write_tier") | ||
| if tier is None: | ||
| continue | ||
| if model.org_via is not None: | ||
| errors.append( | ||
| Error( | ||
| f"{model.__name__}.write_tier = {tier!r} on an org-anchored model.", | ||
| hint="Org-anchored models derive the ORG write tier from org_via; " | ||
| "only a platform-shared model (org_via = None) declares a tier.", | ||
| obj=model, | ||
| id="permissions.E007", | ||
| ) | ||
| ) | ||
| elif tier == WRITE_OBJECT: |
There was a problem hiding this comment.
issue (bug_risk): E007 accepts arbitrary non-None tier strings on platform-shared models. can_obj recognizes only WRITE_SHARED, so a typo or unsupported value passes manage.py check and silently fails closed for every finite org-scoped holder.
Triggers: When a platform-shared model declares a tier value other than WRITE_SHARED or the reserved WRITE_OBJECT.
Suggested fix: Reject tier values outside the defined constants with permissions.E007 before applying the platform-shared and reserved-tier checks.
9f02532 to
c0cb2ac
Compare
afcb988 to
e2cc037
Compare
c0cb2ac to
9aadc2c
Compare
e2cc037 to
7325c5c
Compare
9aadc2c to
b5e7b01
Compare
7325c5c to
8ac9f0f
Compare
…002) Implements the RFC 0002 §Precondition: can_obj consults a model's write tier independently of org_via (its read scope). Behavior-preserving today — can_obj has no production caller yet — and the shared prerequisite the object arm and the clients/notes/tasks cutovers inherit. - common/models.py: OrgScoped.write_tier declaration + WRITE_SHARED / WRITE_OBJECT constants. None derives the safe default: ORG for org-anchored models, fail-closed for platform-shared. - common/permissions/selectors.py: can_obj is tier-aware — ORG (unchanged row filter) for org-anchored; declared WRITE_SHARED -> can_anywhere; undeclared platform-shared fails closed (finding C1 / RFC 0002) with only the global tier acting. Replaces the "do not route platform-shared writes through this" WARNING with the implemented contract. - clients/models.py: ClientProfile declares WRITE_SHARED (RFC 0002 decision #1 — client CHANGE/DELETE on main come from model perms on CASEWORKER, so the cutover preserves them as "holds the permission anywhere"). - common/permissions/checks.py: permissions.E007 — only platform-shared models may declare a tier (org-anchored derive ORG), and WRITE_OBJECT stays reserved until the object arm turns on with the clients cutover. - docs/adr/0001 §2.5: write-tier status note. Tests: can_obj tier unit tests (ORG unchanged, SHARED == can_anywhere, fail-closed default vs global tier) + E007 quiet/error tests.
b5e7b01 to
8a7f4ba
Compare
8ac9f0f to
3af672f
Compare
Summary
Implements the RFC 0002 §Precondition on the grant predicate core:
can_objnow consults a model's write tier independently oforg_via(its read scope). Read scope and write scope no longer fall out of one declaration. This is the shared prerequisite the object-grant arm and every outreach cutover (clients §5.1, notes §5, tasks/referrals via RFC 0003) inherit — and it is behavior-preserving today, becausecan_objhas no production caller yet (verified: only its def + tests).Stacks on the org-admin stack:
#2443 teams → #2444 reports → #2445 members → #2446 org-admin teardown→ this PR.What changes
OrgScoped.write_tier(common/models.py) +WRITE_SHARED/WRITE_OBJECTconstants.Nonederives the safe default: ORG for any org-anchored model (org_vianotNone), fail-closed for platform-shared.can_objis tier-aware (common/permissions/selectors.py):WRITE_SHARED(declared) →can_anywhere— any holder of the permission anywhere may act.scopesis ALL) acts; the read rule never feeds an undeclared write.ClientProfiledeclaresWRITE_SHARED(clients/models.py, RFC 0002 decision Setup Basic VSCode .devcontainer and include terraform and terragrunt #1) — its CHANGE/DELETE onmaincome from model-level perms on the CASEWORKER group, so the cutover preserves them as "holds the permission anywhere" rather than C1 fail-closed. It is the onlyorg_via = Nonemodel in the codebase.permissions.E007— only a platform-shared model may declare a tier (org-anchored models derive ORG from their anchor), andWRITE_OBJECTstays reserved until the object arm turns on with the clients cutover (declaring it today would route writes to a predicate nothing satisfies).Behavior notes / review focus
can_obj;visible()(reads) is untouched and stillorg_via-driven;ClientProfile's declared SHARED tier matches what the old fail-opencan_objwould have returned anyway. This PR is the predicate groundwork, not a cutover.WRITE_OBJECTuntil then.Test plan
manage.py checkclean (E007 quiet withClientProfile).can_anywhere(holder anywhere yes, stranger no, superuser yes); fail-closed default (finite holder denied, global tier allowed); E007 quiet + both error paths.ruffclean on all touched files.Summary by Sourcery
Decouple object write authorization from model read scope while preserving current behavior and establishing safe defaults for future platform-shared write surfaces.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: