Przeglądaj źródła

Fix R02-N03: extract SessionGuard::requireAdminForm + drop SprintController::gateJsonAdmin

Form-handling controllers used to open with the same three-line block:

    $actor = SessionGuard::requireAdmin($this->users);
    if ($actor instanceof Response) return $actor;
    if (!SessionGuard::verifyCsrf($req)) return Response::text('CSRF token invalid', 403);

That's nine sites — SprintController::create / ::delete,
WorkerController::create / ::update, UserController::update / ::tombstone,
SettingsController::update, ImportController::upload / ::commit. A new
form endpoint can silently land without the CSRF or the admin gate and
the surrounding code looks fine in review. The JSON path already had a
twin (SessionGuard::requireAdminJson, SprintController::gateJsonAdmin);
the form path just lacked one.

This commit:

- Adds SessionGuard::requireAdminForm(Request, UserRepository): User|Response
  mirroring the existing requireAdminJson, but speaking the form dialect:
  redirect to /auth/login when anonymous, 403 text on non-admin, 403 text
  on CSRF mismatch.
- Replaces all nine inlined three-line blocks with a single call to the
  helper. Each call site drops from four lines to three.
- Migrates SprintController::gateJsonAdmin's eight call sites to
  SessionGuard::requireAdminJson and removes the private method — it
  was identical to the SessionGuard helper and not sprint-specific.
- Drops the now-unused App\Domain\User import in SprintController.

Net: +37 -64 lines. Tests unchanged at 340/340 (5 skipped). Spec
invariants untouched: vanilla JS, strict CSP, Twig auto-escape,
audit-row-per-mutation, capacity math under §5, audit logging rules in
§7. The 403 shape is unchanged for both paths (text response on the
form path, JSON envelope on the JSON path), so existing acceptance
behaviour is preserved exactly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 1 dzień temu
rodzic
commit
57f4143754

+ 20 - 0
src/Auth/SessionGuard.php

@@ -268,6 +268,26 @@ final class SessionGuard
         return $user;
     }
 
