feat: delete AddOrganizationUsers RPC and migrate all org.AddMember callers to membership#1548
feat: delete AddOrganizationUsers RPC and migrate all org.AddMember callers to membership#1548whoAbhishekSah wants to merge 13 commits intomainfrom
Conversation
…mbers AddOrganizationUsers had no SDK usage and was replaced by AddOrganizationMembers (AdminService) which takes explicit roles and returns per-member results. Removed: - AddOrganizationUsers handler - AddUsers from OrganizationService interface - org.AddUsers() service function - Auth interceptor entry - Handler tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ationMember org.Create and AdminCreate now call membership.AddOrganizationMember instead of org.AddMember to add the creator as owner. Membership dependency injected via SetMembershipService() setter to break circular init order. Behavioral change: new code validates user exists+enabled and role scope. Old code trusted the authn principal blindly. Non-user principals (serviceuser/PAT) are now rejected — org creation is user-only in practice. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
invitation.Accept now calls membershipSvc.AddOrganizationMember instead of orgSvc.AddMember to add user on invitation acceptance. Uses RoleOrganizationViewer (same as old MemberRole). No behavioral change in practice — caller already checks isUserOrgMember before calling, so ErrAlreadyMember won't trigger. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Domain auto-join now calls membershipSvc.AddOrganizationMember instead of orgSvc.AddMember. Uses RoleOrganizationViewer. Removed AddMember from domain's OrgService interface. No behavioral change in practice — caller already checks ListByUser for existing membership before calling. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 2 minutes and 25 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 (6)
📝 WalkthroughWalkthroughIntroduces a dedicated MembershipService and migrates organization membership creation to it. Removes legacy AddMember/AddUsers flows, updates service constructors and mocks, deletes the AddOrganizationUsers API handler and its auth entry, and updates tests to use the new MembershipService. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/organization/service.go (1)
89-112:⚠️ Potential issue | 🟠 MajorNil-pointer risk:
membershipServiceis not enforced before use.
NewServicedoesn't acceptmembershipService, and the field defaults tonil. Any caller that constructs the service viaNewServiceand then callsCreate(line 217) orAdminCreate(line 463) beforeSetMembershipService(...)is invoked will hit a nil-pointer dereference insideAddOrganizationMember.cmd/serve.gowires it correctly today, but tests, CLI tools, or future call sites can easily miss the setter — and the failure mode is a panic at request time rather than an init-time error.Consider one of the following hardenings:
Option A — Defensive check with clear error (minimal change)
func (s Service) Create(ctx context.Context, org Organization) (Organization, error) { + if s.membershipService == nil { + return Organization{}, fmt.Errorf("organization service: membership service not configured") + } principal, err := s.authnService.GetPrincipal(ctx)(and the analogous guard at the top of
AdminCreate).Option B — Return an error from the setter and validate wiring at startup
-func (s *Service) SetMembershipService(ms MembershipService) { - s.membershipService = ms +func (s *Service) SetMembershipService(ms MembershipService) error { + if ms == nil { + return errors.New("membership service is nil") + } + s.membershipService = ms + return nil }This at least surfaces misuse at the wiring boundary in
cmd/serve.go.Option A is the safer runtime guard; Option B catches misconfiguration earlier. Either is preferable to a silent panic.
🧹 Nitpick comments (2)
core/invitation/service_test.go (1)
73-75: Consider adding a test for the newAcceptmembership path.This PR rewires
Accept(...)to callmembershipSvc.AddOrganizationMember(...)withschema.RoleOrganizationViewer, but no test asserts that expectation. Adding aTestService_Acceptcase that mocksmembershipSvc.EXPECT().AddOrganizationMember(ctx, orgID, userID, schema.UserPrincipal, schema.RoleOrganizationViewer).Return(nil)would lock in the behavior and guard against regressions (e.g., role or principal type drift).core/domain/service.go (1)
32-34: Interface declaration — consider consolidating.The same
MembershipServiceinterface shape (AddOrganizationMember(ctx, orgID, principalID, principalType, roleID) error) is now declared independently incore/domain/service.go,core/organization/service.go, andcore/invitation/service.go. That's idiomatic Go (consumer-defined interfaces), but if the contract evolves (e.g., a bulk variant or additional parameters), you'll need to update each site. Fine to keep as-is; worth noting for future consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73cd3d8b-e244-4537-9b5e-2828bb72d5a8
📒 Files selected for processing (14)
cmd/serve.gocore/domain/service.gocore/invitation/mocks/membership_service.gocore/invitation/mocks/organization_service.gocore/invitation/service.gocore/invitation/service_test.gocore/organization/mocks/membership_service.gocore/organization/service.gocore/organization/service_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/organization_service.gointernal/api/v1beta1connect/organization.gointernal/api/v1beta1connect/organization_test.gopkg/server/connect_interceptors/authorization.go
💤 Files with no reviewable changes (7)
- pkg/server/connect_interceptors/authorization.go
- internal/api/v1beta1connect/interfaces.go
- core/organization/service_test.go
- internal/api/v1beta1connect/organization_test.go
- internal/api/v1beta1connect/organization.go
- internal/api/v1beta1connect/mocks/organization_service.go
- core/invitation/mocks/organization_service.go
Coverage Report for CI Build 24651258705Coverage decreased (-0.09%) to 41.718%Details
Uncovered Changes
Coverage Regressions13 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ationMemberRole Add comments at each call site explaining that Add is used because the user is not yet a member (org just created, or verified not a member by the caller). Set would fail with ErrNotMember. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AddOrganizationMember already checks if the user is a member and returns ErrAlreadyMember. No need for a separate ListByUser call before Add — just handle the error. Saves a SpiceDB LookupResources round-trip on the happy path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…om invitation Accept invitation now uses the first role from invite.RoleIDs (falls back to viewer if empty) via membership.AddOrganizationMember. This replaces the old two-step flow: hardcoded viewer AddMember + raw policyService.Create loop for invite roles. Also removes policyService dependency from invitation service entirely — one fewer cross-service dependency. If multiple role support is needed in the future, the comment at the role selection marks where to change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/organization/service.go (1)
108-112: Consider a nil guard onmembershipServiceto fail fast.
membershipServiceis injected via setter after construction, so any caller that forgets to wire it (a real risk as new bootstrap paths are added, or in tests that constructorganization.Servicedirectly) will hit a nil-pointer panic insideCreate/AdminCreateonly when the code path is exercised. A defensive check — either at call sites (lines 206, 453) or a validation inSetMembershipService/at the start ofCreate/AdminCreate— would turn a latent panic into a clear error. Not blocking; just a robustness nit given the intentional setter-injection design.core/invitation/service_test.go (1)
36-91: Add tests for the newAcceptmembership flow.The Coveralls report flags 2 of 4 changed lines in
core/invitation/service.goas uncovered, and the only test here exercises the "already member" short-circuit inCreate. Adding cases forAcceptthat verify: (a)membershipSvc.AddOrganizationMemberis called with the role frominvite.RoleIDs[0]whenconf.WithRoles=true, (b) it falls back toschema.RoleOrganizationViewerwhenWithRoles=falseorRoleIDsis empty, and (c)membership.ErrAlreadyMemberis swallowed and the acceptance continues to group add / audit / delete, would lock in the new behavior and recover coverage.core/invitation/service.go (1)
305-314:getConfigis re-evaluated at accept time — double-check this is intended.
orgRoleIDhere depends on the current value ofconf.WithRolesat accept time, butinvite.RoleIDswas only persisted whenWithRoleswas true at create time (see line 137-140 inCreate). The asymmetry has two quirks:
- If
WithRoleswas true at create and false at accept,invite.RoleIDsis populated but will be ignored — user silently gets viewer.- If
WithRoleswas false at create and true at accept,RoleIDsis empty anyway, so viewer is applied (fine).The first case is probably fine as platform-level policy, but if
WithRoleswas the intent at creation, honoring the storedRoleIDsunconditionally (or at least logging when they are dropped at accept) would be more predictable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17415b4e-af3b-4abb-b7bd-acf7770c20f2
📒 Files selected for processing (5)
cmd/serve.gocore/domain/service.gocore/invitation/service.gocore/invitation/service_test.gocore/organization/service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/domain/service.go
| userOb, userOrgMember, err := s.isUserOrgMember(ctx, invite.OrgID, invite.UserEmailID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Determine the org role to assign. | ||
| // Currently only the first role ID from the invitation is used. If multiple | ||
| // role support is needed in the future, this is the place to change — either | ||
| // loop over invite.RoleIDs calling SetOrganizationMemberRole for each, or | ||
| // extend the membership package to accept multiple roles. | ||
| orgRoleID := schema.RoleOrganizationViewer | ||
| conf := s.getConfig(ctx) | ||
| if conf.WithRoles && len(invite.RoleIDs) > 0 { | ||
| orgRoleID = invite.RoleIDs[0] | ||
| } | ||
|
|
||
| if !userOrgMember { | ||
| // if not, add user to the organization | ||
| if err = s.orgSvc.AddMember(ctx, invite.OrgID, schema.MemberRelationName, authenticate.Principal{ | ||
| ID: userOb.ID, | ||
| Type: schema.UserPrincipal, | ||
| }); err != nil { | ||
| // User is not yet a member — add with the invitation's role. | ||
| // ErrAlreadyMember is possible in a race (user added between invite creation | ||
| // and acceptance) — treat as success since the user is already in the org. | ||
| err = s.membershipSvc.AddOrganizationMember(ctx, invite.OrgID, userOb.ID, schema.UserPrincipal, orgRoleID) | ||
| if err != nil && !errors.Is(err, membership.ErrAlreadyMember) { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full Accept method to understand the complete flow
find . -name "service.go" -path "*/invitation/*" -type fRepository: raystack/frontier
Length of output: 89
🏁 Script executed:
# Get the full Accept method
head -400 core/invitation/service.go | tail -150Repository: raystack/frontier
Length of output: 4127
🏁 Script executed:
# Also check the broader context around line 300-324 and earlier
sed -n '250,330p' core/invitation/service.goRepository: raystack/frontier
Length of output: 2645
🏁 Script executed:
# Look for any role upgrade logic or SetOrganizationMemberRole calls in Accept
rg -A 5 -B 5 "SetOrganizationMemberRole|SetMemberRole" core/invitation/service.goRepository: raystack/frontier
Length of output: 545
🏁 Script executed:
# Check if there are tests for Accept behavior with existing members
rg -A 10 "Accept.*exist|already.*member|TestAccept" core/invitation/service_test.go | head -100Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Let's also check the git history to see what was removed
git log --oneline -20 core/invitation/service.goRepository: raystack/frontier
Length of output: 152
🏁 Script executed:
# Get the git diff for the commit that removed policy CRUD
git show 97a96952 -- core/invitation/service.go | head -300Repository: raystack/frontier
Length of output: 9998
🏁 Script executed:
# Also check if there are any tests that show the expected behavior for existing members accepting invites
rg -B 5 -A 15 "TestAccept.*existing|already.*member" core/invitation/service_test.goRepository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Let's search for tests related to Accept to understand the expected behavior
rg -B 3 -A 20 "func.*Accept" core/invitation/service_test.go | head -150Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check if there's any test for existing members accepting invites
rg -B 5 -A 15 "already.*member|existing.*member" core/invitation/service_test.goRepository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Let's also check the membership service to understand AddOrganizationMember behavior
rg -B 2 -A 10 "AddOrganizationMember" core/membership/service.go | head -50Repository: raystack/frontier
Length of output: 699
🏁 Script executed:
# Try to find the previous version of the Accept method before the refactor
# Look for any backup or see if there's another branch
git log --all --oneline -- core/invitation/service.go | head -10Repository: raystack/frontier
Length of output: 753
🏁 Script executed:
# Check the previous commit to see what Accept looked like
git show 97a96952^:core/invitation/service.go 2>/dev/null | sed -n '/func.*Accept/,/^func/p' | head -100Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check PR discussion or related comments
git log --format="%B" 97a96952Repository: raystack/frontier
Length of output: 679
🏁 Script executed:
# Try to get the previous version by checking 79323c02 (the commit before)
git show 79323c02:core/invitation/service.go 2>/dev/null | sed -n '/func.*Accept/,/^func/p' | head -100Repository: raystack/frontier
Length of output: 2765
🏁 Script executed:
# Let's look at the diff more carefully - check if core/invitation/service.go existed before
git log --diff-filter=A --name-only --pretty=format: -- core/invitation/service.go | head -1Repository: raystack/frontier
Length of output: 87
🏁 Script executed:
# Check git history to see if this file was renamed or created fresh
git log --oneline --follow -- core/invitation/service.go | tail -20Repository: raystack/frontier
Length of output: 152
Behavior change confirmed: invite roles are no longer applied to users who are already org members.
Previously, Accept created per-role org policies via a policyService.Create loop that ran regardless of existing membership status. Now, when userOrgMember == true, the role assignment block is entirely skipped. This is an intentional design change (per the refactoring that removed raw policy CRUD in favor of AddOrganizationMember).
The code comment acknowledges this and suggests the upgrade path for future multi-role support: loop over invite.RoleIDs calling SetOrganizationMemberRole (or equivalent) inside the role-assignment block. If accepting an invite should upgrade an existing member's role, implement that suggestion; otherwise, document explicitly that already-member invitations are role no-ops.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
org.Create now rejects non-user principals upfront. Service users should not create organizations — they are bound to an existing org at creation time. Updated e2e test to expect failure instead of success when a service user attempts to create an org. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
whoAbhishekSah
force-pushed
the
feat/remove-add-org-users-rpc
branch
from
f4cad42 to
3bd7b0e
Compare
whoAbhishekSah
force-pushed
the
feat/remove-add-org-users-rpc
branch
from
8113caa to
00dd71c
Compare
whoAbhishekSah
force-pushed
the
feat/remove-add-org-users-rpc
branch
from
00dd71c to
dd9f650
Compare
…embers All 14 e2e test calls (12 in api_test.go, 2 in onboarding_test.go) migrated from the deleted AddOrganizationUsers RPC to the new AddOrganizationMembers AdminService RPC with explicit viewer role. Role UUID looked up once in SetupSuite via ListRoles. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
whoAbhishekSah
force-pushed
the
feat/remove-add-org-users-rpc
branch
from
dd9f650 to
96e4d3b
Compare
2 participants
Summary
Deletes the
AddOrganizationUsersRPC (no SDK usage, replaced byAddOrganizationMembers) and migrates all callers oforg.AddMember/org.AddUsersto usemembership.AddOrganizationMember. Both functions are then deleted from the organization service.Wins
ListByUser(SpiceDB LookupResources) —AddOrganizationMemberalready checks membership internallypolicyServiceremoved from invitation service entirelyAddMembercheckedBehavioral audit
org.CreateAddMember("owner", principal)— no validationmembership.AddOrganizationMember(ownerRole)— validates user, role, already-memberorg.AdminCreateinvitation.AcceptAddMember(viewer)+policyService.Create(inviteRole)— 2 policiesmembership.AddOrganizationMember(inviteRole)— 1 policy with invited roledomain.ServiceListByUser+AddMember(viewer)membership.AddOrganizationMember(viewer)— handles ErrAlreadyMemberWhat's deleted
AddOrganizationUsershandler + auth interceptor + testsorg.AddUsers(),org.AddMember(),mapPrincipalTypeToAuditType()AddMemberfrom domain/invitationOrgServiceinterfacesListByUserin domain auto-joinpolicyServicedependency from invitation servicepolicyService.Createloop in invitation acceptRelated
🤖 Generated with Claude Code