瀏覽代碼

docs: mark SEC_REVIEW F16 as fixed in 947ab89

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 4 天之前
父節點
當前提交
b8fc612aa6
共有 1 個文件被更改,包括 56 次插入0 次删除
  1. 56 0
      doc/SEC_REVIEW.md

+ 56 - 0
doc/SEC_REVIEW.md

@@ -475,6 +475,62 @@
   to grant Admin until manually revoked. There is no UI/API to list
   tokens by issuer.
 - **Severity: 2**
+- **Status:** Fixed in `947ab89`. Three layers close the gap:
+
+  1. **Schema.** Migration
+     `20260505110000_add_user_id_to_api_tokens` adds nullable
+     `api_tokens.user_id`. On MySQL it carries an FK to `users(id)`
+     with `ON DELETE CASCADE` — a hard-deleted user takes the tokens
+     they issued with them. SET NULL was rejected because reverting
+     to a NULL `user_id` would let the token re-enter the
+     grandfathered legacy path. SQLite cannot add an FK via ALTER
+     TABLE, so deletion-time enforcement on that driver falls back
+     to the application layer (issuer-row lookup returns null →
+     401). The system has no API-level user-deletion path; both
+     drivers behave identically for the actual offboarding flows
+     (disable + role demote).
+  2. **Binding.** `TokensController::create` writes the acting
+     user's id into the new column for `kind=admin` tokens only.
+     Reporter / consumer / service tokens stay user-less — they are
+     device credentials, not delegated user privilege.
+     `TokenRecord` carries the new `userId` field; the create
+     response and list response surface `user_id`, the list also
+     denormalises a `user_label` (display name, email, or `user#N`).
+     Admin tokens minted via `bin/console tokens:create` carry NULL
+     and are grandfathered — operators rotate those after deploy if
+     they want strict binding.
+  3. **Enforcement.** `TokenAuthenticationMiddleware` injects
+     `UserRepository`; for any admin-kind token with non-null
+     `user_id` it loads the issuer and refuses the token (401, same
+     shape as every other auth failure) if the issuer row is
+     missing, has `disabled_at` set, or has a current role that
+     doesn't satisfy the token's bound role
+     (`role.satisfies(token.role)`). NULL `user_id` skips the
+     check, preserving the grandfathered path. ImpersonationMiddleware
+     still validates the impersonated user separately (F11), so a
+     service token that claims to be a disabled user is still 403'd
+     before the role check fires.
+
+  UI: `/app/tokens` adds an Issuer column showing `user_label` (or
+  `user#N` for deleted issuers, or `—` for legacy / console-issued
+  tokens). OpenAPI yaml + `openapi.php` document the new fields.
+
+  Regression tests in
+  `api/tests/Integration/Auth/TokenIssuerBindingTest.php`:
+  `testAdminTokenCreatedViaApiIsBoundToActingAdmin` (binding +
+  audit row attribution),
+  `testReporterTokenCreatedViaApiIsNotBoundToUser` (admin-only
+  binding), `testListSurfacesIssuerLabel` (denormalised label),
+  `testBoundAdminTokenAuthenticatesWhileIssuerActive` (happy path),
+  `testBoundAdminTokenIsRejectedAfterIssuerDisabled` (F11
+  intersect),
+  `testBoundAdminTokenIsRejectedAfterIssuerDemotedBelowTokenRole`
+  (Admin-token-after-Viewer-demote → 401),
+  `testBoundAdminTokenStillAuthenticatesIfIssuerHasMatchingRole`
+  (Viewer token held by Admin issuer still works),
+  `testBoundAdminTokenIsRejectedIfIssuerRowIsGone` (deletion
+  fallback on SQLite),
+  `testLegacyUnboundAdminTokenStillAuthenticates` (grandfathering).
 
 ### F17 — `GET /api/v1/auth/users/{id}` enables enumeration of internal user records
 - **File:** `api/src/Application/Auth/AuthController.php:79-104`