refactor: move project member mutations into membership package#1557
refactor: move project member mutations into membership package#1557whoAbhishekSah merged 5 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 57 minutes and 37 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 (13)
📝 WalkthroughWalkthroughRefactors project-scoped member management functionality from ProjectService to MembershipService. MembershipService gains two new methods: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 24770992888Coverage increased (+0.1%) to 42.369%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 (2)
core/membership/service.go (2)
57-59: Naming:ServiceuserService→ServiceUserService.The codebase consistently uses
ServiceUsercamel-casing (e.g.,schema.ServiceUserPrincipal,serviceuser.ServiceUser, and the existingServiceUserServiceinterface atinternal/api/v1beta1connect/interfaces.go:264). The lowercaseuinServiceuserhere (and theserviceuserServicefield/parameter) breaks that convention.🧹 Proposed rename
-type ServiceuserService interface { +type ServiceUserService interface { Get(ctx context.Context, id string) (serviceuser.ServiceUser, error) }And the corresponding struct field, constructor parameter, and mock file name should be renamed accordingly.
520-560:validateOrgMembershipdoes not check principalDisabled/State.
validatePrincipal(line 362) rejects disabled users withuser.ErrDisabled. The newvalidateOrgMembershiponly fetches the user/service-user/group and checks org ID or org policy presence, so a disabled user, disabled service user, or disabled group could still have their project role mutated viaSetProjectMemberRole. If the intent is "must be an active org member", consider mirroring the disabled-state checks fromvalidatePrincipalacross all three branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed5abf41-c440-4e4c-8045-cdca5d548d90
📒 Files selected for processing (13)
cmd/serve.gocore/membership/errors.gocore/membership/mocks/group_service.gocore/membership/mocks/project_service.gocore/membership/mocks/serviceuser_service.gocore/membership/service.gocore/membership/service_test.gocore/project/mocks/role_service.gocore/project/service.gocore/project/service_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/membership_service.gointernal/api/v1beta1connect/project.go
💤 Files with no reviewable changes (1)
- core/project/mocks/role_service.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
core/membership/service.go (1)
78-102: Constructor parameter list is growing — consider grouping into a dependencies struct.
NewServicenow takes 10 positional arguments (3 added in this PR), and each new principal type added to membership will extend it further. A functional-options or aDependenciesstruct would make call sites (cmd/serve.go, tests) more self-documenting and less error-prone — note how tests have to passnil, nil, niltrailing args for org-only flows (Lines 260, 451 inservice_test.go). Not blocking for this PR.core/membership/service_test.go (1)
470-788: Good coverage; consider a few additional cases.Nice range across the three principal types. A couple of gaps worth adding later:
validateProjectRolehas two success branches (platform-wide null-UUID role vs. custom role owned by the project's parent org). Only one is indirectly exercised — an explicit test for a custom role withOrgID: someOtherOrgIDreturningErrInvalidProjectRolewould lock in the new org-scoping check.SetProjectMemberRolefailure path whenreplacePolicyfails (Delete or Create error) isn't covered.RemoveProjectMemberbest-effort audit behavior whenprojectService.Getfails after successful deletion (Lines 479-481 ofservice.go) isn't asserted — currently the test passes because the mocks allow the audit to be skipped, but an explicit test would document intent.internal/api/v1beta1connect/project.go (1)
381-402: IfvalidateOrgMembershipstarts rejecting disabled users, add auser.ErrDisabledcase here.See the related comment on
core/membership/service.goLines 546-583. IfErrDisabledis surfaced fromSetProjectMemberRole, it will currently fall through toCodeInternaland leak as a 500. Consider adding: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, ErrUserDisabled)(assuming a local
ErrUserDisabled/similar exists; otherwise map to an appropriate existing error).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 436f601d-9095-41a9-b4f7-14db6275f13f
📒 Files selected for processing (4)
core/membership/service.gocore/membership/service_test.gointernal/api/v1beta1connect/project.gopkg/auditrecord/consts.go
Manual test report (Frontier local)Tested against a fresh setup: 2 orgs, 3 projects, 4 users + 1 cross-org user, 3 service users, 3 groups, custom project-scoped roles in each org. Happy paths — all pass
PR-specific claims verified
Unhappy paths
Overall: the mutation paths, validation, cross-org guards, and audit emission for all three principal types behave as the PR describes. 🤖 Tested with Claude Code |
whoAbhishekSah
force-pushed
the
feat/project-member-mutations-to-membership
branch
from
76e15d0 to
aa0eab1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/membership/service_test.go (1)
694-859: Add success coverage for service-user and group project membership paths.The new service supports
user,serviceuser, andgroupprincipals, but these tests only exercise a user success path. A service-user/group success case with audit record assertions would protect the principal-type mapping that this PR specifically changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3fc26c86-670e-4313-908c-87352bbc3605
📒 Files selected for processing (12)
cmd/serve.gocore/membership/errors.gocore/membership/mocks/group_service.gocore/membership/mocks/project_service.gocore/membership/mocks/serviceuser_service.gocore/membership/service.gocore/membership/service_test.gointernal/api/v1beta1connect/errors.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/membership_service.gointernal/api/v1beta1connect/project.gopkg/auditrecord/consts.go
✅ Files skipped from review due to trivial changes (3)
- pkg/auditrecord/consts.go
- internal/api/v1beta1connect/errors.go
- core/membership/mocks/serviceuser_service.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/membership/errors.go
- cmd/serve.go
- internal/api/v1beta1connect/project.go
There was a problem hiding this comment.
Pull request overview
This PR continues the membership centralization effort by moving project member mutation logic (set role/remove member) out of core/project and into core/membership, aligning project membership writes with the same centralized pattern used for org membership.
Changes:
- Add project member mutation APIs to
core/membership(SetProjectMemberRole,RemoveProjectMember) with org-scoped role validation and org-membership validation. - Update v1beta1 Connect handlers/interfaces/mocks to call
membershipServicefor project member mutations. - Add audit record events for project member role changes/removals and expand membership tests/mocks accordingly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/auditrecord/consts.go | Adds new audit event constants for project member mutations. |
| internal/api/v1beta1connect/project.go | Routes project member mutations through membershipService and updates error mapping. |
| internal/api/v1beta1connect/mocks/project_service.go | Removes project service mock methods no longer used by Connect handlers. |
| internal/api/v1beta1connect/mocks/membership_service.go | Adds mock methods for new project membership APIs. |
| internal/api/v1beta1connect/interfaces.go | Moves project member mutation methods from ProjectService to MembershipService interface. |
| internal/api/v1beta1connect/errors.go | Updates already-member/not-member error text to use “principal”. |
| core/membership/service.go | Implements project member role set/remove, org-scoped project role validation, and org membership validation. |
| core/membership/service_test.go | Adds tests covering project member mutations (user/serviceuser/group) and audit record creation. |
| core/membership/mocks/serviceuser_service.go | Adds serviceuser service mock used by membership service tests. |
| core/membership/mocks/project_service.go | Extends membership project mock with Get. |
| core/membership/mocks/group_service.go | Extends membership group mock with Get. |
| core/membership/errors.go | Adds membership-level errors used by new project membership paths. |
| cmd/serve.go | Updates membership service wiring to include serviceUserService dependency. |
Comments suppressed due to low confidence (1)
internal/api/v1beta1connect/project.go:433
RemoveProjectMemberno longer mapsproject.ErrNotExist(or other project fetch errors) and will returnCodeInternalwhen the project doesn't exist. Add anerrors.Is(err, project.ErrNotExist)case (and return the same not-found error used elsewhere in this handler/file).
switch {
case errors.Is(err, membership.ErrNotMember):
return nil, connect.NewError(connect.CodeNotFound, ErrNotMember)
case errors.Is(err, membership.ErrInvalidPrincipalType):
return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest)
default:
return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Move SetProjectMemberRole and RemoveProjectMember from core/project/ into core/membership/ for consistency with the centralized membership pattern. Handlers now call membershipService instead of projectService for project member mutations. Key changes: - Add validateProjectRole with org-scoping (rejects cross-org custom roles) - Add validateOrgMembership with disabled user rejection - Add ServiceuserService dependency for org membership validation - Add audit records at service layer for project member mutations - Use principal-agnostic error messages and audit target types - Reuse principalTypeToAuditType from RemoveOrganizationMember Ref: #1478 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Validate project existence and capture context for audit before performing any destructive operations. This ensures stale policies for invalid project IDs are not silently deleted and audit records are always emitted on success. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ations Cover all principal types in SetProjectMemberRole and RemoveProjectMember tests to protect the principal-type audit mapping. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. Handle user.ErrDisabled in SetProjectMemberRole handler
2. Handle project.ErrNotExist in RemoveProjectMember handler
3. Make ErrAlreadyMember/ErrNotMember resource-agnostic
4. Follow {resource}.{action} pattern for audit events
5. Merge auditProjectMemberRoleChanged/Removed into single
auditProjectMember helper with event + metadata params
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
whoAbhishekSah
force-pushed
the
feat/project-member-mutations-to-membership
branch
from
a5b4ac9 to
041c3d8
Compare
Summary
Moves project member mutations (
SetProjectMemberRole,RemoveProjectMember) fromcore/project/intocore/membership/, continuing the centralized membership pattern from #1478.After this change, all member access mutations (org + project) go through
core/membership/, andcore/project/becomes pure entity CRUD with no access management logic.Key decisions
SetProjectMemberRolenow fetches the project first so the parent org ID is available for role org-scoping and org membership checks in one pass.validateProjectRolerejects custom roles from other orgs (mirrorsvalidateOrgRole). Platform-wide roles and roles belonging to the project's parent org are accepted.validateOrgMembershipnow checksuser.Disabledstate, consistent with org-levelvalidatePrincipal.AuditRecordobjects via the repository (matching the org-level pattern), in addition to the existing handler-level audit logs.user/serviceuser/group) instead of hardcodingUserType.Test plan
go build ./...cleangofmtclean🤖 Generated with Claude Code