Browse Source

Fix R01-N03: explicit env-bootstrap for the first OIDC admin

Replaces the historical "first user wins" auto-promotion in the OIDC
sign-in path. Previously, on a fresh deploy that was reachable on the
public internet before the intended operator could sign in, any tenant
member with a valid Entra account could land on /auth/callback first
and be promoted to admin via `users.count() === 0` — a land-grab the
legitimate operator had no path back from without DB access.

The OIDC path now auto-promotes only when:
- `users.countAdmins() === 0`, AND
- the signing principal matches BOOTSTRAP_ADMIN_OID or
  BOOTSTRAP_ADMIN_EMAIL (case-insensitive, trimmed, hash_equals).

Either / both / neither env var may be set:
- both: signing user must match at least one to be promoted.
- one:  only that channel matters.
- neither: the OIDC path NEVER auto-promotes. Operators are expected
  to seed the first admin via the local-admin fallback (which is
  itself an explicit env-bootstrap — LOCAL_ADMIN_EMAIL +
  LOCAL_ADMIN_PASSWORD_HASH) or by setting users.is_admin = 1
  directly in app.sqlite.

The local-admin sign-in path is unchanged. forceAdmin: true keeps
the configured local user admin on every login, and the BOOTSTRAP_ADMIN
audit row still fires on the first sign-in into an empty users table.
The OIDC promotion's audit row is now tagged after: {is_admin, via:
"oidc"} so /audit can distinguish the two channels.

Files:
- src/Auth/BootstrapAdmin.php (new): isConfigured() + matches(oid, email)
  helpers. Comparisons strtolower-trim-hash_equals; blank incoming
  fields never opportunistically match an absent env value.
- src/Controllers/AuthController.php: callback() drops $isFirstUser
  in favour of $shouldBootstrap; the BOOTSTRAP_ADMIN audit row is
  gated on $shouldBootstrap (not on count===0). The local path is
  byte-identical.
- .env.example: new BOOTSTRAP_ADMIN_OID / BOOTSTRAP_ADMIN_EMAIL block
  with the rationale and a note that the local-admin fallback is its
  own bootstrap and does not require these.
- README.md: replace the "first user to sign in is automatically
  promoted to administrator" line with the post-hardening behaviour
  and a pointer to REVIEW_01 R01-N03.
- doc/admin-manual.md: new §3.5 "Nominating the first administrator
  (OIDC)"; old §3.5 (Local admin) becomes §3.6; the §5.4 reset note
  drops the now-stale "no users, first sign-in becomes admin" wording;
  one §3.5 cross-reference in the troubleshooting table → §3.6.

Tests: 184 / 502 (was 176 / 484). New tests/Auth/BootstrapAdminTest.php
(8 cases) covers the unconfigured default, oid-only / email-only /
both-set matches, case-insensitive trim, blank-incoming-fields-don't-
match, whitespace-only env treated as unset, and either-channel-wins.
The unrelated UserRepositoryTest comments lost a reference to the old
"count === 0" rule — repo's mechanical promoteToAdmin / forceAdmin
behaviour is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 2 ngày trước cách đây
mục cha
commit
f565c86d1d

+ 17 - 0
.env.example

@@ -18,6 +18,21 @@ SESSION_PATH=/var/www/data/sessions
 # 'production' disables verbose error output. Anything else is treated as dev.
 APP_ENV=production
 
