feat: add RemoveOrganizationMember RPC and remove RemoveOrganizationUser#1550
feat: add RemoveOrganizationMember RPC and remove RemoveOrganizationUser#1550rohilsurana wants to merge 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR renames the RemoveOrganizationUser operation to RemoveOrganizationMember and refactors its implementation to use a centralized membership service instead of user-deletion logic. It updates the Proton commit, adds ProjectService and GroupService dependencies to the membership service, and creates corresponding mocks for testing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 24562341248Coverage decreased (-0.1%) to 41.69%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
| func (s *Service) auditOrgMemberRemoved(ctx context.Context, org organization.Organization, principalID, principalType string) { | ||
| s.auditRecordRepository.Create(ctx, auditrecord.AuditRecord{ | ||
| Event: pkgAuditRecord.OrganizationMemberRemovedEvent, | ||
| Resource: auditrecord.Resource{ | ||
| ID: org.ID, | ||
| Type: pkgAuditRecord.OrganizationType, | ||
| Name: org.Title, | ||
| }, | ||
| Target: &auditrecord.Target{ | ||
| ID: principalID, | ||
| Type: pkgAuditRecord.UserType, | ||
| }, | ||
| OrgID: org.ID, | ||
| OccurredAt: time.Now(), | ||
| }) | ||
|
|
||
| audit.GetAuditor(ctx, org.ID).Log(audit.OrgMemberDeletedEvent, audit.Target{ | ||
| ID: principalID, | ||
| Type: principalType, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '^\s*[A-Za-z][A-Za-z0-9]*\s*=\s*"(user|service_?user|app|principal)"' --type=go -g 'pkg/auditrecord/**'
rg -nP '\bType\b\s*=\s*"' --type=go -g 'pkg/auditrecord/**' -C1Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Find audit record related files
find . -name "*.go" -type f | xargs grep -l "AuditRecord" | head -20Repository: raystack/frontier
Length of output: 769
🏁 Script executed:
# Search for audit type constants (different patterns)
rg -i "const.*type.*=" --type=go -g "*.go" | grep -i "audit\|user\|service" | head -30Repository: raystack/frontier
Length of output: 401
🏁 Script executed:
# Look for schema principal type definitions
rg -n "UserPrincipal\|ServiceUserPrincipal" --type=go -g "*.go" | head -20Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Search for existing audit type assignments
rg -n "Target.*Type.*:" --type=go -A2 -B2 | head -50Repository: raystack/frontier
Length of output: 1557
🏁 Script executed:
cat -n core/auditrecord/auditrecord.go | head -150Repository: raystack/frontier
Length of output: 581
🏁 Script executed:
# Search for UserType and other type constants
rg -n "const.*Type\s*=" --type=go core/auditrecord/Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Find principal type constants
rg -n "UserPrincipal\|ServiceUserPrincipal" --type=go | head -20Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check the membership service RemoveOrganizationMember method
sed -n '225,311p' core/membership/service.goRepository: raystack/frontier
Length of output: 2840
🏁 Script executed:
# Check how auditOrgMemberAdded and auditOrgMemberRoleChanged handle types for comparison
sed -n '600,700p' core/membership/service.goRepository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Find the imports in membership/service.go to locate pkgAuditRecord
head -50 core/membership/service.go | grep -E "import|auditrecord|audit"Repository: raystack/frontier
Length of output: 224
🏁 Script executed:
# Read the models file to see Target structure
cat -n core/auditrecord/models/models.goRepository: raystack/frontier
Length of output: 3556
🏁 Script executed:
# Search for type constant definitions (trying different patterns)
rg -n 'const.*=.*"(user|service|app)' --type=go | grep -i audit | head -20Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Find where audit types are defined
find . -name "*.go" -type f | xargs grep -l "OrganizationMemberRemovedEvent\|UserType" | head -10Repository: raystack/frontier
Length of output: 277
🏁 Script executed:
cat -n pkg/auditrecord/consts.goRepository: raystack/frontier
Length of output: 4248
🏁 Script executed:
# Also check the auditOrgMemberRemoved function in context
sed -n '575,610p' core/membership/service.goRepository: raystack/frontier
Length of output: 698
🏁 Script executed:
# Check auditOrgMemberAdded for comparison
rg -n "func.*auditOrgMemberAdded" core/membership/service.go -A 25Repository: raystack/frontier
Length of output: 920
🏁 Script executed:
# Verify schema principal type constants
rg -n "ServiceUserPrincipal\|UserPrincipal" --type=go pkg/schema/ | head -20Repository: raystack/frontier
Length of output: 112
🏁 Script executed:
# Check the schema package structure
find . -path ./pkg/schema -name "*.go" -type f | head -5Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Read schema.go or similar to find principal type constants
rg -n "const.*Principal" --type=go | grep -E "schema|principal" | head -20Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Find where schema is imported
rg "schema\." --type=go core/membership/service.go | head -20Repository: raystack/frontier
Length of output: 1400
🏁 Script executed:
# Search for schema package location
find . -name "*.go" -type f | xargs grep -l "const.*ServiceUserPrincipal" | head -5Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Broader search for ServiceUserPrincipal
rg "ServiceUserPrincipal" --type=go | head -20Repository: raystack/frontier
Length of output: 3651
🏁 Script executed:
# Check RemoveOrganizationMember for schema usage
rg -n "schema\." core/membership/service.go | head -20Repository: raystack/frontier
Length of output: 1476
🏁 Script executed:
# Find schema constant definitions
find . -name "*.go" -type f | xargs grep -l "const.*UserPrincipal.*=" | head -5Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check what principalType values are actually used in the codebase
rg -n 'PrincipalType.*=' --type=go | grep -v "^test/" | head -30Repository: raystack/frontier
Length of output: 1823
🏁 Script executed:
# Verify the validatePrincipal method to understand supported types
rg -n "func.*validatePrincipal" core/membership/service.go -A 20Repository: raystack/frontier
Length of output: 798
🏁 Script executed:
# Verify schema constants for UserPrincipal and ServiceUserPrincipal
find . -name "*.go" -path "*/pkg/schema/*" -o -path "*/pkg/*schema*" -name "*.go" | head -10Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Search for schema package definition more broadly
rg -n "const.*UserPrincipal\s*=" --type=go | head -20Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check how principalType is used elsewhere for audit purposes
rg -n "audit.*principalType\|principalType.*audit" --type=go -B 2 -A 2 | head -40Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Find where schema is defined by looking at imports
rg -n 'import.*".*schema' --type=go core/membership/service.goRepository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Search in the entire repo for schema constant definitions
rg "const \(" --type=go -A 50 | grep -E "UserPrincipal|ServiceUserPrincipal" | head -20Repository: raystack/frontier
Length of output: 264
🏁 Script executed:
# Look for the schema package in any location
find . -type f -name "*.go" | xargs grep -l 'package schema' | head -5Repository: raystack/frontier
Length of output: 141
🏁 Script executed:
# Check pkg directory structure
ls -la pkg/ | head -30Repository: raystack/frontier
Length of output: 1073
🏁 Script executed:
cat -n internal/bootstrap/schema/schema.go | head -200Repository: raystack/frontier
Length of output: 7332
🏁 Script executed:
# Also check what constants are defined in that file
rg -n "const.*Principal" internal/bootstrap/schema/schema.goRepository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check if service users are actually removed in any tests or code
rg "RemoveOrganizationMember.*ServiceUserPrincipal\|ServiceUserPrincipal.*RemoveOrganizationMember" --type=goRepository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check the broader pattern - does anything actually call RemoveOrganizationMember with non-user principals
rg -n "RemoveOrganizationMember" --type=go | grep -v "func\|interface\|mock" | head -20Repository: raystack/frontier
Length of output: 2561
🏁 Script executed:
# Find the API handler for RemoveOrganizationMember
rg -n "func.*RemoveOrganizationMember" --type=go internal/api/ | head -10Repository: raystack/frontier
Length of output: 1594
🏁 Script executed:
# Check the handler implementation
rg -n "RemoveOrganizationMember" internal/api/v1beta1connect/organization.go -A 10 | head -40Repository: raystack/frontier
Length of output: 1745
Audit record hardcodes UserType despite method accepting any principal type.
RemoveOrganizationMember (lines 225–311) accepts any principalType without validation and passes it to auditOrgMemberRemoved. However, auditOrgMemberRemoved unconditionally writes Target.Type: pkgAuditRecord.UserType to the audit repository, while correctly passing principalType to audit.GetAuditor(...).Log(). Removing a service user (or other principals) will produce an incorrect audit record.
This differs from auditOrgMemberAdded and auditOrgMemberRoleChanged, which safely hardcode UserType because their callers enforce validatePrincipal (which only accepts users currently). Since RemoveOrganizationMember has no such guard, the hardcoded type must be dynamic.
Map principalType to the appropriate pkgAuditRecord constant (e.g., UserType for schema.UserPrincipal, ServiceUserType for schema.ServiceUserPrincipal).
| case errors.Is(err, user.ErrNotExist): | ||
| return nil, connect.NewError(connect.CodeNotFound, ErrUserNotExist) | ||
| case errors.Is(err, user.ErrDisabled): | ||
| return nil, connect.NewError(connect.CodeFailedPrecondition, err) |
There was a problem hiding this comment.
Unreachable error branches: user.ErrNotExist / user.ErrDisabled.
membership.Service.RemoveOrganizationMember (see core/membership/service.go lines 225-311) never invokes validatePrincipal and never returns user.ErrNotExist or user.ErrDisabled — it only surfaces orgService.Get errors, ErrNotMember, ErrLastOwnerRole, and wrapped policy/relation errors. These two case arms are dead code and create a misleading impression that the handler validates principal existence.
Either drop them, or (preferable if you want symmetry with AddOrganizationMember) have the service validate the principal before cascading removal.
🧹 Proposed cleanup
case errors.Is(err, organization.ErrNotExist):
return nil, connect.NewError(connect.CodeNotFound, ErrNotFound)
- case errors.Is(err, user.ErrNotExist):
- return nil, connect.NewError(connect.CodeNotFound, ErrUserNotExist)
- case errors.Is(err, user.ErrDisabled):
- return nil, connect.NewError(connect.CodeFailedPrecondition, err)
case errors.Is(err, membership.ErrNotMember):
return nil, connect.NewError(connect.CodeFailedPrecondition, ErrNotMember)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case errors.Is(err, user.ErrNotExist): | |
| return nil, connect.NewError(connect.CodeNotFound, ErrUserNotExist) | |
| case errors.Is(err, user.ErrDisabled): | |
| return nil, connect.NewError(connect.CodeFailedPrecondition, err) | |
| case errors.Is(err, organization.ErrNotExist): | |
| return nil, connect.NewError(connect.CodeNotFound, ErrNotFound) | |
| case errors.Is(err, membership.ErrNotMember): | |
| return nil, connect.NewError(connect.CodeFailedPrecondition, ErrNotMember) |
| "/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) |
There was a problem hiding this comment.
Tighten the permission gate for member removal.
RemoveOrganizationMember now cascades through org/project/group policies, but the interceptor still only requires schema.UpdatePermission. That lets callers who can update an org indirectly delete policies they couldn't manage directly. This should be guarded by the same policy-management permission as the operations it now performs, or the implementation should be split so the lower-privilege path cannot remove scoped policies.
🔐 Proposed fix
"/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)
+ return handler.IsAuthorized(ctx, relation.Object{Namespace: schema.OrganizationNamespace, ID: pbreq.Msg.GetOrgId()}, schema.PolicyManagePermission, req)
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "/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/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.PolicyManagePermission, req) | |
| }, |
2 participants
Summary
RemoveOrganizationMemberRPC toFrontierService, replacing the oldRemoveOrganizationUserAddOrganizationMemberandSetOrganizationMemberRolepatternsChanges
PROTON_COMMITtob9da8ed(addsRemoveOrganizationMember, removesRemoveOrganizationUser)core/membership/service.go: AddRemoveOrganizationMember,removeRelationshelper,auditOrgMemberRemoved, andProjectService/GroupServiceinterfacesRemoveOrganizationUserhandler withRemoveOrganizationMember, delegating entirely to membership serviceprojectServiceandgroupServiceto membership service constructorCascade behavior
When a member is removed from an org:
ErrNotMember)ErrLastOwnerRole)owner+member) at org level and for each org groupProto PR: raystack/proton#475