feat: implement SetProjectMemberRole RPC#1481
Open
whoAbhishekSah wants to merge 12 commits intomainfrom
Open
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add handler, service, and authorization for atomically changing a user's role in a project. Enforces minimum-owner constraint. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a server-side RPC to set a project member's role and implements end-to-end support: service logic, Connect handler, interface/mocks, tests, domain errors, authorization entry, and updates Makefile PROTON_COMMIT. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/project/service_test.go (1)
23-35:⚠️ Potential issue | 🟠 MajorAdd direct unit tests for
SetMemberRolebehavior (currently missing).This update wires new dependencies, but the new project-member role mutation path is not covered here. Please add focused tests for role validation, non-member handling, and last-owner protection to reduce regression risk in the new RPC path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56252940-859b-4c10-83dd-9b50d88a457e
⛔ Files ignored due to path filters (3)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go,!proto/**proto/v1beta1/frontier.pb.validate.gois excluded by!proto/**proto/v1beta1/frontierv1beta1connect/frontier.connect.gois excluded by!proto/**
📒 Files selected for processing (12)
Makefilecmd/serve.gocore/project/errors.gocore/project/mocks/policy_service.gocore/project/mocks/role_service.gocore/project/service.gocore/project/service_test.gointernal/api/v1beta1connect/errors.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/project_service.gointernal/api/v1beta1connect/project.gopkg/server/connect_interceptors/authorization.go
…berRole Project access can come from org-level permissions, so we don't enforce minimum-owner constraint or require existing membership at project level. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pull Request Test Coverage Report for Build 23633134429Details
💛 - Coveralls |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…emberRole Each principal type has its own field in the request (user_id, service_user_id, group_id). Validation checks that the principal exists and belongs to the org that owns the project. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/project/service.go (1)
360-363:⚠️ Potential issue | 🟠 MajorThe last-owner invariant is gone.
Demoting the only direct owner now succeeds and leaves the project with zero project-scoped owners. That regresses the owner-constraint behavior this RPC is supposed to preserve; if org-level fallback is intentionally replacing that invariant, the contract/tests need to change too.
Also applies to: 377-390
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a3f72ab-6cdf-4e25-beb2-3c66806bc560
📒 Files selected for processing (3)
core/project/errors.gocore/project/service.gointernal/api/v1beta1connect/project.go
🚧 Files skipped from review as they are similar to previous changes (2)
- core/project/errors.go
- internal/api/v1beta1connect/project.go
…ate ID fields 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>
12 test cases covering: - validation errors (project/user/serviceuser/group not found) - invalid principal type - org membership check - role validation (not found, wrong scope) - success with no existing policies (add member) - success replacing existing policies (role change) - service user and group principal types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/project/service.go (1)
362-402:⚠️ Potential issue | 🟠 MajorUse the resolved project ID (
prj.ID) consistently.Lines 380 and 396 use the raw
projectIDparameter instead ofprj.ID. Sinces.Get()at line 366 resolves both UUIDs and names, passing a project name will cause the policy filter and create operations to use the name string instead of the canonical UUID.🐛 Proposed fix
existingPolicies, err := s.policyService.List(ctx, policy.Filter{ - ProjectID: projectID, + ProjectID: prj.ID, PrincipalID: principalID, PrincipalType: principalType, }) @@ _, err = s.policyService.Create(ctx, policy.Policy{ RoleID: newRoleID, - ResourceID: projectID, + ResourceID: prj.ID, ResourceType: schema.ProjectNamespace, PrincipalID: principalID, PrincipalType: principalType, })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2d62ec5-29b5-4805-9f36-2c26ab203e72
⛔ Files ignored due to path filters (2)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go,!proto/**proto/v1beta1/frontier.pb.validate.gois excluded by!proto/**
📒 Files selected for processing (9)
Makefilecore/project/errors.gocore/project/mocks/group_service.gocore/project/mocks/serviceuser_service.gocore/project/service.gocore/project/service_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/project_service.gointernal/api/v1beta1connect/project.go
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- internal/api/v1beta1connect/project.go
- core/project/errors.go
…sion Bug 1: Service users and groups don't have org-level policies. Check their OrgID/OrganizationID field directly instead of querying for org policies. Bug 2: Use PolicyManagePermission instead of UpdatePermission for authorization, matching SetOrganizationMemberRole behavior. Prevents org viewers from managing project roles. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/project/service_test.go (1)
59-60: Remove redundant_ = roleServiceno-op assignments.These lines add noise and can be dropped since
roleServiceis passed toproject.NewService(...)in the same scope.Also applies to: 77-78, 122-123, 140-141
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d83355dc-3728-44ea-b279-794bb4b06969
📒 Files selected for processing (3)
core/project/service.gocore/project/service_test.gopkg/server/connect_interceptors/authorization.go
✅ Files skipped from review due to trivial changes (1)
- pkg/server/connect_interceptors/authorization.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/project/service.go
Comment on lines
+977
to
+1245
| func TestService_SetMemberRole(t *testing.T) { | ||
| ctx := context.Background() | ||
| projectID := uuid.New().String() | ||
| orgID := uuid.New().String() | ||
| userID := uuid.New().String() | ||
| suID := uuid.New().String() | ||
| groupID := uuid.New().String() | ||
| roleID := uuid.New().String() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| projectID string | ||
| principalID string | ||
| principalType string | ||
| roleID string | ||
| setup func(*mocks.Repository, *mocks.UserService, *mocks.ServiceuserService, *mocks.GroupService, *mocks.PolicyService, *mocks.RoleService) | ||
| wantErr error | ||
| }{ | ||
| { | ||
| name: "should return error if project does not exist", | ||
| projectID: projectID, | ||
| principalID: userID, | ||
| principalType: schema.UserPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{}, project.ErrNotExist) | ||
| }, | ||
| wantErr: project.ErrNotExist, | ||
| }, | ||
| { | ||
| name: "should return error if user does not exist", | ||
| projectID: projectID, | ||
| principalID: userID, | ||
| principalType: schema.UserPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| userSvc.EXPECT().GetByID(ctx, userID).Return(user.User{}, user.ErrNotExist) | ||
| }, | ||
| wantErr: user.ErrNotExist, | ||
| }, | ||
| { | ||
| name: "should return error if service user does not exist", | ||
| projectID: projectID, | ||
| principalID: suID, | ||
| principalType: schema.ServiceUserPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| suserSvc.EXPECT().Get(ctx, suID).Return(serviceuser.ServiceUser{}, serviceuser.ErrNotExist) | ||
| }, | ||
| wantErr: serviceuser.ErrNotExist, | ||
| }, | ||
| { | ||
| name: "should return error if group does not exist", | ||
| projectID: projectID, | ||
| principalID: groupID, | ||
| principalType: schema.GroupPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| groupSvc.EXPECT().Get(ctx, groupID).Return(group.Group{}, group.ErrNotExist) | ||
| }, | ||
| wantErr: group.ErrNotExist, | ||
| }, | ||
| { | ||
| name: "should return error for invalid principal type", | ||
| projectID: projectID, | ||
| principalID: userID, | ||
| principalType: "invalid", | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| }, | ||
| wantErr: project.ErrInvalidPrincipalType, | ||
| }, | ||
| { | ||
| name: "should return error if user is not an org member", | ||
| projectID: projectID, | ||
| principalID: userID, | ||
| principalType: schema.UserPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| userSvc.EXPECT().GetByID(ctx, userID).Return(user.User{ID: userID}, nil) | ||
| policySvc.EXPECT().List(ctx, policy.Filter{ | ||
| OrgID: orgID, | ||
| PrincipalID: userID, | ||
| PrincipalType: schema.UserPrincipal, | ||
| }).Return([]policy.Policy{}, nil) | ||
| }, | ||
| wantErr: project.ErrNotOrgMember, | ||
| }, | ||
| { | ||
| name: "should return error if role does not exist", | ||
| projectID: projectID, | ||
| principalID: userID, | ||
| principalType: schema.UserPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| userSvc.EXPECT().GetByID(ctx, userID).Return(user.User{ID: userID}, nil) | ||
| policySvc.EXPECT().List(ctx, policy.Filter{ | ||
| OrgID: orgID, | ||
| PrincipalID: userID, | ||
| PrincipalType: schema.UserPrincipal, | ||
| }).Return([]policy.Policy{{ID: "p1"}}, nil) | ||
| roleSvc.EXPECT().Get(ctx, roleID).Return(role.Role{}, role.ErrNotExist) | ||
| }, | ||
| wantErr: role.ErrNotExist, | ||
| }, | ||
| { | ||
| name: "should return error if role scope is not project", | ||
| projectID: projectID, | ||
| principalID: userID, | ||
| principalType: schema.UserPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| userSvc.EXPECT().GetByID(ctx, userID).Return(user.User{ID: userID}, nil) | ||
| policySvc.EXPECT().List(ctx, policy.Filter{ | ||
| OrgID: orgID, | ||
| PrincipalID: userID, | ||
| PrincipalType: schema.UserPrincipal, | ||
| }).Return([]policy.Policy{{ID: "p1"}}, nil) | ||
| roleSvc.EXPECT().Get(ctx, roleID).Return(role.Role{ID: roleID, Scopes: []string{schema.OrganizationNamespace}}, nil) | ||
| }, | ||
| wantErr: project.ErrInvalidProjectRole, | ||
| }, | ||
| { | ||
| name: "should succeed for user with no existing project policies", | ||
| projectID: projectID, | ||
| principalID: userID, | ||
| principalType: schema.UserPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| userSvc.EXPECT().GetByID(ctx, userID).Return(user.User{ID: userID}, nil) | ||
| policySvc.EXPECT().List(ctx, policy.Filter{ | ||
| OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal, | ||
| }).Return([]policy.Policy{{ID: "org-p1"}}, nil) | ||
| roleSvc.EXPECT().Get(ctx, roleID).Return(role.Role{ID: roleID, Scopes: []string{schema.ProjectNamespace}}, nil) | ||
| policySvc.EXPECT().List(ctx, policy.Filter{ | ||
| ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.UserPrincipal, | ||
| }).Return([]policy.Policy{}, nil) | ||
| policySvc.EXPECT().Create(ctx, policy.Policy{ | ||
| RoleID: roleID, ResourceID: projectID, ResourceType: schema.ProjectNamespace, | ||
| PrincipalID: userID, PrincipalType: schema.UserPrincipal, | ||
| }).Return(policy.Policy{}, nil) | ||
| }, | ||
| wantErr: nil, | ||
| }, | ||
| { | ||
| name: "should replace existing policies on role change", | ||
| projectID: projectID, | ||
| principalID: userID, | ||
| principalType: schema.UserPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| userSvc.EXPECT().GetByID(ctx, userID).Return(user.User{ID: userID}, nil) | ||
| policySvc.EXPECT().List(ctx, policy.Filter{ | ||
| OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal, | ||
| }).Return([]policy.Policy{{ID: "org-p1"}}, nil) | ||
| roleSvc.EXPECT().Get(ctx, roleID).Return(role.Role{ID: roleID, Scopes: []string{schema.ProjectNamespace}}, nil) | ||
| policySvc.EXPECT().List(ctx, policy.Filter{ | ||
| ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.UserPrincipal, | ||
| }).Return([]policy.Policy{{ID: "old-p1"}, {ID: "old-p2"}}, nil) | ||
| policySvc.EXPECT().Delete(ctx, "old-p1").Return(nil) | ||
| policySvc.EXPECT().Delete(ctx, "old-p2").Return(nil) | ||
| policySvc.EXPECT().Create(ctx, policy.Policy{ | ||
| RoleID: roleID, ResourceID: projectID, ResourceType: schema.ProjectNamespace, | ||
| PrincipalID: userID, PrincipalType: schema.UserPrincipal, | ||
| }).Return(policy.Policy{}, nil) | ||
| }, | ||
| wantErr: nil, | ||
| }, | ||
| { | ||
| name: "should return error if service user belongs to different org", | ||
| projectID: projectID, | ||
| principalID: suID, | ||
| principalType: schema.ServiceUserPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| suserSvc.EXPECT().Get(ctx, suID).Return(serviceuser.ServiceUser{ID: suID, OrgID: "different-org"}, nil) | ||
| }, | ||
| wantErr: project.ErrNotOrgMember, | ||
| }, | ||
| { | ||
| name: "should return error if group belongs to different org", | ||
| projectID: projectID, | ||
| principalID: groupID, | ||
| principalType: schema.GroupPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| groupSvc.EXPECT().Get(ctx, groupID).Return(group.Group{ID: groupID, OrganizationID: "different-org"}, nil) | ||
| }, | ||
| wantErr: project.ErrNotOrgMember, | ||
| }, | ||
| { | ||
| name: "should succeed for service user principal", | ||
| projectID: projectID, | ||
| principalID: suID, | ||
| principalType: schema.ServiceUserPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| suserSvc.EXPECT().Get(ctx, suID).Return(serviceuser.ServiceUser{ID: suID, OrgID: orgID}, nil) | ||
| roleSvc.EXPECT().Get(ctx, roleID).Return(role.Role{ID: roleID, Scopes: []string{schema.ProjectNamespace}}, nil) | ||
| policySvc.EXPECT().List(ctx, policy.Filter{ | ||
| ProjectID: projectID, PrincipalID: suID, PrincipalType: schema.ServiceUserPrincipal, | ||
| }).Return([]policy.Policy{}, nil) | ||
| policySvc.EXPECT().Create(ctx, policy.Policy{ | ||
| RoleID: roleID, ResourceID: projectID, ResourceType: schema.ProjectNamespace, | ||
| PrincipalID: suID, PrincipalType: schema.ServiceUserPrincipal, | ||
| }).Return(policy.Policy{}, nil) | ||
| }, | ||
| wantErr: nil, | ||
| }, | ||
| { | ||
| name: "should succeed for group principal", | ||
| projectID: projectID, | ||
| principalID: groupID, | ||
| principalType: schema.GroupPrincipal, | ||
| roleID: roleID, | ||
| setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) { | ||
| repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil) | ||
| groupSvc.EXPECT().Get(ctx, groupID).Return(group.Group{ID: groupID, OrganizationID: orgID}, nil) | ||
| roleSvc.EXPECT().Get(ctx, roleID).Return(role.Role{ID: roleID, Scopes: []string{schema.ProjectNamespace}}, nil) | ||
| policySvc.EXPECT().List(ctx, policy.Filter{ | ||
| ProjectID: projectID, PrincipalID: groupID, PrincipalType: schema.GroupPrincipal, | ||
| }).Return([]policy.Policy{}, nil) | ||
| policySvc.EXPECT().Create(ctx, policy.Policy{ | ||
| RoleID: roleID, ResourceID: projectID, ResourceType: schema.ProjectNamespace, | ||
| PrincipalID: groupID, PrincipalType: schema.GroupPrincipal, | ||
| }).Return(policy.Policy{}, nil) | ||
| }, | ||
| wantErr: nil, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| repo := mocks.NewRepository(t) | ||
| userSvc := mocks.NewUserService(t) | ||
| suserSvc := mocks.NewServiceuserService(t) | ||
| groupSvc := mocks.NewGroupService(t) | ||
| policySvc := mocks.NewPolicyService(t) | ||
| roleSvc := mocks.NewRoleService(t) | ||
| relationSvc := mocks.NewRelationService(t) | ||
| authnSvc := mocks.NewAuthnService(t) | ||
|
|
||
| if tt.setup != nil { | ||
| tt.setup(repo, userSvc, suserSvc, groupSvc, policySvc, roleSvc) | ||
| } | ||
|
|
||
| svc := project.NewService(repo, relationSvc, userSvc, policySvc, authnSvc, suserSvc, groupSvc, roleSvc) | ||
| err := svc.SetMemberRole(ctx, tt.projectID, tt.principalID, tt.principalType, tt.roleID) | ||
|
|
||
| if tt.wantErr != nil { | ||
| assert.ErrorIs(t, err, tt.wantErr) | ||
| } else { | ||
| assert.NoError(t, err) | ||
| } | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing unit case for minimum-owner protection invariant.
TestService_SetMemberRole covers many paths, but it doesn’t assert the “cannot remove/downgrade the last project owner” rule. That invariant is central to this RPC; please add an explicit negative test for it to prevent silent regressions.
UpdatePermission is the correct check - project owners and managers can manage members. The authz bypass seen in testing is caused by stale direct owner relations (tracked in #1478), not a wrong permission choice. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SetProjectMemberRoleRPC for atomic role assignment on project membersupdatepermission on the projectCloses #1461 (partially -
RemoveProjectMemberwill be a separate PR)Proton PR: raystack/proton#456
Test plan
updatepermission🤖 Generated with Claude Code