+# ---------------------------------------------------------------------------
+# OIDC bootstrap admin (optional) — nominate the very first administrator up
+# front, so a public-facing first deploy can't be land-grabbed by another
+# tenant member who happens to sign in before you. Auto-promotion to admin
+# happens via OIDC iff no admin exists yet AND the signing user matches one
+# of the values below (case-insensitive). With both variables blank, the
+# OIDC path NEVER auto-promotes — seed the first admin via the local-admin
+# fallback below, or by manually flipping is_admin in the database.
+# Set BOOTSTRAP_ADMIN_OID to the Entra `oid` claim (a GUID, immutable) when
+# you know it; BOOTSTRAP_ADMIN_EMAIL is accepted as a fallback when you only
+# have the email.
+# ---------------------------------------------------------------------------
+BOOTSTRAP_ADMIN_OID=
+BOOTSTRAP_ADMIN_EMAIL=
+
 # ---------------------------------------------------------------------------
 # Local admin (optional) — lets you sign in without Entra, e.g. during initial
 # setup or for a fully on-prem deployment. Set BOTH email and the password
@@ -30,6 +45,8 @@ APP_ENV=production
 # resulting `$2y$...` string verbatim. Single quotes recommended in .env so
 # the `$` in the hash isn't interpreted by the shell.
 # The resulting user is stored with entra_oid = "local:<email>" and is_admin=1.
+# This path is itself an explicit env-bootstrap and does not require the
+# BOOTSTRAP_ADMIN_* variables above.
 # ---------------------------------------------------------------------------
 LOCAL_ADMIN_EMAIL=
 LOCAL_ADMIN_PASSWORD_HASH=

+ 7 - 3
README.md

@@ -58,9 +58,13 @@ docker compose up -d --build
 xdg-open http://localhost:8088   # or just visit it in a browser
 ```
 
-The first user to sign in is automatically promoted to administrator
-(audit-logged as `BOOTSTRAP_ADMIN`). Subsequent admin promotions happen
-through the **Users** page in the hamburger menu.
+If you signed in via the local-admin fallback you are immediately the
+administrator (audit-logged as `BOOTSTRAP_ADMIN` with `via=local`). For
+an OIDC-only deploy, nominate the first admin up front via
+`BOOTSTRAP_ADMIN_OID` or `BOOTSTRAP_ADMIN_EMAIL` in `.env` — without one
+of them set the OIDC path will not auto-promote anyone (this closes
+finding R01-N03 in `doc/REVIEW_01.md`). Subsequent admin promotions
+happen through the **Users** page in the hamburger menu.
 
 The SQLite database is created on first request at `./data/app.sqlite` on
 the host (mounted into the container at `/var/www/data/app.sqlite`).

+ 53 - 9
doc/admin-manual.md

@@ -107,7 +107,45 @@ APP_ENV=production
 `production` silences verbose PHP errors. Any other value (e.g. `dev`)
 turns them on — useful when troubleshooting in a non-public install.
 
-### 3.5 Local admin fallback (optional)
+### 3.5 Nominating the first administrator (OIDC)
+
+Historically the first user to complete sign-in via *any* path was promoted
+to administrator. On a public-facing first deploy that is a land-grab risk
+— anyone with a valid Entra account in your tenant could win the race
+against the intended operator and lock everyone else out
+(see `doc/REVIEW_01.md` finding R01-N03).
+
+The OIDC sign-in path now auto-promotes only when the signing user matches
+an explicit env-bootstrap value:
+
+```
+BOOTSTRAP_ADMIN_OID=00000000-0000-0000-0000-000000000000
+BOOTSTRAP_ADMIN_EMAIL=admin@example.com
+```
+
+| Variable                | What to put there |
+|-------------------------|-------------------|
+| `BOOTSTRAP_ADMIN_OID`   | The Entra `oid` claim (a GUID, immutable for the lifetime of the user) — preferred when known. Visible on the user's "Object ID" line in the Entra portal. |
+| `BOOTSTRAP_ADMIN_EMAIL` | The user's primary email. Accepted as a fallback when only the email is on hand. Matched case-insensitively, after trimming. |
+
+Either / both / neither may be set:
+
+- **Both set**: the signing user is promoted on a match against either field.
+- **One set**: only that channel matters.
+- **Neither set**: the OIDC path will *never* auto-promote. In that case
+  bootstrap the first administrator via the local-admin fallback (§3.6) or
+  by setting `users.is_admin = 1` directly in `app.sqlite`.
+
+Auto-promotion additionally requires that no administrator already exists
+(`countAdmins() === 0`). Once a first admin is in place, subsequent
+promotions go through the **Users** page (§5.1). The promotion is recorded
+in the audit log as `BOOTSTRAP_ADMIN` with `via=oidc`.
+
+The local-admin fallback (§3.6) is itself an explicit env-bootstrap and
+does not require the variables above — its `BOOTSTRAP_ADMIN` audit row is
+tagged `via=local`.
+
+### 3.6 Local admin fallback (optional)
 
 ```
 LOCAL_ADMIN_EMAIL=admin@example.com
