Skip to content

fix(serviceuser): symmetric SpiceDB cleanup on delete#1630

Merged
whoAbhishekSah merged 1 commit into
mainfrom
fix/serviceuser-delete-identity-link-cleanup
May 20, 2026
Merged

fix(serviceuser): symmetric SpiceDB cleanup on delete#1630
whoAbhishekSah merged 1 commit into
mainfrom
fix/serviceuser-delete-identity-link-cleanup

Conversation

@whoAbhishekSah
Copy link
Copy Markdown
Member

Problem

serviceuser.Service.Delete leaks the app/serviceuser:<su>#org@app/organization:<org> identity-link tuple when the SU has no remaining org policies at delete time.

Chain:

  1. Delete calls membership.RemoveOrganizationMember best-effort.
  2. RemoveOrganizationMember returns ErrNotMember early when the SU has zero org policies — never reaches cascadeRemovePrincipal, where the identity-link cleanup lives.
  3. The follow-up relationService.Delete(Subject={SU}) only sweeps tuples where the SU is the Subject. The identity link has the SU as the Object — missed.

Result: tuple lives in SpiceDB forever, even though the SU row is gone.

Fix

Add a second relationService.Delete after the existing Subject-side one, keyed by Object on the SU. Tolerates relation.ErrNotExist for forward-compat. Same shape as #1624.

Tests

New core/serviceuser/service_test.go covers:

  • happy path (both sweeps fire)
  • membership failure swallowed; both sweeps still fire (the leak repro)
  • ErrNotExist from Object-side sweep is tolerated
  • non-ErrNotExist Object-side failure blocks repo delete
  • Subject-side failure short-circuits before Object sweep

Also added a core/serviceuser mockery entry and the generated mocks.

Closes #1629.

🤖 Generated with Claude Code

