refactor(group): migrate RemoveGroupUser handler to membership

core/membership/service.go

@@ -1136,6 +1136,63 @@ func (s *Service) SetGroupMemberRole(ctx context.Context, groupID, principalID,
 	return nil
 }
 
+// RemoveGroupMember removes a principal from a group, cleaning up both their
+// group policies and the matching SpiceDB relations. Returns ErrNotMember if
+// the principal has no policies on this group; ErrLastGroupOwnerRole if they
+// are the sole remaining owner (enforced atomically via the policy guard).
+func (s *Service) RemoveGroupMember(ctx context.Context, groupID, principalID, principalType string) error {
+	grp, err := s.groupService.Get(ctx, groupID)
+	if err != nil {
+		return err
+	}
+
+	principal, err := s.validateGroupPrincipal(ctx, principalID, principalType)
+	if err != nil {
+		return err
+	}
+
+	existing, err := s.policyService.List(ctx, policy.Filter{
+		GroupID:       groupID,
+		PrincipalID:   principalID,
+		PrincipalType: principalType,
+	})
+	if err != nil {
+		return fmt.Errorf("list existing policies: %w", err)
+	}
+	if len(existing) == 0 {
+		return ErrNotMember
+	}
+
+	// Pass empty newRoleID — removal, not role change. The function still
+	// returns the owner role ID for the atomic guard on the delete path.
+	ownerRoleID, err := s.validateMinGroupOwnerConstraint(ctx, groupID, "", existing)
+	if err != nil {
+		return err
+	}
+
+	for _, p := range existing {
+		if err := s.deletePolicy(ctx, p, ownerRoleID); err != nil {
+			if errors.Is(err, policy.ErrLastRoleGuard) {
+				return ErrLastGroupOwnerRole
+			}
+			return fmt.Errorf("delete policy %s: %w", p.ID, err)
+		}
+	}
+
+	if err := s.removeRelations(ctx, groupID, schema.GroupNamespace, principalID, principalType); err != nil {
+		s.log.ErrorContext(ctx, "membership state inconsistent: group policies removed but relation cleanup failed, needs manual fix",
+			"group_id", groupID,
+			"principal_id", principalID,
+			"principal_type", principalType,
+			"error", err,
+		)
+		return err
+	}
+
+	s.auditGroupMemberRemoved(ctx, grp, principal)
+	return nil
+}
+
 // OnGroupCreated wires up SpiceDB relations for a newly-created group:
 // links the group to its parent organization (both directions) and adds the
 // creator as owner via SetGroupMemberRole. If the owner add fails, hierarchy
@@ -1376,3 +1433,35 @@ func (s *Service) auditGroupMemberRoleChanged(ctx context.Context, grp group.Gro
 		"group_id": grp.ID,
 	})
 }
+
+func (s *Service) auditGroupMemberRemoved(ctx context.Context, grp group.Group, p principalInfo) {
+	targetType, _ := principalTypeToAuditType(p.Type)
+	meta := map[string]any{}
+	if p.Email != "" {
+		meta["email"] = p.Email
+	}
+
+	s.auditRecordRepository.Create(ctx, auditrecord.AuditRecord{
+		Event: pkgAuditRecord.GroupMemberRemovedEvent,
+		Resource: auditrecord.Resource{
+			ID:   grp.ID,
+			Type: pkgAuditRecord.GroupType,
+			Name: grp.Title,
+		},
+		Target: &auditrecord.Target{
+			ID:       p.ID,
+			Type:     targetType,
+			Name:     p.Name,
+			Metadata: meta,
+		},
+		OrgID:      grp.OrganizationID,
+		OccurredAt: time.Now(),
+	})
+
+	audit.GetAuditor(ctx, grp.OrganizationID).LogWithAttrs(audit.GroupMemberRemovedEvent, audit.Target{
+		ID:   p.ID,
+		Type: p.Type,
+	}, map[string]string{
+		"group_id": grp.ID,
+	})
+}

core/membership/service_test.go

@@ -1525,3 +1525,141 @@ func TestService_OnGroupCreated(t *testing.T) {
 		assert.ErrorContains(t, err, "db down")
 	})
 }
