Repository navigation
feat(store): user delete keeps the row - #2023
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 46 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Remove user policies during deletion core/deleter/service.go, core/deleter/service_test.go, core/deleter/mocks/membership_service.go, test/e2e/regression/api_test.go |
DeleteUser removes organization memberships, then principal policies, then PATs, and finally the user. Errors during membership or policy removal stop later cleanup. The regression test checks that deletion removes live policies and access. |
User soft deletion and audit timestamp internal/store/postgres/user_repository.go, internal/store/postgres/user_repository_test.go |
Delete now uses soft deletion and records the row’s DeletedAt.Time. Tests verify row retention, later lookup and deletion behavior, and recreation with the same name and email under a different ID. |
Filter PATs by live owner internal/store/postgres/org_pats_repository.go, internal/store/postgres/org_pats_repository_test.go, internal/store/postgres/org_pats_repository_pg_test.go |
The shared PAT query joins only live users. Unit and PostgreSQL tests check that searches exclude PATs of soft-deleted users and return matching totals. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Possibly related PRs
- raystack/frontier#2024: Ends a deleted user’s sessions before deletion. This is related to the retained user row and session cleanup.
- raystack/frontier#2022: Adds principal-policy removal to the user deletion cascade, which this PR also uses.
Suggested reviewers: rohilsurana, whoabhisheksah
Merge Risk: 
dc6c8
-
User deletion now also removes the user's policies before cleaning up PATs and deleting the user. A failure partway through can be recovered by retrying the deletion. No merge-blocking issue remains in the reviewed changes.
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 40499812944Coverage increased (+0.03%) to 55.78%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
The user row gets deleted_at instead of being removed, and the user.deleted audit record takes its occurred_at from deleted_at. A second delete answers not found.
The search inner joins users with the live filter instead of a left join, so the list and the count skip a PAT whose owner is soft-deleted.
dc6c8a8 to
a9ace05
Compare
Summary
Deleting a user now keeps the user row and sets
deleted_at, like the other soft deletes. Theuser.deletedaudit record takes its time fromdeleted_at. The organization PAT search now lists a PAT only when its owner is live.Changes
internal/store/postgres/user_repository.go:Deletesetsdeleted_aton the live row instead of deleting it. It still returns the row and writes theuser.deletedrecord in the same transaction, now withoccurred_atequal todeleted_at. A second delete answersuser.ErrNotExist.internal/store/postgres/org_pats_repository.go: the main query inner joinsuserson the PAT owner withusers.deleted_at IS NULL, instead of a left join. The count and the list both skip a PAT whose owner is deleted.Technical Details
userswithout the live filter.user_pats.user_idisNOT NULLand referencesusers, so every PAT has an owner row, and the inner join drops exactly the PATs whose owner is deleted. The user delete removes the user's PATs before the user row, so this only matters for a PAT created while a delete runs.sessions.user_idreferencesuserswithON DELETE CASCADE, so the hard delete removed the user's sessions with the row. The soft delete does not. The user delete ends the sessions just before the row (fix(user): user delete ends the user's sessions #2024), so a deleted user's session cookie still gets unauthenticated.Test Plan
internal/store/postgres/user_repository_test.goTestDeleteKeepsTheRow: the row stays withdeleted_atset,GetByIDanswers not found, a second delete answersuser.ErrNotExistand writes no second record, the record'soccurred_atequalsdeleted_at, and a new user can be created with the same email and name. With the hard delete back, the row check fails; withtime.Now()back, the time check fails.internal/store/postgres/org_pats_repository_test.go: the count and data queries contain the inner join with the live filter.internal/store/postgres/org_pats_repository_pg_test.go: with one live and one soft-deleted owner, the list and the count return only the live owner's PAT, and a search for the deleted owner's name finds nothing. Both cases fail with the left join this PR replaces. With the live filter in the condition of a left join, the list case fails.TestUserAPIcase 13 passes with this PR onmain: after an admin deletes a logged-in user, the same session cookie gets unauthenticated.go test ./internal/store/postgres/...passes.go test ./test/e2e/...passes.golangci-lint run ./internal/store/postgres/...reports no issues.SQL Safety
softDeletewithgoqu.Ex{"id": id}, and the join condition built withgoqu.On,goqu.Iand thelivehelper.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.