fix: enforce single local-admin row in upsertLocal (SEC_REVIEW F3)

A leaked or shared UI_SERVICE_TOKEN previously meant single-step total
compromise: POST /api/v1/auth/users/upsert-local with any new username
unconditionally minted a fresh is_local=1, role=admin user_id, which
the caller could then drive against every admin endpoint via
X-Acting-User-Id.

Per SPEC §6/§8 the local admin is a single record whose password lives
in the UI's env. Encode that invariant in two places: rewrite
UserRepository::upsertLocal to look up the existing local row by
is_local=1 (rather than display_name), update display_name and
last_login_at on it, and only insert when zero local rows exist; add
a partial unique index uniq_users_one_local enforcing the same shape
at the DB layer (SQLite partial index, MySQL functional index over a
CASE expression).

Regression test in AuthEndpointsTest::testRotatingUsernamesNeverCreates
AdditionalLocalAdmins rotates three usernames over upsert-local and
asserts the same user_id is returned each time and only one is_local=1
row exists. testDbLayerRejectsSecondLocalAdminInsert confirms the
defense-in-depth index rejects a direct duplicate insert.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

api/db/migrations/20260504100000_add_unique_local_user_index.php

@@ -0,0 +1,57 @@
+<?php
+
+declare(strict_types=1);
+
+use App\Infrastructure\Db\Migrations\BaseMigration;
+
+/**
+ * Defense-in-depth for SEC_REVIEW F3: enforce at the DB layer that there
+ * is at most one row in `users` with `is_local = 1`. SPEC §6/§8 says the
+ * local admin is a single record whose password lives in the UI's env;
+ * `UserRepository::upsertLocal` already enforces this in code, but the
+ * DB-level index makes regressions fail loud.
+ *
+ * Implementation:
+ *  - SQLite: native partial unique index `WHERE is_local = 1`.
+ *  - MySQL 8: functional unique index over a CASE expression that yields
+ *    `1` for local rows and `NULL` for non-local rows. Multi-NULL is
+ *    permitted in unique indexes on both engines.
+ *
+ * If a deployment already has multiple `is_local = 1` rows (either from
+ * the F3 exploit window or from operators legitimately renaming the
+ * local admin via the pre-fix code path), this migration will fail. That
+ * is the desired signal: the operator must inspect those rows manually
+ * and consolidate before applying the index.
+ */
+final class AddUniqueLocalUserIndex extends BaseMigration
+{
+    private const INDEX_NAME = 'uniq_users_one_local';
+
+    public function up(): void
+    {
+        if ($this->isMysql()) {
+            $this->execute(
+                'CREATE UNIQUE INDEX ' . self::INDEX_NAME . ' '
+                . 'ON users ((CASE WHEN is_local = 1 THEN 1 ELSE NULL END))'
+            );
+
+            return;
+        }
+
+        $this->execute(
+            'CREATE UNIQUE INDEX ' . self::INDEX_NAME . ' '
+            . 'ON users (is_local) WHERE is_local = 1'
+        );
+    }
+
+    public function down(): void
+    {
+        if ($this->isMysql()) {
+            $this->execute('DROP INDEX ' . self::INDEX_NAME . ' ON users');
+
+            return;
+        }
+
+        $this->execute('DROP INDEX IF EXISTS ' . self::INDEX_NAME);
+    }
+}

api/src/Infrastructure/Auth/UserRepository.php

@@ -47,13 +47,25 @@ final class UserRepository
         return $row === false ? null : $this->hydrate($row);
     }
 
