diff --git a/core/membership/service.go b/core/membership/service.go index bb81be5c3..d4be56d4a 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -1077,9 +1077,12 @@ func (s *Service) AddGroupMember(ctx context.Context, groupID, principalID, prin return nil } -// SetGroupMemberRole changes an existing member's role in a group. -// Returns ErrNotMember if the principal has no existing policy on the group. -// Enforces the min-owner constraint: demoting the last owner returns ErrLastGroupOwnerRole. +// SetGroupMemberRole upserts the role assignment for a principal in a group: +// if the principal has no existing group policy, they are added with the +// requested role; otherwise their existing role is replaced with the +// requested role. New adds require the principal to be a member of the +// group's parent organization. Demoting the last owner returns +// ErrLastGroupOwnerRole. func (s *Service) SetGroupMemberRole(ctx context.Context, groupID, principalID, principalType, roleID string) error { grp, err := s.groupService.Get(ctx, groupID) if err != nil { @@ -1105,11 +1108,32 @@ func (s *Service) SetGroupMemberRole(ctx context.Context, groupID, principalID, if err != nil { return fmt.Errorf("list existing policies: %w", err) } + + // add path: principal has no existing group policy if len(existing) == 0 { - return ErrNotMember + if err := s.validateOrgMembership(ctx, grp.OrganizationID, principalID, principalType); err != nil { + return err + } + createdPolicy, err := s.createPolicy(ctx, groupID, schema.GroupNamespace, principalID, principalType, resolvedRoleID) + if err != nil { + return err + } + if err := s.createRelation(ctx, groupID, schema.GroupNamespace, principalID, principalType, groupRoleToRelation(fetchedRole)); err != nil { + if deleteErr := s.policyService.Delete(ctx, createdPolicy.ID); deleteErr != nil { + s.log.WarnContext(ctx, "orphaned policy: relation creation failed and policy cleanup also failed", + "policy_id", createdPolicy.ID, + "group_id", groupID, + "principal_id", principalID, + "policy_delete_error", deleteErr, + ) + } + return err + } + s.auditGroupMemberAdded(ctx, grp, principal, resolvedRoleID) + return nil } - // skip if the user already has exactly this role + // change path: skip if the principal already has exactly this role if len(existing) == 1 && existing[0].RoleID == resolvedRoleID { return nil } diff --git a/core/membership/service_test.go b/core/membership/service_test.go index 186dd1203..8670b246b 100644 --- a/core/membership/service_test.go +++ b/core/membership/service_test.go @@ -1419,15 +1419,38 @@ func TestService_SetGroupMemberRole(t *testing.T) { wantErrContain string }{ { - name: "should return error if user is not a group member", + name: "should add member on upsert when no existing group policy and user is in org", + setup: func(policySvc *mocks.PolicyService, relSvc *mocks.RelationService, roleSvc *mocks.RoleService, grpSvc *mocks.GroupService, userSvc *mocks.UserService, auditRepo *mocks.AuditRecordRepository) { + grpSvc.EXPECT().Get(ctx, groupID).Return(grp, nil) + userSvc.EXPECT().GetByID(ctx, userID).Return(enabledUser, nil) + roleSvc.EXPECT().Get(ctx, memberRoleID).Return(role.Role{ID: memberRoleID, Name: schema.GroupMemberRole, Scopes: []string{schema.GroupNamespace}}, nil) + policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil) + // org-membership check: user must be in org + userSvc.EXPECT().GetByID(ctx, userID).Return(enabledUser, nil) + policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "org-p1"}}, nil) + // create policy + relation + policySvc.EXPECT().Create(ctx, policy.Policy{ + RoleID: memberRoleID, ResourceID: groupID, ResourceType: schema.GroupNamespace, + PrincipalID: userID, PrincipalType: schema.UserPrincipal, + }).Return(policy.Policy{ID: "new-p"}, nil) + relSvc.EXPECT().Create(ctx, groupMemberRelation(schema.MemberRelationName)).Return(relation.Relation{}, nil) + auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) + }, + roleID: memberRoleID, + wantErr: nil, + }, + { + name: "should reject upsert-add if principal is not a member of the org", setup: func(policySvc *mocks.PolicyService, _ *mocks.RelationService, roleSvc *mocks.RoleService, grpSvc *mocks.GroupService, userSvc *mocks.UserService, _ *mocks.AuditRecordRepository) { grpSvc.EXPECT().Get(ctx, groupID).Return(grp, nil) userSvc.EXPECT().GetByID(ctx, userID).Return(enabledUser, nil) roleSvc.EXPECT().Get(ctx, memberRoleID).Return(role.Role{ID: memberRoleID, Name: schema.GroupMemberRole, Scopes: []string{schema.GroupNamespace}}, nil) policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil) + userSvc.EXPECT().GetByID(ctx, userID).Return(enabledUser, nil) + policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil) }, roleID: memberRoleID, - wantErr: membership.ErrNotMember, + wantErr: membership.ErrNotOrgMember, }, { name: "should skip write when role is unchanged", diff --git a/internal/api/v1beta1connect/group.go b/internal/api/v1beta1connect/group.go index ad39a29cc..a057024c6 100644 --- a/internal/api/v1beta1connect/group.go +++ b/internal/api/v1beta1connect/group.go @@ -504,8 +504,8 @@ func (h *ConnectHandler) SetGroupMemberRole(ctx context.Context, request *connec return nil, connect.NewError(connect.CodeInvalidArgument, err) case errors.Is(err, membership.ErrInvalidGroupRole): return nil, connect.NewError(connect.CodeInvalidArgument, ErrInvalidGroupRole) - case errors.Is(err, membership.ErrNotMember): - return nil, connect.NewError(connect.CodeFailedPrecondition, ErrNotMember) + case errors.Is(err, membership.ErrNotOrgMember): + return nil, connect.NewError(connect.CodeFailedPrecondition, ErrNotOrgMember) case errors.Is(err, membership.ErrLastGroupOwnerRole): return nil, connect.NewError(connect.CodeFailedPrecondition, ErrLastGroupOwnerRole) default: diff --git a/internal/api/v1beta1connect/group_test.go b/internal/api/v1beta1connect/group_test.go index bd12739e1..b6cf8e6c6 100644 --- a/internal/api/v1beta1connect/group_test.go +++ b/internal/api/v1beta1connect/group_test.go @@ -1809,13 +1809,13 @@ func TestConnectHandler_SetGroupMemberRole(t *testing.T) { wantErr: connect.NewError(connect.CodeInvalidArgument, membership.ErrInvalidPrincipalType), }, { - name: "should return failed precondition if principal is not a group member", + name: "should return failed precondition if principal is not a member of the org", setup: func(ms *mocks.MembershipService, os *mocks.OrganizationService) { os.EXPECT().Get(mock.Anything, testOrgID).Return(testOrgMap[testOrgID], nil) - ms.EXPECT().SetGroupMemberRole(mock.Anything, someGroupID, somePrincipalID, schema.UserPrincipal, someRoleID).Return(membership.ErrNotMember) + ms.EXPECT().SetGroupMemberRole(mock.Anything, someGroupID, somePrincipalID, schema.UserPrincipal, someRoleID).Return(membership.ErrNotOrgMember) }, request: baseRequest(), - wantErr: connect.NewError(connect.CodeFailedPrecondition, ErrNotMember), + wantErr: connect.NewError(connect.CodeFailedPrecondition, ErrNotOrgMember), }, { name: "should return failed precondition if demoting last group owner",