From 9720206c61f6590ba69f2515c25b5a77910c441b Mon Sep 17 00:00:00 2001 From: aman Date: Wed, 21 Oct 2026 22:49:57 +0530 Subject: [PATCH 1/2] feat(store): group delete keeps the row The group row now gets deleted_at instead of being removed. Deleting a group that is already deleted, or does not exist, still returns not found. Tests check that the row stays, reads skip it, no live policy is left on the group or names it, and the name can be used again, after DeleteGroup and after an org delete. --- internal/store/postgres/group_repository.go | 2 +- .../store/postgres/group_repository_test.go | 70 +++++++++---------- test/e2e/regression/api_test.go | 49 ++++++++++++- 3 files changed, 82 insertions(+), 39 deletions(-) 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..edc182fc6 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().Contains([]connect.Code{connect.CodeNotFound, connect.CodePermissionDenied}, connect.CodeOf(err)) + _, err = s.testBench.Client.DeleteGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteGroupRequest{Id: deletedGroupID})) + s.Require().Error(err) + s.Assert().Contains([]connect.Code{connect.CodeNotFound, 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})) From e28a8631234fe0e74cd31da3937091de769c8dc7 Mon Sep 17 00:00:00 2001 From: aman Date: Thu, 22 Oct 2026 17:37:52 +0530 Subject: [PATCH 2/2] fix(deleter): org delete skips a group that is already gone The group loop in DeleteOrganization skips group.ErrNotExist, like the resource and domain loops. The group e2e asserts PermissionDenied for GetGroup and a second DeleteGroup on a deleted group. --- core/deleter/service.go | 3 +++ core/deleter/service_test.go | 25 +++++++++++++++++++++++++ test/e2e/regression/api_test.go | 4 ++-- 3 files changed, 30 insertions(+), 2 deletions(-) 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/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index edc182fc6..8e897585b 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -1266,10 +1266,10 @@ func (s *APIRegressionTestSuite) TestGroupAPI() { _, err = s.testBench.Client.GetGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetGroupRequest{Id: deletedGroupID})) s.Require().Error(err) - s.Assert().Contains([]connect.Code{connect.CodeNotFound, connect.CodePermissionDenied}, connect.CodeOf(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().Contains([]connect.Code{connect.CodeNotFound, connect.CodePermissionDenied}, connect.CodeOf(err)) + s.Assert().Equal(connect.CodePermissionDenied, connect.CodeOf(err)) recreateResp, err := s.testBench.Client.CreateGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateGroupRequest{ OrgId: myOrg.GetId(),