feat(perm): teams fully grant-based & header-free — ORG_ADMIN + CASEWORKER role-backed, org in filter/payload (DEV-2559) - #2443
Merged
Merged
Conversation
Contributor
Reviewer's GuideThe PR makes team creation, updates, and deletion grant-only by introducing scoped ORG_ADMIN/ORG_SUPERUSER roles, backfilling Grants from existing organization-admin memberships, and routing mutations through Sequence diagram for grant-only team mutation authorizationsequenceDiagram
actor User
participant TeamsAPI
participant Organization
participant PermissionService
participant GrantStore
participant TeamStore
User->>TeamsAPI: create_team / update_team / delete_team
TeamsAPI->>Organization: get_current_organization(info)
TeamsAPI->>PermissionService: require_can(user, Team.perms.*, org)
PermissionService->>GrantStore: can(user, permission, org)
alt Grant allows permission
PermissionService-->>TeamsAPI: authorized
TeamsAPI->>TeamStore: team_create / team_get / team_update / team_delete
TeamStore-->>User: mutation result
else No scoped Grant
PermissionService-->>User: PermissionDenied
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
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/accounts/groups.py" line_range="52-64" />
<code_context>
+ Team.perms.VIEW,
+]
+
+ORG_ADMIN_ROLE = RoleDef(
+ name=ORG_ADMIN.name,
+ permissions=list(ORG_ADMIN_ROLE_PERMISSIONS),
+ is_invitable=ORG_ADMIN.is_invitable,
+)
+
+ORG_SUPERUSER_ROLE = RoleDef(
+ name=ORG_SUPERUSER.name,
+ permissions=list(ORG_ADMIN_ROLE_PERMISSIONS),
+ is_invitable=ORG_SUPERUSER.is_invitable,
+)
+
+ORG_ADMIN_ROLES: tuple[RoleDef, ...] = (ORG_ADMIN_ROLE, ORG_SUPERUSER_ROLE)
</code_context>
<issue_to_address>
**issue (broader_impact):** Replacing or clearing an organization's legacy permission-group roles deletes every scoped Grant for that user in the organization, including independently assigned grant-only roles such as a Team Admin Grant. After this cutover, assigning or removing an unrelated role can silently revoke the user's team mutation authority.
**Triggers:** When a user has both an ORG_ADMIN/legacy role and an independently assigned scoped Team role, and member-role management calls `clear_roles` or `replace_roles`.
**Suggested fix:** Delete only Grants whose roles correspond to the permission groups being removed, rather than deleting all Grants at the organization scope; preserve independently assigned grants.
</issue_to_address>
Comment on lines
+52
to
+64
| ORG_ADMIN_ROLE = RoleDef( | ||
| name=ORG_ADMIN.name, | ||
| permissions=list(ORG_ADMIN_ROLE_PERMISSIONS), | ||
| is_invitable=ORG_ADMIN.is_invitable, | ||
| ) | ||
|
|
||
| ORG_SUPERUSER_ROLE = RoleDef( | ||
| name=ORG_SUPERUSER.name, | ||
| permissions=list(ORG_ADMIN_ROLE_PERMISSIONS), | ||
| is_invitable=ORG_SUPERUSER.is_invitable, | ||
| ) | ||
|
|
||
| ORG_ADMIN_ROLES: tuple[RoleDef, ...] = (ORG_ADMIN_ROLE, ORG_SUPERUSER_ROLE) |
Contributor
There was a problem hiding this comment.
issue (broader_impact): Replacing or clearing an organization's legacy permission-group roles deletes every scoped Grant for that user in the organization, including independently assigned grant-only roles such as a Team Admin Grant. After this cutover, assigning or removing an unrelated role can silently revoke the user's team mutation authority.
Triggers: When a user has both an ORG_ADMIN/legacy role and an independently assigned scoped Team role, and member-role management calls clear_roles or replace_roles.
Suggested fix: Delete only Grants whose roles correspond to the permission groups being removed, rather than deleting all Grants at the organization scope; preserve independently assigned grants.
vecchp
changed the title
vecchp
changed the title
vecchp
changed the title
This was referenced Sep 9, 2026
vecchp
changed the title
vecchp
added
the
graphql-inspector:approved-breaking-change
|
🚀 Expo continuous deployment is ready for betterangels!
iOS Simulator Build: Simulator Build Link |
…e backfills Review findings on the org admin change page: - GrantInline/DelegatedGrantInline rendered ~540 queries for a ~90-member org (~6 per grant row). autocomplete_fields was the worst offender (a per-row related lookup for every existing FK). Switch both inlines to raw_id_fields + select_related, and make object-grant columns read-only (they are edited on the Grant admin, not the org-scope inline): ~3/grant. - The remaining 3/grant were the stock ForeignKeyRawIdWidget re-querying self.rel.model._default_manager.get(pk=...) per widget per row — invisible to select_related because the widget never looks at the row instance. Add LoadedRowRawIdWidget (renders from the loaded row via the inline form's instance binding) and GrantRowForm, which hands each raw-id FK widget its select_related'd row: ~1/grant. - The last 1/grant came from the row template stringifying the instance: Grant.__str__ walks principal_user, principal_org, role and scope_org, and GrantInline did not select_related scope_org. Load all four FKs on both grant inline querysets: 0/grant. - OrganizationMemberInlineQueryCountTestCase now asserts a strict zero delta: adding six members (six more grant rows) adds no queries at all. - Add OrgAdminAndCaseworkerBackfillTestCase covering backfill_org_admin_ grants and backfill_caseworker_grants (create / idempotent / convert-only), which previously had zero coverage.
…r clients
More of the adversarial review on the teams grant-only cutover:
- The Django User-admin group picker bypassed OrgRoleManager: a superuser
adding a user straight to an org's role-backed PermissionGroup produced a
legacy-only holder with no Grant, who then could not manage teams.
UserAdmin.save_related now mirrors the same transitions OrgRoleManager
performs — module-level mirror_membership_grant/unmirror_membership_grant
(scoped by the group's own org, with OrgRoleManager passing its known org
so the membership path keeps its query count) — keeping group and Grant in
step whether the role came from the org member page or the user page.
Tests: adding mirrors a Grant, removing unmirrors it, label-only groups
conjure none, and an unchanged save changes nothing.
- The teams read's header deprecation is now on record: the ADR and
schema docstrings list the header-only clients that must migrate before
X-Organization-ID is stripped (betterangels-admin TeamsPage already sends
filters.organizationId; mobile useOrgTeams — NoteForm, TaskForm,
FilterTeamsOptions, UserTeamPreferenceSelect — sends only { isActive }),
and note that a role-less member's team read is intentionally grant-gated.
The resolver docstring also records that an omitted org id arrives as ""
and must keep the header fallback until those clients migrate.
- createTeam with an unknown organizationId is now pinned to fail closed
(the shared _org_or_deny path was only covered on the read side), and a
non-numeric organizationId filter is pinned to deny rather than leak a
ValueError.
vecchp
force-pushed
the
feat/perm/teams-grant-only
branch
from
September 9, 2026 23:55
593d848 to
aea8d00
Compare
Comment-only: drop the self-evident explanation blocks (raw-id-vs-autocomplete cost, the readonly object-grant columns, the per-member strict-test narrative, the org-filter auto field note) and compress the widget/form/mirror docstrings to their essential why. The behavior is unchanged and the strict zero-delta query test still guards the N+1 regression the removed comments described.
…change Regression for the review flag that member-role management might delete every scoped Grant at the org: member_roles_replace is add + scoped remove, so a grant-only role assigned independently (a scoped Role with no PermissionGroup) must survive a demotion that revokes the member's invitable roles.
vecchp
enabled auto-merge
September 10, 2026 00:41
…e refusal, m2m mirror - teams/schema: validate client org ids with get_or_none (blank/garbage denies like unknown instead of raising ValueError); empty filter id no longer falls back to the header; missing vs. foreign teams share the standard PERMISSION refusal (no existence oracle). - common/permissions: single PERMISSION_DENIED_MESSAGE. - accounts: enforce the membership <-> Grant mirror at the User.groups m2m edge (accounts.signals) — covers admin/scripts/shell, reverse writes and clear(); UserAdmin.save_related hook removed. Batch role lookups. Cascading PermissionGroup delete deliberately keeps Grants (teardown); the admin delete page says so. - sync_roles provisions one _all_role_defs() list; ORG_SUPERUSER_ROLE derives from ORG_ADMIN_ROLE; test pins role bundles <= templates. - tests: denied-path assertions check the message (no more passing on a crash); new malformed/blank/null org-id and oracle tests; signal tests; backfill tests rebuild pre-cutover state via _add_legacy_membership. - docs: ADR §5.3 + overview note the invariant and actual landing route.
- TeamsPage: skip the query without View (server would refuse). - TeamFormDrawer: only create needs the active org (row-derived otherwise). - ba-platform: drop stale teams mention from the legacy-only comment list.
- tests: drop the PERMISSION_DENIED alias and use the shared message constant; make _assert_denied's expected message required (no defensive branch); drop two redundant sync_roles() setUps the base class covers. - role_manager: remove the now-unused organization kwarg on the mirror helpers and the single-use scoped_role_for_group wrapper (admin batches through scoped_roles_for_groups); inline the single-use ORG_ADMIN_ROLE_PERMISSIONS constant. - perf: batch the admin delete-page role lookup (N -> 1); select_related the organization in _backfill_role_grants; skip the signal when pk_set is empty; skip the message-only uniqueness lookup when a team edit does not change the name. - comments: remove duplicated/stale mirror notes (OrgRoleManager era), compress query-pin and refusal comments, stop restating the mobile client list in two places.
deleteRooms/deleteBeds read the deleted pks back with values_list() and no ordering, so the GraphQL response followed the DB's unspecified row order — DeleteRoomsMutationTestCase::test_delete_multiple_rooms is latently flaky and failed on CI. Return the requested order (duplicates collapsed) and pin the contract with a reversed-request regression test for both bulk deletes.
vecchp
pushed a commit
that referenced
this pull request
Sep 10, 2026
The org-admin cutover (#2443/#2444/#2445) left the legacy org-permission machinery with no consumers. Remove it: - Delete the HasOrgPerm strawberry extension (accounts/extensions.py) + its tests — @hasOrgPerm was already gone from the schema; nothing imports it. - Delete get_user_permitted_org (accounts/permissions.py) and the permissioned_queryset / perm_filter / _perm_q / _org_perm_exists_across_fields legacy predicates plus the now-dead active_org helper (common/permissions/utils.py) — zero remaining consumers after the cutover. - Tests: drop the HasOrgPerm / permissioned_queryset unit tests; docstrings that cited the deleted HasOrgPerm now describe the grant cutover. The X-Organization-ID header is deliberately KEPT: mobile's useOrgTeams callers still send only { isActive } and rely on it, so teams reads keep the deprecated header fallback (filter first, with the blank/malformed-id guards). The OrganizationMiddleware + get_current_organization stay with it; the strip is DEV-2566, done once mobile passes organizationId.
Contributor
Previous review items — status
New findingsF1 (medium) — reconcile-driven group deletion no longer revokes the mirrored authorityVerified empirically with a throwaway probe (since deleted): an org with types This is the direct cost of the deliberate "group deletion must not revoke Grants" rule (needed so teardown can retire legacy rows). The signal can't distinguish teardown from config cleanup, but F2 (low-med, architecture) — signal-hosted invariant: document the exception precisely
F3 (low) — same-role direct Grant is destroyed with the membership
F4 (low) — triple-literal permission message
F5 (nit) —
|
mikefeldberg
approved these changes
Sep 10, 2026
Contributor
Author
|
Implemented — all in #2456 (stacked on #2455; merges right after the chain, #2443 untouched). Per finding:
One observation from doing F3 (no behavior change made — flagging for direction): the admin inline's row-deletion route now only strips the legacy membership — a mirrored Grant survives the row delete by design (the teardown asymmetry the delete-view note already calls out), while the org-type route fully revokes via F1. The Validation: full backend suite 1890 passed / 31 skipped on the branch; ruff clean. |
vecchp
pushed a commit
that referenced
this pull request
Sep 10, 2026
…y, nits
F1 reconcile_org_groups unmirrors stale derived rows before deleting them,
so an org-type change fully revokes (mirrored Grants no longer survive
config cleanup); teardown deletes keep leaving Grants standing.
F2 signal docstring states the sanctioned exception precisely (m2m-manager
writers only); helper duplication already consolidated in #2451.
F3 same-row direct-grant revocation pinned by test + GrantAdmin fieldset
description (unique constraint makes both grants one row).
F4 moot — accounts/extensions.py deleted in 590dc50.
F5 set(dict) -> .keys().
F6 stale delete_orphaned_group refs corrected (services.py, admin.py);
tear-down described as the structural MTI cascade (migration 0007).
F7 LoadedRowRawIdWidget preserves the original widget's using kwarg.
F8 teams/tests/test_authz_registry.py canary added (4 fields).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Cut the teams domain fully over to the grant-based authorization model on
main(ADR 0001 §5.3, teams slice). The teams surface — read and write — now authorizes viacan()/require_canover role-backed templates with backfilled Grants; org membership is no longer consulted for teams.createTeam/updateTeam/deleteTeam): grant-only.ORG_ADMIN/ORG_SUPERUSERare role-backed with a scoped Role carryingteams.*.teamsquery): grant-only.CASEWORKERis role-backed as the RFC 0003 first step — a scopedCaseworkerRole carryingteams.view_team— so the workers who pick teams on notes/tasks read via grants.Why not the §5.3 "all four consumers at once" milestone: on
mainthe member-management codenames (organizations.*) andreports.view_reportsresolve to no concrete model, so a scoped RoleDef carrying them failssync_roles' phantom-ContentType guard. Teams was the slice that could ride Roles today; reports and member management stay legacy-only until their own slices.Changes
accounts/groups.py—ORG_ADMIN_ROLE/ORG_SUPERUSER_ROLEscoped RoleDefs carryingteams.*, exported asORG_ADMIN_ROLES.notes/groups.py—Team.perms.VIEWadded to theCASEWORKERtemplate + scopedCASEWORKER_ROLERoleDef carryingteams.view_team(RFC 0003 first step; the rest of the caseworker bundle stays legacy until RFC 0003).accounts/services.py—sync_roles()provisions all three role sets; the three backfills are deduped into one_backfill_role_grants()helper withbackfill_shelter_grants/backfill_org_admin_grants/backfill_caseworker_grantswrappers.accounts/apps.py—backfill_org_admin_grants()+backfill_caseworker_grants()run in_seed_on_migrateaftersync_roles().teams/schema.py—HasOrgPerm→require_can(…, teams.*); org resolved via a module-level helper that fails closed on an unknown org header (noDoesNotExist).teamsread:require_can(user, teams.view_team, org)— grant-only. Membership helper removed. Org resolution:TeamFilter.organizationIdwins; theX-Organization-IDheader is the deprecated fallback (_resolve_teams_orgfails closed on an unknown org).teams/models.py—TeamdeclaresOrgScoped(org_via = ()); required bypermissions.E005(no migration).common/permissions/domain.py—teamsjoinsLEGACY_INERT_APPS(legacy rows not reported; global tier folds like shelters).teams/tests/test_grant_authorization.py: grant-only read + write contract (role-backed ORG_ADMIN & CASEWORKER read/manage; legacy-only admin denied on mutations; ADD-only cannot update/delete; member-without-grant denied on the read; direct-grant holder & superuser read; grant at org A does not authorize org B). Read-org tests: org filter works without the header, the filter wins over a stale header, and an unknown filter org is denied. Equivalence/report tests strengthened:test_org_admin_member_report_matches_legacy_enforcementnow assertscan()forteams.*(was vacuously asserting the legacy predicate), and newCurrentUserTeamsReportCanEquivalenceTestCasepins entry ≡can()for the teams domain. Role-manager dual-write tests updated (CASEWORKER is role-backed now). Admin/mutation query-count guards updated for grant-holding members. FE: TeamsPage tests assert the query is issued withfilters.organizationIdand that a view-only holder sees neither Add nor the per-row actions menu.schema.graphql— regenerated (@hasOrgPermdrops off the three mutations;TeamFilter.organizationIdandCreateTeamInput.organizationId: ID!added).libs/react/betterangels-adminTeams page + drawer — passesactiveOrg.idasfilters.organizationId(read) and in thecreateTeampayload; gates Add/Edit/Delete on the team grants (see design notes).ThreeDotMenutakescanEdit/canDeleteand conditionally renders each action.docs/adr/0001— §5.3 status note.@monorepo/ba-platformtypes (TeamFilter.organizationId,CreateTeamInput.organizationId).organization_id: auto(strawberry-django resolves the FK column toorganizationId: ID; the hand-writtenfilter_field+Qare gone, schema byte-identical);team_get's org confine is optional; single_resolve_read_org+_org_or_denyhelpers replace the duplicated resolvers.Design notes / deliberate scope
Caseworkergrant (teams.view_team); admins viaORG_ADMIN. The FE gates oncurrentUser.organizationsOrganization[].permissions(grant-native since feat(perm): frontend reachability — grants-based org list + global permissions (DEV-2557) #2414).TeamFilter.organizationId(authoritative, so switching orgs re-runs the query scoped to the right org); theX-Organization-IDheader is kept only as a backward-compatible fallback — the mobile clients (libs/expo) still rely on it — and is marked for later removal. The mutations never read the header:createTeamcarriesorganizationIdin the payload (required — no row exists to scope by yet) andupdateTeam/deleteTeamresolve the row the payload names by id and authorize atteam.organization(the org is an attribute of the row).teams.add_teamand the per-row Edit/Delete actions onteams.change_team/teams.delete_team, so a view-only holder (e.g. a role-backed caseworker) sees a read-only directory — no Add button, no actions menu (previously the actions menu rendered for everyone withView).OrgRoleManager.add_rolesmirrors Grants for new ones (role-backed templates). Legacy groups are kept (dual) so member management/reports keep working off the legacy arm until their slices;teamslegacy rows are inert (suppressed from reports).teams.*/teams.view_teamonly) — a documented divergence from the §5.3 "one template, four consumers" plan, forced by the phantom-ContentType guard. Later slices extend the RoleDefs andsync_rolesreconciles idempotently.Review round 2 — adversarial fixes
_org_or_denyvalidates with the houseget_or_nonepk guard, soorganizationId: "not-an-id"or""(read filter, header, or create payload) is a cleanPERMISSIONrefusal instead of a DjangoValueError. An absent filter keeps the header fallback; an explicitly empty one denies — it never silently falls back.updateTeam/deleteTeamanswer a missing team with the same message as a team the caller may not touch (PERMISSION_DENIED_MESSAGEincommon/permissions/utils.py), closing an existence oracle.User.groupsm2m edge (accounts.signals). Every writer keeps the invariant —OrgRoleManager, the user-admin group picker, scripts, the shell — and the bespokeUserAdmin.save_relatedhook is gone. Reverse writes (permission_group.user_set.add/remove/clear) are covered. A cascading delete of a legacyPermissionGroupdeliberately leaves the Grants (teardown retires legacy rows; the Grants are the successor authority) — the admin delete page now says so instead of implying the capability is revoked.sync_rolesprovisions from one_all_role_defs()list,ORG_SUPERUSER_ROLEderives fromORG_ADMIN_ROLE, and a test asserts every scoped RoleDef's bundle is a subset of its template's._assert_deniedasserts the expected message; new tests cover non-numeric/blank filter, header and payload ids; an explicitly-null filter id still falls back to the header.View(no doomed request); the org guard moved into the create branch only (edits don't need an active org); staleteamsreferences removed from theuseActiveOrgStatecomment; ADR + overview record the mirror invariant and the actual landing route.PERMISSION_DENIED = PERMISSION_DENIED_MESSAGEalias, the now-unusedorganizationkwarg on the mirror helpers, a single-useORG_ADMIN_ROLE_PERMISSIONSconstant, a defensive optional branch in_assert_denied) and pruned duplicated/stale comments. Small perf wins: batched the admin delete-page role lookup (N→1),select_related("organization")in the backfills, an empty-pk_setshort-circuit in the signal, and the message-only uniqueness query is skipped when a team edit doesn't change the name.Verification
manage.py checkclean (incl.permissions.E005);makemigrations --check→ no changes