From 1226179ecbcef7676ce973132df91b34c31646bd Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Thu, 11 Jun 2026 13:41:15 +0530 Subject: [PATCH 1/3] refactor(deleter): delegate org removal cascade to membership, drop inline RemoveUsers --- cmd/serve.go | 2 +- core/deleter/mocks/group_service.go | 49 ------ core/deleter/mocks/membership_service.go | 50 +++++- core/deleter/mocks/organization_service.go | 51 +----- core/deleter/service.go | 81 ++++------ core/deleter/service_test.go | 60 +++++++ core/group/service.go | 52 ------- core/membership/service.go | 34 +++- core/membership/service_test.go | 92 +++++++++++ .../mocks/audit_record_repository.go | 94 ----------- core/organization/service.go | 146 ++---------------- core/organization/service_test.go | 15 +- internal/api/v1beta1connect/interfaces.go | 2 - .../v1beta1connect/mocks/cascade_deleter.go | 48 ------ .../api/v1beta1connect/mocks/group_service.go | 48 ------ 15 files changed, 280 insertions(+), 544 deletions(-) delete mode 100644 core/organization/mocks/audit_record_repository.go diff --git a/cmd/serve.go b/cmd/serve.go index 079a6a299..503689349 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -435,7 +435,7 @@ func buildAPIDependencies( postgres.NewFlowRepository(logger, dbc), mailDialer, tokenService, sessionService, userService, serviceUserService, webAuthConfig, patValidator) groupService := group.NewService(groupRepository, relationService, authnService, policyService) organizationService := organization.NewService(organizationRepository, relationService, userService, - authnService, policyService, preferenceService, auditRecordRepository, roleService) + authnService, policyService, preferenceService, roleService) projectRepository := postgres.NewProjectRepository(dbc) projectService := project.NewService(projectRepository, relationService, userService, policyService, authnService, serviceUserService, groupService, roleService) diff --git a/core/deleter/mocks/group_service.go b/core/deleter/mocks/group_service.go index 2d96d9a9d..c56629d5b 100644 --- a/core/deleter/mocks/group_service.go +++ b/core/deleter/mocks/group_service.go @@ -6,7 +6,6 @@ import ( context "context" group "github.com/raystack/frontier/core/group" - mock "github.com/stretchr/testify/mock" ) @@ -129,54 +128,6 @@ func (_c *GroupService_List_Call) RunAndReturn(run func(context.Context, group.F return _c } -// RemoveUsers provides a mock function with given fields: ctx, groupID, userIDs -func (_m *GroupService) RemoveUsers(ctx context.Context, groupID string, userIDs []string) error { - ret := _m.Called(ctx, groupID, userIDs) - - if len(ret) == 0 { - panic("no return value specified for RemoveUsers") - } - - var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, []string) error); ok { - r0 = rf(ctx, groupID, userIDs) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// GroupService_RemoveUsers_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemoveUsers' -type GroupService_RemoveUsers_Call struct { - *mock.Call -} - -// RemoveUsers is a helper method to define mock.On call -// - ctx context.Context -// - groupID string -// - userIDs []string -func (_e *GroupService_Expecter) RemoveUsers(ctx interface{}, groupID interface{}, userIDs interface{}) *GroupService_RemoveUsers_Call { - return &GroupService_RemoveUsers_Call{Call: _e.mock.On("RemoveUsers", ctx, groupID, userIDs)} -} - -func (_c *GroupService_RemoveUsers_Call) Run(run func(ctx context.Context, groupID string, userIDs []string)) *GroupService_RemoveUsers_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(string), args[2].([]string)) - }) - return _c -} - -func (_c *GroupService_RemoveUsers_Call) Return(_a0 error) *GroupService_RemoveUsers_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *GroupService_RemoveUsers_Call) RunAndReturn(run func(context.Context, string, []string) error) *GroupService_RemoveUsers_Call { - _c.Call.Return(run) - return _c -} - // NewGroupService creates a new instance of GroupService. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewGroupService(t interface { diff --git a/core/deleter/mocks/membership_service.go b/core/deleter/mocks/membership_service.go index 2f2c6ccda..ad519d11c 100644 --- a/core/deleter/mocks/membership_service.go +++ b/core/deleter/mocks/membership_service.go @@ -8,7 +8,6 @@ import ( authenticate "github.com/raystack/frontier/core/authenticate" membership "github.com/raystack/frontier/core/membership" - mock "github.com/stretchr/testify/mock" ) @@ -25,6 +24,55 @@ func (_m *MembershipService) EXPECT() *MembershipService_Expecter { return &MembershipService_Expecter{mock: &_m.Mock} } +// ForceRemoveOrganizationMember provides a mock function with given fields: ctx, orgID, principalID, principalType +func (_m *MembershipService) ForceRemoveOrganizationMember(ctx context.Context, orgID string, principalID string, principalType string) error { + ret := _m.Called(ctx, orgID, principalID, principalType) + + if len(ret) == 0 { + panic("no return value specified for ForceRemoveOrganizationMember") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string, string) error); ok { + r0 = rf(ctx, orgID, principalID, principalType) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// MembershipService_ForceRemoveOrganizationMember_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ForceRemoveOrganizationMember' +type MembershipService_ForceRemoveOrganizationMember_Call struct { + *mock.Call +} + +// ForceRemoveOrganizationMember is a helper method to define mock.On call +// - ctx context.Context +// - orgID string +// - principalID string +// - principalType string +func (_e *MembershipService_Expecter) ForceRemoveOrganizationMember(ctx interface{}, orgID interface{}, principalID interface{}, principalType interface{}) *MembershipService_ForceRemoveOrganizationMember_Call { + return &MembershipService_ForceRemoveOrganizationMember_Call{Call: _e.mock.On("ForceRemoveOrganizationMember", ctx, orgID, principalID, principalType)} +} + +func (_c *MembershipService_ForceRemoveOrganizationMember_Call) Run(run func(ctx context.Context, orgID string, principalID string, principalType string)) *MembershipService_ForceRemoveOrganizationMember_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_ForceRemoveOrganizationMember_Call) Return(_a0 error) *MembershipService_ForceRemoveOrganizationMember_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MembershipService_ForceRemoveOrganizationMember_Call) RunAndReturn(run func(context.Context, string, string, string) error) *MembershipService_ForceRemoveOrganizationMember_Call { + _c.Call.Return(run) + return _c +} + // ListResourcesByPrincipal provides a mock function with given fields: ctx, principal, resourceType, filter func (_m *MembershipService) ListResourcesByPrincipal(ctx context.Context, principal authenticate.Principal, resourceType string, filter membership.ResourceFilter) ([]string, error) { ret := _m.Called(ctx, principal, resourceType, filter) diff --git a/core/deleter/mocks/organization_service.go b/core/deleter/mocks/organization_service.go index 3ef7c9532..ea1e53564 100644 --- a/core/deleter/mocks/organization_service.go +++ b/core/deleter/mocks/organization_service.go @@ -5,9 +5,8 @@ package mocks import ( context "context" - mock "github.com/stretchr/testify/mock" - organization "github.com/raystack/frontier/core/organization" + mock "github.com/stretchr/testify/mock" ) // OrganizationService is an autogenerated mock type for the OrganizationService type @@ -127,54 +126,6 @@ func (_c *OrganizationService_Get_Call) RunAndReturn(run func(context.Context, s return _c } -// RemoveUsers provides a mock function with given fields: ctx, orgID, userIDs -func (_m *OrganizationService) RemoveUsers(ctx context.Context, orgID string, userIDs []string) error { - ret := _m.Called(ctx, orgID, userIDs) - - if len(ret) == 0 { - panic("no return value specified for RemoveUsers") - } - - var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, []string) error); ok { - r0 = rf(ctx, orgID, userIDs) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// OrganizationService_RemoveUsers_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemoveUsers' -type OrganizationService_RemoveUsers_Call struct { - *mock.Call -} - -// RemoveUsers is a helper method to define mock.On call -// - ctx context.Context -// - orgID string -// - userIDs []string -func (_e *OrganizationService_Expecter) RemoveUsers(ctx interface{}, orgID interface{}, userIDs interface{}) *OrganizationService_RemoveUsers_Call { - return &OrganizationService_RemoveUsers_Call{Call: _e.mock.On("RemoveUsers", ctx, orgID, userIDs)} -} - -func (_c *OrganizationService_RemoveUsers_Call) Run(run func(ctx context.Context, orgID string, userIDs []string)) *OrganizationService_RemoveUsers_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(string), args[2].([]string)) - }) - return _c -} - -func (_c *OrganizationService_RemoveUsers_Call) Return(_a0 error) *OrganizationService_RemoveUsers_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *OrganizationService_RemoveUsers_Call) RunAndReturn(run func(context.Context, string, []string) error) *OrganizationService_RemoveUsers_Call { - _c.Call.Return(run) - return _c -} - // NewOrganizationService creates a new instance of OrganizationService. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewOrganizationService(t interface { diff --git a/core/deleter/service.go b/core/deleter/service.go index f1490b857..7b7dc58b5 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -44,7 +44,6 @@ type ProjectService interface { type OrganizationService interface { Get(ctx context.Context, id string) (organization.Organization, error) DeleteModel(ctx context.Context, id string) error - RemoveUsers(ctx context.Context, orgID string, userIDs []string) error } type RoleService interface { @@ -66,12 +65,12 @@ type ResourceService interface { type GroupService interface { List(ctx context.Context, flt group.Filter) ([]group.Group, error) DeleteModel(ctx context.Context, id string) error - RemoveUsers(ctx context.Context, groupID string, userIDs []string) error } type MembershipService interface { OnGroupDeleted(ctx context.Context, groupID string) error ListResourcesByPrincipal(ctx context.Context, principal authenticate.Principal, resourceType string, filter membership.ResourceFilter) ([]string, error) + ForceRemoveOrganizationMember(ctx context.Context, orgID, principalID, principalType string) error } type InvitationService interface { @@ -308,10 +307,13 @@ func (d Service) DeleteCustomers(ctx context.Context, id string) error { return nil } -// RemoveUsersFromOrg removes users from an organization as members +// RemoveUsersFromOrg removes users from an organization as members. The +// org/project/group policy and relation cleanup is delegated to +// membership.ForceRemoveOrganizationMember — the force variant because a +// deletion cascade must succeed even when the user is the org's last owner. +// Custom-resource policies are outside the membership cascade's scope, so +// they are cleaned up here first. func (d Service) RemoveUsersFromOrg(ctx context.Context, orgID string, userIDs []string) error { - var err error - orgProjects, err := d.projService.List(ctx, project.Filter{ OrgID: orgID, }) @@ -321,69 +323,46 @@ func (d Service) RemoveUsersFromOrg(ctx context.Context, orgID string, userIDs [ orgProjectIDs := utils.Map(orgProjects, func(p project.Project) string { return p.ID }) - // it's cheaper to fetch all groups in an org instead of fetching what all groups a user is part of - orgGroups, err := d.groupService.List(ctx, group.Filter{ - OrganizationID: orgID, - }) - if err != nil && !errors.Is(err, group.ErrNotExist) { - return err - } - orgGroupIDs := utils.Map(orgGroups, func(g group.Group) string { - return g.ID - }) + var errs error for _, userID := range userIDs { userPolicies, policyErr := d.policyService.List(ctx, policy.Filter{ PrincipalID: userID, PrincipalType: schema.UserPrincipal, }) - if policyErr != nil && !errors.Is(err, policy.ErrNotExist) { - err = errors.Join(err, policyErr) + if policyErr != nil && !errors.Is(policyErr, policy.ErrNotExist) { + errs = errors.Join(errs, policyErr) continue } for _, pol := range userPolicies { - // delete org level roles switch pol.ResourceType { - case schema.ProjectNamespace: - // delete project level policies - if utils.Contains(orgProjectIDs, pol.ResourceID) { - if policyErr := d.policyService.Delete(ctx, pol.ID); policyErr != nil { - err = errors.Join(err, policyErr) - } - } - case schema.GroupNamespace: - // delete group level policies - if utils.Contains(orgGroupIDs, pol.ResourceID) { - if groupErr := d.groupService.RemoveUsers(ctx, pol.ResourceID, []string{userID}); groupErr != nil { - err = errors.Join(err, groupErr) - } - } - case schema.PlatformNamespace, schema.OrganizationNamespace: - // do nothing + case schema.OrganizationNamespace, schema.ProjectNamespace, schema.GroupNamespace, schema.PlatformNamespace: + // org/project/group policies are handled by the membership + // cascade below; platform policies are out of scope here default: - // delete resource level policies + // delete custom-resource policies for resources owned by org projects userResource, resErr := d.resService.Get(ctx, pol.ResourceID) - if !errors.Is(resErr, resource.ErrNotExist) { - if resErr != nil { - err = errors.Join(err, resErr) - } else if userResource.ProjectID != "" && utils.Contains(orgProjectIDs, userResource.ProjectID) { - // if the resource belong to org project, delete access - if policyErr := d.policyService.Delete(ctx, pol.ID); policyErr != nil { - err = errors.Join(err, policyErr) - } + if errors.Is(resErr, resource.ErrNotExist) { + continue + } + if resErr != nil { + errs = errors.Join(errs, resErr) + continue + } + if userResource.ProjectID != "" && utils.Contains(orgProjectIDs, userResource.ProjectID) { + if policyErr := d.policyService.Delete(ctx, pol.ID); policyErr != nil { + errs = errors.Join(errs, policyErr) } } - } // switch ends + } } - } - if err != nil { - // abort if any error occurred - return err - } - // remove user from org - return d.orgService.RemoveUsers(ctx, orgID, userIDs) + if memberErr := d.membershipService.ForceRemoveOrganizationMember(ctx, orgID, userID, schema.UserPrincipal); memberErr != nil { + errs = errors.Join(errs, memberErr) + } + } + return errs } // DeleteUser visits every org the user has a policy on (disabled orgs too), diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index 459324823..850f302c6 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -258,6 +258,66 @@ func TestDeleteCustomers(t *testing.T) { }) } +func TestRemoveUsersFromOrg(t *testing.T) { + const orgID = "org-1" + const userID = "user-1" + + t.Run("delegates org removal to membership force-remove", func(t *testing.T) { + orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + + projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: orgID}).Return([]project.Project{}, nil) + polSvc.EXPECT().List(mock.Anything, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}). + Return([]policy.Policy{ + {ID: "org-p1", ResourceType: schema.OrganizationNamespace, ResourceID: orgID}, + {ID: "proj-p1", ResourceType: schema.ProjectNamespace, ResourceID: "proj-1"}, + }, nil) + // org/project policies are left to the membership cascade + mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, orgID, userID, schema.UserPrincipal).Return(nil) + + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc) + err := svc.RemoveUsersFromOrg(context.Background(), orgID, []string{userID}) + assert.NoError(t, err) + }) + + t.Run("cleans custom-resource policies owned by org projects", func(t *testing.T) { + orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + + projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: orgID}).Return([]project.Project{{ID: "proj-1"}}, nil) + polSvc.EXPECT().List(mock.Anything, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}). + Return([]policy.Policy{ + {ID: "res-p1", ResourceType: "compute/instance", ResourceID: "res-1"}, + {ID: "res-p2", ResourceType: "compute/instance", ResourceID: "res-2"}, + {ID: "res-p3", ResourceType: "compute/instance", ResourceID: "res-3"}, + }, nil) + // res-1 belongs to an org project: policy deleted + resSvc.EXPECT().Get(mock.Anything, "res-1").Return(resource.Resource{ID: "res-1", ProjectID: "proj-1"}, nil) + polSvc.EXPECT().Delete(mock.Anything, "res-p1").Return(nil) + // res-2 belongs to a project outside the org: untouched + resSvc.EXPECT().Get(mock.Anything, "res-2").Return(resource.Resource{ID: "res-2", ProjectID: "other-proj"}, nil) + // res-3 no longer exists: skipped + resSvc.EXPECT().Get(mock.Anything, "res-3").Return(resource.Resource{}, resource.ErrNotExist) + mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, orgID, userID, schema.UserPrincipal).Return(nil) + + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc) + err := svc.RemoveUsersFromOrg(context.Background(), orgID, []string{userID}) + assert.NoError(t, err) + }) + + t.Run("propagates membership cascade failure", func(t *testing.T) { + orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + + projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: orgID}).Return([]project.Project{}, nil) + polSvc.EXPECT().List(mock.Anything, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}). + Return([]policy.Policy{}, nil) + mbrSvc.EXPECT().ForceRemoveOrganizationMember(mock.Anything, orgID, userID, schema.UserPrincipal). + Return(errors.New("cascade boom")) + + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc) + err := svc.RemoveUsersFromOrg(context.Background(), orgID, []string{userID}) + assert.ErrorContains(t, err, "cascade boom") + }) +} + func TestDeleteUser(t *testing.T) { t.Run("removes user from all orgs, cleans PATs, then deletes user", func(t *testing.T) { orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) diff --git a/core/group/service.go b/core/group/service.go index 79ab5cd3d..cfabd0d7c 100644 --- a/core/group/service.go +++ b/core/group/service.go @@ -174,16 +174,6 @@ func (s Service) ListByOrganization(ctx context.Context, id string) ([]Group, er return s.repository.GetByIDs(ctx, groupIDs, Filter{}) } -// RemoveUsers removes users from a group as members -func (s Service) RemoveUsers(ctx context.Context, groupID string, userIDs []string) error { - group, err := s.repository.GetByID(ctx, groupID) - if err != nil { - return err - } - - return s.removeUsers(ctx, group, userIDs) -} - func (s Service) Enable(ctx context.Context, id string) error { return s.repository.SetState(ctx, id, Enabled) } @@ -214,45 +204,3 @@ func (s Service) DeleteModel(ctx context.Context, id string) error { return audit.NewLogger(ctx, group.OrganizationID).Log(audit.GroupDeletedEvent, audit.GroupTarget(id)) } - -func (s Service) removeUsers(ctx context.Context, group Group, userIDs []string) error { - var err error - - for _, userID := range userIDs { - // remove all access via policies - userPolicies, currentErr := s.policyService.List(ctx, policy.Filter{ - GroupID: group.ID, - PrincipalID: userID, - }) - if currentErr != nil && !errors.Is(currentErr, policy.ErrNotExist) { - err = errors.Join(err, currentErr) - continue - } - for _, pol := range userPolicies { - if policyErr := s.policyService.Delete(ctx, pol.ID); policyErr != nil { - err = errors.Join(err, policyErr) - } - } - - // remove all relations - if currentErr := s.relationService.Delete(ctx, relation.Relation{ - Object: relation.Object{ - ID: group.ID, - Namespace: schema.GroupNamespace, - }, - Subject: relation.Subject{ - ID: userID, - }, - }); currentErr != nil { - err = errors.Join(err, currentErr) - } - - if currentErr == nil { - audit.GetAuditor(ctx, group.OrganizationID).LogWithAttrs(audit.GroupMemberRemovedEvent, audit.GroupTarget(group.ID), map[string]string{ - "userID": userID, - }) - } - } - - return err -} diff --git a/core/membership/service.go b/core/membership/service.go index 5076c6997..3d63b6501 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -378,8 +378,23 @@ func (s *Service) removePoliciesByFilter(ctx context.Context, filter policy.Filt // RemoveOrganizationMember removes a principal from an organization and cascades // the removal through all org projects and groups, cleaning up both policies and -// relations. Returns ErrNotMember if the principal has no policies on this org. +// relations. Returns ErrNotMember if the principal has no policies on this org, +// and ErrLastOwnerRole when removing the org's last owner. func (s *Service) RemoveOrganizationMember(ctx context.Context, orgID, principalID, principalType string) error { + return s.removeOrganizationMember(ctx, orgID, principalID, principalType, true) +} + +// ForceRemoveOrganizationMember is RemoveOrganizationMember without the +// member-and-owner guards: it does not fail when the principal has no +// org-level policies and it removes the org's last owner without complaint. +// It exists for deletion cascades (see core/deleter), where the principal is +// leaving the system entirely and the org may legitimately be left ownerless. +// Anything user-initiated should go through RemoveOrganizationMember instead. +func (s *Service) ForceRemoveOrganizationMember(ctx context.Context, orgID, principalID, principalType string) error { + return s.removeOrganizationMember(ctx, orgID, principalID, principalType, false) +} + +func (s *Service) removeOrganizationMember(ctx context.Context, orgID, principalID, principalType string, guarded bool) error { targetAuditType, err := principalTypeToAuditType(principalType) if err != nil { return err @@ -399,16 +414,19 @@ func (s *Service) RemoveOrganizationMember(ctx context.Context, orgID, principal if err != nil { return fmt.Errorf("list existing policies: %w", err) } - if len(orgPolicies) == 0 { - return ErrNotMember - } // only humans can be the last owner — skip for service users and PATs. + // an empty ownerRoleID makes the cascade delete owner policies unguarded. var ownerRoleID string - if principalType == schema.UserPrincipal { - ownerRoleID, err = s.validateMinOwnerConstraint(ctx, orgID, "", orgPolicies) - if err != nil { - return err + if guarded { + if len(orgPolicies) == 0 { + return ErrNotMember + } + if principalType == schema.UserPrincipal { + ownerRoleID, err = s.validateMinOwnerConstraint(ctx, orgID, "", orgPolicies) + if err != nil { + return err + } } } diff --git a/core/membership/service_test.go b/core/membership/service_test.go index 7ee7576bf..3eda558d6 100644 --- a/core/membership/service_test.go +++ b/core/membership/service_test.go @@ -996,6 +996,98 @@ func TestService_RemoveOrganizationMember(t *testing.T) { } } +func TestService_ForceRemoveOrganizationMember(t *testing.T) { + ctx := context.Background() + orgID := uuid.New().String() + userID := uuid.New().String() + ownerRoleID := uuid.New().String() + + enabledOrg := organization.Organization{ID: orgID, Title: "Test Org"} + orgObj := relation.Object{ID: orgID, Namespace: schema.OrganizationNamespace} + userSub := relation.Subject{ID: userID, Namespace: schema.UserPrincipal} + + type testDeps struct { + policySvc *mocks.PolicyService + relSvc *mocks.RelationService + roleSvc *mocks.RoleService + orgSvc *mocks.OrgService + projSvc *mocks.ProjectService + grpSvc *mocks.GroupService + auditRepo *mocks.AuditRecordRepository + } + + tests := []struct { + name string + setup func(d testDeps) + wantErr error + }{ + { + name: "removes the org's last owner without the guard", + setup: func(d testDeps) { + d.orgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil) + d.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "org-p1", RoleID: ownerRoleID}}, nil) + // no roleSvc.Get / owner-count query: the guard is skipped entirely + d.projSvc.EXPECT().List(ctx, project.Filter{OrgID: orgID}).Return([]project.Project{}, nil) + d.grpSvc.EXPECT().List(ctx, group.Filter{OrganizationID: orgID}).Return([]group.Group{}, nil) + d.policySvc.EXPECT().List(ctx, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{ + {ID: "org-p1", ResourceType: schema.OrganizationNamespace, ResourceID: orgID, RoleID: ownerRoleID}, + }, nil) + // plain delete, not DeleteWithMinRoleGuard + d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil) + d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(nil) + d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(relation.ErrNotExist) + d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) + }, + }, + { + name: "does not return ErrNotMember when principal has no org policies and still cascades", + setup: func(d testDeps) { + d.orgSvc.EXPECT().Get(ctx, orgID).Return(enabledOrg, nil) + d.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil) + d.projSvc.EXPECT().List(ctx, project.Filter{OrgID: orgID}).Return([]project.Project{}, nil) + d.grpSvc.EXPECT().List(ctx, group.Filter{OrganizationID: orgID}).Return([]group.Group{}, nil) + d.policySvc.EXPECT().List(ctx, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{}, nil) + d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(relation.ErrNotExist) + d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(relation.ErrNotExist) + d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) + }, + }, + { + name: "propagates org lookup failure", + setup: func(d testDeps) { + d.orgSvc.EXPECT().Get(ctx, orgID).Return(organization.Organization{}, organization.ErrNotExist) + }, + wantErr: organization.ErrNotExist, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := testDeps{ + policySvc: mocks.NewPolicyService(t), + relSvc: mocks.NewRelationService(t), + roleSvc: mocks.NewRoleService(t), + orgSvc: mocks.NewOrgService(t), + projSvc: mocks.NewProjectService(t), + grpSvc: mocks.NewGroupService(t), + auditRepo: mocks.NewAuditRecordRepository(t), + } + if tt.setup != nil { + tt.setup(d) + } + + svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), d.policySvc, d.relSvc, d.roleSvc, d.orgSvc, mocks.NewUserService(t), d.projSvc, d.grpSvc, mocks.NewServiceuserService(t), d.auditRepo) + + err := svc.ForceRemoveOrganizationMember(ctx, orgID, userID, schema.UserPrincipal) + if tt.wantErr != nil { + assert.ErrorIs(t, err, tt.wantErr) + } else { + assert.NoError(t, err) + } + }) + } +} + func TestService_SetProjectMemberRole(t *testing.T) { ctx := context.Background() projectID := uuid.New().String() diff --git a/core/organization/mocks/audit_record_repository.go b/core/organization/mocks/audit_record_repository.go deleted file mode 100644 index 87c61b8a3..000000000 --- a/core/organization/mocks/audit_record_repository.go +++ /dev/null @@ -1,94 +0,0 @@ -// Code generated by mockery v2.53.5. DO NOT EDIT. - -package mocks - -import ( - context "context" - - models "github.com/raystack/frontier/core/auditrecord/models" - mock "github.com/stretchr/testify/mock" -) - -// AuditRecordRepository is an autogenerated mock type for the AuditRecordRepository type -type AuditRecordRepository struct { - mock.Mock -} - -type AuditRecordRepository_Expecter struct { - mock *mock.Mock -} - -func (_m *AuditRecordRepository) EXPECT() *AuditRecordRepository_Expecter { - return &AuditRecordRepository_Expecter{mock: &_m.Mock} -} - -// Create provides a mock function with given fields: ctx, auditRecord -func (_m *AuditRecordRepository) Create(ctx context.Context, auditRecord models.AuditRecord) (models.AuditRecord, error) { - ret := _m.Called(ctx, auditRecord) - - if len(ret) == 0 { - panic("no return value specified for Create") - } - - var r0 models.AuditRecord - var r1 error - if rf, ok := ret.Get(0).(func(context.Context, models.AuditRecord) (models.AuditRecord, error)); ok { - return rf(ctx, auditRecord) - } - if rf, ok := ret.Get(0).(func(context.Context, models.AuditRecord) models.AuditRecord); ok { - r0 = rf(ctx, auditRecord) - } else { - r0 = ret.Get(0).(models.AuditRecord) - } - - if rf, ok := ret.Get(1).(func(context.Context, models.AuditRecord) error); ok { - r1 = rf(ctx, auditRecord) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - -// AuditRecordRepository_Create_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Create' -type AuditRecordRepository_Create_Call struct { - *mock.Call -} - -// Create is a helper method to define mock.On call -// - ctx context.Context -// - auditRecord models.AuditRecord -func (_e *AuditRecordRepository_Expecter) Create(ctx interface{}, auditRecord interface{}) *AuditRecordRepository_Create_Call { - return &AuditRecordRepository_Create_Call{Call: _e.mock.On("Create", ctx, auditRecord)} -} - -func (_c *AuditRecordRepository_Create_Call) Run(run func(ctx context.Context, auditRecord models.AuditRecord)) *AuditRecordRepository_Create_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(models.AuditRecord)) - }) - return _c -} - -func (_c *AuditRecordRepository_Create_Call) Return(_a0 models.AuditRecord, _a1 error) *AuditRecordRepository_Create_Call { - _c.Call.Return(_a0, _a1) - return _c -} - -func (_c *AuditRecordRepository_Create_Call) RunAndReturn(run func(context.Context, models.AuditRecord) (models.AuditRecord, error)) *AuditRecordRepository_Create_Call { - _c.Call.Return(run) - return _c -} - -// NewAuditRecordRepository creates a new instance of AuditRecordRepository. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. -// The first argument is typically a *testing.T value. -func NewAuditRecordRepository(t interface { - mock.TestingT - Cleanup(func()) -}) *AuditRecordRepository { - mock := &AuditRecordRepository{} - mock.Mock.Test(t) - - t.Cleanup(func() { mock.AssertExpectations(t) }) - - return mock -} diff --git a/core/organization/service.go b/core/organization/service.go index a6b25da27..45d80ab1e 100644 --- a/core/organization/service.go +++ b/core/organization/service.go @@ -4,10 +4,8 @@ import ( "context" "errors" "fmt" - "time" "github.com/raystack/frontier/core/audit" - "github.com/raystack/frontier/core/auditrecord" "github.com/raystack/frontier/core/preference" @@ -16,7 +14,6 @@ import ( "github.com/raystack/frontier/core/authenticate" - pkgAuditRecord "github.com/raystack/frontier/pkg/auditrecord" "github.com/raystack/frontier/pkg/str" "github.com/raystack/frontier/pkg/utils" @@ -64,10 +61,6 @@ type PreferencesService interface { LoadPlatformPreferences(ctx context.Context) (map[string]string, error) } -type AuditRecordRepository interface { - Create(ctx context.Context, auditRecord auditrecord.AuditRecord) (auditrecord.AuditRecord, error) -} - type RoleService interface { Get(ctx context.Context, idOrName string) (role.Role, error) } @@ -78,30 +71,27 @@ type MembershipService interface { } type Service struct { - repository Repository - relationService RelationService - userService UserService - authnService AuthnService - policyService PolicyService - prefService PreferencesService - auditRecordRepository AuditRecordRepository - roleService RoleService - membershipService MembershipService + repository Repository + relationService RelationService + userService UserService + authnService AuthnService + policyService PolicyService + prefService PreferencesService + roleService RoleService + membershipService MembershipService } func NewService(repository Repository, relationService RelationService, userService UserService, authnService AuthnService, policyService PolicyService, - prefService PreferencesService, auditRecordRepository AuditRecordRepository, - roleService RoleService) *Service { + prefService PreferencesService, roleService RoleService) *Service { return &Service{ - repository: repository, - relationService: relationService, - userService: userService, - authnService: authnService, - policyService: policyService, - prefService: prefService, - auditRecordRepository: auditRecordRepository, - roleService: roleService, + repository: repository, + relationService: relationService, + userService: userService, + authnService: authnService, + policyService: policyService, + prefService: prefService, + roleService: roleService, } } @@ -111,31 +101,6 @@ func (s *Service) SetMembershipService(ms MembershipService) { s.membershipService = ms } -// extractPrincipalInfo extracts display name and metadata from principal -func extractPrincipalInfo(principal authenticate.Principal) (string, map[string]any) { - metadata := make(map[string]any) - var name string - - if principal.PAT != nil { - name = principal.PAT.Title - if principal.User != nil { - metadata["user"] = map[string]any{ - "id": principal.User.ID, - "name": principal.User.Name, - "title": principal.User.Title, - "email": principal.User.Email, - } - } - } else if principal.User != nil { - name = principal.User.Title - metadata["email"] = principal.User.Email - } else if principal.ServiceUser != nil { - name = principal.ServiceUser.Title - } - - return name, metadata -} - // Get returns an enabled organization by id or name. Will return `org is disabled` error if the organization is disabled func (s Service) Get(ctx context.Context, idOrName string) (Organization, error) { if utils.IsValidUUID(idOrName) { @@ -280,85 +245,6 @@ func (s Service) Update(ctx context.Context, org Organization) (Organization, er return s.repository.UpdateByName(ctx, org) } -// RemoveUsers removes users from an organization as members -// it doesn't remove user access to projects or other resources provided -// by policies, don't call directly, use cascade deleter -func (s Service) RemoveUsers(ctx context.Context, orgID string, userIDs []string) error { - // Fetch organization once for all users - org, orgErr := s.repository.GetByID(ctx, orgID) - if orgErr != nil { - return orgErr - } - - var err error - for _, userID := range userIDs { - // remove all access via policies - userPolicies, currErr := s.policyService.List(ctx, policy.Filter{ - OrgID: orgID, - PrincipalID: userID, - }) - if currErr != nil && !errors.Is(currErr, policy.ErrNotExist) { - err = errors.Join(err, currErr) - continue - } - for _, pol := range userPolicies { - if policyErr := s.policyService.Delete(ctx, pol.ID); policyErr != nil { - err = errors.Join(err, policyErr) - } - } - - // remove user from org - if currentErr := s.relationService.Delete(ctx, relation.Relation{ - Object: relation.Object{ - ID: orgID, - Namespace: schema.OrganizationNamespace, - }, - Subject: relation.Subject{ - ID: userID, - Namespace: schema.UserPrincipal, - }, - }); currentErr != nil { - err = errors.Join(err, currentErr) - } - - // Get user details for audit record - usr, userErr := s.userService.GetByID(ctx, userID) - var userName string - var userMetadata map[string]any - if userErr == nil { - userName, userMetadata = extractPrincipalInfo(authenticate.Principal{ - ID: usr.ID, - Type: schema.UserPrincipal, - User: &usr, - }) - } - - // Create audit record for member removal - _, auditErr := s.auditRecordRepository.Create(ctx, auditrecord.AuditRecord{ - Event: pkgAuditRecord.OrganizationMemberRemovedEvent, - Resource: auditrecord.Resource{ - ID: orgID, - Type: pkgAuditRecord.OrganizationType, - Name: org.Title, - }, - Target: &auditrecord.Target{ - ID: userID, - Type: pkgAuditRecord.UserType, - Name: userName, - Metadata: userMetadata, - }, - OrgID: orgID, - OccurredAt: time.Now(), - }) - if auditErr != nil { - err = errors.Join(err, auditErr) - } - - audit.GetAuditor(ctx, orgID).Log(audit.OrgMemberDeletedEvent, audit.UserTarget(userID)) - } - return err -} - func (s Service) Enable(ctx context.Context, id string) error { return s.repository.SetState(ctx, id, Enabled) } diff --git a/core/organization/service_test.go b/core/organization/service_test.go index f9d0fbd1a..e88f8399e 100644 --- a/core/organization/service_test.go +++ b/core/organization/service_test.go @@ -22,10 +22,9 @@ func TestService_Get(t *testing.T) { mockAuthnSvc := mocks.NewAuthnService(t) mockPolicySvc := mocks.NewPolicyService(t) mockPrefSvc := mocks.NewPreferencesService(t) - mockAuditRecordRepo := mocks.NewAuditRecordRepository(t) mockRoleSvc := mocks.NewRoleService(t) - svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockAuditRecordRepo, mockRoleSvc) + svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockRoleSvc) t.Run("should return orgs when fetched by id (by calling repo.GetByID)", func(t *testing.T) { IDParam := uuid.New() @@ -81,10 +80,9 @@ func TestService_GetRaw(t *testing.T) { mockAuthnSvc := mocks.NewAuthnService(t) mockPolicySvc := mocks.NewPolicyService(t) mockPrefSvc := mocks.NewPreferencesService(t) - mockAuditRecordRepo := mocks.NewAuditRecordRepository(t) mockRoleSvc := mocks.NewRoleService(t) - svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockAuditRecordRepo, mockRoleSvc) + svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockRoleSvc) t.Run("should return an org based on ID passed", func(t *testing.T) { IDParam := uuid.New() @@ -139,10 +137,9 @@ func TestService_GetDefaultOrgStateOnCreate(t *testing.T) { mockAuthnSvc := mocks.NewAuthnService(t) mockPolicySvc := mocks.NewPolicyService(t) mockPrefSvc := mocks.NewPreferencesService(t) - mockAuditRecordRepo := mocks.NewAuditRecordRepository(t) mockRoleSvc := mocks.NewRoleService(t) - svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockAuditRecordRepo, mockRoleSvc) + svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockRoleSvc) t.Run("should return org state to be set on creation, as per preferences", func(t *testing.T) { expectedPrefs := map[string]string{ @@ -171,10 +168,9 @@ func TestService_AttachToPlatform(t *testing.T) { mockAuthnSvc := mocks.NewAuthnService(t) mockPolicySvc := mocks.NewPolicyService(t) mockPrefSvc := mocks.NewPreferencesService(t) - mockAuditRecordRepo := mocks.NewAuditRecordRepository(t) mockRoleSvc := mocks.NewRoleService(t) - svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockAuditRecordRepo, mockRoleSvc) + svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockRoleSvc) inputOrgID := "some-org-id" relationToBeCreated := relation.Relation{ @@ -214,10 +210,9 @@ func TestService_List(t *testing.T) { mockAuthnSvc := mocks.NewAuthnService(t) mockPolicySvc := mocks.NewPolicyService(t) mockPrefSvc := mocks.NewPreferencesService(t) - mockAuditRecordRepo := mocks.NewAuditRecordRepository(t) mockRoleSvc := mocks.NewRoleService(t) svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, - mockPolicySvc, mockPrefSvc, mockAuditRecordRepo, mockRoleSvc) + mockPolicySvc, mockPrefSvc, mockRoleSvc) return svc, mockRepo } diff --git a/internal/api/v1beta1connect/interfaces.go b/internal/api/v1beta1connect/interfaces.go index c72a37628..3cc04b874 100644 --- a/internal/api/v1beta1connect/interfaces.go +++ b/internal/api/v1beta1connect/interfaces.go @@ -302,7 +302,6 @@ type GroupService interface { GetByIDs(ctx context.Context, ids []string) ([]group.Group, error) List(ctx context.Context, flt group.Filter) ([]group.Group, error) Update(ctx context.Context, grp group.Group) (group.Group, error) - RemoveUsers(ctx context.Context, groupID string, userID []string) error Enable(ctx context.Context, id string) error Disable(ctx context.Context, id string) error } @@ -386,7 +385,6 @@ type CascadeDeleter interface { DeleteProject(ctx context.Context, id string) error DeleteOrganization(ctx context.Context, id string) error DeleteGroup(ctx context.Context, id string) error - RemoveUsersFromOrg(ctx context.Context, orgID string, userIDs []string) error DeleteUser(ctx context.Context, userID string) error } diff --git a/internal/api/v1beta1connect/mocks/cascade_deleter.go b/internal/api/v1beta1connect/mocks/cascade_deleter.go index cb3c272db..6805e1323 100644 --- a/internal/api/v1beta1connect/mocks/cascade_deleter.go +++ b/internal/api/v1beta1connect/mocks/cascade_deleter.go @@ -209,54 +209,6 @@ func (_c *CascadeDeleter_DeleteUser_Call) RunAndReturn(run func(context.Context, return _c } -// RemoveUsersFromOrg provides a mock function with given fields: ctx, orgID, userIDs -func (_m *CascadeDeleter) RemoveUsersFromOrg(ctx context.Context, orgID string, userIDs []string) error { - ret := _m.Called(ctx, orgID, userIDs) - - if len(ret) == 0 { - panic("no return value specified for RemoveUsersFromOrg") - } - - var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, []string) error); ok { - r0 = rf(ctx, orgID, userIDs) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// CascadeDeleter_RemoveUsersFromOrg_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemoveUsersFromOrg' -type CascadeDeleter_RemoveUsersFromOrg_Call struct { - *mock.Call -} - -// RemoveUsersFromOrg is a helper method to define mock.On call -// - ctx context.Context -// - orgID string -// - userIDs []string -func (_e *CascadeDeleter_Expecter) RemoveUsersFromOrg(ctx interface{}, orgID interface{}, userIDs interface{}) *CascadeDeleter_RemoveUsersFromOrg_Call { - return &CascadeDeleter_RemoveUsersFromOrg_Call{Call: _e.mock.On("RemoveUsersFromOrg", ctx, orgID, userIDs)} -} - -func (_c *CascadeDeleter_RemoveUsersFromOrg_Call) Run(run func(ctx context.Context, orgID string, userIDs []string)) *CascadeDeleter_RemoveUsersFromOrg_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(string), args[2].([]string)) - }) - return _c -} - -func (_c *CascadeDeleter_RemoveUsersFromOrg_Call) Return(_a0 error) *CascadeDeleter_RemoveUsersFromOrg_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *CascadeDeleter_RemoveUsersFromOrg_Call) RunAndReturn(run func(context.Context, string, []string) error) *CascadeDeleter_RemoveUsersFromOrg_Call { - _c.Call.Return(run) - return _c -} - // NewCascadeDeleter creates a new instance of CascadeDeleter. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewCascadeDeleter(t interface { diff --git a/internal/api/v1beta1connect/mocks/group_service.go b/internal/api/v1beta1connect/mocks/group_service.go index 6b5512354..6ae0b5d23 100644 --- a/internal/api/v1beta1connect/mocks/group_service.go +++ b/internal/api/v1beta1connect/mocks/group_service.go @@ -348,54 +348,6 @@ func (_c *GroupService_List_Call) RunAndReturn(run func(context.Context, group.F return _c } -// RemoveUsers provides a mock function with given fields: ctx, groupID, userID -func (_m *GroupService) RemoveUsers(ctx context.Context, groupID string, userID []string) error { - ret := _m.Called(ctx, groupID, userID) - - if len(ret) == 0 { - panic("no return value specified for RemoveUsers") - } - - var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string, []string) error); ok { - r0 = rf(ctx, groupID, userID) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// GroupService_RemoveUsers_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemoveUsers' -type GroupService_RemoveUsers_Call struct { - *mock.Call -} - -// RemoveUsers is a helper method to define mock.On call -// - ctx context.Context -// - groupID string -// - userID []string -func (_e *GroupService_Expecter) RemoveUsers(ctx interface{}, groupID interface{}, userID interface{}) *GroupService_RemoveUsers_Call { - return &GroupService_RemoveUsers_Call{Call: _e.mock.On("RemoveUsers", ctx, groupID, userID)} -} - -func (_c *GroupService_RemoveUsers_Call) Run(run func(ctx context.Context, groupID string, userID []string)) *GroupService_RemoveUsers_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(string), args[2].([]string)) - }) - return _c -} - -func (_c *GroupService_RemoveUsers_Call) Return(_a0 error) *GroupService_RemoveUsers_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *GroupService_RemoveUsers_Call) RunAndReturn(run func(context.Context, string, []string) error) *GroupService_RemoveUsers_Call { - _c.Call.Return(run) - return _c -} - // Update provides a mock function with given fields: ctx, grp func (_m *GroupService) Update(ctx context.Context, grp group.Group) (group.Group, error) { ret := _m.Called(ctx, grp) From c0ebbe4cd9827fd0f6e27bb28b5c29e24568e01b Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Fri, 12 Jun 2026 15:36:47 +0530 Subject: [PATCH 2/3] test(deleter): add regression case for policy-list error in RemoveUsersFromOrg --- core/deleter/service_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index 850f302c6..8adebf737 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -316,6 +316,19 @@ func TestRemoveUsersFromOrg(t *testing.T) { err := svc.RemoveUsersFromOrg(context.Background(), orgID, []string{userID}) assert.ErrorContains(t, err, "cascade boom") }) + + t.Run("surfaces policy-list error and skips membership removal for that user", func(t *testing.T) { + orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc := newMocks(t) + + projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: orgID}).Return([]project.Project{}, nil) + polSvc.EXPECT().List(mock.Anything, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}). + Return(nil, errors.New("policy list boom")) + // ForceRemoveOrganizationMember must NOT be called — strict mock fails on unexpected call + + svc := deleter.NewCascadeDeleter(orgSvc, projSvc, resSvc, grpSvc, mbrSvc, polSvc, roleSvc, invSvc, usrSvc, patSvc, suSvc, custSvc, subSvc, invocSvc) + err := svc.RemoveUsersFromOrg(context.Background(), orgID, []string{userID}) + assert.ErrorContains(t, err, "policy list boom") + }) } func TestDeleteUser(t *testing.T) { From 1cfd97757162886e2ba5cf3b350483a25ac6dc03 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Fri, 12 Jun 2026 16:26:03 +0530 Subject: [PATCH 3/3] fix(membership): let force removal cascade proceed on disabled orgs --- core/membership/service.go | 9 ++++++++- core/membership/service_test.go | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/core/membership/service.go b/core/membership/service.go index 3d63b6501..4fa9591f4 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -402,7 +402,14 @@ func (s *Service) removeOrganizationMember(ctx context.Context, orgID, principal org, err := s.orgService.Get(ctx, orgID) if err != nil { - return err + // deletion cascades must keep working on disabled orgs — the deleter + // visits them deliberately so user deletion doesn't leave orphan + // policies behind. Get discards the org payload on ErrDisabled, so + // reconstruct the minimal org the cascade needs. + if guarded || !errors.Is(err, organization.ErrDisabled) { + return err + } + org = organization.Organization{ID: orgID} } // check if principal is a member at org level diff --git a/core/membership/service_test.go b/core/membership/service_test.go index 3eda558d6..dd432e4a5 100644 --- a/core/membership/service_test.go +++ b/core/membership/service_test.go @@ -1052,6 +1052,25 @@ func TestService_ForceRemoveOrganizationMember(t *testing.T) { d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) }, }, + { + name: "proceeds with the cascade when the org is disabled", + setup: func(d testDeps) { + // Get discards the org payload on ErrDisabled; the force path + // must reconstruct the org from the input ID and continue — + // the deleter visits disabled orgs deliberately + d.orgSvc.EXPECT().Get(ctx, orgID).Return(organization.Organization{}, organization.ErrDisabled) + d.policySvc.EXPECT().List(ctx, policy.Filter{OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{{ID: "org-p1", RoleID: ownerRoleID}}, nil) + d.projSvc.EXPECT().List(ctx, project.Filter{OrgID: orgID}).Return([]project.Project{}, nil) + d.grpSvc.EXPECT().List(ctx, group.Filter{OrganizationID: orgID}).Return([]group.Group{}, nil) + d.policySvc.EXPECT().List(ctx, policy.Filter{PrincipalID: userID, PrincipalType: schema.UserPrincipal}).Return([]policy.Policy{ + {ID: "org-p1", ResourceType: schema.OrganizationNamespace, ResourceID: orgID, RoleID: ownerRoleID}, + }, nil) + d.policySvc.EXPECT().Delete(ctx, "org-p1").Return(nil) + d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.OwnerRelationName}).Return(nil) + d.relSvc.EXPECT().Delete(ctx, relation.Relation{Object: orgObj, Subject: userSub, RelationName: schema.MemberRelationName}).Return(relation.ErrNotExist) + d.auditRepo.EXPECT().Create(ctx, mock.Anything).Return(auditrecord.AuditRecord{}, nil) + }, + }, { name: "propagates org lookup failure", setup: func(d testDeps) {