Skip to content

fix(perm): #2443 review follow-ups — reconcile unmirrors, teams canary, nits - #2456

Open
vecchp wants to merge 1 commit into
feat/perm/clients-grant-cutoverfrom
chore/perm-2443-review-fixes
Open

vecchp wants to merge 1 commit into
feat/perm/clients-grant-cutoverfrom
chore/perm-2443-review-fixes

Conversation

@vecchp

@vecchp vecchp commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

What

Follow-up to #2443's adversarial review (Mike's "Previous review items — status" comment): the code fixes for the outstanding findings, stacked on the clients cutover (#2455) so the chain keeps merging bottom-up. #2443 itself is untouched — merge it (and the chain) first.

Findings

Finding Status
F1 (medium) — reconcile-driven group deletion no longer revoked the mirrored authority Fixed (option a). reconcile_org_groups unmirrors each stale derived row's members before deleting it (unmirror_membership_grants), so dropping an org type now fully revokes both arms; teardown deletes keep leaving Grants standing. Pinned by test_reconcile_of_a_stale_group_unmirrors_the_grant.
F2 (low-med) — signal-hosted invariant docs / overclaim / helper duplication Addressed. Receiver docstring now states the sanctioned exception precisely (“every writer that goes through the m2m manager”; bulk_create/raw SQL/loaddata/queryset.update bypass — ADR 0001 §4 phase 2). Helper duplication was already consolidated in #2451 (common/tests/utils.py).
F3 (low) — same-role direct Grant destroyed with the membership Pinned + documented. New test_removing_a_membership_revokes_a_same_row_direct_grant; GrantAdmin now carries a fieldset description warning that the unique constraint makes a direct grant and a membership mirror the same row.
F4 (low) — triple-literal PERMISSION_DENIED_MESSAGE Moot. accounts/extensions.py was deleted by 590dc503 (dead org-admin machinery); no non-test hardcoded literal remains.
F5 (nit)set(scoped_roles_for_groups(objs)) Fixed.keys().
F6 (nit) — stale accounts.signals.delete_orphaned_group refs Fixed in services.py (×2) and the admin.py change_view docstring found alongside; tear-down now described as the structural MTI cascade (migration 0007).
F7 (nit)LoadedRowRawIdWidget drops using Fixed — preserved via getattr(widget, "using", None).
F8 — teams authz registry canary Fixedteams/tests/test_authz_registry.py mirrors the shelters pattern: every Query/Mutation field must be registered with its authorization route (4 fields: teams, create_team, update_team, delete_team).
Previous review item 4 (mobile header hook) Resolved upstream — the X-Organization-ID header is fully retired (#2450 / #2452).

Validation

  • Full backend suite: 1890 passed, 31 skipped (baseline 1886 + the 4 new tests)
  • ruff check + ruff format --check clean on changed files

Landing order unchanged: #2415 + #2443#2444 → … → #2455this, bottom-up.

Summary by Sourcery

Harden organization-group permission reconciliation and teams authorization coverage.

Bug Fixes:

  • Revoke mirrored grants when reconciliation removes stale organization groups while preserving teardown behavior.
  • Clarify and document the membership/grant lifecycle, including the same-row direct-grant deletion behavior.
  • Preserve the database alias when loading related admin widgets.

Enhancements:

  • Correct role-backed group lookup handling and update admin and service documentation for structural group deletion.
  • Add an authorization registry canary requiring every teams query and mutation field to declare its authorization route.

Tests:

  • Add coverage for stale-group reconciliation revocation and same-row direct grant removal.
  • Add teams query and mutation authorization registry tests.

@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 4 days and 19 hours by commenting @sourcery-ai review. Upgrade to get a review now.

@sourcery-ai

sourcery-ai Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

This follow-up fixes reconcile cleanup so stale organization groups revoke mirrored authority without changing teardown behavior, documents the membership/grant and deletion invariants, preserves widget database routing, and adds regression and teams schema authorization-canary coverage.

Sequence diagram for reconciling stale organization groups

sequenceDiagram
    participant Reconciler
    participant PermissionGroup
    participant Member
    participant Grant

    Reconciler->>PermissionGroup: reconcile_org_groups(org)
    Reconciler->>PermissionGroup: PermissionGroup.objects.filter(...).prefetch_related(user_set)
    loop each stale group member
        Reconciler->>Member: unmirror_membership_grants(member, [permission_group])
        Member->>Grant: revoke mirrored Grant
    end
    Reconciler->>PermissionGroup: PermissionGroup.objects.filter(pk__in=...).delete()
    PermissionGroup-->>Reconciler: structural MTI cascade removes group memberships
Loading

File-Level Changes

Change Details Files
Reconcile now revokes mirrored grants before deleting stale organization-derived groups, while preserving teardown deletion semantics.
  • Collect stale groups and their members before deletion.
  • Unmirror each member's grants, then delete the stale permission-group rows.
  • Clarify structural MTI cascade versus reconcile cleanup behavior.
apps/betterangels-backend/accounts/services.py
apps/betterangels-backend/accounts/admin.py
apps/betterangels-backend/accounts/tests/test_role_manager_grants.py
Document the membership-to-grant invariant and its deliberate signal bypasses, and clarify the same-row direct-grant behavior in admin UI.
  • Specify that m2m-manager writers are covered while bulk, raw SQL, fixture, and queryset-update paths bypass the signal.
  • Warn administrators that removing a membership can delete a directly-created grant sharing the unique row.
  • Preserve the database alias when constructing the loaded-row widget.
apps/betterangels-backend/accounts/signals.py
apps/betterangels-backend/accounts/admin.py
Add a teams authorization registry canary that requires every schema field to declare its authorization route.
  • Register the teams query field and create, update, and delete mutation fields with their expected authorization paths.
  • Fail tests when schema fields are added or removed without updating the registry.
apps/betterangels-backend/teams/tests/test_authz_registry.py
Add regression coverage for reconcile revocation and same-row direct-grant removal.
  • Verify stale-group reconciliation removes both the group and mirrored grant.
  • Verify removing a membership revokes a grant that was also created directly.
apps/betterangels-backend/accounts/tests/test_role_manager_grants.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

@vecchp
vecchp force-pushed the feat/perm/clients-grant-cutover branch from a71e056 to c20b0ba Compare September 10, 2026 21:18
…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).
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