Repository navigation
Conversation
DeleteUser deletes every live policy of the user after the org removals and before the PATs and the user row. Policies on disabled projects and groups, on projects in orgs where the user has no org-level policy, and on resources in disabled projects stayed live before.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Policy cleanup contract and mock core/deleter/service.go, core/deleter/mocks/membership_service.go |
MembershipService adds RemovePrincipalPolicies. Its mock adds the method and typed expectation helpers. |
Deletion flow and unit coverage core/deleter/service.go, core/deleter/service_test.go |
DeleteUser removes principal policies after organization membership removal and before personal access token cleanup. Unit tests cover this order and assert that errors stop later deletion steps. |
End-to-end deletion regression test/e2e/regression/api_test.go |
The regression test verifies that user deletion removes live policies and resource access, and removes the user from organization users and role pairs. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Suggested reviewers: whoabhisheksah
Merge Risk: 
08ae2
-
User deletion is set up to remove remaining live policies before deleting the user. No actionable merge-blocking risk remains after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- 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.
Coverage Report for CI Build 40169788070Coverage increased (+0.01%) to 55.739%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Summary
DeleteUsernow deletes every live policy the user holds before it deletes the user. Before, it only removed the user from the orgs where it held an org-level policy, so these policies stayed live and kept naming a user that no longer exists:CreatePolicydoes not check org membership);Changes
core/deleter/service.go: after the org removals,DeleteUsercallsRemovePrincipalPoliciesfor the user, before it deletes the PATs and the user. A failure stops the delete and leaves the user in place; a retry deletes the rest.core/deleter/mocks/membership_service.go: regenerated for the new interface method.Technical Details
membership.Service.RemovePrincipalPolicieslists the live policies by principal, on any resource type and in any org, and deletes each one withpolicy.Service.Delete: the role binding relations first, then the row. The service user delete uses the same call.ForceRemoveOrganizationMemberis unchanged.user.Service.Deletealready removes every relation whose subject is the user, which includes each policy's bearer relation. They still showed inListPoliciesandGetPolicy.Test Plan
core/deleter/service_test.goTestDeleteUser: the user is removed from each org, then its remaining policies are deleted, then its PATs, then the user. An org removal failure stops the delete before the policy step, and a policy step failure stops it before the PATs and the user. The cases fail without the policy step, with its error only logged, or with it after the PAT step.TestUserDeleteLeavesNoAccessAndNoLivePolicy: a user holds policies on an org, a disabled project, a disabled group, a project in another org, and an order inside the disabled project. AfterDeleteUser, no live policy names the user (checked per resource in the database and withListPolicies), the user can get none of them, andListOrganizationUsersnames the user in neitherusersnorrole_pairs. Without the policy step, the policies on the disabled project, the disabled group, the other org's project and the order stay live.go test ./core/deleter/...passes.go test ./test/e2e/...passes.golangci-lint run ./core/deleter/... ./test/e2e/regression/...reports no issues.