Skip to content

fix(membership): allow removing a member from a disabled project or group #2052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AmanGIT07 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(membership): allow removing a member from a disabled project or group #2052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/membership/group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func (s *Service) SetGroupMemberRole(ctx context.Context, groupID, principalID,
// 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)
grp, err := s.getGroupIncludingDisabled(ctx, groupID)
if err != nil {
return err
}
Expand Down
40 changes: 33 additions & 7 deletions core/membership/group_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,10 @@ func TestService_RemoveGroupMember(t *testing.T) {

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"}
disabledGrp := grp
disabledGrp.State = group.Disabled
includeDisabled := group.Filter{IncludeDisabled: true}
lookupErr := errors.New("group lookup failed")

expectOwnerRoleLookup := func(roleSvc *mocks.RoleService) {
roleSvc.EXPECT().Get(ctx, schema.GroupOwnerRole).Return(role.Role{ID: ownerRoleID, Name: schema.GroupOwnerRole}, nil)
Expand All @@ -303,22 +307,29 @@ func TestService_RemoveGroupMember(t *testing.T) {
{
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)
grpSvc.EXPECT().GetByIDs(ctx, []string{groupID}, includeDisabled).Return([]group.Group{}, nil)
},
wantErr: group.ErrNotExist,
},
{
name: "should return error if the group lookup fails",
setup: func(_ *mocks.PolicyService, _ *mocks.RelationService, _ *mocks.RoleService, grpSvc *mocks.GroupService, _ *mocks.UserService, _ *mocks.AuditRecordRepository) {
grpSvc.EXPECT().GetByIDs(ctx, []string{groupID}, includeDisabled).Return(nil, lookupErr)
},
wantErr: lookupErr,
},
{
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)
grpSvc.EXPECT().GetByIDs(ctx, []string{groupID}, includeDisabled).Return([]group.Group{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)
grpSvc.EXPECT().GetByIDs(ctx, []string{groupID}, includeDisabled).Return([]group.Group{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)
},
Expand All @@ -327,7 +338,7 @@ func TestService_RemoveGroupMember(t *testing.T) {
{
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)
grpSvc.EXPECT().GetByIDs(ctx, []string{groupID}, includeDisabled).Return([]group.Group{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)
Expand All @@ -338,7 +349,7 @@ func TestService_RemoveGroupMember(t *testing.T) {
{
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)
grpSvc.EXPECT().GetByIDs(ctx, []string{groupID}, includeDisabled).Return([]group.Group{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)
Expand All @@ -351,7 +362,7 @@ func TestService_RemoveGroupMember(t *testing.T) {
{
name: "should remove a member (non-owner) and delete the member relation",
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)
grpSvc.EXPECT().GetByIDs(ctx, []string{groupID}, includeDisabled).Return([]group.Group{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)
Expand All @@ -364,10 +375,25 @@ func TestService_RemoveGroupMember(t *testing.T) {
},
wantErr: nil,
},
{
name: "should remove a member from a disabled group and delete the member relation",
setup: func(policySvc *mocks.PolicyService, relSvc *mocks.RelationService, roleSvc *mocks.RoleService, grpSvc *mocks.GroupService, userSvc *mocks.UserService, auditRepo *mocks.AuditRecordRepository) {
grpSvc.EXPECT().GetByIDs(ctx, []string{groupID}, includeDisabled).Return([]group.Group{disabledGrp}, 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)
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.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)
grpSvc.EXPECT().GetByIDs(ctx, []string{groupID}, includeDisabled).Return([]group.Group{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)
Expand Down
14 changes: 13 additions & 1 deletion core/membership/project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"

"github.com/raystack/frontier/core/policy"
"github.com/raystack/frontier/core/project"
"github.com/raystack/frontier/core/relation"
"github.com/raystack/frontier/core/role"
"github.com/raystack/frontier/internal/bootstrap/schema"
Expand Down Expand Up @@ -61,7 +62,7 @@ func (s *Service) RemoveProjectMember(ctx context.Context, projectID, principalI
return ErrInvalidPrincipalType
}

prj, err := s.projectService.Get(ctx, projectID)
prj, err := s.getProjectIncludingDisabled(ctx, projectID)
if err != nil {
return err
}
Expand All @@ -78,6 +79,17 @@ func (s *Service) RemoveProjectMember(ctx context.Context, projectID, principalI
return nil
}

func (s *Service) getProjectIncludingDisabled(ctx context.Context, projectID string) (project.Project, error) {
projects, err := s.projectService.List(ctx, project.Filter{ProjectIDs: []string{projectID}, IncludeDisabled: true})
if err != nil {
return project.Project{}, err
}
if len(projects) == 0 {
return project.Project{}, project.ErrNotExist
}
return projects[0], nil
}

// OnProjectCreated runs right after a project is created. It links the project
// to its parent org in SpiceDB and gives the creator the project owner role.
// There is no org-membership check on the creator here: platform superusers
Expand Down
41 changes: 37 additions & 4 deletions core/membership/project_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@ func TestService_RemoveProjectMember(t *testing.T) {
Title: "Test Project",
Organization: organization.Organization{ID: uuid.New().String()},
}
disabledPrj := prj
disabledPrj.State = project.Disabled
lookupFilter := project.Filter{ProjectIDs: []string{projectID}, IncludeDisabled: true}
lookupErr := errors.New("project lookup failed")

tests := []struct {
name string
Expand All @@ -184,10 +188,28 @@ func TestService_RemoveProjectMember(t *testing.T) {
principalType: "app/invalid",
wantErr: membership.ErrInvalidPrincipalType,
},
{
name: "should return ErrNotExist if the project does not exist",
setup: func(_ *mocks.PolicyService, prjSvc *mocks.ProjectService, _ *mocks.AuditRecordRepository) {
prjSvc.EXPECT().List(ctx, lookupFilter).Return([]project.Project{}, nil)
},
principalID: userID,
principalType: schema.UserPrincipal,
wantErr: project.ErrNotExist,
},
{
name: "should return error if the project lookup fails",
setup: func(_ *mocks.PolicyService, prjSvc *mocks.ProjectService, _ *mocks.AuditRecordRepository) {
prjSvc.EXPECT().List(ctx, lookupFilter).Return(nil, lookupErr)
},
principalID: userID,
principalType: schema.UserPrincipal,
wantErr: lookupErr,
},
{
name: "should return error if not a member",
setup: func(policySvc *mocks.PolicyService, prjSvc *mocks.ProjectService, _ *mocks.AuditRecordRepository) {
prjSvc.EXPECT().Get(ctx, projectID).Return(prj, nil)
prjSvc.EXPECT().List(ctx, lookupFilter).Return([]project.Project{prj}, nil)
policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.UserPrincipal, ResourceType: schema.ProjectNamespace}).Return([]policy.Policy{}, nil)
},
principalID: userID,
Expand All @@ -197,7 +219,18 @@ func TestService_RemoveProjectMember(t *testing.T) {
{
name: "should succeed removing a user",
setup: func(policySvc *mocks.PolicyService, prjSvc *mocks.ProjectService, auditRepo *mocks.AuditRecordRepository) {
prjSvc.EXPECT().Get(ctx, projectID).Return(prj, nil)
prjSvc.EXPECT().List(ctx, lookupFilter).Return([]project.Project{prj}, nil)
policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.UserPrincipal, ResourceType: schema.ProjectNamespace}).Return([]policy.Policy{{ID: "p1"}}, nil)
policySvc.EXPECT().Delete(ctx, "p1").Return(nil)
auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
},
principalID: userID,
principalType: schema.UserPrincipal,
},
{
name: "should succeed removing a member from a disabled project",
setup: func(policySvc *mocks.PolicyService, prjSvc *mocks.ProjectService, auditRepo *mocks.AuditRecordRepository) {
prjSvc.EXPECT().List(ctx, lookupFilter).Return([]project.Project{disabledPrj}, nil)
policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.UserPrincipal, ResourceType: schema.ProjectNamespace}).Return([]policy.Policy{{ID: "p1"}}, nil)
policySvc.EXPECT().Delete(ctx, "p1").Return(nil)
auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
Expand All @@ -208,7 +241,7 @@ func TestService_RemoveProjectMember(t *testing.T) {
{
name: "should succeed removing a service user",
setup: func(policySvc *mocks.PolicyService, prjSvc *mocks.ProjectService, auditRepo *mocks.AuditRecordRepository) {
prjSvc.EXPECT().Get(ctx, projectID).Return(prj, nil)
prjSvc.EXPECT().List(ctx, lookupFilter).Return([]project.Project{prj}, nil)
policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: suID, PrincipalType: schema.ServiceUserPrincipal, ResourceType: schema.ProjectNamespace}).Return([]policy.Policy{{ID: "p1"}}, nil)
policySvc.EXPECT().Delete(ctx, "p1").Return(nil)
auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
Expand All @@ -219,7 +252,7 @@ func TestService_RemoveProjectMember(t *testing.T) {
{
name: "should succeed removing a PAT",
setup: func(policySvc *mocks.PolicyService, prjSvc *mocks.ProjectService, auditRepo *mocks.AuditRecordRepository) {
prjSvc.EXPECT().Get(ctx, projectID).Return(prj, nil)
prjSvc.EXPECT().List(ctx, lookupFilter).Return([]project.Project{prj}, nil)
policySvc.EXPECT().List(ctx, policy.Filter{ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.PATPrincipal, ResourceType: schema.ProjectNamespace}).Return([]policy.Policy{{ID: "p1"}}, nil)
policySvc.EXPECT().Delete(ctx, "p1").Return(nil)
auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil)
Expand Down
4 changes: 0 additions & 4 deletions internal/api/v1beta1connect/group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,10 +325,6 @@ func (h *ConnectHandler) ListGroupUsers(ctx context.Context, request *connect.Re
}

func (h *ConnectHandler) RemoveGroupUser(ctx context.Context, request *connect.Request[frontierv1beta1.RemoveGroupUserRequest]) (*connect.Response[frontierv1beta1.RemoveGroupUserResponse], error) {
if _, err := h.getGroup(ctx, request.Msg.GetId()); err != nil {
return nil, err
}

if err := h.membershipService.RemoveGroupMember(ctx, request.Msg.GetId(), request.Msg.GetUserId(), schema.UserPrincipal); err != nil {
switch {
case errors.Is(err, group.ErrNotExist), errors.Is(err, group.ErrInvalidID), errors.Is(err, group.ErrInvalidUUID):
Expand Down
9 changes: 0 additions & 9 deletions internal/api/v1beta1connect/group_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1153,7 +1153,6 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) {
{
name: "should return not found if group does not exist",
setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, ms *mocks.MembershipService) {
gs.EXPECT().Get(mock.Anything, randomID).Return(group.Group{ID: randomID, OrganizationID: randomID}, nil)
ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(group.ErrNotExist)
},
request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{
Expand All @@ -1166,7 +1165,6 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) {
{
name: "should return not found if user does not exist",
setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, ms *mocks.MembershipService) {
gs.EXPECT().Get(mock.Anything, randomID).Return(group.Group{ID: randomID, OrganizationID: randomID}, nil)
ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(user.ErrNotExist)
},
request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{
Expand All @@ -1179,7 +1177,6 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) {
{
name: "should return failed precondition if user is not a group member",
setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, ms *mocks.MembershipService) {
gs.EXPECT().Get(mock.Anything, randomID).Return(group.Group{ID: randomID, OrganizationID: randomID}, nil)
ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(membership.ErrNotMember)
},
request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{
Expand All @@ -1192,7 +1189,6 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) {
{
name: "should return invalid argument if user is the only owner",
setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, ms *mocks.MembershipService) {
gs.EXPECT().Get(mock.Anything, randomID).Return(group.Group{ID: randomID, OrganizationID: randomID}, nil)
ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(membership.ErrLastGroupOwnerRole)
},
request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{
Expand All @@ -1205,7 +1201,6 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) {
{
name: "should return failed precondition if user is disabled",
setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, ms *mocks.MembershipService) {
gs.EXPECT().Get(mock.Anything, randomID).Return(group.Group{ID: randomID, OrganizationID: randomID}, nil)
ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(user.ErrDisabled)
},
request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{
Expand All @@ -1218,7 +1213,6 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) {
{
name: "should return invalid argument if principal type is unsupported",
setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, ms *mocks.MembershipService) {
gs.EXPECT().Get(mock.Anything, randomID).Return(group.Group{ID: randomID, OrganizationID: randomID}, nil)
ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(membership.ErrInvalidPrincipalType)
},
request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{
Expand All @@ -1231,7 +1225,6 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) {
{
name: "should return invalid argument if principal is invalid",
setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, ms *mocks.MembershipService) {
gs.EXPECT().Get(mock.Anything, randomID).Return(group.Group{ID: randomID, OrganizationID: randomID}, nil)
ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(membership.ErrInvalidPrincipal)
},
request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{
Expand All @@ -1244,7 +1237,6 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) {
{
name: "should return internal error for unknown errors",
setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, ms *mocks.MembershipService) {
gs.EXPECT().Get(mock.Anything, randomID).Return(group.Group{ID: randomID, OrganizationID: randomID}, nil)
ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(errors.New("unknown"))
},
request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{
Expand All @@ -1257,7 +1249,6 @@ func TestConnectHandler_RemoveGroupUser(t *testing.T) {
{
name: "should remove user successfully",
setup: func(gs *mocks.GroupService, os *mocks.OrganizationService, ms *mocks.MembershipService) {
gs.EXPECT().Get(mock.Anything, randomID).Return(group.Group{ID: randomID, OrganizationID: randomID}, nil)
ms.EXPECT().RemoveGroupMember(mock.Anything, randomID, randomID, schema.UserPrincipal).Return(nil)
},
request: connect.NewRequest(&frontierv1beta1.RemoveGroupUserRequest{
Expand Down
Loading
Loading