Переглянути джерело

Docs: mark R01-N03 fixed, refresh SPEC §3 / §8 / §9 / §11 / §13

- doc/REVIEW_01.md: flip R01-N03 (HIGH on first deploy, none after)
  to fixed-in-f565c86. Path taken was the explicit env-bootstrap branch
  (BOOTSTRAP_ADMIN_OID / BOOTSTRAP_ADMIN_EMAIL) — preferred over the
  BEGIN IMMEDIATE fix because it eliminates the entire wrong-admin
  outcome instead of just serialising the count check, and because it
  gives operators a deploy-time knob that local-admin already has.
  Strike out R01-N03 in the Suggested-ordering list; next open finding
  is R01-N07 + R01-N05 (proxy / HTTPS handling, paired).
- SPEC §3 (directory layout): add BootstrapAdmin to the Auth/ entry.
- SPEC §8: rewrite the env block + first-admin-bootstrap paragraph.
  Two new env vars documented; the legacy "first user to sign in is
  always admin" wording is gone. Spell out that with both bootstrap
  env vars blank, OIDC never promotes (the security hardening) and
  point at the local-admin fallback as the alternative bootstrap.
- SPEC §9: insert the new shipped entry above the "New sprint form"
  one with the full rationale + the bumped test count.
- SPEC §11: 176 / 484 → 184 / 502.
- SPEC §13: append f565c86 plus the previously-missing 2b8f167 so the
  history block stays current.

Per the §14 maintenance contract: code shipped first (f565c86), docs
ship as their own commit so a future revert can roll either side back
without dragging the other.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 2 днів тому
батько
коміт
f705d7b32e
2 змінених файлів з 83 додано та 7 видалено
  1. 56 5
      SPEC.md
  2. 27 2
      doc/REVIEW_01.md

+ 56 - 5
SPEC.md

@@ -87,7 +87,7 @@ per-cell audit trail.
 │               ├── htmx.min.js         # htmx.org
 │               └── sortable.min.js     # SortableJS
 ├── src/
-│   ├── Auth/            LocalAdmin, OidcClient, SessionGuard
+│   ├── Auth/            BootstrapAdmin, LocalAdmin, OidcClient, SessionGuard
 │   ├── Controllers/     AuthController, WorkerController, SprintController,
 │   │                    TaskController, AuditController, UserController,
 │   │                    SettingsController, ImportController (Phase 20)
@@ -280,6 +280,13 @@ DB_PATH=/var/www/data/app.sqlite
 SESSION_PATH=/var/www/data/sessions
 APP_ENV=production
 
+# Optional explicit OIDC bootstrap (R01-N03). When the `users` table has no
+# admin and the signing user matches one of these (case-insensitive,
+# timing-safe), they are promoted on sign-in. Either or both may be set;
+# leave both blank to disable OIDC auto-promotion entirely.
+BOOTSTRAP_ADMIN_OID=
+BOOTSTRAP_ADMIN_EMAIL=
+
 # Optional local admin fallback (disables when blank).
 # Password is verified with PHP's password_verify() against the bcrypt hash
 # stored in LOCAL_ADMIN_PASSWORD_HASH; the plaintext password never lands on
@@ -291,9 +298,21 @@ LOCAL_ADMIN_PASSWORD_HASH=
 LOCAL_ADMIN_NAME=Local Admin
 ```
 
-First-login bootstrap: when the `users` table is empty at the moment of
-successful login (either OIDC or local), that user is promoted to `is_admin=1`
-with a `BOOTSTRAP_ADMIN` audit row.
+First-admin bootstrap (R01-N03 hardening):
+
+- **OIDC path.** Auto-promotion requires `users.countAdmins() === 0` AND the
+  signing user's `oid`/`email` to match `BOOTSTRAP_ADMIN_OID` /
+  `BOOTSTRAP_ADMIN_EMAIL`. With both env vars blank, OIDC never promotes —
+  the operator must seed the first admin via the local-admin fallback or by
+  flipping `users.is_admin = 1` directly. The promotion emits a
+  `BOOTSTRAP_ADMIN` audit row tagged `via=oidc`.
+- **Local-admin path.** `LOCAL_ADMIN_EMAIL` + `LOCAL_ADMIN_PASSWORD_HASH` is
+  itself an explicit env-bootstrap, so the local user is always promoted on
+  sign-in (`forceAdmin=true`). When the DB was empty before this sign-in, a
+  `BOOTSTRAP_ADMIN` audit row tagged `via=local` is also recorded.
+
+The pre-R01-N03 behaviour (first user to sign in via any path becomes admin)
+is gone — see `src/Auth/BootstrapAdmin.php`.
 
 ## 9. Build phases — status
 
@@ -1183,6 +1202,36 @@ with a `BOOTSTRAP_ADMIN` audit row.
       time is injected so no test sleeps. Tests: 176 / 484 (was 163 /
       417). Sixth fix from `doc/REVIEW_01.md`.
 
+- [x] **R01-N03 — Explicit env-bootstrap for the first OIDC admin**
+      (`f565c86`). The OIDC sign-in path used to auto-promote the very
+      first user to land on `/auth/callback` (`users.count() === 0`).
+      On a public-facing first deploy that was a land-grab — any
+      tenant member with a valid Entra account could win the race
+      against the intended operator. Two new env vars,
+      `BOOTSTRAP_ADMIN_OID` and `BOOTSTRAP_ADMIN_EMAIL`, now name
+      the bootstrap principal up front; OIDC auto-promotion requires
+      `countAdmins() === 0` AND a match (case-insensitive, trimmed,
+      `hash_equals`). With both env vars blank, OIDC NEVER promotes
+      anyone — operators must seed the first admin via the local-
+      admin fallback (itself an explicit env-bootstrap) or by
+      flipping `is_admin` in `app.sqlite`. The local-admin path is
+      unchanged: `forceAdmin: true` continues to keep the configured
+      local user admin on every sign-in, and the `BOOTSTRAP_ADMIN`
+      audit row still fires on the first local sign-in into an empty
+      `users` table — its `after` payload now carries
+      `via=local`/`via=oidc` so `/audit` distinguishes the two
+      channels. New `src/Auth/BootstrapAdmin.php` owns
+      `isConfigured()` + `matches(oid, email)`; blank incoming fields
+      never opportunistically match an absent env value. New
+      `tests/Auth/BootstrapAdminTest.php` (8 cases) pins the matcher;
+      `UserRepositoryTest` comments lost their stale "count === 0"
+      reference but the repo's promoteToAdmin / forceAdmin contract
+      is mechanically unchanged. README + admin-manual rewritten:
+      admin-manual gains a new §3.5 "Nominating the first
+      administrator (OIDC)" and the old §3.5 (Local admin) shifts to
+      §3.6. Tests: 184 / 502 (was 176 / 484). Seventh fix from
+      `doc/REVIEW_01.md`.
+
 - [x] **New sprint form: drop weeks input + task list row hover**
       (`3728106`). The `/sprints/new` form no longer collects an
       `n_weeks` value — the week count is derived from `start_date` /
@@ -1252,7 +1301,7 @@ for f in $(git ls-files '*.php'); do php -l "$f" | tail -1 | sed "s|^|$f: |"; do
 Run the test suite:
 ```bash
 vendor/bin/phpunit
