diff --git a/core/deleter/service.go b/core/deleter/service.go index 98429f0f9..ed72a57a3 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -339,6 +339,9 @@ func (d Service) DeleteOrganization(ctx context.Context, id string) error { } for _, g := range groups { if err = d.DeleteGroup(ctx, g.ID); err != nil { + if errors.Is(err, group.ErrNotExist) { + continue + } return fmt.Errorf("failed to delete org while deleting a group[%s]: %w", g.Name, err) } } diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index 0fad717f9..e111bfa94 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -437,6 +437,31 @@ func TestDeleteOrganization(t *testing.T) { assert.Equal(t, []string{"resource:res-live", "org"}, steps) }) + t.Run("a group that is already gone is skipped and the rest are deleted before the org row", 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{}, nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{OrgID: "org-1"}). + Return([]resource.Resource{}, nil) + m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}). + Return([]group.Group{{ID: "grp-gone", Name: "gone"}, {ID: "grp-live", Name: "live"}}, nil) + m.mbrSvc.EXPECT().OnGroupDeleted(mock.Anything, "grp-gone").Return(group.ErrNotExist) + m.mbrSvc.EXPECT().OnGroupDeleted(mock.Anything, "grp-live").Return(nil) + m.grpSvc.EXPECT().DeleteModel(mock.Anything, "grp-live"). + 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{"group:grp-live", "org"}, steps) + }) + t.Run("a failed org resource delete stops before the org row", func(t *testing.T) { m := newBareMocks(t) diff --git a/internal/store/postgres/group_repository.go b/internal/store/postgres/group_repository.go index f19238ad2..0f45e1821 100644 --- a/internal/store/postgres/group_repository.go +++ b/internal/store/postgres/group_repository.go @@ -304,7 +304,7 @@ func (r GroupRepository) SetState(ctx context.Context, id string, state group.St } func (r GroupRepository) Delete(ctx context.Context, id string) error { - query, params, err := dialect.Delete(TABLE_GROUPS).Where( + query, params, err := softDelete(TABLE_GROUPS).Where( goqu.Ex{ "id": id, }, diff --git a/internal/store/postgres/group_repository_test.go b/internal/store/postgres/group_repository_test.go index 8496d9b9b..51a597d89 100644 --- a/internal/store/postgres/group_repository_test.go +++ b/internal/store/postgres/group_repository_test.go @@ -580,46 +580,42 @@ func (s *GroupRepositoryTestSuite) TestSetState() { } func (s *GroupRepositoryTestSuite) TestDelete() { - type testCase struct { - Description string - GroupID string - ExpectedError error - } + deleted := s.groups[0] - var testCases = []testCase{ - { - Description: "should delete group", - GroupID: s.groups[0].ID, - ExpectedError: nil, - }, - { - Description: "should return error if group not exist", - GroupID: "52e4dc5d-eb93-4e0d-bc72-4f24a67d679f", - ExpectedError: group.ErrNotExist, - }, - { - Description: "should return error if group id is not uuid", - GroupID: "12345", - ExpectedError: group.ErrInvalidUUID, - }, - { - Description: "should return error if group id is empty", - ExpectedError: group.ErrInvalidUUID, - }, - } + s.Run("keeps the row with deleted_at set and hides it", func() { + s.Require().NoError(s.repository.Delete(s.ctx, deleted.ID)) - for _, tc := range testCases { - s.Run(tc.Description, func() { - err := s.repository.Delete(s.ctx, tc.GroupID) - if tc.ExpectedError != nil { - if !errors.Is(err, tc.ExpectedError) { - s.T().Fatalf("got error %v, expected was %v", err, tc.ExpectedError) - } - } else { - s.Assert().NoError(err) - } + var deletedRows int + s.Require().NoError(s.client.QueryRowxContext(s.ctx, + "SELECT count(*) FROM groups WHERE id = $1 AND deleted_at IS NOT NULL", deleted.ID).Scan(&deletedRows)) + s.Assert().Equal(1, deletedRows) + + _, err := s.repository.GetByID(s.ctx, deleted.ID) + s.Assert().ErrorIs(err, group.ErrNotExist) + + listed, err := s.repository.List(s.ctx, group.Filter{OrganizationID: deleted.OrganizationID}) + s.Require().NoError(err) + for _, g := range listed { + s.Assert().NotEqual(deleted.ID, g.ID) + } + }) + s.Run("a second delete returns not found", func() { + s.Assert().ErrorIs(s.repository.Delete(s.ctx, deleted.ID), group.ErrNotExist) + }) + s.Run("a group that does not exist returns not found", func() { + s.Assert().ErrorIs(s.repository.Delete(s.ctx, uuid.NewString()), group.ErrNotExist) + }) + s.Run("an id that is not a uuid returns invalid uuid", func() { + s.Assert().ErrorIs(s.repository.Delete(s.ctx, "12345"), group.ErrInvalidUUID) + s.Assert().ErrorIs(s.repository.Delete(s.ctx, ""), group.ErrInvalidUUID) + }) + s.Run("a new group in the same org can take the deleted group's name", func() { + _, err := s.repository.Create(s.ctx, group.Group{ + Name: deleted.Name, + OrganizationID: deleted.OrganizationID, }) - } + s.Assert().NoError(err) + }) } func TestGroupRepository(t *testing.T) { diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index f8a930bb0..8e897585b 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -1259,6 +1259,32 @@ func (s *APIRegressionTestSuite) TestGroupAPI() { s.Assert().NoError(err) s.Assert().False(checkUserStatus.Msg.GetStatus()) + var deletedRows int + s.Require().NoError(s.dbc.GetContext(context.Background(), &deletedRows, + "SELECT count(*) FROM groups WHERE id = $1 AND deleted_at IS NOT NULL", deletedGroupID)) + s.Assert().Equal(1, deletedRows) + + _, err = s.testBench.Client.GetGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetGroupRequest{Id: deletedGroupID})) + s.Require().Error(err) + s.Assert().Equal(connect.CodePermissionDenied, connect.CodeOf(err)) + _, err = s.testBench.Client.DeleteGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteGroupRequest{Id: deletedGroupID})) + s.Require().Error(err) + s.Assert().Equal(connect.CodePermissionDenied, connect.CodeOf(err)) + + recreateResp, err := s.testBench.Client.CreateGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateGroupRequest{ + OrgId: myOrg.GetId(), + Body: &frontierv1beta1.GroupRequestBody{Name: "group-11"}, + })) + s.Require().NoError(err) + listResp, err := s.testBench.Client.ListOrganizationGroups(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListOrganizationGroupsRequest{OrgId: myOrg.GetId()})) + s.Require().NoError(err) + listedIDs := []string{} + for _, g := range listResp.Msg.GetGroups() { + listedIDs = append(listedIDs, g.GetId()) + } + s.Assert().Contains(listedIDs, recreateResp.Msg.GetGroup().GetId()) + s.Assert().NotContains(listedIDs, deletedGroupID) + _, err = s.testBench.Client.DeleteProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteProjectRequest{ Id: groupPolicyProjResp.Msg.GetProject().GetId(), })) @@ -3732,6 +3758,15 @@ func (s *APIRegressionTestSuite) TestOrganizationDeleteAlsoDeletesDisabledProjec s.Require().NotEmpty(disabledProjectPolicyIDs) s.Require().NotEmpty(disabledGroupPolicyIDs) + _, err = s.testBench.Client.CreatePolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePolicyRequest{ + Body: &frontierv1beta1.PolicyRequestBody{ + RoleId: schema.RoleOrganizationViewer, + Resource: schema.JoinNamespaceAndResourceID(schema.OrganizationNamespace, orgID), + Principal: schema.JoinNamespaceAndResourceID(schema.GroupPrincipal, disabledGroupID), + }, + })) + s.Require().NoError(err) + enabledProjects, err := s.testBench.AdminClient.ListProjects(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListProjectsRequest{OrgId: orgID})) s.Require().NoError(err) s.Require().Len(enabledProjects.Msg.GetProjects(), 1) @@ -3760,7 +3795,7 @@ func (s *APIRegressionTestSuite) TestOrganizationDeleteAlsoDeletesDisabledProjec } }) - s.Run("no group row of the org is left, disabled ones included", func() { + s.Run("no group of the org is listed, 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) @@ -3768,6 +3803,18 @@ func (s *APIRegressionTestSuite) TestOrganizationDeleteAlsoDeletesDisabledProjec } }) + s.Run("both groups stay in the database marked deleted, with no live policy on or naming them", func() { + for _, id := range []string{enabledGroupID, disabledGroupID} { + var deletedRows, livePolicies int + s.Require().NoError(s.dbc.GetContext(context.Background(), &deletedRows, + "SELECT count(*) FROM groups WHERE id = $1 AND deleted_at IS NOT NULL", id)) + s.Require().NoError(s.dbc.GetContext(context.Background(), &livePolicies, + "SELECT count(*) FROM policies WHERE (resource_id = $1 OR principal_id = $1) AND deleted_at IS NULL", id)) + s.Assert().Equal(1, deletedRows, "group %s", id) + s.Assert().Equal(0, livePolicies, "group %s", id) + } + }) + 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}))