+    /**
+     * Form-flavoured admin gate: auth + admin + CSRF. Mirrors
+     * `requireAdminJson` but speaks the form-controller dialect — anonymous
+     * users get a redirect to /auth/login, non-admins get a 403 text
+     * response, and a CSRF mismatch gets a 403 text response. Use this
+     * inside form-handling controller actions to replace the recurring
+     * three-block boilerplate (R02-N03).
+     */
+    public static function requireAdminForm(Request $req, UserRepository $users): User|Response
+    {
+        $user = self::requireAdmin($users);
+        if ($user instanceof Response) {
+            return $user;
+        }
+        if (!self::verifyCsrf($req)) {
+            return Response::text('CSRF token invalid', 403);
+        }
+        return $user;
+    }
+
     /**
      * JSON gate for endpoints any signed-in user may hit (Phase 18:
      * task-status writes are the first such surface). Auth + CSRF, no admin

+ 2 - 8
src/Controllers/ImportController.php

@@ -87,13 +87,10 @@ final class ImportController
 
     public function upload(Request $req): Response
     {
-        $actor = SessionGuard::requireAdmin($this->users);
+        $actor = SessionGuard::requireAdminForm($req, $this->users);
         if ($actor instanceof Response) {
             return $actor;
         }
-        if (!SessionGuard::verifyCsrf($req)) {
-            return Response::text('CSRF token invalid', 403);
-        }
 
         // Validate the upload up-front; fail-closed.
         if (!isset($_FILES['xlsx']) || !is_array($_FILES['xlsx'])) {
@@ -192,13 +189,10 @@ final class ImportController
 
     public function commit(Request $req, array $params): Response
     {
-        $actor = SessionGuard::requireAdmin($this->users);
+        $actor = SessionGuard::requireAdminForm($req, $this->users);
         if ($actor instanceof Response) {
             return $actor;
         }
-        if (!SessionGuard::verifyCsrf($req)) {
-            return Response::text('CSRF token invalid', 403);
-        }
         $token = (string) ($params['token'] ?? '');
         $entry = $this->loadSessionEntry($token, $req, $actor);
         if ($entry === null) {

+ 1 - 4
src/Controllers/SettingsController.php

@@ -92,13 +92,10 @@ final class SettingsController
     /** POST /settings — admin only, CSRF via _csrf form field. */
     public function update(Request $req): Response
     {
-        $actor = SessionGuard::requireAdmin($this->users);
+        $actor = SessionGuard::requireAdminForm($req, $this->users);
         if ($actor instanceof Response) {
             return $actor;
         }
-        if (!SessionGuard::verifyCsrf($req)) {
-            return Response::text('CSRF token invalid', 403);
-        }
 
         $this->pdo->beginTransaction();
         try {

+ 10 - 36
src/Controllers/SprintController.php

@@ -14,7 +14,6 @@ namespace App\Controllers;
 
 use App\Auth\SessionGuard;
 use App\Domain\SprintWeek;
-use App\Domain\User;
 use App\Http\Request;
 use App\Http\Response;
 use App\Http\View;
@@ -90,13 +89,10 @@ final class SprintController
     /** POST /sprints — create sprint + materialise weeks in one tx. */
     public function create(Request $req): Response
     {
-        $actor = SessionGuard::requireAdmin($this->users);
+        $actor = SessionGuard::requireAdminForm($req, $this->users);
         if ($actor instanceof Response) {
             return $actor;
         }
-        if (!SessionGuard::verifyCsrf($req)) {
-            return Response::text('CSRF token invalid', 403);
-        }
 
         $name     = trim($req->postString('name'));
         $start    = $req->postString('start_date');
@@ -346,7 +342,7 @@ final class SprintController
     /** PATCH /sprints/{id} — JSON — update name / dates / reserve_fraction. */
     public function updateMeta(Request $req, array $params): Response
     {
-        $gate = $this->gateJsonAdmin($req);
+        $gate = SessionGuard::requireAdminJson($req, $this->users);
         if ($gate instanceof Response) {
             return $gate;
         }
@@ -507,7 +503,7 @@ final class SprintController
     /** POST /sprints/{id}/weeks — JSON — resize the week set. */
     public function replaceWeeks(Request $req, array $params): Response
     {
-        $gate = $this->gateJsonAdmin($req);
+        $gate = SessionGuard::requireAdminJson($req, $this->users);
         if ($gate instanceof Response) {
             return $gate;
         }
@@ -586,7 +582,7 @@ final class SprintController
     /** POST /sprints/{id}/workers — JSON — add a worker to the sprint. */
     public function addWorker(Request $req, array $params): Response
     {
-        $gate = $this->gateJsonAdmin($req);
+        $gate = SessionGuard::requireAdminJson($req, $this->users);
         if ($gate instanceof Response) {
             return $gate;
         }
@@ -646,7 +642,7 @@ final class SprintController
     /** DELETE /sprints/{id}/workers/{sw_id} — JSON — remove a worker from the sprint. */
     public function removeWorker(Request $req, array $params): Response
     {
-        $gate = $this->gateJsonAdmin($req);
+        $gate = SessionGuard::requireAdminJson($req, $this->users);
         if ($gate instanceof Response) {
             return $gate;
         }
@@ -702,7 +698,7 @@ final class SprintController
     /** POST /sprints/{id}/workers/reorder — JSON — apply an ordering. */
     public function reorderWorkers(Request $req, array $params): Response
     {
-        $gate = $this->gateJsonAdmin($req);
+        $gate = SessionGuard::requireAdminJson($req, $this->users);
         if ($gate instanceof Response) {
             return $gate;
         }
@@ -759,7 +755,7 @@ final class SprintController
     /** PATCH /sprints/{id}/workers/{sw_id} — JSON — edit RTB. */
     public function updateWorker(Request $req, array $params): Response
     {
-        $gate = $this->gateJsonAdmin($req);
+        $gate = SessionGuard::requireAdminJson($req, $this->users);
         if ($gate instanceof Response) {
             return $gate;
         }
@@ -803,7 +799,7 @@ final class SprintController
     /** PATCH /sprints/{id}/week-cells — JSON — batch upsert of sprint_worker_days. */
     public function updateWeekCells(Request $req, array $params): Response
     {
-        $gate = $this->gateJsonAdmin($req);
+        $gate = SessionGuard::requireAdminJson($req, $this->users);
         if ($gate instanceof Response) {
             return $gate;
         }
@@ -925,7 +921,7 @@ final class SprintController
      */
     public function updateWeekDays(Request $req, array $params): Response
     {
-        $gate = $this->gateJsonAdmin($req);
+        $gate = SessionGuard::requireAdminJson($req, $this->users);
         if ($gate instanceof Response) {
             return $gate;
         }
@@ -1017,13 +1013,10 @@ final class SprintController
      */
     public function delete(Request $req, array $params): Response
     {
-        $actor = SessionGuard::requireAdmin($this->users);
+        $actor = SessionGuard::requireAdminForm($req, $this->users);
         if ($actor instanceof Response) {
             return $actor;
         }
-        if (!SessionGuard::verifyCsrf($req)) {
-            return Response::text('CSRF token invalid', 403);
-        }
 
         $sprintId = (int) $params['id'];
         $sprint   = $this->sprints->find($sprintId);
@@ -1162,25 +1155,6 @@ final class SprintController
     // Shared helpers
     // ------------------------------------------------------------------
 
-    /**
-     * Admin gate for JSON endpoints. Returns the signed-in User on success,
-     * or an `Response::err(...)` JSON envelope on failure. Also enforces CSRF.
-     */
-    private function gateJsonAdmin(Request $req): User|Response
-    {
-        $user = SessionGuard::currentUser($this->users);
-        if ($user === null) {
-            return Response::err('unauthenticated', 'Sign in required', 401);
-        }
-        if (!$user->isAdmin) {
-            return Response::err('forbidden', 'Admin access required', 403);
-        }
-        if (!SessionGuard::verifyCsrf($req)) {
-            return Response::err('csrf', 'CSRF token invalid', 403);
-        }
-        return $user;
-    }
-
     private static function isIsoDate(string $s): bool
     {
         $d = DateTimeImmutable::createFromFormat('Y-m-d', $s);

+ 2 - 8
src/Controllers/UserController.php

@@ -70,13 +70,10 @@ final class UserController
     /** POST /users/{id} — form — toggle is_admin. */
     public function update(Request $req, array $params): Response
     {
-        $actor = SessionGuard::requireAdmin($this->users);
+        $actor = SessionGuard::requireAdminForm($req, $this->users);
         if ($actor instanceof Response) {
             return $actor;
         }
-        if (!SessionGuard::verifyCsrf($req)) {
-            return Response::text('CSRF token invalid', 403);
-        }
 
         $id = (int) $params['id'];
         $target = $this->users->find($id);
@@ -188,13 +185,10 @@ final class UserController
      */
     public function tombstone(Request $req, array $params): Response
     {
-        $actor = SessionGuard::requireAdmin($this->users);
+        $actor = SessionGuard::requireAdminForm($req, $this->users);
         if ($actor instanceof Response) {
             return $actor;
         }
-        if (!SessionGuard::verifyCsrf($req)) {
-            return Response::text('CSRF token invalid', 403);
-        }
 
         $id = (int) $params['id'];
         $target = $this->users->find($id);

+ 2 - 8
src/Controllers/WorkerController.php

@@ -61,13 +61,10 @@ final class WorkerController
     /** POST /workers — create a worker. */
     public function create(Request $req): Response
     {
-        $actor = SessionGuard::requireAdmin($this->users);
+        $actor = SessionGuard::requireAdminForm($req, $this->users);
         if ($actor instanceof Response) {
             return $actor;
         }
-        if (!SessionGuard::verifyCsrf($req)) {
-            return Response::text('CSRF token invalid', 403);
-        }
 
         $name    = trim($req->postString('name'));
         $rtbRaw  = $req->postString('default_rtb');
@@ -110,13 +107,10 @@ final class WorkerController
     /** POST /workers/{id} (form) — update a worker's editable fields. */
     public function update(Request $req, array $params): Response
     {
-        $actor = SessionGuard::requireAdmin($this->users);
+        $actor = SessionGuard::requireAdminForm($req, $this->users);
         if ($actor instanceof Response) {
             return $actor;
         }
-        if (!SessionGuard::verifyCsrf($req)) {
-            return Response::text('CSRF token invalid', 403);
-        }
 
         $id = (int) $params['id'];
         $existing = $this->workers->find($id);