+
+func TestService_RemoveGroupMember(t *testing.T) {
+	ctx := context.Background()
+	orgID := uuid.New().String()
+	groupID := uuid.New().String()
+	userID := uuid.New().String()
+	ownerRoleID := uuid.New().String()
+	memberRoleID := uuid.New().String()
+
+	enabledUser := user.User{ID: userID, Title: "test-user", Email: "test@acme.dev", State: user.Enabled}
+	grp := group.Group{ID: groupID, OrganizationID: orgID, Title: "Test Group"}
+
+	expectOwnerRoleLookup := func(roleSvc *mocks.RoleService) {
+		roleSvc.EXPECT().Get(ctx, schema.GroupOwnerRole).Return(role.Role{ID: ownerRoleID, Name: schema.GroupOwnerRole}, nil)
+	}
+
+	tests := []struct {
+		name           string
+		setup          func(*mocks.PolicyService, *mocks.RelationService, *mocks.RoleService, *mocks.GroupService, *mocks.UserService, *mocks.AuditRecordRepository)
+		principalType  string
+		wantErr        error
+		wantErrContain string
+	}{
+		{
+			name: "should return error if group does not exist",
+			setup: func(_ *mocks.PolicyService, _ *mocks.RelationService, _ *mocks.RoleService, grpSvc *mocks.GroupService, _ *mocks.UserService, _ *mocks.AuditRecordRepository) {
+				grpSvc.EXPECT().Get(ctx, groupID).Return(group.Group{}, group.ErrNotExist)
+			},
+			wantErr: group.ErrNotExist,
+		},
+		{
+			name: "should return error if principal type is unsupported",
+			setup: func(_ *mocks.PolicyService, _ *mocks.RelationService, _ *mocks.RoleService, grpSvc *mocks.GroupService, _ *mocks.UserService, _ *mocks.AuditRecordRepository) {
+				grpSvc.EXPECT().Get(ctx, groupID).Return(grp, nil)
+			},
+			principalType: schema.ServiceUserPrincipal,
+			wantErr:       membership.ErrInvalidPrincipalType,
+		},
+		{
+			name: "should return ErrNotMember if principal has no group policy",
+			setup: func(policySvc *mocks.PolicyService, _ *mocks.RelationService, _ *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)
+				policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil)
+			},
+			wantErr: membership.ErrNotMember,
+		},
+		{
+			name: "should return ErrLastGroupOwnerRole when removing last owner (pre-check fires)",
+			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)
+				policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "p1", RoleID: ownerRoleID}}, nil)
+				expectOwnerRoleLookup(roleSvc)
+				policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID, RoleID: ownerRoleID}).Return([]policy.Policy{{ID: "p1"}}, nil)
+			},
+			wantErr: membership.ErrLastGroupOwnerRole,
+		},
+		{
+			name: "should surface ErrLastGroupOwnerRole when DeleteWithMinRoleGuard races (TOCTOU)",
+			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)
+				policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "p1", RoleID: ownerRoleID}}, nil)
+				expectOwnerRoleLookup(roleSvc)
+				// pre-check sees two owners, but the DB guard catches the race
+				policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID, RoleID: ownerRoleID}).Return([]policy.Policy{{ID: "p1"}, {ID: "p2"}}, nil)
+				policySvc.EXPECT().DeleteWithMinRoleGuard(ctx, "p1", ownerRoleID).Return(policy.ErrLastRoleGuard)
+			},
+			wantErr: membership.ErrLastGroupOwnerRole,
+		},
+		{
+			name: "should remove a member (non-owner) and delete both relations",
+			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)
+				policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "p1", RoleID: memberRoleID}}, nil)
+				expectOwnerRoleLookup(roleSvc)
+				// member-role policy → plain Delete (no guard)
+				policySvc.EXPECT().Delete(ctx, "p1").Return(nil)
+				obj := relation.Object{ID: groupID, Namespace: schema.GroupNamespace}
+				sub := relation.Subject{ID: userID, Namespace: schema.UserPrincipal}
+				relSvc.EXPECT().Delete(ctx, relation.Relation{Object: obj, Subject: sub, RelationName: schema.OwnerRelationName}).Return(relation.ErrNotExist)
+				relSvc.EXPECT().Delete(ctx, relation.Relation{Object: obj, Subject: sub, RelationName: schema.MemberRelationName}).Return(nil)
+				auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
+			},
+			wantErr: nil,
+		},
+		{
+			name: "should remove an owner via atomic guard when more owners remain",
+			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)
+				policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "p1", RoleID: ownerRoleID}}, nil)
+				expectOwnerRoleLookup(roleSvc)
+				policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID, RoleID: ownerRoleID}).Return([]policy.Policy{{ID: "p1"}, {ID: "p2"}}, nil)
+				policySvc.EXPECT().DeleteWithMinRoleGuard(ctx, "p1", ownerRoleID).Return(nil)
+				obj := relation.Object{ID: groupID, Namespace: schema.GroupNamespace}
+				sub := relation.Subject{ID: userID, Namespace: schema.UserPrincipal}
+				relSvc.EXPECT().Delete(ctx, relation.Relation{Object: obj, Subject: sub, RelationName: schema.OwnerRelationName}).Return(nil)
+				relSvc.EXPECT().Delete(ctx, relation.Relation{Object: obj, Subject: sub, RelationName: schema.MemberRelationName}).Return(relation.ErrNotExist)
+				auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
+			},
+			wantErr: nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockPolicySvc := mocks.NewPolicyService(t)
+			mockRelSvc := mocks.NewRelationService(t)
+			mockRoleSvc := mocks.NewRoleService(t)
+			mockGrpSvc := mocks.NewGroupService(t)
+			mockUserSvc := mocks.NewUserService(t)
+			mockAuditRepo := mocks.NewAuditRecordRepository(t)
+
+			if tt.setup != nil {
+				tt.setup(mockPolicySvc, mockRelSvc, mockRoleSvc, mockGrpSvc, mockUserSvc, mockAuditRepo)
+			}
+
+			svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), mockPolicySvc, mockRelSvc, mockRoleSvc, mocks.NewOrgService(t), mockUserSvc, mocks.NewProjectService(t), mockGrpSvc, mocks.NewServiceuserService(t), mockAuditRepo)
+
+			principalType := tt.principalType
+			if principalType == "" {
+				principalType = schema.UserPrincipal
+			}
+			err := svc.RemoveGroupMember(ctx, groupID, userID, principalType)
+
+			if tt.wantErr != nil {
+				assert.ErrorIs(t, err, tt.wantErr)
+			} else if tt.wantErrContain != "" {
+				assert.ErrorContains(t, err, tt.wantErrContain)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}

