feat: move SetOrganizationMemberRole to membership package#1541
feat: move SetOrganizationMemberRole to membership package#1541whoAbhishekSah wants to merge 10 commits intomainfrom
Conversation
Moves SetMemberRole logic from organization service into the membership package as SetOrganizationMemberRole, with relation cleanup added (fixes the known leak where demoting owner to viewer left the org#owner relation in place). - New: membership.SetOrganizationMemberRole with validateMinOwnerConstraint, replacePolicy, replaceRelation - Removed from organization service: SetMemberRole, validateSetMemberRoleRequest, getUserOrgPolicies, validateMinOwnerConstraint, replaceUserOrgPolicies - Handler rewired from orgService.SetMemberRole to membershipService.SetOrganizationMemberRole - Added 6 unit tests covering skip-unchanged, last-owner constraint, owner->viewer demotion (with relation swap), viewer->owner promotion Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
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 44 minutes and 49 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR moves organization member role-update logic from core/organization to core/membership by adding Service.SetOrganizationMemberRole(principalType-aware), removing the old organization method, updating interfaces and mocks, and rewiring the API handler and tests to call the membership service. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 |
Coverage Report for CI Build 24493841489Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.03%) to 41.698%Details
Uncovered Changes
Coverage Regressions46 previously-covered lines in 7 files lost coverage.
Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
core/membership/service_test.go (1)
276-428: Add a regression for relation-replacement failures.This suite only covers the success path for deleting old relations and creating the new one. A case where
RelationService.DeleteorCreatefails duringSetOrganizationMemberRolewould lock in the owner-demotion fix and catch the partial-update paths in this new flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7017a580-65f7-4314-aa5d-0fccb1f9afc1
📒 Files selected for processing (9)
core/membership/service.gocore/membership/service_test.gocore/organization/service.gocore/organization/service_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/membership_service.gointernal/api/v1beta1connect/mocks/organization_service.gointernal/api/v1beta1connect/organization.gointernal/api/v1beta1connect/organization_test.go
💤 Files with no reviewable changes (3)
- core/organization/service_test.go
- core/organization/service.go
- internal/api/v1beta1connect/mocks/organization_service.go
Silently ignoring all relation delete errors would mask a failed owner relation delete during demotion, leaving both owner and member relations in place. Now only relation.ErrNotExist is ignored. Added 2 tests: real error fails the operation, not-found is ignored. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
core/membership/service.go (2)
185-188:⚠️ Potential issue | 🔴 CriticalRetry can silently skip relation repair after a partial failure.
If Line 194 succeeds and Line 199 fails, a retry hits the no-op at Line 186 and returns early, leaving policy/relation inconsistent.
Suggested fix
- // skip if the user already has exactly this role - if len(existing) == 1 && existing[0].RoleID == roleID { - return nil - } + // keep policy no-op optimization, but still repair relation state + if len(existing) == 1 && existing[0].RoleID == roleID { + relationName := orgRoleToRelation(fetchedRole) + return s.replaceRelation(ctx, orgID, schema.OrganizationNamespace, principalID, principalType, relationName) + }Also applies to: 194-200
203-204:⚠️ Potential issue | 🟠 MajorAudit record write failures are being dropped in the role-change path.
Line 344 ignores
AuditRecordRepository.Createerrors, so callers get success even when audit persistence fails.Suggested fix
- s.auditOrgMemberRoleChanged(ctx, org, usr, roleID) - return nil + if err := s.auditOrgMemberRoleChanged(ctx, org, usr, roleID); err != nil { + return err + } + return nil } -func (s *Service) auditOrgMemberRoleChanged(ctx context.Context, org organization.Organization, usr user.User, roleID string) { - s.auditRecordRepository.Create(ctx, auditrecord.AuditRecord{ +func (s *Service) auditOrgMemberRoleChanged(ctx context.Context, org organization.Organization, usr user.User, roleID string) error { + if _, err := s.auditRecordRepository.Create(ctx, auditrecord.AuditRecord{ Event: pkgAuditRecord.OrganizationMemberRoleChangedEvent, Resource: auditrecord.Resource{ ID: org.ID, @@ - OccurredAt: time.Now(), - }) + OccurredAt: time.Now(), + }); err != nil { + return fmt.Errorf("create audit record: %w", err) + } @@ audit.GetAuditor(ctx, org.ID).LogWithAttrs(audit.OrgMemberRoleChangedEvent, audit.Target{ ID: usr.ID, Type: schema.UserPrincipal, }, map[string]string{ "role_id": roleID, }) + return nil }Based on learnings: In
core/organization/service.go(SetMemberRolemethod), returning an audit error after the business operation has already succeeded is an intentional design choice.Also applies to: 343-363
🧹 Nitpick comments (1)
core/membership/service.go (1)
258-260: Comment text inreplaceRelationis outdated.Line 260 says delete errors are ignored, but the implementation only ignores
relation.ErrNotExistand fails on other errors.Suggested fix
-// on the resource, then creates a new relation with the given name. Delete errors -// are ignored because the relation may not exist. +// on the resource, then creates a new relation with the given name. Delete errors +// are ignored only when they are relation.ErrNotExist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2245ba2d-333b-413b-aa99-ccb4bf25fa8a
📒 Files selected for processing (2)
core/membership/service.gocore/membership/service_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/membership/service_test.go
If replaceRelation fails after replacePolicy succeeds, the policy now has the new role. A retry would hit the early-return (same role) and never repair the relation. To prevent this, rollback the policy to the old role so the next retry sees the old state and redoes both. If rollback also fails, log at error level with all context (org_id, principal_id, old/new role, both errors) since the membership state is now inconsistent and needs manual intervention. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
If replaceRelation fails after replacePolicy succeeds, log at error level with full context (org_id, principal_id, role_id, expected relation, error) for manual investigation. Removes the rollback complexity which was getting unwieldy without solving the full cross-store consistency problem. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
If replacePolicy deletes the existing policies but fails to create the new one, the user has a relation with no policy. Log at error level with full context for manual investigation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eleted If replaceRelation deletes existing relations but fails to create the new one, the user has a policy with no relation. Log at error level with full context for manual investigation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…delete Caller passes which relations to clean up instead of hardcoding owner/member. Org passes [owner, member], project will skip the call entirely, group can pass its own set. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
Moves the existing
SetOrganizationMemberRoleRPC implementation fromcore/organizationintocore/membership. Also fixes the known leak where demoting an org owner to viewer left theorg#ownerdirect relation in place.What changes
New in
core/membership/SetOrganizationMemberRole(ctx, orgID, principalID, principalType, roleID)— handles the full role-change flowvalidateMinOwnerConstraint,replacePolicy,replaceRelationRemoved from
core/organization/SetMemberRolevalidateSetMemberRoleRequestgetUserOrgPoliciesvalidateMinOwnerConstraintreplaceUserOrgPoliciesHandler
SetOrganizationMemberRoleRPC handler now callsmembershipService.SetOrganizationMemberRolewithschema.UserPrincipalmembership.ErrNotMember,membership.ErrInvalidOrgRole,membership.ErrLastOwnerRole)Bug fix — relation cleanup
Previously,
org.SetMemberRolereplaced policies but never touched the explicitorg#ownerororg#memberrelations. So demoting an owner to viewer left theownerrelation in place, continuing to grant owner permissions via SpiceDB.Now
replaceRelationdeletes bothownerandmemberrelations for the principal before creating the new one matching the target role. Verified by theshould succeed demoting owner to viewer with multiple ownerstest.Edge cases handled
app/user(serviceusers bound at creation, not changed via this RPC)ErrNotMemberTest plan
core/membership/covering all edge cases aboveRelated
🤖 Generated with Claude Code