diff --git a/core/policy/filter.go b/core/policy/filter.go index ac51bbcc47..a0ad62c06f 100644 --- a/core/policy/filter.go +++ b/core/policy/filter.go @@ -11,6 +11,7 @@ type Filter struct { PrincipalID string PrincipalIDs []string ResourceType string + ResourceID string RolePermissions []string } diff --git a/core/resource/service.go b/core/resource/service.go index 09961dc129..b2274e5060 100644 --- a/core/resource/service.go +++ b/core/resource/service.go @@ -504,6 +504,9 @@ func (s Service) Delete(ctx context.Context, namespaceID, id string) error { if err != nil { return err } + if err := s.deletePolicies(ctx, res); err != nil { + return err + } if err := s.relationService.Delete(ctx, relation.Relation{ Object: relation.Object{ @@ -521,6 +524,19 @@ func (s Service) Delete(ctx context.Context, namespaceID, id string) error { return nil } +func (s Service) deletePolicies(ctx context.Context, res Resource) error { + policies, err := s.policyService.List(ctx, policy.Filter{ResourceID: res.ID, ResourceType: res.NamespaceID}) + if err != nil { + return fmt.Errorf("list resource policies: %w", err) + } + for _, pol := range policies { + if err := s.policyService.Delete(ctx, pol.ID); err != nil && !errors.Is(err, policy.ErrNotExist) { + return fmt.Errorf("delete resource policy %s: %w", pol.ID, err) + } + } + return nil +} + // 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 the project delete cascade soft-deletes resources diff --git a/core/resource/service_test.go b/core/resource/service_test.go index 837acdadd9..9746ccba19 100644 --- a/core/resource/service_test.go +++ b/core/resource/service_test.go @@ -666,12 +666,14 @@ func TestDelete(t *testing.T) { } res := resource.Resource{ID: "r1", Name: "res-1", Title: "Resource One", NamespaceID: "resource/item", ProjectID: proj.ID} lookup := project.Filter{ProjectIDs: []string{proj.ID}, IncludeDisabled: true} + resPolicies := policy.Filter{ResourceID: "r1", ResourceType: "resource/item"} t.Run("deletes relations then the row, and writes the audit record", func(t *testing.T) { - repo, relationSvc, _, projectSvc, _, _, auditRepo, _, svc := newTestService(t) + repo, relationSvc, _, projectSvc, _, _, auditRepo, policySvc, svc := newTestService(t) repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) + policySvc.EXPECT().List(mock.Anything, resPolicies).Return([]policy.Policy{}, nil) relationSvc.EXPECT().Delete(mock.Anything, relation.Relation{ Object: relation.Object{ID: "r1", Namespace: "resource/item"}, }).Return(nil) @@ -687,12 +689,14 @@ func TestDelete(t *testing.T) { }) t.Run("deletes an org resource and names the org in the audit record", func(t *testing.T) { - repo, relationSvc, _, _, orgSvc, _, auditRepo, _, svc := newTestService(t) + repo, relationSvc, _, _, orgSvc, _, auditRepo, policySvc, svc := newTestService(t) org := organization.Organization{ID: uuid.New().String(), Title: "Org Two"} orgRes := resource.Resource{ID: "r2", Name: "store-1", NamespaceID: "resource/store", OrgID: org.ID} repo.EXPECT().GetByID(mock.Anything, "r2").Return(orgRes, nil) orgSvc.EXPECT().GetRaw(mock.Anything, org.ID).Return(org, nil) + policySvc.EXPECT().List(mock.Anything, policy.Filter{ResourceID: "r2", ResourceType: "resource/store"}). + Return([]policy.Policy{}, nil) relationSvc.EXPECT().Delete(mock.Anything, relation.Relation{ Object: relation.Object{ID: "r2", Namespace: "resource/store"}, }).Return(nil) @@ -717,10 +721,11 @@ func TestDelete(t *testing.T) { }) t.Run("ignores relation not exist error", func(t *testing.T) { - repo, relationSvc, _, projectSvc, _, _, auditRepo, _, svc := newTestService(t) + repo, relationSvc, _, projectSvc, _, _, auditRepo, policySvc, svc := newTestService(t) repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) + policySvc.EXPECT().List(mock.Anything, resPolicies).Return([]policy.Policy{}, nil) relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(relation.ErrNotExist) repo.EXPECT().Delete(mock.Anything, "r1").Return(nil) auditRepo.EXPECT().Create(mock.Anything, mock.AnythingOfType("models.AuditRecord")). @@ -731,10 +736,11 @@ func TestDelete(t *testing.T) { }) t.Run("returns relation delete error", func(t *testing.T) { - repo, relationSvc, _, projectSvc, _, _, _, _, svc := newTestService(t) + repo, relationSvc, _, projectSvc, _, _, _, policySvc, svc := newTestService(t) repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) + policySvc.EXPECT().List(mock.Anything, resPolicies).Return([]policy.Policy{}, nil) relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(errors.New("spicedb down")) err := svc.Delete(ctx, "resource/item", "r1") @@ -742,10 +748,11 @@ func TestDelete(t *testing.T) { }) t.Run("writes no audit record when the row delete fails", func(t *testing.T) { - repo, relationSvc, _, projectSvc, _, _, _, _, svc := newTestService(t) + repo, relationSvc, _, projectSvc, _, _, _, policySvc, svc := newTestService(t) repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) + policySvc.EXPECT().List(mock.Anything, resPolicies).Return([]policy.Policy{}, nil) relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(nil) repo.EXPECT().Delete(mock.Anything, "r1").Return(errors.New("db down")) @@ -754,12 +761,13 @@ func TestDelete(t *testing.T) { }) t.Run("deletes a resource of a disabled project", func(t *testing.T) { - repo, relationSvc, _, projectSvc, _, _, auditRepo, _, svc := newTestService(t) + repo, relationSvc, _, projectSvc, _, _, auditRepo, policySvc, svc := newTestService(t) disabled := proj disabled.State = project.Disabled repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{disabled}, nil) + policySvc.EXPECT().List(mock.Anything, resPolicies).Return([]policy.Policy{}, nil) relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(nil) repo.EXPECT().Delete(mock.Anything, "r1").Return(nil) auditRepo.EXPECT().Create(mock.Anything, mock.MatchedBy(func(rec auditmodels.AuditRecord) bool { @@ -779,6 +787,85 @@ func TestDelete(t *testing.T) { err := svc.Delete(ctx, "resource/item", "r1") assert.ErrorIs(t, err, project.ErrNotExist) }) + + t.Run("deletes the policies on the resource before its relations and row", func(t *testing.T) { + repo, relationSvc, _, projectSvc, _, _, auditRepo, policySvc, svc := newTestService(t) + var steps []string + + repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) + projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) + policySvc.EXPECT().List(mock.Anything, resPolicies).Return([]policy.Policy{{ID: "pol-1"}, {ID: "pol-2"}}, nil) + policySvc.EXPECT().Delete(mock.Anything, mock.Anything). + Run(func(_ context.Context, id string) { steps = append(steps, "policy:"+id) }). + Return(nil) + relationSvc.EXPECT().Delete(mock.Anything, mock.Anything). + Run(func(_ context.Context, _ relation.Relation) { steps = append(steps, "relations") }). + Return(nil) + repo.EXPECT().Delete(mock.Anything, "r1"). + Run(func(_ context.Context, _ string) { steps = append(steps, "row") }). + Return(nil) + auditRepo.EXPECT().Create(mock.Anything, mock.AnythingOfType("models.AuditRecord")). + Return(auditmodels.AuditRecord{}, nil) + + err := svc.Delete(ctx, "resource/item", "r1") + assert.NoError(t, err) + assert.Equal(t, []string{"policy:pol-1", "policy:pol-2", "relations", "row"}, steps) + }) + + t.Run("skips a policy that is already gone", func(t *testing.T) { + repo, relationSvc, _, projectSvc, _, _, auditRepo, policySvc, svc := newTestService(t) + + repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) + projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) + policySvc.EXPECT().List(mock.Anything, resPolicies).Return([]policy.Policy{{ID: "pol-1"}}, nil) + policySvc.EXPECT().Delete(mock.Anything, "pol-1").Return(policy.ErrNotExist) + relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(nil) + repo.EXPECT().Delete(mock.Anything, "r1").Return(nil) + auditRepo.EXPECT().Create(mock.Anything, mock.AnythingOfType("models.AuditRecord")). + Return(auditmodels.AuditRecord{}, nil) + + err := svc.Delete(ctx, "resource/item", "r1") + assert.NoError(t, err) + }) + + t.Run("stops at the first failed policy delete and keeps the resource", func(t *testing.T) { + repo, _, _, projectSvc, _, _, _, policySvc, svc := newTestService(t) + + repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) + projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) + policySvc.EXPECT().List(mock.Anything, resPolicies).Return([]policy.Policy{{ID: "pol-1"}, {ID: "pol-2"}}, nil) + policySvc.EXPECT().Delete(mock.Anything, "pol-1").Return(errors.New("spicedb down")) + + err := svc.Delete(ctx, "resource/item", "r1") + assert.ErrorContains(t, err, "spicedb down") + policySvc.AssertNotCalled(t, "Delete", mock.Anything, "pol-2") + }) + + t.Run("keeps the resource when listing its policies fails", func(t *testing.T) { + repo, _, _, projectSvc, _, _, _, policySvc, svc := newTestService(t) + + repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) + projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) + policySvc.EXPECT().List(mock.Anything, resPolicies).Return(nil, errors.New("db down")) + + err := svc.Delete(ctx, "resource/item", "r1") + assert.ErrorContains(t, err, "db down") + }) + + t.Run("looks up the policies with the namespace stored on the resource", func(t *testing.T) { + repo, relationSvc, _, projectSvc, _, _, auditRepo, policySvc, svc := newTestService(t) + + repo.EXPECT().GetByID(mock.Anything, "r1").Return(res, nil) + projectSvc.EXPECT().List(mock.Anything, lookup).Return([]project.Project{proj}, nil) + policySvc.EXPECT().List(mock.Anything, resPolicies).Return([]policy.Policy{}, nil) + relationSvc.EXPECT().Delete(mock.Anything, mock.Anything).Return(nil) + repo.EXPECT().Delete(mock.Anything, "r1").Return(nil) + auditRepo.EXPECT().Create(mock.Anything, mock.AnythingOfType("models.AuditRecord")). + Return(auditmodels.AuditRecord{}, nil) + + err := svc.Delete(ctx, "resource/other", "r1") + assert.NoError(t, err) + }) } func TestPurge(t *testing.T) { diff --git a/internal/store/postgres/policy_repository.go b/internal/store/postgres/policy_repository.go index 0b755270f8..029c9d3d18 100644 --- a/internal/store/postgres/policy_repository.go +++ b/internal/store/postgres/policy_repository.go @@ -120,6 +120,11 @@ func applyListFilter(stmt *goqu.SelectDataset, flt policy.Filter) *goqu.SelectDa "resource_type": flt.ResourceType, }) } + if flt.ResourceID != "" { + stmt = stmt.Where(goqu.Ex{ + "resource_id": flt.ResourceID, + }) + } if flt.RoleID != "" { stmt = stmt.Where(goqu.Ex{ "role_id": flt.RoleID, diff --git a/internal/store/postgres/policy_repository_test.go b/internal/store/postgres/policy_repository_test.go index 504f308f79..5bd85b7e99 100644 --- a/internal/store/postgres/policy_repository_test.go +++ b/internal/store/postgres/policy_repository_test.go @@ -379,6 +379,30 @@ func (s *PolicyRepositoryTestSuite) TestList() { ProjectID: uuid.NewString(), }, }, + { + Description: "should get the policies on one resource", + ExpectedPolicys: []policy.Policy{ + { + RoleID: s.roles[0].ID, + PrincipalID: s.userID, + PrincipalType: schema.UserPrincipal, + ResourceID: s.orgID, + ResourceType: "ns1", + }, + }, + flt: policy.Filter{ + ResourceID: s.orgID, + ResourceType: "ns1", + }, + }, + { + Description: "should get no policies for another resource", + ExpectedPolicys: nil, + flt: policy.Filter{ + ResourceID: uuid.NewString(), + ResourceType: "ns1", + }, + }, } for _, tc := range testCases { diff --git a/test/e2e/regression/api_test.go b/test/e2e/regression/api_test.go index f8a930bb04..29d80d9812 100644 --- a/test/e2e/regression/api_test.go +++ b/test/e2e/regression/api_test.go @@ -2351,6 +2351,40 @@ func (s *APIRegressionTestSuite) TestResourceAPI() { s.Require().NoError(err) resourceID := createResourceResp.Msg.GetResource().GetId() + userResp, err := s.testBench.Client.CreateUser(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateUserRequest{Body: &frontierv1beta1.UserRequestBody{ + Title: "user_proj_resource_4", Email: "user-proj-resource-4@raystack.org", Name: "user_proj_resource_4", + }})) + s.Require().NoError(err) + userID := userResp.Msg.GetUser().GetId() + createPolicyResp, err := s.testBench.Client.CreatePolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePolicyRequest{ + Body: &frontierv1beta1.PolicyRequestBody{ + RoleId: computeManagerRoleName, + Resource: schema.JoinNamespaceAndResourceID(computeOrderNamespace, resourceID), + Principal: schema.JoinNamespaceAndResourceID(schema.UserPrincipal, userID), + }, + })) + s.Require().NoError(err) + policyID := createPolicyResp.Msg.GetPolicy().GetId() + userCanUpdateResource := func() bool { + resp, err := s.testBench.AdminClient.CheckFederatedResourcePermission(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CheckFederatedResourcePermissionRequest{ + Resource: schema.JoinNamespaceAndResourceID(computeOrderNamespace, resourceID), + Permission: schema.UpdatePermission, + Subject: schema.JoinNamespaceAndResourceID(schema.UserPrincipal, userID), + })) + s.Require().NoError(err) + return resp.Msg.GetStatus() + } + liveRoleBindingRows := func() int { + var rows int + s.Require().NoError(s.dbc.GetContext(context.Background(), &rows, + "SELECT count(*) FROM relations WHERE object_namespace_name = $1 AND object_id = $2 AND deleted_at IS NULL", + schema.RoleBindingNamespace, policyID)) + return rows + } + s.Require().Len(livePoliciesMatching(s.T(), ctxOrgAdminAuth, s.testBench.Client, &frontierv1beta1.ListPoliciesRequest{UserId: userID}), 1) + s.Require().True(userCanUpdateResource()) + s.Require().Equal(2, liveRoleBindingRows()) + _, err = s.testBench.Client.DeleteProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteProjectResourceRequest{ ProjectId: projectID, Id: resourceID, @@ -2369,6 +2403,12 @@ func (s *APIRegressionTestSuite) TestResourceAPI() { s.Require().NoError(err) s.Assert().Empty(listResp.Msg.GetResources()) + s.Assert().Empty(livePoliciesMatching(s.T(), ctxOrgAdminAuth, s.testBench.Client, &frontierv1beta1.ListPoliciesRequest{UserId: userID})) + _, err = s.testBench.Client.GetPolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetPolicyRequest{Id: policyID})) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + s.Assert().False(userCanUpdateResource()) + s.Assert().Zero(liveRoleBindingRows()) + _, err = s.testBench.Client.DeleteProjectResource(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteProjectResourceRequest{ ProjectId: projectID, Id: resourceID, @@ -2616,6 +2656,15 @@ func (s *APIRegressionTestSuite) TestResourceAPI() { _, err = s.testBench.Client.DeleteOrganizationResource(ctxMember, deleteRequest(resourceID)) s.Assert().Equal(connect.CodePermissionDenied, connect.CodeOf(err)) + createPolicyResp, err := s.testBench.Client.CreatePolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePolicyRequest{ + Body: &frontierv1beta1.PolicyRequestBody{ + RoleId: computeManagerRoleName, + Resource: schema.JoinNamespaceAndResourceID(computeOrderNamespace, resourceID), + Principal: schema.JoinNamespaceAndResourceID(schema.UserPrincipal, memberIDs[1]), + }, + })) + s.Require().NoError(err) + _, err = s.testBench.Client.DeleteOrganizationResource(ctxCreator, deleteRequest(resourceID)) s.Require().NoError(err) @@ -2630,6 +2679,9 @@ func (s *APIRegressionTestSuite) TestResourceAPI() { s.Require().NoError(err) s.Assert().False(checkResp.Msg.GetStatus()) + _, err = s.testBench.Client.GetPolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetPolicyRequest{Id: createPolicyResp.Msg.GetPolicy().GetId()})) + s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err)) + recreateResp, err := s.testBench.Client.CreateOrganizationResource(ctxCreator, createRequest()) s.Require().NoError(err) recreatedID := recreateResp.Msg.GetResource().GetId()