feat: implement RemoveProjectMember RPC#1504
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 17 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 ignored due to path filters (2)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a server-side RemoveProjectMember RPC and flow: service method, interface/mocks, handler, authorization entry, error handling, audit events; includes Makefile PROTON_COMMIT update and new sentinel error ErrNotMember. (33 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/project/service_test.go (1)
1247-1353: Good test coverage for the core RemoveMember functionality.The tests cover the main paths: project not found, principal not a member, successful deletion, and different principal types.
Consider adding tests for error propagation from
policyService.Listand partial failure scenarios wherepolicyService.Deletefails mid-iteration. These would help ensure error handling is robust, though they're not blocking.internal/api/v1beta1connect/project.go (1)
420-427: Consider validatingprincipalTypefor clearer error responses.Currently, an invalid
principalType(e.g., "invalid") will result in an empty policy list and returnErrNotMembermapped toCodeNotFound. While functionally acceptable, this could be confusing for API consumers.For consistency with
SetProjectMemberRole(which validates principal type and returnsErrInvalidPrincipalType→CodeInvalidArgument), consider adding similar validation to the service layer.♻️ Suggested service-layer validation
In
core/project/service.go, add validation at the start ofRemoveMember:func (s Service) RemoveMember(ctx context.Context, projectID, principalID, principalType string) error { + switch principalType { + case schema.UserPrincipal, schema.ServiceUserPrincipal, schema.GroupPrincipal: + // valid + default: + return ErrInvalidPrincipalType + } + _, err := s.Get(ctx, projectID)Then add error mapping in the handler:
switch { case errors.Is(err, project.ErrNotExist): return nil, connect.NewError(connect.CodeNotFound, ErrNotFound) case errors.Is(err, project.ErrNotMember): return nil, connect.NewError(connect.CodeNotFound, ErrNotFound) + case errors.Is(err, project.ErrInvalidPrincipalType): + return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) default:
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2bdc25d8-cf3e-405b-8cc9-62d7d73098fd
⛔ 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 (8)
Makefilecore/project/errors.gocore/project/service.gocore/project/service_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/project_service.gointernal/api/v1beta1connect/project.gopkg/server/connect_interceptors/authorization.go
Pull Request Test Coverage Report for Build 23936358393Details
💛 - Coveralls |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/v1beta1connect/project.go (1)
407-433: Consider adding audit logging for member removal.Other mutating project operations like
CreateProjectandUpdateProjectemit audit events. For compliance and operational visibility, consider adding an audit log entry when a member is removed.Example audit logging addition
func (h *ConnectHandler) RemoveProjectMember(ctx context.Context, request *connect.Request[frontierv1beta1.RemoveProjectMemberRequest]) (*connect.Response[frontierv1beta1.RemoveProjectMemberResponse], error) { errorLogger := NewErrorLogger() + auditor := audit.GetAuditor(ctx, "") projectID := request.Msg.GetProjectId() principalID := request.Msg.GetPrincipalId() principalType := request.Msg.GetPrincipalType() if err := h.projectService.RemoveMember(ctx, projectID, principalID, principalType); err != nil { // ... error handling ... } + auditor.Log(audit.ProjectMemberRemovedEvent, audit.ProjectTarget(projectID), + audit.WithMetadata(map[string]string{ + "principal_id": principalID, + "principal_type": principalType, + })) return connect.NewResponse(&frontierv1beta1.RemoveProjectMemberResponse{}), nil }Note: This would require defining
audit.ProjectMemberRemovedEventif it doesn't exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 259d821d-7444-4493-9a04-185dc1856806
📒 Files selected for processing (3)
core/project/service.gocore/project/service_test.gointernal/api/v1beta1connect/project.go
E2E Test Results — 33/33 PASSTested on clean database (fresh Frontier + SpiceDB). Happy Path — 7/7 PASS
Authorization — 7/7 PASS
Edge Cases — 7/7 PASS
Validation — 9/9 PASS
Org Admin Implicit Access — 3/3 PASS
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/v1beta1connect/project.go (1)
425-434: Consider using a handler-level error constant for consistency.Line 429 uses
project.ErrNotMemberdirectly while other cases use handler-level constants (ErrProjectNotFound,ErrBadRequest). This leaks the service layer error to the client response.For consistency with the rest of the handler and to maintain separation between service and API layers, consider defining a handler-level constant (e.g.,
ErrMemberNotFound).♻️ Suggested fix
Define a constant (likely in
errors.goor similar):var ErrMemberNotFound = errors.New("principal is not a member of the project")Then update the handler:
case errors.Is(err, project.ErrNotMember): - return nil, connect.NewError(connect.CodeNotFound, project.ErrNotMember) + return nil, connect.NewError(connect.CodeNotFound, ErrMemberNotFound)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ed71a76-0ac3-462c-b9e0-23ce78506b0e
📒 Files selected for processing (2)
core/audit/audit.gointernal/api/v1beta1connect/project.go
whoAbhishekSah
force-pushed
the
feat/remove-project-member
branch
from
7b3dfa9 to
d214d07
Compare
Add handler, service, and authorization for removing a principal from a project. Supports user, service user, and group principals. - Deletes all project-level policies for the principal - Validates principal_type (returns invalid_argument for unknown types) - Returns distinct errors for project not found vs member not found - Authorization checks update permission on the project - Audit logging for both SetProjectMemberRole and RemoveProjectMember - 6 unit tests covering validation and success paths Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
whoAbhishekSah
force-pushed
the
feat/remove-project-member
branch
from
d214d07 to
575d7d8
Compare
3 participants
Summary
RemoveProjectMemberRPC for removing a principal from a projectprincipal_id+principal_typeupdatepermission on the project (consistent withSetProjectMemberRole)SetProjectMemberRoleandRemoveProjectMemberCloses #1461
Proton PR: raystack/proton#465
Test plan
🤖 Generated with Claude Code