-# → OK (176 tests, 484 assertions)
+# → OK (184 tests, 502 assertions)
 ```
 
 The Phase 20 parser tests need `ext-dom`, `ext-zip`, `ext-xmlreader`,
@@ -1299,6 +1348,8 @@ before acting — nothing here is load-bearing once it grows stale.
 ## 13. Git history (as of this writing)
 
 ```
+f565c86 Fix R01-N03: explicit env-bootstrap for the first OIDC admin
+2b8f167 Docs: mark R01-N06 fixed, refresh SPEC §3 / §4 / §7 / §9 / §11 / §13
 e295432 Fix R01-N06: throttle local-admin login by (ip, email)
 851f8cf Docs: mark R01-N11 fixed, refresh SPEC §9 / §13
 4ae1817 Fix R01-N11: whitelist column in AuditRepository::distinctColumn

+ 27 - 2
doc/REVIEW_01.md

@@ -98,7 +98,32 @@ note. Do not delete entries — they're history.
 
 ### R01-N03 — First-user-is-admin bootstrap is a land-grab
 - **Severity**: HIGH on first deploy, none after.
-- **Status**: open — documented in SPEC §8 as intended.
+- **Status**: fixed-in-`f565c86` — went with the explicit env-bootstrap
+  branch. New `src/Auth/BootstrapAdmin.php` exposes
+  `isConfigured()` + `matches(oid, email)`, reading the new
+  `BOOTSTRAP_ADMIN_OID` / `BOOTSTRAP_ADMIN_EMAIL` env vars
+  (case-insensitive, trimmed, `hash_equals`). `AuthController::callback`
+  now gates promotion on `users.countAdmins() === 0 &&
+  BootstrapAdmin::matches($oid, $email)` — with both env vars blank the
+  OIDC path NEVER auto-promotes (operators must seed via the local-admin
+  fallback or by flipping `is_admin` in `app.sqlite`). Switching from
+  `count() === 0` to `countAdmins() === 0` also closes the SQLite
+  `DEFERRED`-tx race the original finding called out: even if two
+  flows concurrently see the same count, the BootstrapAdmin match
+  eliminates the "wrong user wins" outcome — only the configured
+  principal is ever promoted. The local-admin path is unchanged: it
+  is itself an explicit env-bootstrap (`LOCAL_ADMIN_EMAIL` +
+  `LOCAL_ADMIN_PASSWORD_HASH`) and `forceAdmin: true` continues to
+  keep the configured local user admin. Both paths' BOOTSTRAP_ADMIN
+  audit rows now carry `via=oidc` / `via=local` so `/audit` can
+  distinguish them. Operator docs: new `doc/admin-manual.md` §3.5
+  "Nominating the first administrator (OIDC)" (old §3.5 → §3.6),
+  README's quick-setup paragraph rewritten, `.env.example` gains
+  the new env block. New `tests/Auth/BootstrapAdminTest.php` (8
+  cases) pins the matcher: unconfigured, oid-only, email-only,
+  both-set, case-insensitive trim, blank-incoming-fields-don't-match,
+  whitespace-only-env-treated-as-unset, either-channel-wins.
+  Tests: 184 / 502 (was 176 / 484).
 - **Where**:
   - `src/Controllers/AuthController.php` lines 86-116 (OIDC) and 217-254
     (local).
@@ -683,7 +708,7 @@ A reasonable cadence (do not treat as binding):
 5. ~~**R01-N01** (local-admin password hashing)~~ — fixed in `857df15`
    (hash-only, no plaintext fallback per operator decision).
 6. ~~**R01-N06** (login throttling)~~ — fixed in `e295432`.
-7. **R01-N03** (first-user bootstrap hardening).
+7. ~~**R01-N03** (first-user bootstrap hardening)~~ — fixed in `f565c86`.
 8. **R01-N07** + **R01-N05** (proxy / HTTPS handling) — paired.
 9. **R01-N08** (idle session timeout + CSRF rotation).
 10. **R01-N10** (bind-not-concat sweep) — mechanical.