fix(admin): revoke reaches an admin who created another admin

README.md

@@ -10,7 +10,7 @@ Vault42 issues its own tokens and is an OAuth2 *client* of other providers. It i
 | ![Go](https://img.shields.io/badge/Go-1.26.6-00ADD8?style=flat&logo=go&logoColor=white) | ![Vue](https://img.shields.io/badge/Vue-3.5.41-4FC08D?style=flat&logo=vuedotjs&logoColor=white) | ![.NET](https://img.shields.io/badge/.NET-10.0-512BD4?style=flat&logo=dotnet&logoColor=white) | ![License](https://img.shields.io/badge/License-MIT-155724?style=flat&labelColor=000) |
 | ![Go Tests](https://img.shields.io/badge/Tests-4910-155724?style=flat&labelColor=000) | ![Vue Tests](https://img.shields.io/badge/Tests-1305-155724?style=flat&labelColor=000) | ![C# Tests](https://img.shields.io/badge/Tests-264-155724?style=flat&labelColor=000) | ![Total](https://img.shields.io/badge/Total-6479_tests-155724?style=flat&labelColor=000) |
 | ![Go Coverage](https://img.shields.io/badge/Coverage-100.00%25_reachable-155724?style=flat&labelColor=000) | ![Vue Coverage](https://img.shields.io/badge/Coverage-99.76%25-155724?style=flat&labelColor=000) | ![C# Coverage](https://img.shields.io/badge/Coverage-100.00%25-155724?style=flat&labelColor=000) | ![Locales](https://img.shields.io/badge/Locales-38-555?style=flat&labelColor=000) |
-| ![Go Lines](https://img.shields.io/badge/Lines-48129-555?style=flat&labelColor=000) | ![Vue Lines](https://img.shields.io/badge/Lines-6800-555?style=flat&labelColor=000) | ![C# Lines](https://img.shields.io/badge/Lines-2435-555?style=flat&labelColor=000) | ![Standards](https://img.shields.io/badge/Standards-11-555?style=flat&labelColor=000) |
+| ![Go Lines](https://img.shields.io/badge/Lines-48153-555?style=flat&labelColor=000) | ![Vue Lines](https://img.shields.io/badge/Lines-6800-555?style=flat&labelColor=000) | ![C# Lines](https://img.shields.io/badge/Lines-2435-555?style=flat&labelColor=000) | ![Standards](https://img.shields.io/badge/Standards-11-555?style=flat&labelColor=000) |
 | ![Go Deps](https://img.shields.io/badge/Deps-3-555?style=flat&labelColor=000) | ![Vue Deps](https://img.shields.io/badge/Deps-3-555?style=flat&labelColor=000) | ![C# Deps](https://img.shields.io/badge/Deps-6-555?style=flat&labelColor=000) | ![Requirements](https://img.shields.io/badge/Requirements-456-555?style=flat&labelColor=000) |
 | ![Go Transitive Deps](https://img.shields.io/badge/Transitive-15-555?style=flat&labelColor=000) | ![Vue Transitive Deps](https://img.shields.io/badge/Transitive-95-555?style=flat&labelColor=000) | ![C# Transitive Deps](https://img.shields.io/badge/Transitive-26-555?style=flat&labelColor=000) | ![Total Deps](https://img.shields.io/badge/Deps-148_total-555?style=flat&labelColor=000) |
 <!-- /badges -->

docs/UPGRADING.md

@@ -32,12 +32,17 @@ chart.
 
 **Secret.** No new keys. The Deployment mounts the same eight it did in 1.0.3.
 
-**Schema.** v1.0.3 shipped 39 migrations; this release ships 39, so an upgrade applies 0.
-There is no schema change in 1.0.4 and nothing to migrate. Take the backup anyway: the point
+**Schema.** v1.0.3 shipped 39 migrations; this release ships 40, so an upgrade applies 1.
+042 changes `auth.admin_users.created_by` from `NO ACTION` to `ON DELETE SET NULL`. It moves
+no data and adds no column. Until it runs, `POST /admin/admins/{id}/revoke` answers 500 for
+any admin who has created another admin, and that account and its live sessions stay --
+revoke is the only containment lever the admin plane has. Take the backup anyway: the point
 of step 1 is that you have one when something else goes wrong.
 
-**Behaviour.** None. 1.0.4 is documentation, three generator fixes and the gates that hold
-them; no request path, token, database or configuration behaviour differs from 1.0.3. The
+**Behaviour.** One fix, no new surface: `POST /admin/admins/{id}/revoke` now succeeds
+against an admin who has created other admins, and those accounts survive with their
+`created_by` cleared -- the account that authorized them is recorded in the revoke's audit
+row instead. No route, token or configuration changes shape. The
 one thing an operator may notice is that `docs/deps.md` and the README badge figures now
 report numbers that differ slightly from 1.0.3's, because those were being counted wrongly --
 see the 1.0.4 entry in [CHANGELOG.md](../CHANGELOG.md).

docs/admin-gateway.md

@@ -312,7 +312,7 @@ The UPDATE half is a real ceiling: it compares against `OLD.role`, which comes f
 - **TOTP secrets**: Encrypted at rest with AES-256 (master key)
 - **TOTP replay prevention**: Each accepted TOTP code's time-step counter is stored per admin. Replayed codes (same or earlier counter) are rejected within the ±1 period window
 - **Account lockout**: Configurable failed attempts threshold and lockout duration. Lockout counter is atomic (SQL `RETURNING` clause) -- immune to race conditions under concurrent login attempts
-- **Admin revocation**: Deleting an admin CASCADE deletes all sessions -- no race window between session revoke and admin revoke
+- **Admin revocation**: Deleting an admin CASCADE deletes all sessions -- no race window between session revoke and admin revoke. `created_by` is `ON DELETE SET NULL` (migration 042) so revoking an admin who opened other accounts succeeds and those accounts survive with their provenance moved into the audit row; before 042 it was `NO ACTION`, and revoking any admin who had created another failed with a 500 that left the account and its live sessions in place
 - **Audit trail**: All admin mutations logged with admin ID, timestamp, IP, user agent
 - **No external dependencies**: Uses stdlib HTTP only (no frameworks)
 - **RBAC hardcoded**: Permission maps defined in Go code, not database -- immune to SQL injection escalation

docs/badges.json

@@ -9,8 +9,8 @@
   "reachableCoverageNum": 100.00,
   "packages": 43,
   "goFiles": 192,
-  "goLines": 48129,
-  "testFiles": 927,
+  "goLines": 48153,
+  "testFiles": 929,
   "directDeps": 3,
   "transitiveDeps": 15,
   "totalTests": 6479,
@@ -20,7 +20,7 @@
       "tests": 4910,
       "coverage": "100.00% of reachable",
       "coverageNum": 100.00,
-      "lines": 48129,
+      "lines": 48153,
       "deps": 3,
       "transitiveDeps": 15
     },

internal/adminapi/admin_revoke_audit_test.go

@@ -0,0 +1,82 @@
+package adminapi
+
+import (
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/42-v/vault42/internal/model"
+)
+
+// Revoking an admin who opened other accounts nulls their created_by, so the
+// answer to "who authorized this account" leaves the row. It has to land
+// somewhere, and the audit trail is the only place left.
+func TestRevokeAdmin_RecordsTheProvenanceItIsAboutToErase(t *testing.T) {
+	const target = "00000000-0000-0000-0000-0000000000aa"
+
+	repo := newFakeAdminRepo()
+	repo.users[target] = &model.AdminUser{
+		ID: target, Username: "doomed", Role: "viewer", CreatedBy: "root",
+	}
+
+	var captured []*model.AuditEntry
+	h := &Handler{admins: repo, auditLog: captureAudit(&captured)}
+
+	rec := httptest.NewRecorder()
+	r := withActor(httptest.NewRequest(http.MethodPost, "/admin/admins/"+target+"/revoke", nil))
+	r.SetPathValue("id", target)
+	h.RevokeAdmin(rec, r)
+
+	if rec.Code != http.StatusOK {
+		t.Fatalf("status = %d: %s", rec.Code, rec.Body.String())
+	}
+	if len(captured) == 0 {
+		t.Fatal("no audit entry for a successful revoke")
+	}
+	last := captured[len(captured)-1]
+	if got := last.Metadata["revoked_admin_created_by"]; got != "root" {
+		t.Errorf("revoked_admin_created_by = %v, want \"root\". Migration 042 nulls that "+
+			"column on the accounts the revoked admin opened, so this row is where the "+
+			"provenance survives.", got)
+	}
+	if got := last.Metadata["outcome"]; got != "revoked" {
+		t.Errorf("outcome = %v, want \"revoked\"", got)
+	}
+}
+
+// A revoke that fails is the event an operator most needs to see: this route is
+// the only containment lever there is, so a 500 means a compromised account is
+// still live and nothing else can stop it. It used to return before the audit
+// call and leave no trace at all.
+func TestRevokeAdmin_AuditsAFailedContainmentAttempt(t *testing.T) {
+	const target = "00000000-0000-0000-0000-0000000000bb"
+
+	repo := newFakeAdminRepo()
+	repo.users[target] = &model.AdminUser{ID: target, Username: "doomed", Role: "viewer"}
+	repo.errRevoke = errors.New("violates foreign key constraint")
+
+	var captured []*model.AuditEntry
+	h := &Handler{admins: repo, auditLog: captureAudit(&captured)}
+
+	rec := httptest.NewRecorder()
+	r := withActor(httptest.NewRequest(http.MethodPost, "/admin/admins/"+target+"/revoke", nil))
+	r.SetPathValue("id", target)
+	h.RevokeAdmin(rec, r)
+
+	if rec.Code != http.StatusInternalServerError {
+		t.Fatalf("status = %d, want 500", rec.Code)
+	}
+	if len(captured) == 0 {
+		t.Fatal("a failed revoke wrote no audit row at all. The operator is told the request " +
+			"failed and the trail says the attempt never happened.")
+	}
+	last := captured[len(captured)-1]
+	if got := last.Metadata["outcome"]; got != "failed" {
+		t.Errorf("outcome = %v, want \"failed\"", got)
+	}
+	if got, _ := last.Metadata["error"].(string); got == "" {
+		t.Error("the audit row carries no reason, so it cannot distinguish a constraint " +
+			"violation from the database being down")
+	}
+}

internal/adminapi/handler.go

@@ -1121,17 +1121,41 @@ func (h *Handler) RevokeAdmin(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Read the row before deleting it. 042 makes created_by ON DELETE SET NULL,
+	// so revoking a creator nulls that column on every account it opened; this
+	// is where the answer to "who authorized this account" is preserved. A read
+	// failure is not fatal -- containment matters more than provenance -- so the
+	// revoke proceeds with the field absent rather than refusing.
+	var revokedCreatedBy string
+	if target, err := h.admins.GetByID(r.Context(), id); err == nil && target != nil {
+		revokedCreatedBy = target.CreatedBy
+	}
+
 	// Revoke admin first — sessions CASCADE delete via FK.
 	// This eliminates the race window where an in-flight request could
 	// pass SessionAuth between session revoke and admin revoke.
 	if err := h.admins.Revoke(r.Context(), id); err != nil {
+		// Audited on the way out. A failed revoke is the event an operator most
+		// needs to see: this route is the only containment lever there is, so a
+		// 500 here means a compromised account is still live and nothing else
+		// can stop it. Returning before the audit call left no trace at all.
+		_ = h.auditLog.Log(r.Context(), audit.AdminAccountRevoke, actor.ID, id, r.RemoteAddr, r.UserAgent(), "", "", map[string]interface{}{
+			"revoked_admin_id": id,
+			"outcome":          "failed",
+			"error":            err.Error(),
+		})
 		httputil.WriteError(w, http.StatusInternalServerError, "internal_error")
 		return
 	}
 
-	_ = h.auditLog.Log(r.Context(), audit.AdminAccountRevoke, actor.ID, id, r.RemoteAddr, r.UserAgent(), "", "", map[string]interface{}{
+	meta := map[string]interface{}{
 		"revoked_admin_id": id,
-	})
+		"outcome":          "revoked",
+	}
+	if revokedCreatedBy != "" {
+		meta["revoked_admin_created_by"] = revokedCreatedBy
+	}
+	_ = h.auditLog.Log(r.Context(), audit.AdminAccountRevoke, actor.ID, id, r.RemoteAddr, r.UserAgent(), "", "", meta)
 
 	httputil.WriteJSON(w, http.StatusOK, map[string]string{"status": "revoked"})
 }

migrations/042_admin_revoke_reaches_a_creator.sql

@@ -0,0 +1,49 @@
+-- ============================================================================
+-- 042: an admin who created another admin can still be revoked
+-- ============================================================================
+--
+-- POST /admin/admins/{id}/revoke could not revoke any admin who had created
+-- another admin. 001:252 declares
+--
+--     created_by UUID REFERENCES auth.admin_users(id)
+--
+-- with no ON DELETE clause, so the default NO ACTION applies, and
+-- AdminUserRepo.Revoke is a bare DELETE. Deleting a row that another row still
+-- names raises 23503 and the whole statement fails: the account stays, and so
+-- do its live sessions -- admin_sessions.admin_id cascades (001:257), but only
+-- if the parent delete succeeds, and it does not.
+--
+-- created_by is never null in practice. adminapi/handler.go sets it on every
+-- create and 016:84-88 raises when it is null once any admin exists, so the
+-- admin graph is a tree. The bootstrap super_admin becomes unrevokable the
+-- moment it creates one other admin, which is the first thing an operator does.
+--
+-- That matters more than an ordinary 500 because revoke is the only containment
+-- lever there is. adminapi/router.go:125-128 is the entire admin-management
+-- surface: list, create, revoke. There is no admin update, no admin lock and no
+-- per-admin-session revoke, so an admin whose credentials are known to be
+-- compromised cannot be stopped at all.
+--
+-- The existing coverage passes because it only ever revokes a leaf:
+-- tests/integration/postgres_admin_test.go sets target.CreatedBy = admin.ID and
+-- revokes the target, never the creator.
+--
+-- SET NULL rather than CASCADE, deliberately. CASCADE would delete the revoked
+-- admin's entire created subtree -- revoking one compromised account would
+-- silently remove every account it had ever created, which is a far worse
+-- outcome than the bug. SET NULL costs provenance on the children instead, and
+-- RevokeAdmin now writes the old created_by into the audit metadata so the
+-- answer to "who authorized this account" survives in the trail.
+--
+-- Neither role-escalation trigger fires on this. deny_role_escalation_on_insert
+-- is BEFORE INSERT only (016:129-132) and deny_role_escalation short-circuits
+-- unless the role changes (001:394-428); an FK-driven SET NULL touches neither.
+--
+-- Idempotent: the DROP is conditional and the ADD follows it, so re-running
+-- lands on the same constraint.
+
+ALTER TABLE auth.admin_users DROP CONSTRAINT IF EXISTS admin_users_created_by_fkey;
+
+ALTER TABLE auth.admin_users
+    ADD CONSTRAINT admin_users_created_by_fkey
+    FOREIGN KEY (created_by) REFERENCES auth.admin_users(id) ON DELETE SET NULL;

tests/integration/postgres_admin_revoke_creator_test.go

@@ -0,0 +1,88 @@
+package integration_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/42-v/vault42/internal/repository/postgres"
+)
+
+// TestRevokeReachesAnAdminWhoCreatedAnother proves the containment lever works
+// on the account it is most likely to be aimed at.
+//
+// auth.admin_users.created_by referenced the same table with no ON DELETE
+// clause, so the default NO ACTION applied and deleting a row another row still
+// named raised 23503. AdminUserRepo.Revoke is a bare DELETE, so the statement
+// failed, the account stayed, and its live sessions stayed with it --
+// admin_sessions cascades, but only if the parent delete succeeds.
+//
+// created_by is set on every create and migration 016 raises when it is null
+// once any admin exists, so the admin graph is a tree: the bootstrap
+// super_admin becomes unrevokable the moment it opens one other account, which
+// is the first thing an operator does. And revoke is the only lever there is --
+// the admin surface is list, create, revoke, with no update, no lock and no
+// per-admin-session revoke.
+//
+// The existing repo test passes because it only ever revokes a leaf. This one
+// revokes upward, which is the case that failed.
+func TestRevokeReachesAnAdminWhoCreatedAnother(t *testing.T) {
+	adminPool, _, cleanup := setupPostgres(t)
+	defer cleanup()
+	ctx := context.Background()
+	repo := postgres.NewAdminUserRepo(&postgres.DB{Pool: adminPool})
+
+	creator := makeAdmin("creator-"+randomID()[:8], "super_admin")
+	if err := repo.Create(ctx, creator); err != nil {
+		t.Fatalf("create creator: %v", err)
+	}
+	child := makeAdmin("child-"+randomID()[:8], "viewer")
+	child.CreatedBy = creator.ID
+	if err := repo.Create(ctx, child); err != nil {
+		t.Fatalf("create child: %v", err)
+	}
+
+	// A live session on the account being revoked: cascading it is the whole
+	// point of deleting the admin row first, and it cascades only if that
+	// delete succeeds.
+	var sessions int
+	if _, err := adminPool.Exec(ctx,
+		`INSERT INTO auth.admin_sessions (id, admin_id, token_hash, ip, created_at, expires_at)
+		 VALUES (gen_random_uuid(), $1, 'hash', '203.0.113.9', NOW(), NOW() + INTERVAL '1 hour')`,
+		creator.ID); err != nil {
+		t.Fatalf("seed session: %v", err)
+	}
+
+	if err := repo.Revoke(ctx, creator.ID); err != nil {
+		t.Fatalf("revoking an admin who created another failed: %v\n"+
+			"This is the only containment lever there is: an admin whose credentials "+
+			"are known to be compromised cannot be stopped by any other route.", err)
+	}
+
+	if got, _ := repo.GetByID(ctx, creator.ID); got != nil {
+		t.Fatal("the creator is still present after Revoke")
+	}
+
+	if err := adminPool.QueryRow(ctx,
+		`SELECT COUNT(*) FROM auth.admin_sessions WHERE admin_id = $1`, creator.ID).Scan(&sessions); err != nil {
+		t.Fatalf("count sessions: %v", err)
+	}
+	if sessions != 0 {
+		t.Fatalf("%d live sessions survived the revoke; the cascade only runs when the "+
+			"parent delete succeeds", sessions)
+	}
+
+	// SET NULL, not CASCADE. The child must survive: cascading would delete the
+	// revoked admin's entire created subtree, so revoking one compromised
+	// account would silently remove every account it had ever opened.
+	got, err := repo.GetByID(ctx, child.ID)
+	if err != nil {
+		t.Fatalf("read child: %v", err)
+	}
+	if got == nil {
+		t.Fatal("the child account was deleted with its creator. The constraint must be " +
+			"ON DELETE SET NULL; CASCADE here turns one revoke into a purge.")
+	}
+	if got.CreatedBy != "" {
+		t.Fatalf("child created_by = %q, want it cleared by SET NULL", got.CreatedBy)
+	}
+}