Skip to content

refactor: remove unused serviceuser-user relation from SpiceDB schema#1561

Merged
whoAbhishekSah merged 2 commits intomainfrom
chore/remove-serviceuser-user-relation
Apr 22, 2026
Merged

refactor: remove unused serviceuser-user relation from SpiceDB schema#1561
whoAbhishekSah merged 2 commits intomainfrom
chore/remove-serviceuser-user-relation

Conversation

@whoAbhishekSah
Copy link
Copy Markdown
Member

@whoAbhishekSah whoAbhishekSah commented Apr 21, 2026

Summary

  • Remove the user relation from app/serviceuser SpiceDB schema — it was never written (handler never sets CreatedByUser)
  • Simplify manage permission to just org->serviceusermanage
  • Remove the CreatedByUser transient field from the service user struct and the conditional relation creation code

Why this is safe to delete

  1. The relation was never written. The CreateServiceUser handler (serviceuser.go:145) constructs the ServiceUser struct without setting CreatedByUser. The conditional block in service.go:110 (if len(serviceUser.CreatedByUser) > 0) was therefore never entered — zero serviceuser#user@<creator> tuples exist in SpiceDB.

  2. The manage permission loses nothing. Before: permission manage = org->serviceusermanage + user. Since user was never populated, the effective permission was already just org->serviceusermanage. This change makes the schema match reality.

  3. Creator attribution is already captured. Two independent audit mechanisms record who created a service user:

    • Repository-level audit record (serviceuser_repository.go:125): BuildAuditRecord captures the authenticated actor (ID, type, name) from request context via enrichActorFromContext, stored in the audit_records table within the same transaction as the insert.
    • Handler-level audit log (serviceuser.go:163): audit.GetAuditor(ctx, orgID).LogWithAttrs(ServiceUserCreatedEvent, ...) emits a separate audit event with the acting user from context.

    Both paths derive the creator from the auth context — no struct field or SpiceDB relation needed.

Context

