diff --git a/core/deleter/service.go b/core/deleter/service.go index 75ce2a4df..9f6478b11 100644 --- a/core/deleter/service.go +++ b/core/deleter/service.go @@ -223,9 +223,7 @@ func (d Service) DeleteProject(ctx context.Context, id string) error { } } - // the project row is removed for good below, so every resource row of the - // project, deleted ones included, has to go with it - // TODO(fix): soft-delete the live resources instead once project delete is soft + // TODO(fix): soft-delete the live resources instead of purging every row resources, err := d.resService.List(ctx, resource.Filter{ ProjectID: id, IncludeDeleted: true, @@ -239,7 +237,10 @@ func (d Service) DeleteProject(ctx context.Context, id string) error { } } - return d.projService.DeleteModel(ctx, id) + if err := d.projService.DeleteModel(ctx, id); err != nil && !errors.Is(err, project.ErrNotExist) { + return err + } + return nil } // DeleteGroup orchestrates teardown of a single group: clears every member's diff --git a/core/deleter/service_test.go b/core/deleter/service_test.go index 36dba4e3b..e272b02d1 100644 --- a/core/deleter/service_test.go +++ b/core/deleter/service_test.go @@ -113,17 +113,36 @@ func (m deleterMocks) build() *deleter.Service { func TestDeleteProject(t *testing.T) { t.Run("deletes policies, resources, then project model", func(t *testing.T) { m := newMocks(t) + var steps []string m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). Return([]policy.Policy{{ID: "pol-1"}, {ID: "pol-2"}}, nil) - m.polSvc.EXPECT().Delete(mock.Anything, "pol-1").Return(nil) - m.polSvc.EXPECT().Delete(mock.Anything, "pol-2").Return(nil) + m.polSvc.EXPECT().Delete(mock.Anything, "pol-1"). + Run(func(_ context.Context, id string) { steps = append(steps, "policy:"+id) }).Return(nil) + m.polSvc.EXPECT().Delete(mock.Anything, "pol-2"). + Run(func(_ context.Context, id string) { steps = append(steps, "policy:"+id) }).Return(nil) m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1", IncludeDeleted: true}). Return([]resource.Resource{{ID: "res-1", NamespaceID: "ns-1", Name: "r1"}}, nil) - m.resSvc.EXPECT().Purge(mock.Anything, "ns-1", "res-1").Return(nil) + m.resSvc.EXPECT().Purge(mock.Anything, "ns-1", "res-1"). + Run(func(_ context.Context, _ string, id string) { steps = append(steps, "resource:"+id) }).Return(nil) - m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(nil) + m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1"). + Run(func(_ context.Context, id string) { steps = append(steps, "project:"+id) }).Return(nil) + + err := m.build().DeleteProject(context.Background(), "proj-1") + assert.NoError(t, err) + assert.Equal(t, []string{"policy:pol-1", "policy:pol-2", "resource:res-1", "project:proj-1"}, steps) + }) + + t.Run("a project that is already gone counts as deleted", func(t *testing.T) { + m := newMocks(t) + + m.polSvc.EXPECT().List(mock.Anything, policy.Filter{ProjectID: "proj-1"}). + Return([]policy.Policy{}, nil) + m.resSvc.EXPECT().List(mock.Anything, resource.Filter{ProjectID: "proj-1", IncludeDeleted: true}). + Return([]resource.Resource{}, nil) + m.projSvc.EXPECT().DeleteModel(mock.Anything, "proj-1").Return(project.ErrNotExist) err := m.build().DeleteProject(context.Background(), "proj-1") assert.NoError(t, err) diff --git a/core/resource/service.go b/core/resource/service.go index 99cf4fabc..d169e4cc9 100644 --- a/core/resource/service.go +++ b/core/resource/service.go @@ -504,7 +504,7 @@ func (s Service) Delete(ctx context.Context, namespaceID, id string) error { // Purge removes the resource and its SpiceDB tuples for good. Only the project // delete cascade uses it; the API uses Delete, which keeps the row. -// TODO(fix): remove once project delete is soft +// TODO(fix): remove once the project delete cascade soft-deletes resources func (s Service) Purge(ctx context.Context, namespaceID, id string) error { if err := s.relationService.Delete(ctx, relation.Relation{ Object: relation.Object{ diff --git a/internal/store/postgres/project_repository.go b/internal/store/postgres/project_repository.go index 5c2dbf833..d1f4bc3bc 100644 --- a/internal/store/postgres/project_repository.go +++ b/internal/store/postgres/project_repository.go @@ -381,7 +381,7 @@ func (r ProjectRepository) SetState(ctx context.Context, id string, state projec } func (r ProjectRepository) Delete(ctx context.Context, id string) error { - query, params, err := dialect.Delete(TABLE_PROJECTS).Where( + query, params, err := softDelete(TABLE_PROJECTS).Where( goqu.Ex{ "id": id, }, @@ -392,17 +392,12 @@ func (r ProjectRepository) Delete(ctx context.Context, id string) error { if err = r.dbc.WithTxn(ctx, sql.TxOptions{}, func(tx *sqlx.Tx) error { var result projectWithOrgName - err := r.dbc.WithTimeout(ctx, TABLE_PROJECTS, "Delete", func(ctx context.Context) error { + if err := r.dbc.WithTimeout(ctx, TABLE_PROJECTS, "Delete", func(ctx context.Context) error { return tx.QueryRowxContext(ctx, query, params...).StructScan(&result) - }) - if errors.Is(err, sql.ErrNoRows) { - // already gone: nothing deleted, nothing to audit - return nil - } - if err != nil { + }); err != nil { return err } - return InsertAuditRecordInTx(ctx, tx, buildProjectAuditRecord(ctx, auditrecord.ProjectDeletedEvent, result, time.Now().UTC())) + return InsertAuditRecordInTx(ctx, tx, buildProjectAuditRecord(ctx, auditrecord.ProjectDeletedEvent, result, result.DeletedAt.Time)) }); err != nil { err = checkPostgresError(err) switch { diff --git a/internal/store/postgres/project_repository_test.go b/internal/store/postgres/project_repository_test.go index 575ab2176..be0138b09 100644 --- a/internal/store/postgres/project_repository_test.go +++ b/internal/store/postgres/project_repository_test.go @@ -309,15 +309,36 @@ func (s *ProjectRepositoryTestSuite) TestCreate() { } func (s *ProjectRepositoryTestSuite) TestDelete() { - s.Run("should delete a project and audit it", func() { - s.Require().NoError(s.repository.Delete(s.ctx, s.projects[1].ID)) - s.Equal(1, s.auditCount(pkgAuditRecord.ProjectDeletedEvent, s.projects[1].ID)) + deleted := s.projects[1] + + s.Run("keeps the row with deleted_at set, hides it, and audits the delete", func() { + s.Require().NoError(s.repository.Delete(s.ctx, deleted.ID)) + s.Equal(1, s.auditCount(pkgAuditRecord.ProjectDeletedEvent, deleted.ID)) + + var deletedRows int + s.Require().NoError(s.client.QueryRowxContext(s.ctx, + "SELECT count(*) FROM projects WHERE id = $1 AND deleted_at IS NOT NULL", deleted.ID).Scan(&deletedRows)) + s.Equal(1, deletedRows) + + _, err := s.repository.GetByID(s.ctx, deleted.ID) + s.ErrorIs(err, project.ErrNotExist) }) - s.Run("should skip a project that does not exist without auditing", func() { + s.Run("a second delete returns not found and writes no second audit record", func() { + s.ErrorIs(s.repository.Delete(s.ctx, deleted.ID), project.ErrNotExist) + s.Equal(1, s.auditCount(pkgAuditRecord.ProjectDeletedEvent, deleted.ID)) + }) + s.Run("a project that does not exist returns not found without an audit record", func() { id := uuid.NewString() - s.Require().NoError(s.repository.Delete(s.ctx, id)) + s.ErrorIs(s.repository.Delete(s.ctx, id), project.ErrNotExist) s.Equal(0, s.auditCount(pkgAuditRecord.ProjectDeletedEvent, id)) }) + s.Run("a new project can take the deleted project's name", func() { + _, err := s.repository.Create(s.ctx, project.Project{ + Name: deleted.Name, + Organization: deleted.Organization, + }) + s.NoError(err) + }) } func (s *ProjectRepositoryTestSuite) TestList() { diff --git a/internal/store/postgres/resource_repository.go b/internal/store/postgres/resource_repository.go index 76e214285..e0925ba6b 100644 --- a/internal/store/postgres/resource_repository.go +++ b/internal/store/postgres/resource_repository.go @@ -286,9 +286,8 @@ func (r ResourceRepository) Delete(ctx context.Context, id string) error { return nil } -// Purge removes the row for good. The project delete cascade uses it, since a -// resource row cannot outlive its project row. -// TODO(fix): remove once project delete is soft +// Purge removes the row for good. Only the project delete cascade uses it. +// TODO(fix): remove once the project delete cascade soft-deletes resources func (r ResourceRepository) Purge(ctx context.Context, id string) error { query, params, err := dialect.Delete(TABLE_RESOURCES).Where( goqu.Ex{ diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index 4a46ebced..f8a930bb0 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -3783,6 +3783,168 @@ func (s *APIRegressionTestSuite) TestOrganizationDeleteAlsoDeletesDisabledProjec }) } +func (s *APIRegressionTestSuite) TestProjectDeleteKeepsTheRowAndRemovesAccess() { + ctxOrgAdminAuth := testbench.ContextWithAuth(context.Background(), s.adminCookie) + + createOrgResp, err := s.testBench.Client.CreateOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ + Body: &frontierv1beta1.OrganizationRequestBody{ + Title: "org project soft delete", + Name: "org-project-soft-delete", + }, + })) + s.Require().NoError(err) + orgID := createOrgResp.Msg.GetOrganization().GetId() + + createProjectResp, err := s.testBench.Client.CreateProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{ + Body: &frontierv1beta1.ProjectRequestBody{ + Name: "project-soft-delete", + Title: "project soft delete", + OrgId: orgID, + }, + })) + s.Require().NoError(err) + projectID := createProjectResp.Msg.GetProject().GetId() + projectResource := schema.JoinNamespaceAndResourceID(schema.ProjectNamespace, projectID) + + createUser := func(name string) string { + resp, err := s.testBench.Client.CreateUser(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateUserRequest{ + Body: &frontierv1beta1.UserRequestBody{ + Email: name + "@raystack.org", + Name: name, + }, + })) + s.Require().NoError(err) + return resp.Msg.GetUser().GetId() + } + ownerID := createUser("project-soft-delete-owner") + viewerID := createUser("project-soft-delete-viewer") + groupMemberID := createUser("project-soft-delete-group-member") + + addOrgMembersResp, err := s.testBench.AdminClient.AddOrganizationMembers(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.AddOrganizationMembersRequest{ + OrgId: orgID, + Members: []*frontierv1beta1.OrgMemberEntry{ + {UserId: ownerID, RoleId: s.orgOwnerRole}, + {UserId: groupMemberID, RoleId: s.orgViewerRole}, + }, + })) + requireAddOrgMembersSuccess(s.T(), addOrgMembersResp, err) + + createGroupResp, err := s.testBench.Client.CreateGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateGroupRequest{ + OrgId: orgID, + Body: &frontierv1beta1.GroupRequestBody{Name: "project-soft-delete-group"}, + })) + s.Require().NoError(err) + groupID := createGroupResp.Msg.GetGroup().GetId() + _, err = s.testBench.Client.SetGroupMemberRole(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.SetGroupMemberRoleRequest{ + GroupId: groupID, + PrincipalId: groupMemberID, + PrincipalType: schema.UserPrincipal, + RoleId: s.groupMemberRole, + })) + s.Require().NoError(err) + + createServiceUserResp, err := s.testBench.Client.CreateServiceUser(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateServiceUserRequest{ + OrgId: orgID, + })) + s.Require().NoError(err) + serviceUserID := createServiceUserResp.Msg.GetServiceuser().GetId() + + var createdPolicyIDs []string + for _, principal := range []string{ + schema.JoinNamespaceAndResourceID(schema.UserPrincipal, viewerID), + schema.JoinNamespaceAndResourceID(schema.GroupPrincipal, groupID), + schema.JoinNamespaceAndResourceID(schema.ServiceUserPrincipal, serviceUserID), + } { + resp, err := s.testBench.Client.CreatePolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePolicyRequest{ + Body: &frontierv1beta1.PolicyRequestBody{ + RoleId: schema.RoleProjectViewer, + Resource: projectResource, + Principal: principal, + }, + })) + s.Require().NoError(err) + createdPolicyIDs = append(createdPolicyIDs, resp.Msg.GetPolicy().GetId()) + } + + subjects := map[string]string{ + "org owner": schema.JoinNamespaceAndResourceID(schema.UserPrincipal, ownerID), + "user": schema.JoinNamespaceAndResourceID(schema.UserPrincipal, viewerID), + "group member": schema.JoinNamespaceAndResourceID(schema.UserPrincipal, groupMemberID), + "service user": schema.JoinNamespaceAndResourceID(schema.ServiceUserPrincipal, serviceUserID), + } + canGetProject := func(subject string) bool { + resp, err := s.testBench.AdminClient.CheckFederatedResourcePermission(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CheckFederatedResourcePermissionRequest{ + Resource: projectResource, + Permission: schema.GetPermission, + Subject: subject, + })) + s.Require().NoError(err) + return resp.Msg.GetStatus() + } + for name, subject := range subjects { + s.Require().Truef(canGetProject(subject), "%s before the delete", name) + } + + var policyIDs []string + for _, p := range livePoliciesMatching(s.T(), ctxOrgAdminAuth, s.testBench.Client, &frontierv1beta1.ListPoliciesRequest{ProjectId: projectID}) { + policyIDs = append(policyIDs, p.GetId()) + } + s.Require().Subset(policyIDs, createdPolicyIDs) + + _, err = s.testBench.Client.DeleteProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteProjectRequest{Id: projectID})) + s.Require().NoError(err) + + s.Run("nobody who had access can get the project", func() { + for name, subject := range subjects { + s.Assert().Falsef(canGetProject(subject), "%s after the delete", name) + } + }) + + s.Run("every policy on the project is marked deleted", func() { + for _, id := range policyIDs { + _, err := s.testBench.Client.GetPolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetPolicyRequest{Id: id})) + s.Require().Error(err) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + + var deletedRows int + s.Require().NoError(s.dbc.GetContext(context.Background(), &deletedRows, + "SELECT count(*) FROM policies WHERE id = $1 AND deleted_at IS NOT NULL", id)) + s.Assert().Equal(1, deletedRows) + } + + var liveRows int + s.Require().NoError(s.dbc.GetContext(context.Background(), &liveRows, + "SELECT count(*) FROM policies WHERE resource_type = $1 AND resource_id = $2 AND deleted_at IS NULL", + schema.ProjectNamespace, projectID)) + s.Assert().Equal(0, liveRows) + }) + + s.Run("the project row stays in the database marked deleted", func() { + var deletedRows int + s.Require().NoError(s.dbc.GetContext(context.Background(), &deletedRows, + "SELECT count(*) FROM projects WHERE id = $1 AND deleted_at IS NOT NULL", projectID)) + s.Assert().Equal(1, deletedRows) + }) + + s.Run("a second delete is refused", func() { + _, err := s.testBench.Client.DeleteProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteProjectRequest{Id: projectID})) + s.Require().Error(err) + s.Assert().Contains([]connect.Code{connect.CodeNotFound, connect.CodePermissionDenied}, connect.CodeOf(err)) + }) + + s.Run("a new project can take the name", func() { + recreateResp, err := s.testBench.Client.CreateProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateProjectRequest{ + Body: &frontierv1beta1.ProjectRequestBody{ + Name: "project-soft-delete", + Title: "project soft delete again", + OrgId: orgID, + }, + })) + s.Require().NoError(err) + s.Assert().NotEqual(projectID, recreateResp.Msg.GetProject().GetId()) + }) +} + func TestEndToEndAPIRegressionTestSuite(t *testing.T) { suite.Run(t, new(APIRegressionTestSuite)) }