fix(admin): revoke reaches an admin who created another admin - #89
Open
42-v wants to merge 2 commits into
Open
Conversation
POST /admin/admins/{id}/revoke could not revoke any admin who had opened another
account. The account survived, and so did its live sessions.
auth.admin_users.created_by referenced its own table with no ON DELETE clause,
so NO ACTION applied and deleting a row another row still named raised 23503.
AdminUserRepo.Revoke is a bare DELETE, so the whole statement failed.
admin_sessions cascades, but only when the parent delete succeeds, and it did
not -- so the sessions the revoke exists to kill stayed open.
created_by is set on every create and 016 raises when it is null once any admin
exists, so the admin graph is a tree. The bootstrap super_admin became
unrevokable the moment it opened one other account, which is the first thing an
operator does.
That is worse than an ordinary 500 because revoke is the only containment lever
the admin plane has: router.go:125-128 is list, create, revoke, with no admin
update, no admin lock and no per-admin-session revoke. An admin whose
credentials were known to be compromised could not be stopped at all.
042 makes the constraint ON DELETE SET NULL. Deliberately not CASCADE: that
would delete the revoked admin's entire created subtree, so revoking one
compromised account would silently remove every account it had ever opened.
The handler now reads the row before deleting it and writes the old created_by
into the audit metadata, so "who authorized this account" survives the SET NULL,
and it audits the failure path as well. Returning 500 before the audit call left
no trace of the one event an operator most needs to see: the containment attempt
that did not work.
The existing repo test passes on the unfixed code because it only ever revokes a
leaf. The new one revokes upward and asserts all three properties: the creator
is gone, its sessions cascaded to zero, and the child survives with created_by
cleared. Removing the migration fails it.
docs/admin-gateway.md asserted the opposite of the behaviour in the tree.
The coverage gate counted two statements neither covered nor excluded: the provenance read that keeps created_by before the SET NULL erases it, and the audit row written when the revoke itself fails. Both are now driven. The first asserts that a successful revoke carries revoked_admin_created_by, which is where the answer to "who authorized this account" survives once 042 nulls the column on every account the revoked admin opened. The second asserts a failed revoke writes a row at all, with an outcome and a reason -- it used to return 500 before reaching the audit call, so the operator was told the request failed and the trail said the attempt never happened. Wired through captureAudit and a directly constructed Handler, which is the idiom audit_actor_test.go already uses for this question. newTestHandler takes an audit repository but builds its logger from testAuditLog(), so rows written through the handler never reach a repository passed in -- worth knowing before writing the next audit assertion against it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
POST /admin/admins/{id}/revokecould not revoke any admin who had opened another account. The account survived, and so did its live sessions.auth.admin_users.created_byreferenced its own table with noON DELETEclause, soNO ACTIONapplied and deleting a row another row still named raised 23503.AdminUserRepo.Revokeis a bareDELETE, so the whole statement failed.admin_sessionscascades — but only when the parent delete succeeds, and it did not, so the sessions the revoke exists to kill stayed open.created_byis set on every create and migration 016 raises when it is null once any admin exists, so the admin graph is a tree. The bootstrapsuper_adminbecame unrevokable the moment it opened one other account — which is the first thing an operator does.Why this outranks an ordinary 500: revoke is the only containment lever the admin plane has.
router.go:125-128is the entire surface — list, create, revoke. No admin update, no admin lock, no per-admin-session revoke. An admin whose credentials were known to be compromised could not be stopped at all.The fix
Migration 042 makes the constraint
ON DELETE SET NULL. Deliberately notCASCADE— that would delete the revoked admin's entire created subtree, so revoking one compromised account would silently remove every account it had ever opened. Worse than the bug.The handler now:
created_byinto the audit metadata, so "who authorized this account" survives theSET NULLCoverage
The existing repo test passes on the unfixed code because it only ever revokes a leaf (
target.CreatedBy = admin.ID, then revokes the target). The new test revokes upward and asserts all three properties:created_byclearedSET NULL, notCASCADERemoving the migration fails it with the defect named.
docs/admin-gateway.mdasserted the opposite of the tree's behaviour and is corrected;UPGRADING.mdcarries the migration count and what the upgrade does.Verified:
go build ./...,go vet,go test -race ./internal/adminapi/,tests/spec+tests/compliancegreen,golangci-lint0 issues on a cleaned cache.