-    public function findLocalByUsername(string $username): ?User
+    /**
+     * Finds the single local-admin row, if any. SPEC §6/§8 dictates that
+     * the system has at most one local admin (its password lives in the
+     * UI's env, the API only mirrors the identity record). The DB has a
+     * partial unique index enforcing the same invariant.
+     *
+     * Identifying by `is_local=1` rather than by `display_name` matches
+     * the structural invariant — `display_name` is mutable (operators can
+     * rename the local admin via `LOCAL_ADMIN_USERNAME`) and matching on
+     * it would otherwise let a service-token holder mint additional Admin
+     * users by varying the `username` payload (SEC_REVIEW F3).
+     */
+    public function findLocal(): ?User
     {
         /** @var array<string, mixed>|false $row */
         $row = $this->connection->fetchAssociative(
             'SELECT id, subject, email, display_name, role, is_local FROM users '
-            . 'WHERE is_local = :local AND display_name = :name LIMIT 1',
-            ['local' => 1, 'name' => $username]
+            . 'WHERE is_local = :local ORDER BY id ASC LIMIT 1',
+            ['local' => 1]
         );
 
         return $row === false ? null : $this->hydrate($row);
@@ -118,13 +130,22 @@ final class UserRepository
 
     public function upsertLocal(string $username): User
     {
-        $existing = $this->findLocalByUsername($username);
+        $existing = $this->findLocal();
         $now = (new DateTimeImmutable('now', new DateTimeZone('UTC')))->format('Y-m-d H:i:s');
 
         if ($existing !== null) {
+            // Bind the (single) existing local row to this login. If the
+            // operator has rotated `LOCAL_ADMIN_USERNAME` in UI env, the
+            // display_name follows. Role is always Admin per SPEC §8 — we
+            // re-assert it here so a row tampered with directly in the DB
+            // is healed on next login.
             $this->connection->update(
                 'users',
-                ['last_login_at' => $now],
+                [
+                    'display_name' => $username,
+                    'role' => Role::Admin->value,
+                    'last_login_at' => $now,
+                ],
                 ['id' => $existing->id]
             );
 
@@ -132,7 +153,7 @@ final class UserRepository
                 id: $existing->id,
                 subject: null,
                 email: null,
-                displayName: $existing->displayName ?? $username,
+                displayName: $username,
                 role: Role::Admin,
                 isLocal: true,
             );

api/tests/Integration/Auth/AuthEndpointsTest.php

@@ -17,17 +17,16 @@ final class AuthEndpointsTest extends AppTestCase
     public function testUpsertLocalCreatesUserOnFirstCall(): void
     {
         $token = $this->createToken(TokenKind::Service);
-        $adminId = $this->createUser(Role::Admin, isLocal: true);
 
+        // No local row exists yet; the first upsert mints it.
         $response = $this->request(
             'POST',
             '/api/v1/auth/users/upsert-local',
             [
                 'Authorization' => 'Bearer ' . $token,
-                'X-Acting-User-Id' => (string) $adminId,
                 'Content-Type' => 'application/json',
             ],
-            json_encode(['username' => 'second']) ?: null
+            json_encode(['username' => 'admin']) ?: null
         );
         self::assertSame(200, $response->getStatusCode());
 
@@ -35,8 +34,76 @@ final class AuthEndpointsTest extends AppTestCase
         self::assertIsInt($body['user_id']);
         self::assertSame('admin', $body['role']);
         self::assertNull($body['email']);
-        self::assertSame('second', $body['display_name']);
+        self::assertSame('admin', $body['display_name']);
         self::assertTrue($body['is_local']);
+
+        self::assertSame(
+            1,
+            (int) $this->db->fetchOne('SELECT COUNT(*) FROM users WHERE is_local = 1'),
+            'first call must create exactly one local row'
+        );
+    }
+
+    /**
+     * SEC_REVIEW F3 regression. A service-token holder must not be able
+     * to mint additional Admin user rows by rotating the `username` field
+     * over `POST /api/v1/auth/users/upsert-local`. Before the fix every
+     * fresh username produced a new is_local=1, role=admin row that the
+     * caller could then impersonate via X-Acting-User-Id.
+     */
+    public function testRotatingUsernamesNeverCreatesAdditionalLocalAdmins(): void
+    {
+        $token = $this->createToken(TokenKind::Service);
+        $adminId = $this->createUser(Role::Admin, isLocal: true);
+
+        $headers = [
+            'Authorization' => 'Bearer ' . $token,
+            'Content-Type' => 'application/json',
+        ];
+
+        $userIds = [];
+        foreach (['attacker-1', 'attacker-2', 'attacker-3'] as $name) {
+            $body = $this->decode($this->request(
+                'POST',
+                '/api/v1/auth/users/upsert-local',
+                $headers,
+                json_encode(['username' => $name]) ?: null
+            ));
+            $userIds[] = $body['user_id'];
+            self::assertSame($adminId, $body['user_id']);
+            self::assertSame('admin', $body['role']);
+            self::assertSame($name, $body['display_name']);
+        }
+
+        // Same user_id returned every call.
+        self::assertSame([$adminId, $adminId, $adminId], $userIds);
+
+        // And only one is_local=1 row in the DB.
+        self::assertSame(
+            1,
+            (int) $this->db->fetchOne('SELECT COUNT(*) FROM users WHERE is_local = 1'),
+            'rotating usernames must not mint additional local-admin rows'
+        );
+    }
+
+    /**
+     * Defense-in-depth: even if application code regresses, the partial
+     * unique index added in 20260504100000_add_unique_local_user_index
+     * must reject a second is_local=1 insert.
+     */
+    public function testDbLayerRejectsSecondLocalAdminInsert(): void
+    {
+        $this->createUser(Role::Admin, isLocal: true);
+
+        $this->expectException(\Doctrine\DBAL\Exception\UniqueConstraintViolationException::class);
+
+        $this->db->insert('users', [
+            'subject' => null,
+            'email' => null,
+            'display_name' => 'second-local',
+            'role' => Role::Admin->value,
+            'is_local' => 1,
+        ]);
     }
 
     public function testUpsertLocalIsIdempotent(): void