feat: add RemoveOrganizationMember RPC and remove RemoveOrganizationUser#1550
feat: add RemoveOrganizationMember RPC and remove RemoveOrganizationUser#1550rohilsurana wants to merge 6 commits intomainfrom
Conversation
|
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 40 minutes and 3 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 (4)
📝 WalkthroughWalkthroughAdds membership removal at the membership-service layer (RemoveOrganizationMember), new ProjectService/GroupService deps and mocks, updates API handler/signature/tests/authorization, reorders service wiring in startup, adds an invalid-principal error, and bumps the Makefile PROTON_COMMIT. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 |
Coverage Report for CI Build 24725159879Coverage increased (+0.1%) to 42.205%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. 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 (4)
test/e2e/regression/api_test.go (1)
290-294: Strengthen this regression to assert cascade cleanup.This swap only proves the principal disappears from
ListUsers. The new RPC’s risky behavior is the cascade through project/group policies and relations, and that can regress while this test still passes. Add at least one post-removal access check against a project, group, or resource created under the org.internal/api/v1beta1connect/organization_test.go (1)
1009-1111: Consider adding a non-user principal test case.The PR objective highlights that
RemoveOrganizationMember"supports all principal types (not limited to users)", but every test case hardcodesschema.UserPrincipal. A table entry exercisingschema.ServiceUserPrincipalwould guard against regressions in principal-type plumbing from the handler through the service.core/membership/service.go (2)
225-242: Consider validatingprincipalTypeat the entry.Unlike
AddOrganizationMember/SetOrganizationMemberRole, there's novalidatePrincipal(or even a simple allowlist check) forprincipalType. An empty or unknown value silently falls through topolicyService.List, almost always returns zero rows, and surfaces asErrNotMember— indistinguishable from a legitimate "not a member" case. Rejecting unknown principal types up front (viaErrInvalidPrincipalor similar) would give callers clearer errors and prevent misleading audit entries for malformed inputs.
262-268: BroadpolicyService.Listquery for cross-org users.
policy.Filter{PrincipalID, PrincipalType}with noOrgIDreturns the principal's policies across every org they belong to; the subsequentswitchthen filters down to this org's resources. For users who are members of many orgs, this fetches significantly more rows than necessary.If
policy.Filtersupports it, scoping project/group lookups to the pre-computedorgProjectIDs/orgGroupIDs(or filtering per-resource-type withOrgID) would be tighter. Not a correctness issue, but worth considering for hot paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d8d6e4c-3662-440a-b319-6bc66d03f48d
⛔ Files ignored due to path filters (2)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go,!proto/**proto/v1beta1/frontierv1beta1connect/frontier.connect.gois excluded by!proto/**
📒 Files selected for processing (12)
Makefilecmd/serve.gocore/membership/mocks/group_service.gocore/membership/mocks/project_service.gocore/membership/service.gocore/membership/service_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/membership_service.gointernal/api/v1beta1connect/organization.gointernal/api/v1beta1connect/organization_test.gopkg/server/connect_interceptors/authorization.gotest/e2e/regression/api_test.go
Test Results (9/9 passed)
|
rohilsurana
force-pushed
the
feat/remove-organization-member
branch
from
fadaea3 to
14b5c34
Compare
rohilsurana
force-pushed
the
feat/remove-organization-member
branch
from
1b43105 to
2d70908
Compare
rohilsurana
force-pushed
the
feat/remove-organization-member
branch
from
2d70908 to
b0ac6f9
Compare
There was a problem hiding this comment.
Pull request overview
This PR replaces the legacy RemoveOrganizationUser RPC with a more general RemoveOrganizationMember RPC, and moves cascade-removal logic into the membership service to support additional principal types and consistent cleanup across org resources.
Changes:
- Introduces
RemoveOrganizationMemberRPC (and removesRemoveOrganizationUser) across proto/connect stubs, handlers, interceptors, and e2e coverage. - Adds membership-service cascade removal: deletes org/project/group policies and removes org/group relations; adds corresponding audit record.
- Updates wiring and tests to use the new RPC and new membership service constructor dependencies.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test/e2e/regression/api_test.go |
Updates regression coverage to call RemoveOrganizationMember with principal type. |
proto/v1beta1/frontierv1beta1connect/frontier.connect.go |
Regenerated connect client/handler stubs for the new RPC name and request/response types. |
pkg/server/connect_interceptors/authorization.go |
Updates authorization validation mapping for the new RPC (currently has a merge conflict). |
internal/api/v1beta1connect/organization.go |
Replaces handler implementation to delegate removal to membership service and map new errors. |
internal/api/v1beta1connect/organization_test.go |
Rewrites handler tests to mock membership service instead of org/user/deleter services. |
internal/api/v1beta1connect/mocks/membership_service.go |
Adds mock for RemoveOrganizationMember. |
internal/api/v1beta1connect/interfaces.go |
Extends MembershipService interface with RemoveOrganizationMember. |
core/membership/service.go |
Adds cascade member removal, relation cleanup helper, and audit record helper; adds project/group service deps. |
core/membership/service_test.go |
Updates constructors to pass new project/group service mocks. |
core/membership/mocks/project_service.go |
Adds generated mock for ProjectService used by membership service. |
core/membership/mocks/group_service.go |
Adds generated mock for GroupService used by membership service. |
core/membership/errors.go |
Adds ErrInvalidPrincipalType for new RPC validation. |
cmd/serve.go |
Wires project/group services into membership service constructor. |
Makefile |
Updates PROTON_COMMIT (commit differs from PR description). |
Comments suppressed due to low confidence (1)
pkg/server/connect_interceptors/authorization.go:368
- File contains unresolved git merge-conflict markers (<<<<<<<, =======, >>>>>>>) inside
authorizationValidationMap, which will not compile and may also drop/duplicate RPC validation entries (e.g., AddOrganizationUsers vs RemoveOrganizationMember). Resolve the conflict by removing the markers and keeping the correct authorization handlers (remove the old RemoveOrganizationUser entry, add/keep RemoveOrganizationMember, and ensure AddOrganizationUsers validation still exists).
"/raystack.frontier.v1beta1.FrontierService/RemoveOrganizationMember": func(ctx context.Context, handler *v1beta1connect.ConnectHandler, req connect.AnyRequest) error {
pbreq := req.(*connect.Request[frontierv1beta1.RemoveOrganizationMemberRequest])
return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.OrganizationNamespace, ID: pbreq.Msg.GetOrgId()}, schema.UpdatePermission, req)
},
"/raystack.frontier.v1beta1.FrontierService/SetOrganizationMemberRole": func(ctx context.Context, handler *v1beta1connect.ConnectHandler, req connect.AnyRequest) error {
pbreq := req.(*connect.Request[frontierv1beta1.SetOrganizationMemberRoleRequest])
return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.OrganizationNamespace, ID: pbreq.Msg.GetOrgId()}, schema.PolicyManagePermission, req)
},
"/raystack.frontier.v1beta1.FrontierService/ListOrganizationInvitations": func(ctx context.Context, handler *v1beta1connect.ConnectHandler, req connect.AnyRequest) error {
pbreq := req.(*connect.Request[frontierv1beta1.ListOrganizationInvitationsRequest])
return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.OrganizationNamespace, ID: pbreq.Msg.GetOrgId()}, schema.InvitationListPermission, req)
},
"/raystack.frontier.v1beta1.FrontierService/CreateOrganizationInvitation": func(ctx context.Context, handler *v1beta1connect.ConnectHandler, req connect.AnyRequest) error {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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: cce64d6a-cbeb-4c30-a7da-ec78e343f1d5
⛔ Files ignored due to path filters (2)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go,!proto/**proto/v1beta1/frontierv1beta1connect/frontier.connect.gois excluded by!proto/**
📒 Files selected for processing (13)
Makefilecmd/serve.gocore/membership/errors.gocore/membership/mocks/group_service.gocore/membership/mocks/project_service.gocore/membership/service.gocore/membership/service_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/membership_service.gointernal/api/v1beta1connect/organization.gointernal/api/v1beta1connect/organization_test.gopkg/server/connect_interceptors/authorization.gotest/e2e/regression/api_test.go
✅ Files skipped from review due to trivial changes (2)
- core/membership/errors.go
- core/membership/mocks/project_service.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/server/connect_interceptors/authorization.go
- internal/api/v1beta1connect/interfaces.go
- Makefile
- internal/api/v1beta1connect/organization.go
- test/e2e/regression/api_test.go
- internal/api/v1beta1connect/organization_test.go
Summary
RemoveOrganizationMemberRPC toFrontierService, replacing the oldRemoveOrganizationUserAddOrganizationMemberandSetOrganizationMemberRolepatternsChanges
PROTON_COMMITtoa855a70(addsRemoveOrganizationMember, removesRemoveOrganizationUser)core/membership/service.go: AddRemoveOrganizationMember,removeRelationshelper,principalTypeToAuditTypevalidator,auditOrgMemberRemoved, andProjectService/GroupServiceinterfacescore/membership/errors.go: AddErrInvalidPrincipalTypefor unknown principal typesRemoveOrganizationUserhandler withRemoveOrganizationMember, delegating entirely to membership service. Only reachable error branches retained.UpdatePermissionfor backward compatibility with existing org admin/manager roles.projectServiceandgroupServiceto membership service constructorCascade behavior
When a member is removed from an org:
principalTypeToAuditType(elseErrInvalidPrincipalType)ErrNotMember)ErrLastOwnerRole)owner+member) at org level and for each org groupProto PR: raystack/proton#475