Skip to content

refactor(deleter): move custom-resource policy cleanup to resource service#1800

Open
whoAbhishekSah wants to merge 1 commit into
mainfrom
resource-remove-principal-access
Open

refactor(deleter): move custom-resource policy cleanup to resource service#1800
whoAbhishekSah wants to merge 1 commit into
mainfrom
resource-remove-principal-access

Conversation

@whoAbhishekSah

@whoAbhishekSah whoAbhishekSah commented Jul 24, 2026

Copy link
Copy Markdown
Member

What

When a user is deleted, the deleter walked the user's policies itself and deleted the ones on custom resources. That was the last spot where it removed access without going through a domain service.

This moves that cleanup into the resource service, as a new method RemovePrincipalAccess. The deleter now calls it once per user and no longer touches policies directly for this path.

Behavior

Unchanged. Deleting a user still removes their access to resources in the org's projects. Org, project, and group membership is still handled by the membership cascade.

🤖 Generated with Claude Code

@vercel

vercel Bot commented Jul 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview, Comment Jul 24, 2026 4:19am

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@whoAbhishekSah, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 49 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: bad0cfc7-7404-4377-8318-9cc9b6e9de01

📥 Commits

Reviewing files that changed from the base of the PR and between fbfd35d and e9f9e55.

📒 Files selected for processing (7)
  • cmd/serve.go
  • core/deleter/mocks/resource_service.go
  • core/deleter/service.go
  • core/deleter/service_test.go
  • core/resource/mocks/policy_service.go
  • core/resource/service.go
  • core/resource/service_test.go
📝 Walkthrough

Walkthrough

Resource services now remove principal policies scoped to organization projects. Organization member removal delegates this cleanup before membership removal, with updated constructor wiring, mocks, and tests.

Changes

Principal access cleanup

Layer / File(s) Summary
Resource principal cleanup
core/resource/service.go, core/resource/mocks/policy_service.go, core/resource/service_test.go
Adds policy-service dependency wiring and RemovePrincipalAccess, which lists principal policies, filters them by project resources, deletes matching policies, aggregates delete errors, and adds coverage for the new behavior.
Organization removal integration
core/deleter/service.go, core/deleter/mocks/resource_service.go, core/deleter/service_test.go
Replaces inline policy cleanup with ResourceService.RemovePrincipalAccess and updates organization-removal tests and mocks for the revised sequence.
Runtime dependency wiring
cmd/serve.go
Passes policyService into the resource service constructor.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

Suggested reviewers: amangit07

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@whoAbhishekSah
whoAbhishekSah force-pushed the resource-remove-principal-access branch from 3789133 to fbfd35d Compare July 24, 2026 04:09
@whoAbhishekSah whoAbhishekSah changed the title feat(deleter): move custom-resource policy cleanup to resource service refactor(deleter): move custom-resource policy cleanup to resource service Jul 24, 2026
@coveralls

coveralls commented Jul 24, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 30066468066

Coverage increased (+0.02%) to 46.759%

Details

  • Coverage increased (+0.02%) from the base build.
  • Patch coverage: 3 uncovered changes across 2 files (32 of 35 lines covered, 91.43%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
core/resource/service.go 32 30 93.75%
cmd/serve.go 1 0 0.0%
Total (3 files) 35 32 91.43%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 38983
Covered Lines: 18228
Line Coverage: 46.76%
Coverage Strength: 14.32 hits per line

💛 - Coveralls

…rvice

When a user is deleted, the deleter walked the user's policies itself
and deleted the ones on custom resources. That was the last place it
removed access without going through a domain service.

Move that cleanup into the resource service as RemovePrincipalAccess.
The deleter now calls it once per user and no longer touches policies
directly for this path. Behavior is unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (2)
core/resource/service_test.go (1)

561-582: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a multi-project case to TestRemovePrincipalAccess.

Current tests only pass a single project ID. A case with two+ projectIDs (each contributing distinct resources to projectResourceIDs) would directly cover the map-merging logic across projects.

core/resource/service.go (1)

473-484: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff

Consider batching resource lookups for the delete path.

RemovePrincipalAccess currently calls repository.List once per project, and resource.Filter.ProjectID only supports a single project at a time. If this path can be extended to list resources for many projects in one query, it would avoid N sequential DB calls before deleting related policies.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: df8088e1-0ade-4bb4-89bb-719866b4d7a6

📥 Commits

Reviewing files that changed from the base of the PR and between a0618e0 and fbfd35d.

📒 Files selected for processing (7)
  • cmd/serve.go
  • core/deleter/mocks/resource_service.go
  • core/deleter/service.go
  • core/deleter/service_test.go
  • core/resource/mocks/policy_service.go
  • core/resource/service.go
  • core/resource/service_test.go

@whoAbhishekSah

whoAbhishekSah commented Jul 24, 2026

Copy link
Copy Markdown
Member Author

Tested the DeleteUser RPC end to end on a local server built from this branch.

Setup

  • Local Postgres + SpiceDB. Custom permissions reconciled first so custom resource namespaces exist (resource/aoi, resource/workflow).
  • 2 orgs, 3 projects, 5 users. Every user joined through CreateOrganizationInvitation + AcceptOrganizationInvitation.
  • 4 custom resources created. Creating a resource writes no policy row — only SpiceDB tuples (owner, project). So all resource policies were created explicitly.
  • 2 custom org roles carrying resource_aoi_* permissions. Policies assigned at org, project, and resource level.
  • After each call, verified the policies and users tables and SpiceDB relation_tuple rows directly.

Results

Case Access before delete State after DeleteUser Result
user in 1 org org policy, project policy, 2 resource policies (custom role on resource/aoi resources in two projects) 0 policy rows, 0 SpiceDB tuples, user row gone pass
user in 2 orgs a resource policy in each org's project, memberships in both orgs policies in both orgs gone, memberships gone, user row gone pass
user removed from org first (RemoveOrganizationMember), then deleted 1 resource policy left over from the org removal (that RPC never reaches the deleter) user row and SpiceDB tuples gone, but the policy row stays orphaned pre-existing gap, unchanged by this PR

Note on the gap: RemovePrincipalAccess runs per org the user is still a member of. A user who already left the org keeps orphan resource policy rows in Postgres after deletion. This is tracked in #1801 and will be handled separately.

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