fix(resource): deleting a resource also deletes the policies on it

core/policy/filter.go

@@ -11,6 +11,7 @@ type Filter struct {
 	PrincipalID   string
 	PrincipalIDs  []string
 	ResourceType  string
+	ResourceID    string
 
 	RolePermissions []string
 }

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

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,21 +736,23 @@ 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")
 		assert.ErrorContains(t, err, "spicedb down")
 	})
 
 	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) {

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,

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 {

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()