Skip to content

fix(deleter): cascade service user delete on organization delete#1586

Merged
AmanGIT07 merged 1 commit intomainfrom
fix/deleter-serviceuser-cleanup
Apr 30, 2026
Merged

fix(deleter): cascade service user delete on organization delete#1586
AmanGIT07 merged 1 commit intomainfrom
fix/deleter-serviceuser-cleanup

Conversation

@AmanGIT07
Copy link
Copy Markdown
Contributor

Summary

DeleteOrganization did not remove the service users owned by the org. Their rows lingered in serviceusers with org_id pointing at a deleted UUID, their credentials stayed in serviceuser_credentials, and the auth path happily continued to return a Principal{Type: ServiceUserPrincipal} for them. This PR adds the missing cleanup to the cascade deleter so that deleting an org actually deletes its service users (and their credentials and SpiceDB relations).

Refs umbrella issue #1585.

Changes

  • New ServiceUserService interface and field on core/deleter.Service (List, Delete).
  • New cleanup loop in DeleteOrganization between groups and roles: lists SUs by OrgID and calls serviceUserService.Delete per SU. Reuses the existing SU delete pipeline, which already removes credentials, org-membership policies, and SpiceDB tuples.
  • cmd/serve.go wires the existing serviceUserService into NewCascadeDeleter.
  • core/deleter package added to .mockery.yaml; core/deleter/mocks/service_user_service.go regenerated.
  • Test updates: newMocks returns the new mock; the full-cascade test asserts SU expectations; two new failure-path subtests cover SU list and SU delete error propagation.

Technical Details

Why between groups and roles in the cascade? The existing order is policies → projects → groups → SUs (new) → roles → invitations → billing → org. Service users are principals owned by the org (like groups), so they sit alongside groups in the cascade. Placing them after the bulk policy delete is fine: serviceUserService.Delete invokes RemoveOrganizationMember, which is best-effort and logs a warning if the policy is already gone.

No database FK added in this PR. A complementary serviceusers.org_id FK with ON DELETE RESTRICT is intentionally deferred. It needs a one-time backfill for any pre-existing orphaned rows; tracking that separately keeps this PR small and shippable.

No proto / API changes. This is a purely internal correctness fix in the cascade deleter.

Test Plan

  • go test ./core/deleter/... passes
  • Build verified: go build ./core/deleter/... ./cmd/...
  • New unit tests: cascade deletes SU; SU list error propagates; SU delete error propagates with the SU id in the message
  • Manual verification on a dev DB: create org + SU + credential, call DeleteOrganization, verify SU row, credential rows, and SpiceDB tuples are gone, and that the credential no longer authenticates

🤖 Generated with Claude Code

Service users belonging to a deleted organization were left orphaned in
the serviceusers table — their credentials remained valid and the auth
path returned a successful Principal for them after the parent org was
gone. Add a service user cleanup step in DeleteOrganization between
groups and roles; reuses serviceUserService.Delete which already removes
credentials, org-membership policies, and SpiceDB relations.

Refs: #1585

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 30, 2026

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

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview, Comment Apr 30, 2026 8:11am

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 462e9c56-6440-4a54-b9c7-3841d44275f6

📥 Commits

Reviewing files that changed from the base of the PR and between 629838d and 7e18337.

📒 Files selected for processing (5)
  • .mockery.yaml
  • cmd/serve.go
  • core/deleter/mocks/service_user_service.go
  • core/deleter/service.go
  • core/deleter/service_test.go

📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Service users are now automatically deleted as part of the organization cascade deletion process, ensuring comprehensive cleanup of all organization-related resources and dependencies.

Walkthrough

The pull request extends the cascade deleter service to handle service user deletion during organization cascade operations. It introduces ServiceUserService as a new dependency, wires it through the constructor, and integrates service user listing and deletion into the organization deletion flow before other cascade operations.

Changes

Cohort / File(s) Summary
Configuration
.mockery.yaml
Added mock generation configuration for the core/deleter package with all interfaces enabled, directing output to core/deleter/mocks.
Service Layer
core/deleter/service.go, core/deleter/mocks/service_user_service.go
Introduced ServiceUserService interface with List and Delete methods. Updated NewCascadeDeleter constructor to accept serviceUserService parameter. Enhanced DeleteOrganization to fetch and delete all service users before processing other cascade deletions. Auto-generated mock implementation for testing.
Dependency Wiring
cmd/serve.go
Updated buildAPIDependencies to pass serviceUserService to deleter.NewCascadeDeleter constructor call.
Test Updates
core/deleter/service_test.go
Extended test suite to wire ServiceUserService mock into all cascade deleter instantiations. Added tests verifying service user deletion during organization cascade delete and error propagation scenarios for List and Delete failures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • rohilsurana
  • whoAbhishekSah
🚥 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.

✏️ 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.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.

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

@AmanGIT07 AmanGIT07 enabled auto-merge (squash) April 30, 2026 08:17
@coveralls
Copy link
Copy Markdown

Coverage Report for CI Build 25154773359

Coverage increased (+0.01%) to 41.963%

Details

  • Coverage increased (+0.01%) from the base build.
  • Patch coverage: 2 uncovered changes across 1 file (20 of 22 lines covered, 90.91%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
cmd/serve.go 2 0 0.0%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 37254
Covered Lines: 15633
Line Coverage: 41.96%
Coverage Strength: 11.88 hits per line

💛 - Coveralls

@AmanGIT07 AmanGIT07 merged commit b95679a into main Apr 30, 2026
8 checks passed
@AmanGIT07 AmanGIT07 deleted the fix/deleter-serviceuser-cleanup branch April 30, 2026 08:23
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.

3 participants