@@ -153,11 +191,15 @@ Then:
   until they expire (the hash is only consulted at sign-in time).
 
 The local-admin user is recorded in the database as
-`entra_oid = "local:<email>"` with `is_admin = 1`. If the `users` table
-is empty at the moment of the first successful login (OIDC or local),
-that user is auto-promoted to admin and an audit entry of type
-`BOOTSTRAP_ADMIN` is recorded. This is how the very first administrator
-is created — there is no separate "create admin" step.
+`entra_oid = "local:<email>"` with `is_admin = 1`, and is always promoted
+to admin on every successful local sign-in (so a manual demotion in the
+Users page is undone on the next local sign-in). When the `users` table
+was empty before the sign-in, an audit entry of type `BOOTSTRAP_ADMIN`
+with `via=local` is also recorded.
+
+For OIDC-only deployments — where you don't want the local fallback at
+all — leave `LOCAL_ADMIN_EMAIL` / `LOCAL_ADMIN_PASSWORD_HASH` blank and
+nominate the first admin via the OIDC bootstrap (§3.5).
 
 To disable the fallback later, blank both variables and restart.
 
@@ -311,8 +353,10 @@ rm -rf ./data
 docker compose up -d
 ```
 
-Migrations run again on the next request and you are back to "no users,
-first sign-in becomes admin".
+Migrations run again on the next request. The first administrator is
+re-seeded as described in §3.5 (OIDC bootstrap) or §3.6 (local-admin
+fallback) — there is no longer an unconditional "first sign-in becomes
+admin" path.
 
 ### 5.5 Updating to a new release
 
@@ -333,7 +377,7 @@ before pulling.
 | Symptom | Likely cause | Fix |
 |---|---|---|
 | `/auth/login` returns "redirect URI mismatch" | The redirect URI registered in Entra does not equal `{APP_BASE_URL}/auth/callback`. | Update either `APP_BASE_URL` in `.env` or the Entra app registration. |
-| `/auth/local` returns 404 | `LOCAL_ADMIN_EMAIL` or `LOCAL_ADMIN_PASSWORD_HASH` is blank. | Set both (see §3.5 for the hash recipe), then `docker compose restart`. |
+| `/auth/local` returns 404 | `LOCAL_ADMIN_EMAIL` or `LOCAL_ADMIN_PASSWORD_HASH` is blank. | Set both (see §3.6 for the hash recipe), then `docker compose restart`. |
 | Sign-in succeeds but every page shows "not authorised" | The user has no `is_admin` flag and is trying to access an admin-only page. | Promote them via Users (logged in as another admin). |
 | Container restarts in a loop | Most often a malformed `.env` line or a permission problem on `./data`. | `docker compose logs` will show the PHP fatal. Check that `./data` is writable by the `www-data` user inside the container (uid 33 on the Apache image). |
 | CSS looks broken / unstyled | The Node build stage was skipped or used a stale layer. | `docker compose build --no-cache` and restart. |

+ 77 - 0
src/Auth/BootstrapAdmin.php

@@ -0,0 +1,77 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Auth;
+
+/**
+ * Explicit env-bootstrap for the very first administrator (R01-N03).
+ *
+ * Historically the first user to complete sign-in was auto-promoted to admin
+ * ("first user wins"). On a public-facing first deploy, any tenant member
+ * with a valid Entra account could win the race against the intended
+ * operator and lock everyone else out.
+ *
+ * Now the operator nominates the bootstrap admin up front via
+ *     BOOTSTRAP_ADMIN_OID    Entra `oid` (or `sub`) — preferred, immutable.
+ *     BOOTSTRAP_ADMIN_EMAIL  email claim — accepted as a fallback when the
+ *                            operator does not yet know the oid.
+ * Either / both / neither may be set:
+ *   - both set: a signing user must match at least one to be promoted.
+ *   - one set:  only that channel matters.
+ *   - neither set: OIDC sign-in NEVER auto-promotes. The local-admin
+ *                  fallback (its own explicit env-bootstrap) remains the
+ *                  recommended way to seed the first admin.
+ *
+ * The local-admin sign-in path is unaffected: `LOCAL_ADMIN_EMAIL` plus
+ * `LOCAL_ADMIN_PASSWORD_HASH` is itself the explicit env-bootstrap that
+ * grant fixes — see `LocalAdmin` and `AuthController::loginLocal`.
+ */
+final class BootstrapAdmin
+{
+    public static function isConfigured(): bool
+    {
+        return self::oid() !== '' || self::email() !== '';
+    }
+
+    public static function oid(): string
+    {
+        $v = getenv('BOOTSTRAP_ADMIN_OID');
+        return is_string($v) ? trim($v) : '';
+    }
+
+    public static function email(): string
+    {
+        $v = getenv('BOOTSTRAP_ADMIN_EMAIL');
+        return is_string($v) ? trim($v) : '';
+    }
+
+    /**
+     * Does the signing user match the configured bootstrap principal?
+     * Returns false when neither env var is set, when both incoming
+     * fields are blank, or when no configured value matches. Comparisons
+     * are case-insensitive (Entra emails are mixed-case in
+     * `preferred_username`; GUIDs are commonly lower-cased but not
+     * guaranteed) and use `hash_equals` for timing safety.
+     */
+    public static function matches(string $oid, string $email): bool
+    {
+        if (!self::isConfigured()) {
+            return false;
+        }
+
+        $oidIn   = strtolower(trim($oid));
+        $emailIn = strtolower(trim($email));
+
+        $oidExpected   = strtolower(self::oid());
+        $emailExpected = strtolower(self::email());
+
+        if ($oidExpected !== '' && $oidIn !== '' && hash_equals($oidExpected, $oidIn)) {
+            return true;
+        }
+        if ($emailExpected !== '' && $emailIn !== '' && hash_equals($emailExpected, $emailIn)) {
+            return true;
+        }
+        return false;
+    }
+}

+ 14 - 6
src/Controllers/AuthController.php

@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace App\Controllers;
 
+use App\Auth\BootstrapAdmin;
 use App\Auth\LocalAdmin;
 use App\Auth\OidcClient;
 use App\Auth\SessionGuard;
@@ -85,12 +86,19 @@ final class AuthController
             return Response::redirect('/?auth_error=1');
         }
 
+        // R01-N03: explicit env-bootstrap. The OIDC path no longer auto-
+        // promotes the first user to land — an attacker with a valid
+        // tenant account could otherwise win the race to /auth/callback.
+        // The signing principal must match BOOTSTRAP_ADMIN_OID or
+        // BOOTSTRAP_ADMIN_EMAIL, AND no admin must yet exist.
+        $shouldBootstrap = $this->users->countAdmins() === 0
+            && BootstrapAdmin::matches($oid, $email);
+
         $this->pdo->beginTransaction();
         try {
-            $isFirstUser = $this->users->count() === 0;
-            $result      = $this->users->upsertFromOidc($oid, $email, $name, $isFirstUser);
-            $user        = $result['user'];
-            $before      = $result['before']?->toAuditSnapshot();
+            $result = $this->users->upsertFromOidc($oid, $email, $name, $shouldBootstrap);
+            $user   = $result['user'];
+            $before = $result['before']?->toAuditSnapshot();
 
             $action = $before === null ? 'CREATE' : 'UPDATE';
             $this->audit->record(
@@ -105,13 +113,13 @@ final class AuthController
                 userAgent:  $req->userAgent(),
             );
 
-            if ($isFirstUser) {
+            if ($shouldBootstrap) {
                 $this->audit->record(
                     action:     'BOOTSTRAP_ADMIN',
                     entityType: 'user',
                     entityId:   $user->id,
                     before:     null,
-                    after:      ['is_admin' => 1],
+                    after:      ['is_admin' => 1, 'via' => 'oidc'],
                     userId:     $user->id,
                     userEmail:  $user->email,
                     ipAddress:  $req->ip(),

+ 132 - 0
tests/Auth/BootstrapAdminTest.php

@@ -0,0 +1,132 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Auth;
+
+use App\Auth\BootstrapAdmin;
+use App\Tests\TestCase;
+
+/**
+ * Lock-in tests for R01-N03 — the OIDC bootstrap matcher. The matcher
+ * is the only thing standing between an unconfigured deploy and the
+ * historical "first user wins" land-grab.
+ */
+final class BootstrapAdminTest extends TestCase
+{
+    /** @var array<string, string|false> */
+    private array $envBackup = [];
+
+    /** @var string[] */
+    private array $envKeys = [
+        'BOOTSTRAP_ADMIN_OID',
+        'BOOTSTRAP_ADMIN_EMAIL',
+    ];
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        foreach ($this->envKeys as $k) {
+            $this->envBackup[$k] = getenv($k);
+            putenv($k);
+        }
+    }
+
+    protected function tearDown(): void
+    {
+        foreach ($this->envKeys as $k) {
+            $prev = $this->envBackup[$k] ?? false;
+            if ($prev === false) {
+                putenv($k);
+            } else {
+                putenv("{$k}={$prev}");
+            }
+        }
+        parent::tearDown();
+    }
+
+    public function testNotConfiguredByDefault(): void
+    {
+        self::assertFalse(BootstrapAdmin::isConfigured());
+        self::assertFalse(
+            BootstrapAdmin::matches('any-oid', 'any@example.com'),
+            'unconfigured matcher must never promote anyone'
+        );
+    }
+
+    public function testMatchesByOid(): void
+    {
+        putenv('BOOTSTRAP_ADMIN_OID=00000000-0000-0000-0000-000000000001');
+        self::assertTrue(BootstrapAdmin::isConfigured());
+        self::assertTrue(BootstrapAdmin::matches(
+            '00000000-0000-0000-0000-000000000001',
+            'unrelated@example.com'
+        ));
+        self::assertFalse(BootstrapAdmin::matches(
+            '00000000-0000-0000-0000-000000000002',
+            'unrelated@example.com'
+        ));
+    }
+
+    public function testMatchesByEmail(): void
+    {
+        putenv('BOOTSTRAP_ADMIN_EMAIL=admin@example.com');
+        self::assertTrue(BootstrapAdmin::isConfigured());
+        self::assertTrue(BootstrapAdmin::matches(
+            'oid-does-not-matter',
+            'admin@example.com'
+        ));
+        self::assertFalse(BootstrapAdmin::matches(
+            'oid-does-not-matter',
+            'attacker@example.com'
+        ));
+    }
+
+    public function testEitherChannelMatchesWhenBothConfigured(): void
+    {
+        putenv('BOOTSTRAP_ADMIN_OID=oid-1');
+        putenv('BOOTSTRAP_ADMIN_EMAIL=admin@example.com');
+
+        self::assertTrue(BootstrapAdmin::matches('oid-1',  'someone-else@x'));
+        self::assertTrue(BootstrapAdmin::matches('oid-99', 'admin@example.com'));
+        self::assertFalse(BootstrapAdmin::matches('oid-99', 'someone-else@x'));
+    }
+
+    public function testMatchIsCaseInsensitiveAndTrimmed(): void
+    {
+        putenv('BOOTSTRAP_ADMIN_EMAIL=Admin@Example.COM');
+        self::assertTrue(BootstrapAdmin::matches('', '  admin@example.com  '));
+
+        putenv('BOOTSTRAP_ADMIN_EMAIL');
+        putenv('BOOTSTRAP_ADMIN_OID=ABCDEF-1234');
+        self::assertTrue(BootstrapAdmin::matches('  abcdef-1234  ', ''));
+    }
+
+    public function testBlankIncomingFieldsNeverMatch(): void
+    {
+        // Blank email in env shouldn't match a blank claim — config is
+        // OID-only here, so an empty email coming back from Entra must
+        // not opportunistically match the absent BOOTSTRAP_ADMIN_EMAIL.
+        putenv('BOOTSTRAP_ADMIN_OID=oid-1');
+        self::assertFalse(BootstrapAdmin::matches('', ''));
+        self::assertFalse(BootstrapAdmin::matches('', 'bob@x'));
+    }
+
+    public function testWhitespaceOnlyEnvTreatedAsUnset(): void
+    {
+        putenv('BOOTSTRAP_ADMIN_OID=   ');
+        putenv('BOOTSTRAP_ADMIN_EMAIL=   ');
+        self::assertFalse(BootstrapAdmin::isConfigured());
+        self::assertFalse(BootstrapAdmin::matches('anything', 'anything@x'));
+    }
+
+    public function testOidValueOverEmailIsPreferred(): void
+    {
+        // Both are set; if the user's oid matches we promote regardless of
+        // a (presumably stale) BOOTSTRAP_ADMIN_EMAIL. Sanity check that
+        // either-channel-wins still holds.
+        putenv('BOOTSTRAP_ADMIN_OID=primary-oid');
+        putenv('BOOTSTRAP_ADMIN_EMAIL=admin@example.com');
+        self::assertTrue(BootstrapAdmin::matches('primary-oid', 'noone@x'));
+    }
+}

+ 7 - 4
tests/Repositories/UserRepositoryTest.php

@@ -8,7 +8,10 @@ use App\Repositories\UserRepository;
 use App\Tests\TestCase;
 
 /**
- * Covers the first-user-is-admin bootstrap rule from spec §4.
+ * Covers the upsert + admin-promotion contract used by the auth flows.
+ * Caller-side gating (R01-N03's BOOTSTRAP_ADMIN_* env-bootstrap) lives in
+ * `AuthController` + `BootstrapAdmin`; this suite only pins the repo's
+ * mechanical promoteToAdmin / forceAdmin behaviour.
  */
 final class UserRepositoryTest extends TestCase
 {
@@ -23,7 +26,7 @@ final class UserRepositoryTest extends TestCase
             oid:            'oid-alice',
             email:          'alice@example.com',
             name:           'Alice',
-            promoteToAdmin: true, // caller's decision — normally based on count === 0
+            promoteToAdmin: true, // caller's decision — see BootstrapAdmin::matches
         );
 
         $this->assertTrue($r['user']->isAdmin);
@@ -36,8 +39,8 @@ final class UserRepositoryTest extends TestCase
         $users = new UserRepository($pdo);
 
         $users->upsertFromOidc('oid-alice', 'alice@x', 'Alice', true);
-        $secondPromote = $users->count() === 0;  // false
-        $r2 = $users->upsertFromOidc('oid-bob', 'bob@x', 'Bob', $secondPromote);
+        // Caller passes promoteToAdmin=false because an admin already exists.
+        $r2 = $users->upsertFromOidc('oid-bob', 'bob@x', 'Bob', false);
 
         $this->assertFalse($r2['user']->isAdmin);
     }