refactor(membership): make SetGroupMemberRole an upsert

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
 	}

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",

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:

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",