fix(group): resolve PAT to user before owner wiring on group create#1658
fix(group): resolve PAT to user before owner wiring on group create#1658AmanGIT07 wants to merge 2 commits into
Conversation
CreateGroup writes the Postgres row before calling membership.OnGroupCreated. For PAT-authenticated principals the membership service rejects the app/pat principal type when assigning the group owner role, returning ErrInvalidPrincipalType. The error propagates back to the caller but the Postgres row is not rolled back, leaving an orphan with no SpiceDB hierarchy and no owner policy. Resolve the PAT to its underlying user via principal.ResolveSubject() before invoking OnGroupCreated, mirroring core/project/service.go. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughService.Create now resolves authenticated principals to their underlying subject via ChangesPrincipal resolution in group ownership
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f2545f0-e0c7-493b-8a5f-02d49ba1c962
📒 Files selected for processing (2)
core/group/service.gocore/group/service_test.go
Coverage Report for CI Build 26625095931Coverage increased (+0.005%) to 42.852%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Closes the orphan window for all OnGroupCreated failure modes, not just PAT. Errors are joined so the original membership failure is preserved when rollback itself fails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
CreateGroup writes the Postgres row before calling
membership.OnGroupCreated. For PAT-authenticated principals,OnGroupCreated→SetGroupMemberRole→validateGroupPrincipalrejects theapp/patprincipal type and returnsErrInvalidPrincipalType. The error propagates to the caller, butgroups.repository.Createis not rolled back, leaving an orphan row with no SpiceDB hierarchy and no owner policy. Resolve the PAT to its underlying user viaprincipal.ResolveSubject()before invokingOnGroupCreated, mirroringcore/project/service.go:Create.Changes
core/group/service.go:Create— callprincipal.ResolveSubject()and pass the resolvedsubjectID, subjectTypetoOnGroupCreated. PAT collapses to its underlying user; user/service-user principals pass through unchanged.core/group/service_test.go— addTestService_Createsubtest covering the PAT case; asserts the membership service receives(userID, app/user)rather than(patID, app/pat).Test Plan