fix(role): reject deletion of a role still in use by policies#1683
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR prevents deletion of roles that are referenced by policies. It defines a new ChangesRole deletion with in-use constraint
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 27186856054Coverage increased (+0.01%) to 43.495%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
d688fd7 to
8772c28
Compare
8772c28 to
69a3d6c
Compare
69a3d6c to
a1f47f4
Compare
1daa527 to
68e04f1
Compare
68e04f1 to
ac2dcec
Compare
Deleting a role used to strip its app/role:<id>#<perm>@* tuples from SpiceDB *before* the row delete. Because policies.role_id has a foreign key to roles(id) with no ON DELETE rule, deleting an in-use role then failed the row delete with an opaque 500 — after the permission tuples were already gone, leaving a half-deleted role that still existed but granted nothing. Make role deletion a clean guard instead of a destructive cascade: attempt the row delete first, and only remove the permission tuples once it succeeds. The role repository maps the foreign-key violation to a new role.ErrInUse, which the handlers return as FailedPrecondition. Deleting a role that still has policies is therefore rejected with a clear message and zero side effects; the caller must remove those policies first. This intentionally avoids cascading policy deletion, which would revoke access for every principal granted the role and, for a widely-assigned role, fan out into many serial SpiceDB deletes inside one request. Adds unit tests for the in-use rejection / clean-delete paths and an e2e test asserting an in-use role can't be deleted (FailedPrecondition, nothing torn down) and deletes cleanly once its policy is removed. Refs #1661 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ac2dcec to
e2f2d58
Compare
Manual verification ✅Tested the Setup: created an org, a custom role (
Confirms the fix end-to-end: the FK guard fires before any SpiceDB tuples are removed (no more half-deleted role / opaque 500), the error surfaces cleanly as |
Problem
Deleting a role touches two stores: SpiceDB (the role's permission tuples) and Frontier's Postgres
rolestable (the role row). The old order was: delete the SpiceDB tuples first, then delete the row in Postgres.But the Postgres
policiestable has a foreign key toroles, so if a policy still uses the role, the Postgres row delete fails — after the SpiceDB tuples were already removed. Result: a broken role that still exists in Postgres but grants nothing in SpiceDB, plus a confusing500.Change
Deleting a role now fails cleanly if the role is still used by any policy:
rolesrow first; only remove the SpiceDB permission tuples if that succeeds.FailedPreconditionand the message "role is in use by one or more policies" — neither store is touched.We don't auto-delete the policies, because that would silently revoke access for everyone using the role and could be a lot of deletes in one request.
Tests
TestOrganizationRoleDeleteInUse): deleting an in-use role is rejected and nothing is torn down; after the policy is removed, the role deletes cleanly with no leftover SpiceDB tuples.Addresses gap (1) of #1661.