From 758dc3f1b7140542ce599a9d34573a212b8cf664 Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Wed, 13 May 2026 11:00:06 +0530 Subject: [PATCH 1/2] refactor(membership): make SetGroupMemberRole an upsert SetGroupMemberRole now adds a new member when the principal has no existing group policy, and changes the role otherwise. New adds validate that the principal is a member of the parent organization. The min-owner constraint still applies to demotions. This unifies add and role-change into one public RPC so the SDK can use a single mutation for group membership writes, eliminating the need to expose AddGroupMember as a proto RPC. - Drops ErrNotMember from the SetGroupMemberRole path; ErrNotOrgMember surfaces instead when an upsert-add targets a non-org-member. - Handler error mapping updated; new ErrNotOrgMember Connect mapping. - New unit tests cover both upsert paths: add (with org-member check) and the existing change paths remain intact. AddGroupMember (service-only) is unchanged and continues to be used internally by OnGroupCreated, where the creator is always a fresh add. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/membership/service.go | 32 +++++++++++++++++++---- core/membership/service_test.go | 27 +++++++++++++++++-- internal/api/v1beta1connect/group.go | 4 +-- internal/api/v1beta1connect/group_test.go | 6 ++--- 4 files changed, 57 insertions(+), 12 deletions(-) diff --git a/core/membership/service.go b/core/membership/service.go index bb81be5c3..52e623aa8 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -1077,9 +1077,10 @@ 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 a principal's role in a group: adds the principal +// as a member if they don't already have a group policy, or changes their role +// if they do. 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 +1106,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", From e7741259b766422bd654216ddbdaead89b4c7256 Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Wed, 13 May 2026 11:02:22 +0530 Subject: [PATCH 2/2] docs(membership): clarify SetGroupMemberRole upsert doc Make explicit that both paths use the requested role: add with role on a new add, replace existing role with the requested role on a change. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/membership/service.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/membership/service.go b/core/membership/service.go index 52e623aa8..d4be56d4a 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -1077,10 +1077,12 @@ func (s *Service) AddGroupMember(ctx context.Context, groupID, principalID, prin return nil } -// SetGroupMemberRole upserts a principal's role in a group: adds the principal -// as a member if they don't already have a group policy, or changes their role -// if they do. New adds require the principal to be a member of the group's -// parent organization. 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 {