From e8a88ae0798b092b501ce5ae55b6a527fc5345ce Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Thu, 14 May 2026 13:44:59 +0530 Subject: [PATCH 1/2] refactor(group): migrate RemoveGroupUser handler to membership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The RemoveGroupUser RPC handler now delegates to a new membershipService.RemoveGroupMember, completing the symmetry with the AddGroupUsers migration (#1608). The proto RPC is unchanged. - Adds RemoveGroupMember to core/membership/service.go. Validates principal, enforces the min-owner constraint via the existing atomic DeleteWithMinRoleGuard, cleans up both group#owner and group#member relations, and emits an audit event. - Moves the min-owner pre-check out of the handler. Previously the handler called userService.ListByGroup(group.AdminRole) and compared counts; that race-prone two-step is replaced by validateMinGroupOwner + the atomic guard pattern already used by SetGroupMemberRole. - Adds the GroupMemberRemovedEvent audit constant. groupService.RemoveUsers / removeUsers are intentionally kept — they serve the user-cascade path (core/deleter and group.Delete), which must bypass the min-owner constraint when the user is being destroyed system-wide. Matches the org pattern where Service.RemoveUsers and RemoveOrganizationMember coexist. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/membership/service.go | 89 +++++++++++ core/membership/service_test.go | 138 ++++++++++++++++++ internal/api/v1beta1connect/group.go | 40 +++-- internal/api/v1beta1connect/group_test.go | 82 ++++++----- internal/api/v1beta1connect/interfaces.go | 1 + .../mocks/membership_service.go | 49 +++++++ pkg/auditrecord/consts.go | 1 + 7 files changed, 341 insertions(+), 59 deletions(-) diff --git a/core/membership/service.go b/core/membership/service.go index d24da6a2e..5cd0bee9e 100644 --- a/core/membership/service.go +++ b/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, + }) +} diff --git a/core/membership/service_test.go b/core/membership/service_test.go index 2b16bab5a..4a99ce85f 100644 --- a/core/membership/service_test.go +++ b/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) + } + }) + } +} diff --git a/internal/api/v1beta1connect/group.go b/internal/api/v1beta1connect/group.go index 105890acb..5ae01eb87 100644 --- a/internal/api/v1beta1connect/group.go +++ b/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 } diff --git a/internal/api/v1beta1connect/group_test.go b/internal/api/v1beta1connect/group_test.go index 1a34640f0..8e128f4cf 100644 --- a/internal/api/v1beta1connect/group_test.go +++ b/internal/api/v1beta1connect/group_test.go @@ -1381,27 +1381,17 @@ func TestConnectHandler_AddGroupUsers(t *testing.T) { func TestConnectHandler_RemoveGroupUser(t *testing.T) { randomID := utils.NewString() - adminRoleID := "admin-role-id" - expectAdminLookup := func(ms *mocks.MembershipService, us *mocks.UserService, users []user.User, err error) { - ms.EXPECT().ListPrincipalsByResource(mock.Anything, randomID, schema.GroupNamespace, membership.MemberFilter{ - PrincipalType: schema.UserPrincipal, - RoleIDs: []string{adminRoleID}, - }).Return(membersFromUsers(users), err).Once() - if err == nil { - us.EXPECT().GetByIDs(mock.Anything, userIDsOf(users)).Return(users, nil).Once() - } - } tests := []struct { name string - setup func(gs *mocks.GroupService, os *mocks.OrganizationService, us *mocks.UserService, rs *mocks.RoleService, ms *mocks.MembershipService) + setup func(os *mocks.OrganizationService, ms *mocks.MembershipService) request *connect.Request[frontierv1beta1.RemoveGroupUserRequest] want *connect.Response[frontierv1beta1.RemoveGroupUserResponse] wantErr error }{ { name: "should return error if organization does not exist", - setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, us *mocks.UserService, rs *mocks.RoleService, ms *mocks.MembershipService) { + setup: func(os *mocks.OrganizationService, _ *mocks.MembershipService) { os.EXPECT().Get(mock.Anything, randomID).Return(organization.Organization{}, organization.ErrNotExist) }, request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{ @@ -1414,7 +1404,7 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) { }, { name: "should return error if organization is disabled", - setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, us *mocks.UserService, rs *mocks.RoleService, ms *mocks.MembershipService) { + setup: func(os *mocks.OrganizationService, _ *mocks.MembershipService) { os.EXPECT().Get(mock.Anything, randomID).Return(organization.Organization{}, organization.ErrDisabled) }, request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{ @@ -1426,11 +1416,10 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) { wantErr: connect.NewError(connect.CodeNotFound, ErrOrgDisabled), }, { - name: "should return error if user service fails when checking owners", - setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, us *mocks.UserService, rs *mocks.RoleService, ms *mocks.MembershipService) { + name: "should return not found if group does not exist", + setup: func(os *mocks.OrganizationService, ms *mocks.MembershipService) { os.EXPECT().Get(mock.Anything, randomID).Return(organization.Organization{ID: randomID}, nil) - rs.EXPECT().Get(mock.Anything, group.AdminRole).Return(role.Role{ID: adminRoleID}, nil).Once() - expectAdminLookup(ms, us, nil, errors.New("user service error")) + ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(group.ErrNotExist) }, request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{ Id: randomID, @@ -1438,14 +1427,41 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) { UserId: randomID, }), want: nil, - wantErr: connect.NewError(connect.CodeInternal, ErrInternalServerError), + wantErr: connect.NewError(connect.CodeNotFound, ErrGroupNotFound), + }, + { + name: "should return not found if user does not exist", + setup: func(os *mocks.OrganizationService, ms *mocks.MembershipService) { + os.EXPECT().Get(mock.Anything, randomID).Return(organization.Organization{ID: randomID}, nil) + ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(user.ErrNotExist) + }, + request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{ + Id: randomID, + OrgId: randomID, + UserId: randomID, + }), + want: nil, + wantErr: connect.NewError(connect.CodeNotFound, ErrUserNotExist), + }, + { + name: "should return failed precondition if user is not a group member", + setup: func(os *mocks.OrganizationService, ms *mocks.MembershipService) { + os.EXPECT().Get(mock.Anything, randomID).Return(organization.Organization{ID: randomID}, nil) + ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(membership.ErrNotMember) + }, + request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{ + Id: randomID, + OrgId: randomID, + UserId: randomID, + }), + want: nil, + wantErr: connect.NewError(connect.CodeFailedPrecondition, ErrNotMember), }, { - name: "should return error if user is the only admin", - setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, us *mocks.UserService, rs *mocks.RoleService, ms *mocks.MembershipService) { + name: "should return invalid argument if user is the only owner", + setup: func(os *mocks.OrganizationService, ms *mocks.MembershipService) { os.EXPECT().Get(mock.Anything, randomID).Return(organization.Organization{ID: randomID}, nil) - rs.EXPECT().Get(mock.Anything, group.AdminRole).Return(role.Role{ID: adminRoleID}, nil).Once() - expectAdminLookup(ms, us, []user.User{{ID: randomID}}, nil) + ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(membership.ErrLastGroupOwnerRole) }, request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{ Id: randomID, @@ -1456,12 +1472,10 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) { wantErr: connect.NewError(connect.CodeInvalidArgument, ErrGroupMinOwnerCount), }, { - name: "should return error if group service fails to remove user", - setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, us *mocks.UserService, rs *mocks.RoleService, ms *mocks.MembershipService) { + name: "should return internal error for unknown errors", + setup: func(os *mocks.OrganizationService, ms *mocks.MembershipService) { os.EXPECT().Get(mock.Anything, randomID).Return(organization.Organization{ID: randomID}, nil) - rs.EXPECT().Get(mock.Anything, group.AdminRole).Return(role.Role{ID: adminRoleID}, nil).Once() - expectAdminLookup(ms, us, []user.User{{ID: "other-admin"}, {ID: randomID}}, nil) - gs.EXPECT().RemoveUsers(mock.Anything, randomID, []string{randomID}).Return(errors.New("group service error")) + ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(errors.New("unknown")) }, request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{ Id: randomID, @@ -1473,11 +1487,9 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) { }, { name: "should remove user successfully", - setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, us *mocks.UserService, rs *mocks.RoleService, ms *mocks.MembershipService) { + setup: func(os *mocks.OrganizationService, ms *mocks.MembershipService) { os.EXPECT().Get(mock.Anything, randomID).Return(organization.Organization{ID: randomID}, nil) - rs.EXPECT().Get(mock.Anything, group.AdminRole).Return(role.Role{ID: adminRoleID}, nil).Once() - expectAdminLookup(ms, us, []user.User{{ID: "other-admin"}, {ID: randomID}}, nil) - gs.EXPECT().RemoveUsers(mock.Anything, randomID, []string{randomID}).Return(nil) + ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(nil) }, request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{ Id: randomID, @@ -1491,19 +1503,13 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - mockGroupSvc := new(mocks.GroupService) mockOrgSvc := new(mocks.OrganizationService) - mockUserSvc := new(mocks.UserService) - mockRoleSvc := new(mocks.RoleService) mockMembershipSvc := new(mocks.MembershipService) if tt.setup != nil { - tt.setup(mockGroupSvc, mockOrgSvc, mockUserSvc, mockRoleSvc, mockMembershipSvc) + tt.setup(mockOrgSvc, mockMembershipSvc) } h := ConnectHandler{ - groupService: mockGroupSvc, orgService: mockOrgSvc, - userService: mockUserSvc, - roleService: mockRoleSvc, membershipService: mockMembershipSvc, } got, err := h.RemoveGroupUser(context.Background(), tt.request) diff --git a/internal/api/v1beta1connect/interfaces.go b/internal/api/v1beta1connect/interfaces.go index 57d9d4cd6..1e60224b7 100644 --- a/internal/api/v1beta1connect/interfaces.go +++ b/internal/api/v1beta1connect/interfaces.go @@ -406,6 +406,7 @@ type MembershipService interface { RemoveProjectMember(ctx context.Context, projectID, principalID, principalType string) error ListPrincipalsByResource(ctx context.Context, resourceID, resourceType string, filter membership.MemberFilter) ([]membership.Member, error) SetGroupMemberRole(ctx context.Context, groupID, principalID, principalType, roleID string) error + RemoveGroupMember(ctx context.Context, groupID, principalID, principalType string) error OnGroupCreated(ctx context.Context, groupID, orgID, creatorID, creatorType string) error } diff --git a/internal/api/v1beta1connect/mocks/membership_service.go b/internal/api/v1beta1connect/mocks/membership_service.go index ec7f2c277..a85eba94d 100644 --- a/internal/api/v1beta1connect/mocks/membership_service.go +++ b/internal/api/v1beta1connect/mocks/membership_service.go @@ -183,6 +183,55 @@ func (_c *MembershipService_OnGroupCreated_Call) RunAndReturn(run func(context.C return _c } +// RemoveGroupMember provides a mock function with given fields: ctx, groupID, principalID, principalType +func (_m *MembershipService) RemoveGroupMember(ctx context.Context, groupID string, principalID string, principalType string) error { + ret := _m.Called(ctx, groupID, principalID, principalType) + + if len(ret) == 0 { + panic("no return value specified for RemoveGroupMember") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string, string) error); ok { + r0 = rf(ctx, groupID, principalID, principalType) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// MembershipService_RemoveGroupMember_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemoveGroupMember' +type MembershipService_RemoveGroupMember_Call struct { + *mock.Call +} + +// RemoveGroupMember is a helper method to define mock.On call +// - ctx context.Context +// - groupID string +// - principalID string +// - principalType string +func (_e *MembershipService_Expecter) RemoveGroupMember(ctx interface{}, groupID interface{}, principalID interface{}, principalType interface{}) *MembershipService_RemoveGroupMember_Call { + return &MembershipService_RemoveGroupMember_Call{Call: _e.mock.On("RemoveGroupMember", ctx, groupID, principalID, principalType)} +} + +func (_c *MembershipService_RemoveGroupMember_Call) Run(run func(ctx context.Context, groupID string, principalID string, principalType string)) *MembershipService_RemoveGroupMember_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string), args[2].(string), args[3].(string)) + }) + return _c +} + +func (_c *MembershipService_RemoveGroupMember_Call) Return(_a0 error) *MembershipService_RemoveGroupMember_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MembershipService_RemoveGroupMember_Call) RunAndReturn(run func(context.Context, string, string, string) error) *MembershipService_RemoveGroupMember_Call { + _c.Call.Return(run) + return _c +} + // RemoveOrganizationMember provides a mock function with given fields: ctx, orgID, principalID, principalType func (_m *MembershipService) RemoveOrganizationMember(ctx context.Context, orgID string, principalID string, principalType string) error { ret := _m.Called(ctx, orgID, principalID, principalType) diff --git a/pkg/auditrecord/consts.go b/pkg/auditrecord/consts.go index 4045d2eca..845e4773f 100644 --- a/pkg/auditrecord/consts.go +++ b/pkg/auditrecord/consts.go @@ -46,6 +46,7 @@ const ( // Group Member Events GroupMemberAddedEvent Event = "group.member_added" GroupMemberRoleChangedEvent Event = "group.member_role_changed" + GroupMemberRemovedEvent Event = "group.member_removed" // KYC Events KYCVerifiedEvent Event = "kyc.verified" From 56c4f7990118fb3d5797e41004ac7c4e8630402e Mon Sep 17 00:00:00 2001 From: Abhishek Sah Date: Thu, 14 May 2026 13:58:56 +0530 Subject: [PATCH 2/2] test(group): cover full RemoveGroupUser handler error mapping Adds table cases for user.ErrDisabled, membership.ErrInvalidPrincipalType, and membership.ErrInvalidPrincipal so the handler's error mapping contract is fully locked. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/api/v1beta1connect/group_test.go | 42 +++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/internal/api/v1beta1connect/group_test.go b/internal/api/v1beta1connect/group_test.go index 8e128f4cf..7a0549819 100644 --- a/internal/api/v1beta1connect/group_test.go +++ b/internal/api/v1beta1connect/group_test.go @@ -1471,6 +1471,48 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) { want: nil, wantErr: connect.NewError(connect.CodeInvalidArgument, ErrGroupMinOwnerCount), }, + { + name: "should return failed precondition if user is disabled", + setup: func(os *mocks.OrganizationService, ms *mocks.MembershipService) { + os.EXPECT().Get(mock.Anything, randomID).Return(organization.Organization{ID: randomID}, nil) + ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(user.ErrDisabled) + }, + request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{ + Id: randomID, + OrgId: randomID, + UserId: randomID, + }), + want: nil, + wantErr: connect.NewError(connect.CodeFailedPrecondition, user.ErrDisabled), + }, + { + name: "should return invalid argument if principal type is unsupported", + setup: func(os *mocks.OrganizationService, ms *mocks.MembershipService) { + os.EXPECT().Get(mock.Anything, randomID).Return(organization.Organization{ID: randomID}, nil) + ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(membership.ErrInvalidPrincipalType) + }, + request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{ + Id: randomID, + OrgId: randomID, + UserId: randomID, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, membership.ErrInvalidPrincipalType), + }, + { + name: "should return invalid argument if principal is invalid", + setup: func(os *mocks.OrganizationService, ms *mocks.MembershipService) { + os.EXPECT().Get(mock.Anything, randomID).Return(organization.Organization{ID: randomID}, nil) + ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(membership.ErrInvalidPrincipal) + }, + request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{ + Id: randomID, + OrgId: randomID, + UserId: randomID, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, membership.ErrInvalidPrincipal), + }, { name: "should return internal error for unknown errors", setup: func(os *mocks.OrganizationService, ms *mocks.MembershipService) {