Repository navigation
fix(resource): deleting a resource also deletes the policies on it - #2015
Conversation
Delete now lists the live policies on the resource and deletes each one through the policy service before it removes the resource's relations and row. A policy that is already gone is skipped; any other error stops the delete. policy.Filter gains ResourceID for the lookup.
Coverage Report for CI Build 39773582069Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.08%) to 55.67%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions280 previously-covered lines in 7 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
Issue #1993 requires resource deletion to remove the policies granted on the resource. The PR adds cleanup to resource.Service.Delete, and the direct project-resource and organization-resource paths… |
Make the project cascade use the policy-cleaning Delete path, or add equivalent policy cleanup to the cascade. Add a regression test that deletes a project and verifies that policies on its resources are absent from ListPolicies and `Ge… |
-
Check name Status Explanation Out of Scope Changes check The ResourceIDfilter, repository filtering, policy cleanup in resource deletion, and the unit, repository, and end-to-end tests directly support issue#1993. No unrelated change is demonstrated.
Full details: Linked Issues check
-
Explanation
Issue
#1993requires resource deletion to remove the policies granted on the resource. The PR adds cleanup toresource.Service.Delete, and the direct project-resource and organization-resource paths use that behavior. The project cascade still callsresService.Purgeincore/deleter/service.go. This PR does not change that call, andPurgeis not shown to perform policy cleanup. Therefore, policies on resources deleted by project deletion can remain.Resolution
Make the project cascade use the policy-cleaning
Deletepath, or add equivalent policy cleanup to the cascade. Add a regression test that deletes a project and verifies that policies on its resources are absent fromListPoliciesandGetPolicy.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
deletePolicies takes the loaded resource, so a wrong namespace from a caller cannot leave policies behind. Tests cover a failed policy list, the stop at the first failed policy delete, and the stored namespace; the e2e case also checks the user's permission and the policy's role binding rows in relations.
Summary
Deleting a resource now also deletes the policies granted on it. Before, the delete removed the resource's SpiceDB relations and soft-deleted its row, but each policy on it kept its row and its two role binding relations, and
ListPoliciesandGetPolicystill returned it. This applies toDeleteProjectResource,DeleteOrganizationResource, and the organization delete for the org's own resources. The project delete reaches it once #2013 replaces itsPurgecall withDelete.Closes #1993 together with #2013.
Changes
core/policy/filter.go:FiltergainsResourceID.internal/store/postgres/policy_repository.go:ListandCountfilter onresource_idwhenResourceIDis set.core/resource/service.go:Deletelists the live policies on the resource by the id and namespace stored on the resource and deletes each one withpolicy.Service.Delete, before it deletes the resource's relations and row. A policy that is already gone is skipped. Any other error stops the delete and leaves the resource in place; the policies deleted before it stay deleted, and a retry deletes the rest.Technical Details
policy.Service.Deleteremoves the policy's role binding relations, then soft-deletes the row and writes thepolicy.deletedaudit record in the same transaction. Noapp.policy.deletedevent is published; only theDeletePolicyRPC publishes it, and the project delete removes its policies the same way.resource_idcondition uses the existingpolicies_resource_id_resource_type_idxindex.Test Plan
core/resource/service_test.goTestDelete: the policies on the resource are deleted before its relations and row; a policy that is already gone is skipped; a failed policy list or a failed policy delete leaves the relations and row alone, and the policies after a failed delete are not tried; the lookup uses the namespace stored on the resource, not the one the caller passed. The other cases that reach the delete expect the policy lookup.internal/store/postgres/policy_repository_test.goTestList:ResourceIDreturns the policies on that resource and none for another id.TestResourceAPIcase 4: afterDeleteProjectResource,ListPoliciesfor the user granted a role on the resource is empty,GetPolicyanswers not found, the user's permission on the resource is gone, and the policy's two role binding rows inrelationsare marked deleted.TestResourceAPIcase 7: afterDeleteOrganizationResource,GetPolicyfor the member's policy on the resource answers not found.relationscheck catches it. Without theresource_idcondition, the repository case fails.go test ./core/... ./internal/api/...and the policy repository suite pass.TestEndToEndAPIRegressionTestSuite,TestEndToEndServiceRegistrationRegressionTestSuite,TestEndToEndPATRegressionTestSuite,TestEndToEndServiceUsersRegressionTestSuiteandTestEndToEndOnboardingRegressionTestSuitepass.golangci-lint run ./core/resource/... ./core/policy/... ./internal/store/postgres/... ./test/e2e/regression/...reports no issues.SQL Safety
resource_idthroughgoqu.Ex{}.ToSQL()callers capture and forward params (query, params, err := stmt.ToSQL(); db.…Context(ctx, …, query, params...)). Neverquery, _, err := ….?placeholders inside single-quoted SQL literals ingoqu.L.//nolint:forbidigoor// #nosec G20xannotations.