SU creation writes both Subject-side relations (e.g. platform sudo) and
Object-side relations (the serviceuser#org identity link). Delete only
swept the Subject side, so the identity-link tuple leaked whenever the
membership cascade was skipped — most commonly when the SU had no
remaining org policies at delete time, since RemoveOrganizationMember
returns ErrNotMember early and never reaches the cascade where that
cleanup lives.

Add an Object-side wildcard delete after the existing Subject-side one,
tolerating relation.ErrNotExist. Same shape as the policy.Delete fix.

Also add a mockery entry for core/serviceuser and seed unit coverage
for Service.Delete (happy path, membership-failure-swallowed path,
ErrNotExist tolerance, both failure short-circuit paths).

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

vercel Bot commented May 20, 2026

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

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview, Comment May 20, 2026 4:40am

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 20, 2026

Review Change Stack

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: b66ba24c-150f-417c-bd5e-f1b126c33362

📥 Commits

Reviewing files that changed from the base of the PR and between 3f550de and 0ecae7a.

📒 Files selected for processing (7)
  • .mockery.yaml
  • core/serviceuser/mocks/credential_repository.go
  • core/serviceuser/mocks/membership_service.go
  • core/serviceuser/mocks/relation_service.go
  • core/serviceuser/mocks/repository.go
  • core/serviceuser/service.go
  • core/serviceuser/service_test.go

📝 Walkthrough

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Enhanced service user deletion to gracefully handle missing relations and ensure comprehensive cleanup of all associated relationships and data.
  • Tests

    • Added comprehensive test coverage for service user deletion scenarios and error handling.

Walkthrough

This PR fixes a relation tuple leak in service user deletion by making the cleanup symmetric. It adds mockery configuration to generate test mocks, updates Service.Delete to clean relations where the service user is the object (not just subject), and provides comprehensive test coverage for the corrected behavior.

Changes

Service User Delete Cleanup

Layer / File(s) Summary
Mockery configuration and generated mocks
.mockery.yaml, core/serviceuser/mocks/credential_repository.go, core/serviceuser/mocks/membership_service.go, core/serviceuser/mocks/relation_service.go, core/serviceuser/mocks/repository.go
Mockery v2 is configured to generate mocks for the serviceuser package interfaces. Four mock types are autogenerated: CredentialRepository, MembershipService, RelationService, and Repository, each with expecter helpers and Run/Return/RunAndReturn call configuration support.
Service delete logic: symmetric relation cleanup
core/serviceuser/service.go
The Delete method adds an errors import, expands its cleanup comments, and changes the final relation deletion to target relations where the service user is the Object (the identity-link tuple case). The method now suppresses relation.ErrNotExist errors on the object-side deletion, allowing cleanup to continue to the repository delete even when no such relations exist.
Delete method test coverage
core/serviceuser/service_test.go
TestService_Delete validates the updated delete behavior using mocks. It covers error paths: membership removal failures are swallowed, ErrNotExist from the object-side relation deletion is tolerated, non-ErrNotExist object-side errors prevent the repository delete, and subject-side relation deletion errors short-circuit before the object-side sweep. A test helper newTestService constructs the service with mocked dependencies.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • raystack/frontier#1570: Related rework of Service.Delete that introduced membership-based removal and initial relation cleanup logic.

Suggested reviewers

  • rohilsurana
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed PR successfully implements the symmetric SpiceDB cleanup fix specified in issue #1629, adding Object-side relation deletion after Subject-side deletion with proper error handling.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the symmetric SpiceDB cleanup objective: mockery configuration, mock generation, service logic fix, and corresponding unit tests.

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

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

@coveralls
Copy link
Copy Markdown

Coverage Report for CI Build 26141719376

Coverage increased (+0.1%) to 42.554%

Details

  • Coverage increased (+0.1%) from the base build.
  • Patch coverage: 8 of 8 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 37945
Covered Lines: 16147
Line Coverage: 42.55%
Coverage Strength: 11.91 hits per line

💛 - Coveralls

@whoAbhishekSah
Copy link
Copy Markdown
Member Author

whoAbhishekSah commented May 20, 2026

Manual verification — direct SpiceDB tuple inspection

Tested against a local Frontier (PR1630 build) with SpiceDB on Postgres. For each case I snapshot the relation_tuple table before/after the operation and confirm the SU↔org identity-link tuple no longer leaks. Postgres rows for serviceusers / serviceuser_credentials / policies also verified.

# Scenario Result
1 Smoke — exact #1629 repro (create SU → create policy → delete policy → delete SU) pass — 0 tuples reference the SU after delete
2a DeleteServiceUser — bare SU (never had explicit policies) pass — 0 leaked tuples
2b DeleteServiceUser — SU has live org + project policies at delete time pass — 0 SU-side tuples; postgres policies rows for SU = 0
2c DeleteServiceUser — SU has both a key credential and an opaque token pass — credentials rows → 0, tuples → 0
3 DeleteOrganization cascade with 3 SUs in the org: bare / live-policy / policy-then-deleted (the leaky shape) pass — 0 leaked tuples for any of the SUs; 0 tuples referencing org or its project
4 Cross-resource — SU as principal on a project, then DeleteServiceUser partial — SU↔org identity link clean (PR1630's scope); see note
5 Idempotency — DeleteServiceUser twice, then on a non-existent UUID pass — second call returns not_found, no side effects
6 Regression — SU auth (Basic <tokenID>:<secret>) works while alive, hard-fails after delete pass

The SQL probe used at every checkpoint:

SELECT namespace, object_id, relation, userset_namespace, userset_object_id
FROM relation_tuple
WHERE deleted_xid = '9223372036854775807'::xid8
  AND (object_id = '<SU>' OR userset_object_id = '<SU>');

@whoAbhishekSah whoAbhishekSah merged commit b88b40e into main May 20, 2026
8 checks passed
@whoAbhishekSah whoAbhishekSah deleted the fix/serviceuser-delete-identity-link-cleanup branch May 20, 2026 05:41
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.

bug(serviceuser): SU→org identity-link relation tuple leaks on DeleteServiceUser when no org policies remain

3 participants