Skip to content

feat(perm): notes grant cutover — slice 2 (RFC 0003, DEV-2561) - #2462

Open
vecchp wants to merge 4 commits into
feat/perm/tasks-grant-cutoverfrom
feat/perm/notes-grant-cutover
Open

vecchp wants to merge 4 commits into
feat/perm/tasks-grant-cutoverfrom
feat/perm/notes-grant-cutover

Conversation

@vecchp

@vecchp vecchp commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Context

RFC 0003 slice 2 — the notes domain cutover to grant authority (ADR 0001; epic DEV-2551, ticket DEV-2561). Stacks on #2457 (tasks slice) and merges after it.

This PR carries both tranches of the slice:

  • T1 (cd955753, already pushed): role-back + SHARED reads — the CASEWORKER role gains the note bundle; note/notes reads cut over; pins.
  • T2 (this): the write cutover — org-scoped writes, payload org on create (tolerant staging), service-request gating, parity + canary suites, mobile wiring.

Design

  • Writes fetch through the write-scoped selector. New writable(qs, user, perm) — the write rule as a queryset filter (can_obj now delegates to it; one source of truth) — plus the mutation leaf get_writable_or_deny(qs, pk, user, perm): the fetch is the gate, one query instead of fetch-then-check, and missing/forbidden share one refusal.
  • Reads SHARED, writes org-scoped (RFC 0002 §Precondition — read tier ≠ write tier).
  • Creates take the payload org: CreateNoteInput.organizationId (optional) + resolve_org_or_deny + require_can(ADD). Tolerant window: pre-payload builds keep creating via their legacy CASEWORKER group; the strict flip lands once the sending build is live (same recipe as feat(perm): task domain grant cutover — slice 1 (RFC 0003, DEV-2561) #2457/feat(perm): tasks createTask requires the payload org — strict flip (hold for mobile adoption) #2459; see docs/api-compatibility.md from docs: API compatibility — staged rollouts for mobile-visible schema changes #2460).
  • ServiceRequest is not OrgScoped in this slice (its service hop cannot resolve until OrganizationService is org-scoped): SR create rides the owning note; SR delete is gated by the org of the note carrying it; detached SRs fail closed. The SR perms return to the scoped role with the service-catalog cutover.
  • Guardian write paths replaced on all 7 note mutations + 2 upload gates; template model-level perms moved together with the writes (landing model-level perms early over-permits through the legacy guardian filter — the T1 trap).
  • Attachments ride the note gate: the upload mutations no longer declare HasPerm(Attachment ADD); the scoped role carries no Attachment/SR perms (E005 — a scoped role may only carry perms of OrgScoped-declared models).

Enforcement

  • common/tests/test_org_mutation_gating.py: notes.schema registered; writable(/get_writable_or_deny( count as gate markers; a new scanner test rejects raw write-target fetches in Mutation bodies (exceptions carry reasons — the allowlist is the migration ledger).
  • docs/styleguides/python.md: authority-through-selectors rules, including platform-only (holds_globally) guidance.
  • New suites: notes/tests/test_grant_authorization.py (role-backed, grant-only, legacy-inert, cross-org SHARED reads + scoped writes, compat window, detached-SR pin) and notes/tests/test_authz_registry.py (every Query/Mutation field registered with its route).
  • E005 coherence: stale E005 test example fixed (Note → Attachment — Note is OrgScoped since the cutover).

Staging

  1. Merge feat(perm): task domain grant cutover — slice 1 (RFC 0003, DEV-2561) #2457 → this PR (tolerant; legacy create fallback intact).
  2. Publish the mobile build sending organizationId (wired here via useActiveOrgId).
  3. Strict flip PR (organizationId required, fallback removed) — gated on adoption.

Tests

notes + selectors + gating + parity + canary: 138 green. Cross-app battery (tasks/teams/clients/common/accounts): 1118 passed. ruff + format clean. Mobile: tsc + eslint clean on touched files (vitest runs in CI).

Follow-ups

  • tasks write gates → get_writable_or_deny (drops its RAW_FETCH_EXEMPT entry; re-pins).
  • clients documents + referrals: still on the guardian PermissionedQuerySet; convert with their slices.
  • Design: "access classes" declaration matrix (GLOBAL/PARENT cells) + mixin rename — next-slice artifacts.

Summary by Sourcery

Complete the notes domain cutover from legacy guardian authorization to grant-backed shared reads and organization-scoped writes while preserving compatibility for pre-payload clients.

New Features:

  • Cut over notes authorization to grant-backed, shared-read and organization-scoped write access, including payload-selected organization creation.
  • Add mobile note creation wiring to submit the active organization.

Bug Fixes:

  • Ensure unauthorized, missing, malformed, cross-organization, and detached service-request operations fail closed with consistent permission responses.
  • Prevent legacy guardian permissions from over-authorizing note and related write paths.

Enhancements:

  • Centralize write authorization through queryset-based writable selectors and mutation fetch gates.
  • Gate service-request operations and note attachment uploads through the owning note's organization-scoped permissions.
  • Document and enforce authorization routes for all notes queries and mutations with mutation-gating checks.

Documentation:

  • Document selector-based read, write, create, refusal, and mutation-gating authorization conventions.

Tests:

  • Add grant authorization coverage for role-backed access, legacy-inert behavior, shared reads, scoped writes, compatibility creates, service-request behavior, and upload gates.
  • Add authorization registry and raw-fetch scanner coverage for the notes GraphQL schema.

…3 slice 2)

- Note is OrgScoped (org_via=()) — the anchor for the org-scoped write arm (T2).
- CASEWORKER_ROLE carries the note bundle (Note A/V/C/D, ServiceRequest
  A/V/C/D, Attachment A/V).  The TEMPLATE stays ADD+VIEW for now: flipping
  model-level CHANGE/DELETE before the write cutover makes the legacy guardian
  prefilter treat them as 'all rows' and over-permits (caught by the notes
  permissions suite) — they move with the write cutover.
- Reads: note field -> HasPerm(VIEW) + can_anywhere_checker; notes list ->
  NoteType.get_queryset hook (visible_rows_for_holder, memoized);
  services/service_categories/interaction_authors -> can_anywhere_checker.
  invalidate_scope_cache drops the new memo.
- Query-count pins re-measured (the checker adds the grant lookups).

Next (T2): write paths -> can_obj, create payload org + tolerant staging,
service-request org reach, parity suite + authz canary.

Tests: notes 116 passed; cross-app battery (tasks/teams/clients/gating) 530 passed; ruff clean.
- writable(qs, user, perm): the write rule as a queryset filter.  can_obj()
  now delegates to it, so the single-row check and the queryset filter cannot
  drift (unsaved rows are no longer writable — fail-closed tightening).
- get_writable_or_deny(qs, pk, user, perm, *, message=): the mutation leaf —
  the fetch *is* the gate (unfetchable means unwritable), one query instead
  of fetch-then-check, and missing/forbidden share one refusal.
- Gating tripwire: writable(/get_writable_or_deny( count as gate markers;
  notes.schema registered; new scanner test rejects raw
  get_or_none(<Model>.objects.all(), pk) fetches in Mutation bodies
  (RAW_FETCH_EXEMPT is the migration ledger; tasks converts in a follow-up).
- Selector tests: writable equivalence with can_obj, WRITE_SHARED all-or-none,
  undeclared platform-shared fail-closed without row-table access, and
  delegated-org authority answered with only the target passed (no topology
  at call sites).
- Styleguide: authority-through-selectors rules (reads visible/checkers,
  writes writable/get_writable_or_deny, creates require_can, platform-only
  fields gate on the global tier — can_anywhere admits scoped holders).
- E005 test modernization: the "unscoped model" example is now Attachment
  (Note has been OrgScoped since the notes cutover; the old premise was
  stale but the gap was never exercised by the subset battery).
…0003, DEV-2561)

T1 (already on this branch): role-back + SHARED reads.  This commits the
write cutover:

- Note is OrgScoped; CASEWORKER_ROLE carries the note bundle; the TEMPLATE
  moves with the writes (flipping model-level perms before the guardian write
  paths are gone over-permits via filter_for_user — T1 trap).
- Reads SHARED: note field HasPerm(VIEW)+can_anywhere_checker; notes list via
  visible_rows_for_holder; service catalogs gated on Note ADD.
- Writes fetch through the write-scoped selector: update/update_location/
  revert/delete via get_writable_or_deny (delete keeps its custom message);
  service-request create gated on the owning note; service-request delete
  gated by the org of the note carrying it — detached SRs fail closed.
- ServiceRequest is deliberately NOT OrgScoped (its service hop cannot
  resolve until OrganizationService is org-scoped).  Consequence: the scoped
  role carries no SR or Attachment perms (E005 — scoped RoleDefs may only
  carry perms on OrgScoped-declared models); the SR gates ride the note and
  attachments ride the note gate (upload mutations no longer declare the
  Attachment HasPerm).
- Create takes the payload org: CreateNoteInput.organizationId (optional) +
  resolve_org_or_deny + require_can(ADD).  Tolerant staging: pre-payload
  builds still create via their legacy CASEWORKER group (PermissionError
  converted to the standard refusal); strict flip lands once the sending
  build is live (mirrors #2459).
- Tests: pins re-measured (each converted gate -1 query; reverts +3 then -1);
  parity suite notes/tests/test_grant_authorization.py (role-backed, grant-
  only, legacy-inert, cross-org SHARED reads, org-scoped writes, compat
  window, detached-SR pin); authz registry canary
  notes/tests/test_authz_registry.py; schema.graphql regenerated (additive
  organizationId + directive swaps only).
- NoteEditorScreen: organizationId: activeOrgId via the shared useActiveOrgId
  hook; submission is skipped when no active org is known (matches the
  tasks-flow convention: "no active org to anchor the create to").
- Regenerated: libs/ba-platform types (CreateNoteInput.organizationId) and
  libs/expo/shared/clients schema (directive + input diff).

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @vecchp, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 5 days and 16 hours by commenting @sourcery-ai review. Upgrade to get a review now.

@sourcery-ai

sourcery-ai Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

This PR completes the notes grant-authority cutover by introducing a single write-scoped selector and fetch-as-gate pattern, migrating all note mutation and upload authorization to organization-scoped grants while preserving SHARED reads, adding a staged payload-organization create path with mobile wiring, and covering the behavior with registry, parity, canary, and regression suites.

Sequence diagram for organization-scoped note mutation authorization

sequenceDiagram
    participant Client
    participant Mutation
    participant Selector as writable
    participant Gate as get_writable_or_deny
    participant NoteRecord as Note
    participant Service as note_update

    Client->>Mutation: update_note(data)
    Mutation->>Gate: get_writable_or_deny(Note.objects.all(), id, user, CHANGE)
    Gate->>Selector: writable(qs, user, CHANGE)
    Selector->>NoteRecord: Filter rows by organization grant
    NoteRecord-->>Selector: Writable note or no row
    Selector-->>Gate: Scoped queryset
    alt note is writable
        Gate-->>Mutation: Note
        Mutation->>Service: note_update(note, organization)
        Service-->>Client: Updated note
    else missing or forbidden
        Gate-->>Client: PermissionDenied
    end
Loading

Sequence diagram for payload-organization note creation

sequenceDiagram
    participant Mobile as Mobile client
    participant Mutation as create_note
    participant Resolver as resolve_org_or_deny
    participant Authority as require_can
    participant Service as note_create
    participant NoteEntity as Note

    Mobile->>Mutation: createNote(organizationId, payload)
    Mutation->>Resolver: resolve_org_or_deny(organizationId)
    Resolver-->>Mutation: Organization
    Mutation->>Authority: require_can(user, ADD, org)
    alt grant allows ADD
        Authority-->>Mutation: Allowed
        Mutation->>Service: note_create(organization, payload)
        Service->>NoteEntity: Create organization-linked note
        NoteEntity-->>Mobile: Created note
    else grant denied
        Authority-->>Mobile: PermissionDenied
    end
Loading

File-Level Changes

Change Details Files
Centralize grant-aware write authorization in queryset selectors and mutation fetch helpers.
  • Added writable() as the write-scope counterpart to visible() and made can_obj() delegate to it.
  • Added get_writable_or_deny() for single-query, fail-closed target fetches with uniform missing/forbidden errors.
  • Documented selector-based authorization and added mutation raw-fetch enforcement with migration exceptions.
apps/betterangels-backend/common/permissions/selectors.py
apps/betterangels-backend/common/permissions/utils.py
apps/betterangels-backend/common/tests/test_org_mutation_gating.py
docs/styleguides/python.md
Cut the notes domain over from Guardian/queryset permissions to grant-backed, organization-scoped writes and shared reads.
  • Made Note inherit OrgScoped while keeping note reads SHARED and writes scoped to the note organization.
  • Replaced Guardian gates across note CRUD, location, revert, service-request, and upload mutations with write-scoped fetches.
  • Authorized service-request operations through the owning note and failed closed for detached requests.
  • Moved note model-level permissions into the Caseworker role and removed scoped-role Attachment and ServiceRequest permissions for E005 coherence.
apps/betterangels-backend/notes/models.py
apps/betterangels-backend/notes/groups.py
apps/betterangels-backend/notes/schema.py
apps/betterangels-backend/notes/types.py
apps/betterangels-backend/notes/services.py
apps/betterangels-backend/schema.graphql
Add payload-organization create authorization with a staged compatibility fallback.
  • Added optional organizationId to CreateNoteInput and generated schema output.
  • Resolved the payload organization and enforced ADD authority with require_can() when supplied.
  • Retained legacy Caseworker-group creation for pre-payload mobile builds until the strict follow-up flip.
  • Passed organization context through note and service-request creation paths without assigning Guardian rows on grant-authorized flows.
apps/betterangels-backend/notes/schema.py
apps/betterangels-backend/notes/types.py
apps/betterangels-backend/notes/services.py
apps/betterangels-backend/schema.graphql
Wire the mobile note editor to send the active organization on creates.
  • Read the active organization through useActiveOrgId.
  • Included organizationId in create-note variables and skipped creation when no active organization is available.
libs/expo/betterangels/src/lib/screens/NoteForm/NoteEditorScreen.tsx
libs/ba-platform/src/lib/apollo/graphql/__generated__/types.ts
libs/expo/shared/clients/src/lib/apollo/graphql/__generated__/schema.ts
Expand authorization regression coverage, registry canaries, and permission-model parity tests.
  • Added grant-only CRUD, SHARED-read/scoped-write, cross-organization, compatibility-window, malformed-org, upload, and detached-service-request scenarios.
  • Registered every notes Query and Mutation field with its authorization route.
  • Updated selector, E005, mutation, query, permission, and attachment tests for the new gates and query counts.
apps/betterangels-backend/notes/tests/test_grant_authorization.py
apps/betterangels-backend/notes/tests/test_authz_registry.py
apps/betterangels-backend/common/tests/test_permissions_selectors.py
apps/betterangels-backend/common/tests/test_permission_checks.py
apps/betterangels-backend/notes/tests/test_mutations.py
apps/betterangels-backend/notes/tests/test_note_attachment_mutations.py
apps/betterangels-backend/notes/tests/test_permissions.py
apps/betterangels-backend/notes/tests/test_queries.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants