diff --git a/core/deleter/service.go b/core/deleter/service.go index f8218be686..089c6c17d5 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -303,9 +303,7 @@ func (d Service) DeleteOrganization(ctx context.Context, id string) error { } // delete all related projects - projects, err := d.projService.List(ctx, project.Filter{ - OrgID: id, - }) + projects, err := d.listOrgProjects(ctx, id) if err != nil { return err } @@ -316,7 +314,7 @@ func (d Service) DeleteOrganization(ctx context.Context, id string) error { } // delete all related groups - groups, err := d.groupService.List(ctx, group.Filter{OrganizationID: id}) + groups, err := d.listOrgGroups(ctx, id) if err != nil { return err } @@ -547,6 +545,14 @@ func (d Service) DeleteUser(ctx context.Context, userID string) error { return d.userService.Delete(ctx, userID) } +func (d Service) listOrgProjects(ctx context.Context, orgID string) ([]project.Project, error) { + return d.projService.List(ctx, project.Filter{OrgID: orgID, IncludeDisabled: true}) +} + +func (d Service) listOrgGroups(ctx context.Context, orgID string) ([]group.Group, error) { + return d.groupService.List(ctx, group.Filter{OrganizationID: orgID, IncludeDisabled: true}) +} + // ensureDeletable collects everything that blocks deleting the organization and // returns it all as one BlockedError, so the caller gets a full checklist // instead of discovering blockers one retry at a time. diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index bc8975f4d0..b59be04f5e 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -63,6 +63,12 @@ type deleterMocks struct { } func newMocks(t *testing.T) deleterMocks { + t.Helper() + m := newBareMocks(t) + return m +} + +func newBareMocks(t *testing.T) deleterMocks { t.Helper() m := deleterMocks{ orgSvc: mocks.NewOrganizationService(t), @@ -158,6 +164,25 @@ func TestDeleteProject(t *testing.T) { }) } +func expectEmptyOrgTail(m deleterMocks, steps *[]string) { + m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). + Return([]serviceuser.ServiceUser{}, nil) + m.invSvc.EXPECT().List(mock.Anything, invitation.Filter{OrgID: "org-1"}). + Return([]invitation.Invitation{}, nil) + m.kycSvc.EXPECT().DeleteKyc(mock.Anything, "org-1").Return(nil) + m.domainSvc.EXPECT().List(mock.Anything, domain.Filter{OrgID: "org-1"}). + Return([]domain.Domain{}, nil) + m.patSvc.EXPECT().DeleteAllByOrg(mock.Anything, "org-1").Return(nil) + m.polSvc.EXPECT().List(mock.Anything, policy.Filter{OrgID: "org-1"}). + Return([]policy.Policy{}, nil) + m.roleSvc.EXPECT().List(mock.Anything, role.Filter{OrgID: "org-1"}). + Return([]role.Role{}, nil) + m.orgSvc.EXPECT().DeleteModel(mock.Anything, "org-1"). + Run(func(context.Context, string) { *steps = append(*steps, "org") }).Return(nil) + m.roleSvc.EXPECT().Get(mock.Anything, schema.RoleOrganizationOwner). + Return(role.Role{}, errors.New("no owners in this test")) +} + // expectOwnerNotified wires the owner lookup and one delivered mail; a // successful delete must always end in the notice being sent. func expectOwnerNotified(m deleterMocks) { @@ -200,7 +225,7 @@ func TestDeleteOrganization(t *testing.T) { m.custSvc.EXPECT().Delete(mock.Anything, "cust-1").Return(nil) // projects (each triggers DeleteProject) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{{ID: "proj-1", Name: "p1"}}, nil) m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). Return([]policy.Policy{}, nil) @@ -209,7 +234,7 @@ func TestDeleteOrganization(t *testing.T) { m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(nil) // groups (DeleteGroup: OnGroupDeleted then DeleteModel) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{{ID: "grp-1", Name: "g1"}}, nil) m.mbrSvc.EXPECT().OnGroupDeleted(mock.Anything, "grp-1").Return(nil) m.grpSvc.EXPECT().DeleteModel(mock.Anything, "grp-1").Return(nil) @@ -269,6 +294,93 @@ func TestDeleteOrganization(t *testing.T) { assert.NoError(t, err) }) + t.Run("disabled projects and groups are deleted with the org and the org row goes last", func(t *testing.T) { + m := newBareMocks(t) + var steps []string + + m.orgSvc.EXPECT().GetRaw(mock.Anything, "org-1"). + Return(organization.Organization{ID: "org-1"}, nil) + m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). + Return([]customer.Customer{}, nil) + + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). + Return([]project.Project{{ID: "proj-on", Name: "on"}, {ID: "proj-off", Name: "off", State: project.Disabled}}, nil) + for _, id := range []string{"proj-on", "proj-off"} { + m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: id}). + Return([]policy.Policy{}, nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: id, IncludeDeleted: true}). + Return([]resource.Resource{}, nil) + m.projSvc.EXPECT().DeleteModel(mock.Anything, id). + Run(func(_ context.Context, id string) { steps = append(steps, "project:"+id) }).Return(nil) + } + + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). + Return([]group.Group{{ID: "grp-on", Name: "on"}, {ID: "grp-off", Name: "off", State: group.Disabled}}, nil) + for _, id := range []string{"grp-on", "grp-off"} { + m.mbrSvc.EXPECT().OnGroupDeleted(mock.Anything, id).Return(nil) + m.grpSvc.EXPECT().DeleteModel(mock.Anything, id). + Run(func(_ context.Context, id string) { steps = append(steps, "group:"+id) }).Return(nil) + } + + expectEmptyOrgTail(m, &steps) + + err := m.build().DeleteOrganization(context.Background(), "org-1") + assert.NoError(t, err) + assert.Equal(t, []string{ + "project:proj-on", "project:proj-off", "group:grp-on", "group:grp-off", "org", + }, steps) + }) + + t.Run("an org with only enabled children lists nothing extra to delete", func(t *testing.T) { + m := newBareMocks(t) + var steps []string + + m.orgSvc.EXPECT().GetRaw(mock.Anything, "org-1"). + Return(organization.Organization{ID: "org-1"}, nil) + m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). + Return([]customer.Customer{}, nil) + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). + Return([]project.Project{{ID: "proj-on", Name: "on"}}, nil) + m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-on"}). + Return([]policy.Policy{}, nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-on", IncludeDeleted: true}). + Return([]resource.Resource{}, nil) + m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-on"). + Run(func(_ context.Context, id string) { steps = append(steps, "project:"+id) }).Return(nil) + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). + Return([]group.Group{{ID: "grp-on", Name: "on"}}, nil) + m.mbrSvc.EXPECT().OnGroupDeleted(mock.Anything, "grp-on").Return(nil) + m.grpSvc.EXPECT().DeleteModel(mock.Anything, "grp-on"). + Run(func(_ context.Context, id string) { steps = append(steps, "group:"+id) }).Return(nil) + + expectEmptyOrgTail(m, &steps) + + err := m.build().DeleteOrganization(context.Background(), "org-1") + assert.NoError(t, err) + assert.Equal(t, []string{"project:proj-on", "group:grp-on", "org"}, steps) + }) + + t.Run("a failed disabled project delete stops before the org row", func(t *testing.T) { + m := newBareMocks(t) + + m.orgSvc.EXPECT().GetRaw(mock.Anything, "org-1"). + Return(organization.Organization{ID: "org-1"}, nil) + m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). + Return([]customer.Customer{}, nil) + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). + Return([]project.Project{{ID: "proj-off", Name: "off", State: project.Disabled}}, nil) + m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-off"}). + Return([]policy.Policy{}, nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-off", IncludeDeleted: true}). + Return([]resource.Resource{}, nil) + m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-off").Return(errors.New("boom")) + m.roleSvc.EXPECT().Get(mock.Anything, schema.RoleOrganizationOwner). + Return(role.Role{}, errors.New("no owners in this test")) + + err := m.build().DeleteOrganization(context.Background(), "org-1") + assert.ErrorContains(t, err, "project[off]") + }) + t.Run("already deleted org returns not found without touching anything", func(t *testing.T) { m := newMocks(t) @@ -420,9 +532,9 @@ func TestDeleteOrganization(t *testing.T) { m.creditSvc.EXPECT().DeleteByAccountID(mock.Anything, "cust-1").Return(nil) m.custSvc.EXPECT().Delete(mock.Anything, "cust-1").Return(nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return([]serviceuser.ServiceUser{}, nil) @@ -470,9 +582,9 @@ func TestDeleteOrganization(t *testing.T) { m.creditSvc.EXPECT().DeleteByAccountID(mock.Anything, "cust-1").Return(nil) m.custSvc.EXPECT().Delete(mock.Anything, "cust-1").Return(nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return([]serviceuser.ServiceUser{}, nil) @@ -574,9 +686,9 @@ func TestDeleteOrganization(t *testing.T) { m.creditSvc.EXPECT().DeleteByAccountID(mock.Anything, "cust-1").Return(nil) m.custSvc.EXPECT().Delete(mock.Anything, "cust-1").Return(nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return([]serviceuser.ServiceUser{}, nil) @@ -679,9 +791,9 @@ func TestDeleteOrganization(t *testing.T) { Return(organization.Organization{ID: "org-1", State: organization.Disabled}, nil) m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). Return([]customer.Customer{}, nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return([]serviceuser.ServiceUser{}, nil) @@ -731,9 +843,9 @@ func TestDeleteOrganization(t *testing.T) { Return(organization.Organization{ID: "org-1"}, nil) m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). Return([]customer.Customer{}, nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return(nil, errors.New("su list failed")) @@ -753,9 +865,9 @@ func TestDeleteOrganization(t *testing.T) { Return(organization.Organization{ID: "org-1"}, nil) m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). Return([]customer.Customer{}, nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return([]serviceuser.ServiceUser{{ID: "su-1"}}, nil) @@ -777,9 +889,9 @@ func TestDeleteOrganization(t *testing.T) { Return(organization.Organization{ID: "org-1"}, nil) m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). Return([]customer.Customer{}, nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return([]serviceuser.ServiceUser{}, nil) @@ -820,9 +932,9 @@ func TestDeleteOrganization(t *testing.T) { Return(organization.Organization{ID: "org-1"}, nil) m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). Return([]customer.Customer{}, nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return([]serviceuser.ServiceUser{}, nil) @@ -846,9 +958,9 @@ func TestDeleteOrganization(t *testing.T) { Return(organization.Organization{ID: "org-1"}, nil) m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). Return([]customer.Customer{}, nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return([]serviceuser.ServiceUser{}, nil) @@ -874,9 +986,9 @@ func TestDeleteOrganization(t *testing.T) { Return(organization.Organization{ID: "org-1"}, nil) m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). Return([]customer.Customer{}, nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return([]serviceuser.ServiceUser{}, nil) @@ -908,9 +1020,9 @@ func TestDeleteOrganization(t *testing.T) { Return(organization.Organization{ID: "org-1"}, nil) m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). Return([]customer.Customer{}, nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return([]serviceuser.ServiceUser{}, nil) @@ -1067,9 +1179,9 @@ func TestForfeitAuditRecord(t *testing.T) { m.checkoutSvc.EXPECT().DeleteByCustomer(mock.Anything, "cust-1").Return(nil) m.creditSvc.EXPECT().DeleteByAccountID(mock.Anything, "cust-1").Return(nil) m.custSvc.EXPECT().Delete(mock.Anything, "cust-1").Return(nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return([]serviceuser.ServiceUser{}, nil) @@ -1118,9 +1230,9 @@ func TestForfeitAuditRecord(t *testing.T) { Return(organization.Organization{ID: "org-1", Title: "Org One"}, nil) m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}). Return([]customer.Customer{}, nil) - m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1"}). + m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}). Return([]project.Project{}, nil) - m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1"}). + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). Return([]group.Group{}, nil) m.suSvc.EXPECT().List(mock.Anything, serviceuser.Filter{OrgID: "org-1"}). Return([]serviceuser.ServiceUser{}, nil) diff --git a/core/group/service.go b/core/group/service.go index 3ab29980d8..4ff47bfc68 100644 --- a/core/group/service.go +++ b/core/group/service.go @@ -197,10 +197,14 @@ func (s Service) Disable(ctx context.Context, id string) error { // responsibility — see core/deleter.DeleteGroup for the orchestration that // pairs this with membership.OnGroupDeleted. func (s Service) DeleteModel(ctx context.Context, id string) error { - group, err := s.repository.GetByID(ctx, id) + found, err := s.repository.GetByIDs(ctx, []string{id}, Filter{IncludeDisabled: true}) if err != nil { return err } + if len(found) == 0 { + return ErrNotExist + } + group := found[0] if err := s.repository.Delete(ctx, id); err != nil { return err diff --git a/core/group/service_test.go b/core/group/service_test.go index f5a7da59f7..79c30b1608 100644 --- a/core/group/service_test.go +++ b/core/group/service_test.go @@ -411,3 +411,31 @@ func TestService_List_PrincipalFilter(t *testing.T) { assert.Empty(t, result) }) } + +func TestService_DeleteModel(t *testing.T) { + ctx := context.Background() + newSvc := func(t *testing.T) (*group.Service, *mocks.Repository) { + t.Helper() + mockRepo := mocks.NewRepository(t) + svc := group.NewService(mockRepo, mocks.NewRelationService(t), mocks.NewAuthnService(t), mocks.NewPolicyService(t)) + return svc, mockRepo + } + + t.Run("deletes a disabled group", func(t *testing.T) { + svc, repo := newSvc(t) + disabled := group.Group{ID: "grp-off", OrganizationID: "org-1", State: group.Disabled} + repo.EXPECT().GetByIDs(mock.Anything, []string{"grp-off"}, group.Filter{IncludeDisabled: true}). + Return([]group.Group{disabled}, nil) + repo.EXPECT().Delete(mock.Anything, "grp-off").Return(nil) + + assert.NoError(t, svc.DeleteModel(ctx, "grp-off")) + }) + + t.Run("a group that does not exist is not found", func(t *testing.T) { + svc, repo := newSvc(t) + repo.EXPECT().GetByIDs(mock.Anything, []string{"grp-x"}, group.Filter{IncludeDisabled: true}). + Return([]group.Group{}, nil) + + assert.ErrorIs(t, svc.DeleteModel(ctx, "grp-x"), group.ErrNotExist) + }) +} diff --git a/core/membership/group.go b/core/membership/group.go index a1e025c088..79003b1579 100644 --- a/core/membership/group.go +++ b/core/membership/group.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + "github.com/raystack/frontier/core/group" "github.com/raystack/frontier/core/policy" "github.com/raystack/frontier/core/relation" "github.com/raystack/frontier/core/role" @@ -217,7 +218,7 @@ func (s *Service) RemoveAllGroupMembers(ctx context.Context, groupID string) err // Errors are joined; partial failures are logged so a retry can complete // the cleanup. func (s *Service) OnGroupDeleted(ctx context.Context, groupID string) error { - grp, err := s.groupService.Get(ctx, groupID) + grp, err := s.getGroupIncludingDisabled(ctx, groupID) if err != nil { return err } @@ -235,6 +236,17 @@ func (s *Service) OnGroupDeleted(ctx context.Context, groupID string) error { return errs } +func (s *Service) getGroupIncludingDisabled(ctx context.Context, groupID string) (group.Group, error) { + found, err := s.groupService.GetByIDs(ctx, []string{groupID}, group.Filter{IncludeDisabled: true}) + if err != nil { + return group.Group{}, err + } + if len(found) == 0 { + return group.Group{}, group.ErrNotExist + } + return found[0], nil +} + // removeGroupAsPrincipalPolicies deletes every policy where the given group // is the principal — e.g. policies created by `CreatePolicy(principal=group:X, // resource=project:Y, role=viewer)` that grant the group access to other diff --git a/core/membership/group_test.go b/core/membership/group_test.go index d607f6cedc..184ff804a8 100644 --- a/core/membership/group_test.go +++ b/core/membership/group_test.go @@ -484,7 +484,7 @@ func TestService_OnGroupDeleted(t *testing.T) { relSvc := mocks.NewRelationService(t) grpSvc := mocks.NewGroupService(t) - grpSvc.EXPECT().Get(ctx, groupID).Return(grp, nil) + grpSvc.EXPECT().GetByIDs(ctx, []string{groupID}, group.Filter{IncludeDisabled: true}).Return([]group.Group{grp}, nil) // RemoveAllGroupMembers — no member policies policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID}).Return([]policy.Policy{}, nil) // removeGroupAsPrincipalPolicies — one policy granting this group access elsewhere @@ -509,9 +509,38 @@ func TestService_OnGroupDeleted(t *testing.T) { assert.NoError(t, svc.OnGroupDeleted(ctx, groupID)) }) + t.Run("cleans up a disabled group", func(t *testing.T) { + policySvc := mocks.NewPolicyService(t) + relSvc := mocks.NewRelationService(t) + grpSvc := mocks.NewGroupService(t) + + disabled := grp + disabled.State = group.Disabled + grpSvc.EXPECT().GetByIDs(ctx, []string{groupID}, group.Filter{IncludeDisabled: true}). + Return([]group.Group{disabled}, nil) + policySvc.EXPECT().List(ctx, policy.Filter{GroupID: groupID}).Return([]policy.Policy{}, nil) + policySvc.EXPECT().List(ctx, policy.Filter{ + PrincipalType: schema.GroupPrincipal, + PrincipalID: groupID, + }).Return([]policy.Policy{}, nil) + relSvc.EXPECT().Delete(ctx, relation.Relation{ + Object: relation.Object{ID: groupID, Namespace: schema.GroupNamespace}, + Subject: relation.Subject{ID: orgID, Namespace: schema.OrganizationNamespace}, + RelationName: schema.OrganizationRelationName, + }).Return(nil) + + svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), policySvc, relSvc, + mocks.NewRoleService(t), mocks.NewOrgService(t), mocks.NewUserService(t), + mocks.NewProjectService(t), grpSvc, mocks.NewServiceuserService(t), + mocks.NewAuditRecordRepository(t)) + + assert.NoError(t, svc.OnGroupDeleted(ctx, groupID)) + }) + t.Run("returns error if group not found", func(t *testing.T) { grpSvc := mocks.NewGroupService(t) - grpSvc.EXPECT().Get(ctx, groupID).Return(group.Group{}, group.ErrNotExist) + grpSvc.EXPECT().GetByIDs(ctx, []string{groupID}, group.Filter{IncludeDisabled: true}). + Return([]group.Group{}, nil) svc := membership.NewService(slog.New(slog.NewTextHandler(io.Discard, nil)), mocks.NewPolicyService(t), mocks.NewRelationService(t), diff --git a/core/membership/mocks/group_service.go b/core/membership/mocks/group_service.go index 2e84c67651..2bf581edbc 100644 --- a/core/membership/mocks/group_service.go +++ b/core/membership/mocks/group_service.go @@ -80,6 +80,80 @@ func (_c *GroupService_Get_Call) RunAndReturn(run func(context.Context, string) return _c } +// GetByIDs provides a mock function with given fields: ctx, ids, flt +func (_m *GroupService) GetByIDs(ctx context.Context, ids []string, flt ...group.Filter) ([]group.Group, error) { + _va := make([]interface{}, len(flt)) + for _i := range flt { + _va[_i] = flt[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, ids) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + if len(ret) == 0 { + panic("no return value specified for GetByIDs") + } + + var r0 []group.Group + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, []string, ...group.Filter) ([]group.Group, error)); ok { + return rf(ctx, ids, flt...) + } + if rf, ok := ret.Get(0).(func(context.Context, []string, ...group.Filter) []group.Group); ok { + r0 = rf(ctx, ids, flt...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]group.Group) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, []string, ...group.Filter) error); ok { + r1 = rf(ctx, ids, flt...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GroupService_GetByIDs_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetByIDs' +type GroupService_GetByIDs_Call struct { + *mock.Call +} + +// GetByIDs is a helper method to define mock.On call +// - ctx context.Context +// - ids []string +// - flt ...group.Filter +func (_e *GroupService_Expecter) GetByIDs(ctx interface{}, ids interface{}, flt ...interface{}) *GroupService_GetByIDs_Call { + return &GroupService_GetByIDs_Call{Call: _e.mock.On("GetByIDs", + append([]interface{}{ctx, ids}, flt...)...)} +} + +func (_c *GroupService_GetByIDs_Call) Run(run func(ctx context.Context, ids []string, flt ...group.Filter)) *GroupService_GetByIDs_Call { + _c.Call.Run(func(args mock.Arguments) { + variadicArgs := make([]group.Filter, len(args)-2) + for i, a := range args[2:] { + if a != nil { + variadicArgs[i] = a.(group.Filter) + } + } + run(args[0].(context.Context), args[1].([]string), variadicArgs...) + }) + return _c +} + +func (_c *GroupService_GetByIDs_Call) Return(_a0 []group.Group, _a1 error) *GroupService_GetByIDs_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *GroupService_GetByIDs_Call) RunAndReturn(run func(context.Context, []string, ...group.Filter) ([]group.Group, error)) *GroupService_GetByIDs_Call { + _c.Call.Return(run) + return _c +} + // List provides a mock function with given fields: ctx, flt func (_m *GroupService) List(ctx context.Context, flt group.Filter) ([]group.Group, error) { ret := _m.Called(ctx, flt) diff --git a/core/membership/service.go b/core/membership/service.go index 44081304ec..0a1db8084b 100644 --- a/core/membership/service.go +++ b/core/membership/service.go @@ -54,6 +54,7 @@ type ProjectService interface { type GroupService interface { Get(ctx context.Context, idOrName string) (group.Group, error) List(ctx context.Context, flt group.Filter) ([]group.Group, error) + GetByIDs(ctx context.Context, ids []string, flt ...group.Filter) ([]group.Group, error) } type ServiceuserService interface { diff --git a/core/project/filter.go b/core/project/filter.go index c55cde33a8..cbdb7b9343 100644 --- a/core/project/filter.go +++ b/core/project/filter.go @@ -10,6 +10,7 @@ type Filter struct { WithMemberCount bool ProjectIDs []string State State + IncludeDisabled bool // NonInherited filters out projects that are inherited from access given through an organization NonInherited bool Pagination *pagination.Pagination diff --git a/internal/store/postgres/group_repository.go b/internal/store/postgres/group_repository.go index 8e013a877a..f19238ad24 100644 --- a/internal/store/postgres/group_repository.go +++ b/internal/store/postgres/group_repository.go @@ -178,9 +178,10 @@ func (r GroupRepository) List(ctx context.Context, flt group.Filter) ([]group.Gr if flt.OrganizationID != "" { sqlStatement = sqlStatement.Where(goqu.Ex{"org_id": flt.OrganizationID}) } - if flt.State != "" { + switch { + case flt.State != "": sqlStatement = sqlStatement.Where(goqu.Ex{"state": flt.State}) - } else { + case !flt.IncludeDisabled: sqlStatement = sqlStatement.Where(notDisabledGroupExp) } if flt.GroupIDs != nil { diff --git a/internal/store/postgres/group_repository_test.go b/internal/store/postgres/group_repository_test.go index 0450a2e6bc..8496d9b9b4 100644 --- a/internal/store/postgres/group_repository_test.go +++ b/internal/store/postgres/group_repository_test.go @@ -657,3 +657,20 @@ func (s *GroupRepositoryTestSuite) TestSkipsSoftDeletedGroups() { err = s.repository.SetState(s.ctx, deleted.ID, group.Disabled) s.Assert().ErrorIs(err, group.ErrNotExist) } + +func (s *GroupRepositoryTestSuite) TestListIncludeDisabled() { + orgID := s.orgs[1].ID + + hidden, err := s.repository.List(s.ctx, group.Filter{OrganizationID: orgID}) + s.Require().NoError(err) + s.Assert().Len(hidden, 1) + s.Assert().Equal(group.Enabled, hidden[0].State) + + all, err := s.repository.List(s.ctx, group.Filter{OrganizationID: orgID, IncludeDisabled: true}) + s.Require().NoError(err) + states := map[group.State]int{} + for _, g := range all { + states[g.State]++ + } + s.Assert().Equal(map[group.State]int{group.Enabled: 1, group.Disabled: 1}, states) +} diff --git a/internal/store/postgres/project_repository.go b/internal/store/postgres/project_repository.go index d75614444b..5c2dbf833b 100644 --- a/internal/store/postgres/project_repository.go +++ b/internal/store/postgres/project_repository.go @@ -201,12 +201,13 @@ func (r ProjectRepository) List(ctx context.Context, flt project.Filter) ([]proj "id": goqu.Op{"in": flt.ProjectIDs}, }) } - if flt.State == "" { - stmt = stmt.Where(notDisabledProjectExp) - } else { + switch { + case flt.State != "": stmt = stmt.Where(goqu.Ex{ "state": flt.State.String(), }) + case !flt.IncludeDisabled: + stmt = stmt.Where(notDisabledProjectExp) } if flt.Pagination != nil { diff --git a/internal/store/postgres/project_repository_test.go b/internal/store/postgres/project_repository_test.go index 283aaeecb4..575ab21763 100644 --- a/internal/store/postgres/project_repository_test.go +++ b/internal/store/postgres/project_repository_test.go @@ -521,3 +521,24 @@ func (s *ProjectRepositoryTestSuite) TestSkipsSoftDeletedProjects() { err = s.repository.SetState(s.ctx, deleted.ID, project.Disabled) s.Assert().ErrorIs(err, project.ErrNotExist) } + +func (s *ProjectRepositoryTestSuite) TestListIncludeDisabled() { + disabled := s.projects[0] + orgID := disabled.Organization.ID + s.Require().NoError(s.repository.SetState(s.ctx, disabled.ID, project.Disabled)) + + hidden, err := s.repository.List(s.ctx, project.Filter{OrgID: orgID}) + s.Require().NoError(err) + for _, p := range hidden { + s.Assert().NotEqual(disabled.ID, p.ID) + } + + all, err := s.repository.List(s.ctx, project.Filter{OrgID: orgID, IncludeDisabled: true}) + s.Require().NoError(err) + s.Assert().Len(all, len(hidden)+1) + ids := []string{} + for _, p := range all { + ids = append(ids, p.ID) + } + s.Assert().Contains(ids, disabled.ID) +} diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index 0c40700818..f0c89cdeae 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -3284,6 +3284,106 @@ func (s *APIRegressionTestSuite) TestOrganizationDeleteHidesTheOrgAndRemovesWhat }) } +func (s *APIRegressionTestSuite) TestOrganizationDeleteAlsoDeletesDisabledProjectsAndGroups() { + ctxOrgAdminAuth := testbench.ContextWithAuth(context.Background(), s.adminCookie) + + orgResp, err := s.testBench.Client.CreateOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ + Body: &frontierv1beta1.OrganizationRequestBody{Title: "org-disabled-children", Name: "org-disabled-children"}, + })) + s.Require().NoError(err) + orgID := orgResp.Msg.GetOrganization().GetId() + + createProject := func(name string) string { + resp, err := s.testBench.Client.CreateProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{ + Body: &frontierv1beta1.ProjectRequestBody{Name: name, OrgId: orgID}, + })) + s.Require().NoError(err) + return resp.Msg.GetProject().GetId() + } + createGroup := func(name string) string { + resp, err := s.testBench.Client.CreateGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateGroupRequest{ + OrgId: orgID, + Body: &frontierv1beta1.GroupRequestBody{Name: name}, + })) + s.Require().NoError(err) + return resp.Msg.GetGroup().GetId() + } + + enabledProjectID := createProject("enabled-project") + disabledProjectID := createProject("disabled-project") + enabledGroupID := createGroup("enabled-group") + disabledGroupID := createGroup("disabled-group") + + _, err = s.testBench.Client.DisableProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DisableProjectRequest{Id: disabledProjectID})) + s.Require().NoError(err) + _, err = s.testBench.Client.DisableGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DisableGroupRequest{Id: disabledGroupID})) + s.Require().NoError(err) + + policyIDs := func(req *frontierv1beta1.ListPoliciesRequest) []string { + resp, err := s.testBench.Client.ListPolicies(ctxOrgAdminAuth, connect.NewRequest(req)) + s.Require().NoError(err) + ids := []string{} + for _, p := range resp.Msg.GetPolicies() { + ids = append(ids, p.GetId()) + } + return ids + } + disabledProjectPolicyIDs := policyIDs(&frontierv1beta1.ListPoliciesRequest{ProjectId: disabledProjectID}) + disabledGroupPolicyIDs := policyIDs(&frontierv1beta1.ListPoliciesRequest{GroupId: disabledGroupID}) + s.Require().NotEmpty(disabledProjectPolicyIDs) + s.Require().NotEmpty(disabledGroupPolicyIDs) + + enabledProjects, err := s.testBench.AdminClient.ListProjects(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListProjectsRequest{OrgId: orgID})) + s.Require().NoError(err) + s.Require().Len(enabledProjects.Msg.GetProjects(), 1) + s.Require().Equal(enabledProjectID, enabledProjects.Msg.GetProjects()[0].GetId()) + enabledGroups, err := s.testBench.AdminClient.ListGroups(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListGroupsRequest{OrgId: orgID})) + s.Require().NoError(err) + s.Require().Len(enabledGroups.Msg.GetGroups(), 1) + s.Require().Equal(enabledGroupID, enabledGroups.Msg.GetGroups()[0].GetId()) + disabledProjects, err := s.testBench.AdminClient.ListProjects(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListProjectsRequest{OrgId: orgID, State: "disabled"})) + s.Require().NoError(err) + s.Require().Len(disabledProjects.Msg.GetProjects(), 1) + s.Require().Equal(disabledProjectID, disabledProjects.Msg.GetProjects()[0].GetId()) + disabledGroups, err := s.testBench.AdminClient.ListGroups(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListGroupsRequest{OrgId: orgID, State: "disabled"})) + s.Require().NoError(err) + s.Require().Len(disabledGroups.Msg.GetGroups(), 1) + s.Require().Equal(disabledGroupID, disabledGroups.Msg.GetGroups()[0].GetId()) + + _, err = s.testBench.Client.DeleteOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteOrganizationRequest{Id: orgID})) + s.Require().NoError(err) + + s.Run("no project row of the org is left, disabled ones included", func() { + for _, state := range []string{"", "disabled"} { + resp, err := s.testBench.AdminClient.ListProjects(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListProjectsRequest{OrgId: orgID, State: state})) + s.Require().NoError(err) + s.Assert().Empty(resp.Msg.GetProjects(), "state %q", state) + } + }) + + s.Run("no group row of the org is left, disabled ones included", func() { + for _, state := range []string{"", "disabled"} { + resp, err := s.testBench.AdminClient.ListGroups(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListGroupsRequest{OrgId: orgID, State: state})) + s.Require().NoError(err) + s.Assert().Empty(resp.Msg.GetGroups(), "state %q", state) + } + }) + + s.Run("no policy of the disabled project or group is left", func() { + for _, id := range append(disabledProjectPolicyIDs, disabledGroupPolicyIDs...) { + _, err := s.testBench.Client.GetPolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetPolicyRequest{Id: id})) + s.Require().Error(err) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + } + }) + + s.Run("the org is gone, a second delete is refused", func() { + _, err := s.testBench.Client.DeleteOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteOrganizationRequest{Id: orgID})) + s.Require().Error(err) + s.Assert().Contains([]connect.Code{connect.CodeNotFound, connect.CodePermissionDenied}, connect.CodeOf(err)) + }) +} + func TestEndToEndAPIRegressionTestSuite(t *testing.T) { suite.Run(t, new(APIRegressionTestSuite)) }