internal/api/v1beta1connect/group.go

@@ -458,29 +458,27 @@ func (h *ConnectHandler) RemoveGroupUser(ctx context.Context, request *connect.R
 		}
 	}
 
-	// before deleting the user, check if the user is the only owner of the group
-	ownerRole, err := h.roleService.Get(ctx, group.AdminRole)
-	if err != nil {
-		errorLogger.LogServiceError(ctx, request, "RemoveGroupUser.roleService.Get", err,
-			"role", group.AdminRole)
-		return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError)
-	}
-	owners, err := h.listGroupUsers(ctx, request.Msg.GetId(), ownerRole.ID)
-	if err != nil {
-		errorLogger.LogServiceError(ctx, request, "RemoveGroupUser.listGroupUsers", err,
-			"group_id", request.Msg.GetId())
-		return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError)
-	}
-	if len(owners) == 1 && owners[0].ID == request.Msg.GetUserId() {
-		return nil, connect.NewError(connect.CodeInvalidArgument, ErrGroupMinOwnerCount)
-	}
-
-	// delete the user
-	if err := h.groupService.RemoveUsers(ctx, request.Msg.GetId(), []string{request.Msg.GetUserId()}); err != nil {
-		errorLogger.LogServiceError(ctx, request, "RemoveGroupUser.RemoveUsers", err,
+	if err := h.membershipService.RemoveGroupMember(ctx, request.Msg.GetId(), request.Msg.GetUserId(), schema.UserPrincipal); err != nil {
+		errorLogger.LogServiceError(ctx, request, "RemoveGroupUser.RemoveGroupMember", err,
 			"group_id", request.Msg.GetId(),
 			"user_id", request.Msg.GetUserId())
-		return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError)
+
+		switch {
+		case errors.Is(err, group.ErrNotExist), errors.Is(err, group.ErrInvalidID), errors.Is(err, group.ErrInvalidUUID):
+			return nil, connect.NewError(connect.CodeNotFound, ErrGroupNotFound)
+		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, user.ErrDisabled)
+		case errors.Is(err, membership.ErrInvalidPrincipalType), errors.Is(err, membership.ErrInvalidPrincipal):
+			return nil, connect.NewError(connect.CodeInvalidArgument, err)
+		case errors.Is(err, membership.ErrNotMember):
+			return nil, connect.NewError(connect.CodeFailedPrecondition, ErrNotMember)
+		case errors.Is(err, membership.ErrLastGroupOwnerRole):
+			return nil, connect.NewError(connect.CodeInvalidArgument, ErrGroupMinOwnerCount)
+		default:
+			return nil, connect.NewError(connect.CodeInternal, ErrInternalServerError)
+		}
 	}
 	return connect.NewResponse(&frontierv1beta1.RemoveGroupUserResponse{}), nil
 }