refactor: centralize org member additions via membership package#1548
refactor: centralize org member additions via membership package#1548whoAbhishekSah merged 19 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a MembershipService abstraction and moves organization membership creation to it; removes legacy AddMember/AddUsers flows and related mocks/handlers; updates service constructors/wiring, mocks, handlers, error mappings, and tests to use AddOrganizationMember and membership-centered validation. 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 24706930225Coverage decreased (-0.1%) to 42.065%Details
Uncovered Changes
Coverage Regressions14 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
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
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
whoAbhishekSah
force-pushed
the
feat/remove-add-org-users-rpc
branch
from
dd9f650 to
96e4d3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/regression/api_test.go (1)
330-353:⚠️ Potential issue | 🟠 MajorKeep the disabled-org preference regression specific.
These tests now accept any create error, so they would pass if org creation returns
Internalafter partially persisting an ownerless disabled org. Assert the expected Connect code and the post-condition: either creation succeeds with a disabled org, or failure leaves no created org behind.Example post-condition if failure is intended
_, err = s.testBench.Client.CreateOrganization(ctxOrgUserAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ Body: &frontierv1beta1.OrganizationRequestBody{ Title: "org acme 5", Name: "org-acme-5", }, })) - s.Assert().Error(err) + s.Require().Error(err) + s.Assert().NotEqual(connect.CodeInternal, connect.CodeOf(err)) + + _, getErr := s.testBench.Client.GetOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetOrganizationRequest{ + Id: "org-acme-5", + })) + s.Require().Error(getErr) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(getErr))Adjust the expected code/state if the intended behavior is to create a disabled organization successfully.
Also applies to: 2473-2493
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1674e3c0-c366-4c87-8761-72a668578b12
📒 Files selected for processing (8)
core/organization/errors.gocore/organization/service.gointernal/api/v1beta1connect/domain.gointernal/api/v1beta1connect/invitations.gointernal/api/v1beta1connect/organization.gotest/e2e/regression/api_test.gotest/e2e/regression/onboarding_test.gotest/e2e/regression/serviceusers_test.go
✅ Files skipped from review due to trivial changes (1)
- core/organization/errors.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/regression/serviceusers_test.go
Manual RPC Test ReportRan thorough local RPC tests against this branch. All behavioral claims verified.
Minor observations (not blockers)
LGTM from a behavioral standpoint. The |
This RPC has been superseded by AddOrganizationMembers on the AdminService, which supports explicit role assignment per member. The server-side handler was already removed in raystack/frontier#1548. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…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>
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>
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>
…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>
…n handlers AcceptOrganizationInvitation and JoinOrganization handlers called orgService.Get before calling services that internally validate the org via membership.AddOrganizationMember. Removed the redundant call to save a DB round-trip. Added missing error cases for errors that can now bubble up from membership: user.ErrDisabled, organization.ErrDisabled, organization.ErrNotExist, membership.ErrInvalidOrgRole, membership.ErrAlreadyMember. Also fixed JoinOrganization to use errors.Is instead of == for domain.ErrDomainsMisMatch comparison. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eptOrganizationInvitation tests Handlers no longer call orgService.Get directly — org validation is done internally by the downstream services (domain.Join, invitation.Accept). Updated tests to route org-related errors through the downstream service mocks instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… setup When PlatformDisableOrgsOnCreate is true, the org was persisted as disabled before AddOrganizationMember ran, which rejects disabled orgs — leaving an ownerless row. Now creates as Enabled, completes membership + platform setup, then sets Disabled if the preference requires it. Also adds requireAddOrgMembersSuccess helper to e2e tests to assert per-member results from AddOrganizationMembers (not just the RPC error). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…led state With the create-enabled-then-disable fix, org creation succeeds when PlatformDisableOrgsOnCreate is true — the returned org is in disabled state. Updated both test cases to assert NoError + disabled state instead of expecting failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
whoAbhishekSah
force-pushed
the
feat/remove-add-org-users-rpc
branch
from
33efa91 to
708f25f
Compare
This RPC has been superseded by AddOrganizationMembers on the AdminService, which supports explicit role assignment per member. The server-side handler was already removed in raystack/frontier#1548. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This RPC has been superseded by AddOrganizationMembers on the AdminService, which supports explicit role assignment per member. The server-side handler was already removed in raystack/frontier#1548. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 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.Key changes
Bug fixes
viewer+ invite role). New flow creates 1 policy with the invite's role directly. If no role is specified, falls back toviewer.PlatformDisableOrgsOnCreateis true, the org is now created asEnabled, membership + platform setup completes, then it's set toDisabled. Previously, the disabled org was persisted first andAddOrganizationMemberrejected it.org.Createnow returnsErrUserPrincipalOnlyfor non-user principals.Optimizations
ListByUser(SpiceDBLookupResources) —AddOrganizationMemberalready checks membership internally and returnsErrAlreadyMember.policyServiceremoved from invitation service entirely. All policy+relation operations go through the membership package.Cleanup
orgService.Getremoved fromJoinOrganizationandAcceptOrganizationInvitationhandlers. Org validation now happens inside the downstream services (domain.Join,invitation.Accept→membership.AddOrganizationMember). Error cases (ErrDisabled,ErrNotExist) are still surfaced to the client viaerrors.Ismatching.AddOrganizationUsershandler + auth interceptor entry,org.AddUsers(),org.AddMember(),mapPrincipalTypeToAuditType(),AddMemberfrom domain/invitationOrgServiceinterfaces, rawpolicyService.Createloop in invitation accept.Behavioral changes
org.CreateAddMember("owner")— no validationmembership.AddOrganizationMember(owner)— validates user, role, org stateorg.AdminCreateinvitation.AcceptAddMember(viewer)+policyService.Create(inviteRole)= 2 policiesmembership.AddOrganizationMember(inviteRole)= 1 policydomain.JoinListByUser+AddMember(viewer)membership.AddOrganizationMember(viewer),ErrAlreadyMembertreated as successJoinOrganizationhandlerorgService.Get+domainService.JoindomainService.Joinonly (org validated internally)AcceptOrganizationInvitationhandlerorgService.Get+invitationService.AcceptinvitationService.Acceptonly (org validated internally)AddOrganizationMemberrejects → ownerless rowErrUserPrincipalOnlyTest updates
AddOrganizationUserscalls toAddOrganizationMembersrequireAddOrgMembersSuccesshelper to assert per-member results (not just RPC error)JoinOrganizationandAcceptOrganizationInvitation(removed staleorgService.Getexpectations)Related
🤖 Generated with Claude Code