From 54fb6deb032a5cea07bc58a6c9760de2aa93569b Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 13:31:34 +0200 Subject: [PATCH] Make the RBAC move retry-safe and order-safe, and round-trip permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three faults in the same migration: - up()'s removal loop was driven by operatorUserIds() (model_has_roles WHERE model_type=User) via the $carried array — but carryAcross() repoints model_has_roles to Operator for every operator before a single users row is deleted. A run that stops partway (the customer-conflict abort, or a dropped connection on real MariaDB, which this file deliberately runs without a transaction) leaves that worklist empty on the next attempt, so `migrate` records the migration as applied while every leftover users row still grants a live portal login on the operator's own password. Now driven by `operators` joined to `users` on email: that finds every carried-but-not-yet-deleted row regardless of which run carried it. - operatorUserIds() had no ORDER BY. carryAcross() remaps four owned columns in place, keyed on the old users.id, while operators.id hands out new values in the same order the list is walked — unordered, a later operator's freshly assigned id can equal an earlier operator's still-unprocessed users.id, and the second remap catches a row the first one already rewrote. One operator ends up silently owning another operator's VPN peer. Added ->orderBy('model_id'). - down() restored model_has_roles but not model_has_permissions, so an operator holding a direct (not role-granted) permission lost it on rollback. Mirrored the same move carryAcross() does forward. Also removes seed_roles_and_permissions' now-stale User::OPERATOR_ROLES/hasAnyRole()/assignRole() bootstrap loop — all three gone from User per this branch's own work — which only a deep rollback (down() of the move migration recreates is_admin users) would ever have reached, but would fatal when it did. --- ...7_25_133900_seed_roles_and_permissions.php | 9 - ..._29_100000_move_rbac_to_operator_guard.php | 80 +++++++-- tests/Feature/Admin/RbacMoveTest.php | 156 ++++++++++++++++++ 3 files changed, 224 insertions(+), 21 deletions(-) diff --git a/database/migrations/2026_07_25_133900_seed_roles_and_permissions.php b/database/migrations/2026_07_25_133900_seed_roles_and_permissions.php index 35f63a6..fe268cd 100644 --- a/database/migrations/2026_07_25_133900_seed_roles_and_permissions.php +++ b/database/migrations/2026_07_25_133900_seed_roles_and_permissions.php @@ -1,6 +1,5 @@ syncPermissions($perms); } - // Migrate existing operators BEFORE the gate switches to console.view: - // every legacy is_admin user becomes an Owner so none are locked out. - User::query()->where('is_admin', true)->get()->each(function (User $u) { - if (! $u->hasAnyRole(User::OPERATOR_ROLES)) { - $u->assignRole('Owner'); - } - }); - app(PermissionRegistrar::class)->forgetCachedPermissions(); } diff --git a/database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php b/database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php index 28fd127..f6f3c8c 100644 --- a/database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php +++ b/database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php @@ -4,6 +4,7 @@ use App\Models\Operator; use App\Models\User; use Illuminate\Database\Migrations\Migration; use Illuminate\Database\Schema\Blueprint; +use Illuminate\Support\Collection; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Schema; use Illuminate\Support\Str; @@ -88,9 +89,8 @@ return new class extends Migration // until the values are settled. $this->dropOwnedColumnForeignKeys(); - $carried = []; foreach ($this->operatorUserIds() as $userId) { - $carried[] = $this->carryAcross($userId); + $this->carryAcross($userId); } // Every operator exists now and every owned-column value already @@ -101,32 +101,83 @@ return new class extends Migration // longer carries the role it used to. $this->addOwnedColumnForeignKeys('operators'); - foreach ($carried as $row) { - if ($row !== null) { - $this->removeUserRow($row); - } + // NOT driven by operatorUserIds(): carryAcross() above repoints + // model_has_roles to Operator unconditionally, for every operator, + // before a single `users` row is deleted. So on a retry after a + // partial run — the customer-conflict abort below, or simply a + // dropped connection, since this file deliberately runs without a + // transaction (see the class docblock) — model_has_roles already has + // nothing left with model_type=User, even though a `users` row can + // still be sitting there untouched. A worklist built from that query + // would find nothing to remove and `migrate` would record this + // migration as applied with the leftover row still granting a live + // portal login. Driven by `operators` joined to `users` on email + // instead: that join finds every carried-but-not-yet-deleted row + // regardless of which run carried it, so a second `migrate` actually + // finishes the job rather than silently no-opping. + foreach ($this->carriedUserRows() as $row) { + $this->removeUserRow($row); } app(PermissionRegistrar::class)->forgetCachedPermissions(); } - /** Every users row that currently holds a console role. */ + /** + * Every `users` row that currently holds a console role and has not yet + * been carried across to `operators`. + * + * Ordered by model_id: carryAcross() below remaps the four OWNED_COLUMNS + * in place, keyed on the OLD users.id, while operators.id — auto- + * increment — hands out its values in exactly the order this list is + * walked. Unordered, a later operator's freshly assigned operators.id + * can equal an earlier operator's still-unprocessed users.id, and the + * second remap's `WHERE user_id = X` then catches a row the first remap + * already rewrote to that same X. Collision-free today only because + * MariaDB happens to satisfy this query from an index that already + * returns rows in ascending model_id order; forcing it to answer from + * PRIMARY (role_id, model_id, model_type) instead returns them out of + * order, and the collision hands one operator another operator's VPN + * peer — silently, since both updates report success. + */ private function operatorUserIds(): array { return DB::table('model_has_roles') ->where('model_type', User::class) + ->orderBy('model_id') ->pluck('model_id') ->unique() ->all(); } - /** @return object|null the carried `users` row, for removeUserRow() afterwards — null if it was already gone. */ - private function carryAcross(int $userId): ?object + /** + * Every `users` row whose email already has a matching `operators` row — + * meaning carryAcross() has already run for it, in this pass or an + * earlier one, and only the delete in removeUserRow() is still + * outstanding. See the comment in up() for why this drives the removal + * loop instead of operatorUserIds()/model_has_roles. + * + * Ordered by users.id for the same reason operatorUserIds() is: a + * deterministic order is one less thing that can vary between a run that + * was tested and a run that was not. + * + * @return Collection + */ + private function carriedUserRows(): Collection + { + return DB::table('users') + ->join('operators', 'operators.email', '=', 'users.email') + ->orderBy('users.id') + ->select('users.*') + ->get(); + } + + /** Idempotent: safe to call again for a userId that was already carried across. */ + private function carryAcross(int $userId): void { $row = DB::table('users')->where('id', $userId)->first(); if ($row === null) { - return null; + return; } // Idempotent: re-running must fix a half-finished move, not fail on the @@ -169,8 +220,6 @@ return new class extends Migration DB::table('vpn_peers')->where('created_by', $userId)->update(['created_by' => $operator->id]); DB::table('maintenance_windows')->where('created_by', $userId)->update(['created_by' => $operator->id]); DB::table('app_secrets')->where('updated_by', $userId)->update(['updated_by' => $operator->id]); - - return $row; } /** @@ -268,6 +317,13 @@ return new class extends Migration ->where('model_type', Operator::class)->where('model_id', $row->id) ->update(['model_type' => User::class, 'model_id' => $userId]); + // Mirrors carryAcross(): that method moves BOTH model_has_roles and + // model_has_permissions forward, so an operator holding a direct + // (not role-granted) permission does not lose it on the way back. + DB::table('model_has_permissions') + ->where('model_type', Operator::class)->where('model_id', $row->id) + ->update(['model_type' => User::class, 'model_id' => $userId]); + DB::table('vpn_peers')->where('user_id', $row->id)->update(['user_id' => $userId]); DB::table('vpn_peers')->where('created_by', $row->id)->update(['created_by' => $userId]); DB::table('maintenance_windows')->where('created_by', $row->id)->update(['created_by' => $userId]); diff --git a/tests/Feature/Admin/RbacMoveTest.php b/tests/Feature/Admin/RbacMoveTest.php index 79d8150..96ca58c 100644 --- a/tests/Feature/Admin/RbacMoveTest.php +++ b/tests/Feature/Admin/RbacMoveTest.php @@ -3,6 +3,7 @@ use App\Models\Customer; use App\Models\Operator; use App\Models\User; +use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\DB; use Illuminate\Support\Str; use Spatie\Permission\Models\Permission; @@ -148,3 +149,158 @@ it('remaps vpn_peers and maintenance_windows ownership to the new operator, not $window = DB::table('maintenance_windows')->where('title', 'Netz-Wartung')->first(); expect($window->created_by)->not->toBeNull()->and($window->created_by)->toBe($operatorId); }); + +it('finishes the move on a second run after a customer conflict is resolved by hand, instead of quietly no-opping', function () { + // Reproduces the exact live-server sequence this migration's own docblock + // promises: run 1 aborts on the customer conflict (by design), the + // operator resolves it by hand exactly as the exception says to, and a + // second run of the SAME migration is expected to actually finish the + // job — not silently succeed while leaving every `users` row it was + // supposed to remove still sitting there. + // + // Before the fix, operatorUserIds() drove BOTH which `users` rows to + // carry across AND which to delete. carryAcross() repoints + // model_has_roles to Operator for EVERY operator in the first loop, + // before a single deletion is attempted — so by the time run 2 starts, + // model_has_roles has nothing left with model_type=User, the removal + // loop iterates zero times, and `migrate` marks the migration applied + // with BOTH users rows still there: a live portal login for an operator, + // on their operator password. + $ownerRoleId = DB::table('roles')->where('name', 'Owner')->value('id'); + + // Given the smaller users.id so it is the first one removeUserRow() + // reaches once the worklist is ordered (see the ordering test below) — + // matching the live repro, where the conflicted operator's removal + // aborted before ANY other operator's users row was touched. + $conflictedId = DB::table('users')->insertGetId([ + 'name' => 'Beides', 'email' => 'beides@clupilot.test', 'password' => bcrypt('beides-passwort'), + 'is_admin' => true, 'created_at' => now(), 'updated_at' => now(), + ]); + $plainId = DB::table('users')->insertGetId([ + 'name' => 'Nur-Operator', 'email' => 'nur-operator@clupilot.test', 'password' => bcrypt('nur-operator-passwort'), + 'is_admin' => true, 'created_at' => now(), 'updated_at' => now(), + ]); + + DB::table('model_has_roles')->insert([ + ['role_id' => $ownerRoleId, 'model_type' => User::class, 'model_id' => $conflictedId], + ['role_id' => $ownerRoleId, 'model_type' => User::class, 'model_id' => $plainId], + ]); + + $customer = Customer::factory()->create(); + DB::table('customers')->where('id', $customer->id)->update(['user_id' => $conflictedId]); + + // Run 1: aborts on the conflict, exactly as designed. + try { + (require base_path('database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php'))->up(); + $this->fail('The migration should have refused.'); + } catch (RuntimeException $e) { + expect($e->getMessage())->toContain('beides@clupilot.test'); + } + + // Both survive run 1 — including the operator who was never in + // conflict, which is the bug this test guards against. + expect(DB::table('users')->where('id', $conflictedId)->exists())->toBeTrue() + ->and(DB::table('users')->where('id', $plainId)->exists())->toBeTrue(); + + // Resolved by hand, exactly as the exception instructs. + DB::table('customers')->where('id', $customer->id)->update(['user_id' => null]); + + // Run 2 must not silently no-op: it has to actually finish the move. + (require base_path('database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php'))->up(); + + expect(DB::table('users')->where('id', $conflictedId)->exists())->toBeFalse() + ->and(DB::table('users')->where('id', $plainId)->exists())->toBeFalse(); + + $beides = Operator::where('email', 'beides@clupilot.test')->first(); + $nurOperator = Operator::where('email', 'nur-operator@clupilot.test')->first(); + + expect($beides)->not->toBeNull()->and($beides->hasRole('Owner'))->toBeTrue() + ->and($nurOperator)->not->toBeNull()->and($nurOperator->hasRole('Owner'))->toBeTrue(); + + // The exact failure mode this bug allowed: a leftover `users` row that + // Auth::guard('web') would still accept, on the operator's own password. + expect(Auth::guard('web')->attempt(['email' => 'beides@clupilot.test', 'password' => 'beides-passwort']))->toBeFalse(); +}); + +it('orders operatorUserIds so a remapped operators.id cannot collide with an unprocessed users.id', function () { + // The consequence of an unordered worklist (I3): a later operator's + // freshly assigned operators.id can equal an earlier operator's + // still-unprocessed users.id, and the second UPDATE ... WHERE user_id = X + // then catches a row the first one already rewrote — one operator + // silently inherits another operator's VPN peer, and + // VpnPeerPolicy::downloadConfig is owner-only, so it would hand over + // someone else's private key. + // + // Not testable end-to-end on SQLite: `model_has_roles` carries a + // covering index on (model_id, model_type), and the query this migration + // runs — WHERE model_type = ? — is satisfied entirely from that index + // regardless of insertion order or of whether ->orderBy('model_id') is + // present in the code. SQLite always hands back ascending model_id + // "by luck", exactly as the finding says MariaDB does normally; MariaDB + // only reveals the bug when forced onto its PRIMARY index instead. So + // this asserts the one thing that actually distinguishes the fix on this + // engine: the SQL sent to the database carries an explicit ORDER BY. + $migration = require base_path('database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php'); + + $queries = []; + DB::listen(function ($query) use (&$queries) { + $queries[] = $query->sql; + }); + + (new ReflectionMethod($migration, 'operatorUserIds'))->invoke($migration); + + $worklistQueries = array_values(array_filter( + $queries, + fn (string $sql) => str_contains($sql, 'model_has_roles') && str_starts_with(strtolower($sql), 'select'), + )); + + expect($worklistQueries)->not->toBeEmpty(); + expect(strtolower(end($worklistQueries)))->toContain('order by'); +}); + +it('restores direct permissions on rollback, not only role-granted ones', function () { + // The forward move carries BOTH model_has_roles and model_has_permissions + // (carryAcross() above) — down() only restored the first, so an operator + // holding a permission directly (never through any role) silently lost it + // on a rollback. + $operator = Operator::factory()->role('Read-only')->create(); + // 'hosts.manage' is not part of Read-only's set (see the seed migration's + // matrix) — granted directly, so this is only present because of the + // direct grant, not because of the role. + $operator->givePermissionTo('hosts.manage'); + + (require base_path('database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php'))->down(); + + $userId = DB::table('users')->where('email', $operator->email)->value('id'); + $permissionId = DB::table('permissions')->where('name', 'hosts.manage')->value('id'); + + expect($userId)->not->toBeNull() + ->and(DB::table('model_has_permissions') + ->where('model_type', User::class)->where('model_id', $userId) + ->where('permission_id', $permissionId) + ->exists())->toBeTrue(); +}); + +it('survives a deep rollback and re-migrate without fatal-ing on a stale User role reference', function () { + // seed_roles_and_permissions used to bootstrap legacy is_admin users into + // the Owner role via User::OPERATOR_ROLES / hasAnyRole() / assignRole() + // — all three gone from User now (see R21; Operator::OPERATOR_ROLES is + // the current home for the constant). Harmless on an ordinary `migrate` + // (that loop only ever ran historically, back when User still had them) + // and on a fresh install (no is_admin users exist yet at that point in + // the migration order) — but move_rbac_to_operator_guard's own down() + // recreates `users` rows with is_admin => true, so a deep rollback + // through both migrations followed by `migrate` used to fatal on the + // very first is_admin row it found. + $rbacMove = require base_path('database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php'); + $seedRoles = require base_path('database/migrations/2026_07_25_133900_seed_roles_and_permissions.php'); + + Operator::factory()->role('Owner')->create(); + + $rbacMove->down(); // recreates a `users` row with is_admin => true + $seedRoles->down(); // deletes the roles/permissions this migration owns + + $seedRoles->up(); // must not fatal on the stale reference + + expect(DB::table('roles')->where('name', 'Owner')->where('guard_name', 'web')->exists())->toBeTrue(); +});