Part of the membership package migration (#1478). This cleans up dead code before migrating service user org membership to the core/membership package.

Test plan

  • Build passes
  • Service user handler tests pass
  • Verify no regressions in service user creation/deletion e2e

🤖 Generated with Claude Code

The `user` relation on `app/serviceuser` and the `CreatedByUser` field
were scaffolded but never wired up — the handler never populates the
creator, so the relation is never written. The audit record already
captures who created the service user via the actor context.

This simplifies the `manage` permission to just `org->serviceusermanage`.

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

vercel Bot commented Apr 21, 2026

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

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 21, 2026

Warning

Rate limit exceeded

@whoAbhishekSah has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 22 minutes and 28 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 28 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 43f1bcca-1222-4d87-8023-bfefbaed002f

📥 Commits

Reviewing files that changed from the base of the PR and between a295c77 and f9e4038.

📒 Files selected for processing (4)
  • core/serviceuser/service.go
  • core/serviceuser/serviceuser.go
  • internal/bootstrap/schema/base_schema.zed
  • internal/bootstrap/testdata/compiled_schema.zed

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.

Update the test golden file to match the removed `user` relation
and simplified `manage` permission on `app/serviceuser`.

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

Coverage Report for CI Build 24713216959

Coverage increased (+0.02%) to 42.085%

Details

  • Coverage increased (+0.02%) from the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 36749
Covered Lines: 15466
Line Coverage: 42.09%
Coverage Strength: 11.93 hits per line

💛 - Coveralls

@whoAbhishekSah
Copy link
Copy Markdown
Member Author

Manual end-to-end test report

Simulated the full SDK flow against this branch: create SU → project policy → token → authed calls. Also tested the delete path to check for regressions.

SDK flow

Step Action Result
1 CreateServiceUser(org_id, title) ✅ SU created. SpiceDB relations written: serviceuser:<id>#member@organization:<org> + back-ref organization:<org>#org@serviceuser:<id>. No serviceuser#user@... relation — confirms the PR claim that this was never written.
1b policies table for SU ✅ empty (no org-level role assigned — matches SDK behaviour)
2 CreatePolicyForProject with app_project_owner ✅ Policy row serviceuser:<id> → app_project_owner → project:<id> inserted.
3 CreateServiceUserToken ✅ Opaque token returned. serviceuser_credentials row with type=opaque_token.
4 GetOrganization authed as SU ✅ 200
4b GetCurrentUser authed as SU ✅ returns SU identity (id, title, org_id)
4c GetProject authed as SU ✅ 200 (project-level policy resolves)

Schema + permission (the focus of this PR)

Check Result
app/serviceuser definition post-change ✅ only relation org + permission manage = org->serviceusermanageuser relation removed cleanly
manage still gates correctly — org owner can ListServiceUserTokens / DeleteServiceUserToken ✅ 200 (alice, org owner via org->serviceusermanage)
Non-member denied manage ✅ 403 (eve, not a member of the org)

Delete flow

Check Result
DeleteServiceUser → 200
serviceusers row hard-deleted (not soft)
serviceuser_credentials cascaded
Deleted SU can no longer authenticate ✅ 401 on subsequent authed call
Idempotent re-delete ✅ 404 service user not found
serviceuser#member@organization cleaned
serviceuser#bearer@rolebinding cleaned (from project policy)
Back-ref organization#org@serviceuser ⚠️ leaks — survives delete
policies row for SU ⚠️ leaksapp_project_owner → project row remains for non-existent principal

Observations

1. Leak in delete flow — pre-existing, not introduced here. Reproduced on a fresh SU with no tokens/policies; the leaks still occur. Root cause in core/serviceuser/service.go:158:

s.relationService.Delete(ctx, relation.Relation{
    Subject: relation.Subject{ID: id, Namespace: schema.ServiceUserPrincipal},
})

Only deletes relations where the SU is the Subject. Relations where the SU is the Object (e.g. organization#org@serviceuser) and policies rows are untouched. Since this PR explicitly frames itself as prep for migrating service-user org membership to core/membership (#1478), that follow-up is the right place to fix the cleanup — unrelated to this schema change.

2. Minor auth-contract note. The SDK description in the test ask mentioned "bearer" auth, but opaque SU tokens actually authenticate via Authorization: Basic base64(<credential_id>:<secret>) (see core/authenticate/authenticators.go:199-226 — decodes, splits on :, passes clientID=credential UUID to GetBySecret). Bearer is only for the JWK/JWT flow via CreateServiceUserJWK. Raw opaque token + Bearer → 401. Not a PR issue — pre-existing.

Verdict

LGTM. The user relation was genuinely dead code and removing it doesn't change any observable behaviour. The manage permission continues to gate correctly, and no path in the create/authenticate/delete flows depends on the removed relation. Delete-flow leaks noted are pre-existing and orthogonal — best fixed as part of the upcoming core/membership migration.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes an unused user relation and related dead code from the ServiceUser authorization model, aligning the SpiceDB schema with how service users are actually created and audited in the codebase.

Changes:

  • Remove relation user: app/user from app/serviceuser in the SpiceDB schema.
  • Simplify app/serviceuser.manage permission to org->serviceusermanage only.
  • Remove the transient CreatedByUser field and the (previously unreachable) conditional relation creation logic.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
internal/bootstrap/testdata/compiled_schema.zed Updates compiled schema snapshot to drop serviceuser.user relation and simplify manage permission.
internal/bootstrap/schema/base_schema.zed Removes serviceuser.user relation and updates manage permission definition in the base schema.
core/serviceuser/serviceuser.go Removes unused transient CreatedByUser field from ServiceUser struct.
core/serviceuser/service.go Removes conditional creation of serviceuser#user@... relation during service user creation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@whoAbhishekSah whoAbhishekSah merged commit 79267cb into main Apr 22, 2026
12 checks passed
@whoAbhishekSah whoAbhishekSah deleted the chore/remove-serviceuser-user-relation branch April 22, 2026 08:02
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.

4 participants