feat(membership): accept PAT principals; add SetPATAllProjectsRole#1665
feat(membership): accept PAT principals; add SetPATAllProjectsRole#1665AmanGIT07 wants to merge 5 commits into
Conversation
AddOrganizationMember, SetOrganizationMemberRole, and RemoveProjectMember now accept app/pat principals — they skip the org#member/org#owner relation writes since the schema disallows app/pat there. New SetPATAllProjectsRole writes a pat_granted policy on the org for the all-projects cascade. UserPATService is wired post-construction to break the userpat <-> membership cycle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 12 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds UserPATService wiring and mock, validates PAT principals (org match and expiry), special-cases org/project membership flows for PATs (audit-only for PATs), implements SetPATAllProjectsRole and RemoveAllPATPolicies, and adds tests for PAT behaviors. Sequence Diagram(s)sequenceDiagram
participant MembershipService
participant UserPATService
participant PolicyService
participant AuditService
MembershipService->>UserPATService: GetByID(ctx, patID)
MembershipService->>PolicyService: List(org, patID) / Delete(matching pat_granted) / Create(pat_granted)
MembershipService->>AuditService: Audit(member/role change)
Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Coverage Report for CI Build 26831985435Coverage increased (+0.1%) to 43.184%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/membership/service_test.go (1)
1251-1266: ⚡ Quick winCover the plain org-membership policy collision here.
SetPATAllProjectsRolelists PAT policies by(orgID, principalID, principalType)and filterspat_grantedin Go, whileAddOrganizationMemberalso creates an org-scoped PAT policy with an emptyGrantRelation. This test only proves we ignoregranted; it won't catch a regression where a normal PAT org policy is mistaken for the all-projects grant and gets deleted or causes a false no-op. Please add a case with both a regular PAT org policy (GrantRelation == "") and apat_grantedpolicy in the returned list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43e5a05c-1ea2-4153-aa6a-d6e6d4424454
📒 Files selected for processing (5)
cmd/serve.gocore/membership/errors.gocore/membership/mocks/user_pat_service.gocore/membership/service.gocore/membership/service_test.go
- SetOrganizationMemberRole skips ErrNotMember for PAT so it acts as an upsert (PAT may not have a prior org policy). - Gate validateMinOwnerConstraint to UserPrincipal in both SetOrganizationMemberRole and RemoveOrganizationMember — only humans can be the last owner. - validateOrgMembership PAT branch returns ErrInvalidPrincipal (matching validatePrincipal) when userPATService is not wired. - ListPrincipalsByResource filters out pat_granted policies for org listings via new excludePATAllProjects helper — that grant lives on the org but means project access, not org membership. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PATs are single-org and hold no SpiceDB owner/member relations, so their teardown reduces to listing-and-deleting their policies. Adds: - RemoveAllPATPolicies(ctx, patID) — public method for PAT teardown - removePoliciesByFilter(ctx, filter) — private list-and-delete primitive Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…granted Verifies that when a PAT has both a granted org policy and a pat_granted all-projects policy, SetPATAllProjectsRole only replaces the pat_granted one — the role-match no-op check operates on pat_granted policies only, and the granted policy is not deleted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Membership package now accepts
app/patprincipals across org/project member methods. AddsSetPATAllProjectsRolefor thepat_granted(all-projects) write path.Changes
validatePrincipalandvalidateOrgMembershipacceptapp/pat— resolve via newUserPATServiceinterface, checkpat.OrgID == orgIDandpat.ExpiresAt.After(time.Now())AddOrganizationMember/SetOrganizationMemberRoleskiporg#member/org#ownerrelation writes for PAT (schema disallowsapp/patthere)RemoveProjectMemberallowlist now includes PATSetPATAllProjectsRole(orgID, patID, roleID)writes onepat_grantedpolicy on the org; idempotent; replaces prior all-projects roleSetUserPATServicesetter wired incmd/serve.goErrPrincipalExpiredTechnical Details
pat_grantedfilter applied in Go afterpolicyService.List—policy.Filterhas noGrantRelationfieldUserPATServiceunset →validatePrincipalreturnsErrInvalidPrincipalfor PATTest Plan
TestService_AddOrganizationMember_PAT,TestService_SetOrganizationMemberRole_PAT,TestService_SetPATAllProjectsRole, PAT row inTestService_RemoveProjectMembergo test ./core/membership/... ./core/userpat/... ./internal/api/v1beta1connect/...passesgo build ./...passes