From f54d00b3e71167cdcfb582956af4d1b62771b337 Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 11:00:09 +0200 Subject: [PATCH] Move the value remap and constraint swap before the delete, and fix the seeder --- ..._29_100000_move_rbac_to_operator_guard.php | 192 +++++++++++++++--- ...nt_operator_owned_columns_at_operators.php | 66 ------ database/seeders/DatabaseSeeder.php | 11 +- tests/Feature/Admin/RbacMoveTest.php | 74 +++++++ 4 files changed, 241 insertions(+), 102 deletions(-) delete mode 100644 database/migrations/2026_07_29_100001_point_operator_owned_columns_at_operators.php 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 d4fb62b..28fd127 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 @@ -3,7 +3,9 @@ use App\Models\Operator; use App\Models\User; use Illuminate\Database\Migrations\Migration; +use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Schema; use Illuminate\Support\Str; use Spatie\Permission\PermissionRegistrar; @@ -17,23 +19,95 @@ use Spatie\Permission\PermissionRegistrar; * customer-facing one among them — so duplicating the set under a second guard * would create two role sets that drift apart. Moving them leaves `users` with * no roles at all, which is exactly right. + * + * Four columns elsewhere are foreign keys into `users` that have only ever + * held an operator's id — never a customer's: + * + * - vpn_peers.user_id ON DELETE RESTRICT + * - vpn_peers.created_by ON DELETE SET NULL + * - maintenance_windows.created_by ON DELETE SET NULL + * - app_secrets.updated_by ON DELETE SET NULL + * + * (customers.user_id is also a `users` FK, ON DELETE SET NULL, but it is the + * one link a `users` row genuinely has to a customer — that one is read, not + * rewritten; see removeUserRow().) + * + * Deleting an operator's `users` row before those four are dealt with does not + * fail safely. RESTRICT aborts the delete outright — on a live server this + * operator owns a VPN peer or two and the migration would never get past the + * first one. The three SET NULL columns are worse: the delete would succeed + * and silently erase who created that maintenance window or last rotated that + * secret. So the order in this migration is: create every operator, remap + * those four columns from the old users.id to the new operators.id (the map + * is built once, in carryAcross(), while both ids are still in hand — not + * reconstructed from email afterwards), swap the four constraints to point at + * `operators`, and only then delete the `users` rows. By the time a row is + * deleted nothing anywhere still points at it, so it does not matter which + * ON DELETE rule used to apply. + * + * Deliberately NOT wrapped in DB::transaction(). Laravel's own migrator + * already does that for a grammar that supports transactional DDL — SQLite + * (tests) and Postgres — via $withinTransaction (default true, left alone + * here). MySQL/MariaDB's grammar reports supportsSchemaTransactions() as + * false precisely because an ALTER TABLE there commits on its own regardless + * of any surrounding transaction, so the migrator does not open one and this + * file must not open one either: a manual DB::transaction() around the DDL + * below does not get rolled back by it on a real server — it gets desynced by + * it, and the migration fails on its own final commit() with "There is no + * active transaction" even on the fully successful path. Found by actually + * running this against dev's MariaDB, not by the test suite, which uses + * SQLite and does not surface the difference. What keeps this safe without a + * transaction is the same thing that keeps it safe on SQLite if the migrator's + * own transaction rolls back: every step is idempotent (firstOrNew, guarded + * drops, remaps keyed on a still-matching id), so a run that stops partway — + * the customer-conflict abort below, or a dropped connection — leaves + * something a second run can finish rather than something a second run + * chokes on. */ return new class extends Migration { + /** @var array */ + private const OWNED_COLUMNS = [ + ['table' => 'vpn_peers', 'column' => 'user_id', 'onDelete' => 'restrict'], + ['table' => 'vpn_peers', 'column' => 'created_by', 'onDelete' => 'null'], + ['table' => 'maintenance_windows', 'column' => 'created_by', 'onDelete' => 'null'], + ['table' => 'app_secrets', 'column' => 'updated_by', 'onDelete' => 'null'], + ]; + public function up(): void { - DB::transaction(function () { - app(PermissionRegistrar::class)->forgetCachedPermissions(); + app(PermissionRegistrar::class)->forgetCachedPermissions(); - DB::table('permissions')->where('guard_name', 'web')->update(['guard_name' => 'operator']); - DB::table('roles')->where('guard_name', 'web')->update(['guard_name' => 'operator']); + DB::table('permissions')->where('guard_name', 'web')->update(['guard_name' => 'operator']); + DB::table('roles')->where('guard_name', 'web')->update(['guard_name' => 'operator']); - foreach ($this->operatorUserIds() as $userId) { - $this->carryAcross($userId); + // Dropped before any value is remapped: the four columns are + // about to hold operators.id instead of users.id, and the + // constraint that is still active while that happens has to + // accept neither, one, or both — simplest is to accept anything + // until the values are settled. + $this->dropOwnedColumnForeignKeys(); + + $carried = []; + foreach ($this->operatorUserIds() as $userId) { + $carried[] = $this->carryAcross($userId); + } + + // Every operator exists now and every owned-column value already + // points at one — safe to require that going forward, and doing + // it here (before a single `users` row is gone) means a + // customer-conflict abort just below leaves the columns correctly + // constrained against `operators`, not against an id that no + // longer carries the role it used to. + $this->addOwnedColumnForeignKeys('operators'); + + foreach ($carried as $row) { + if ($row !== null) { + $this->removeUserRow($row); } + } - app(PermissionRegistrar::class)->forgetCachedPermissions(); - }); + app(PermissionRegistrar::class)->forgetCachedPermissions(); } /** Every users row that currently holds a console role. */ @@ -46,12 +120,13 @@ return new class extends Migration ->all(); } - private function carryAcross(int $userId): void + /** @return object|null the carried `users` row, for removeUserRow() afterwards — null if it was already gone. */ + private function carryAcross(int $userId): ?object { $row = DB::table('users')->where('id', $userId)->first(); if ($row === null) { - return; + return null; } // Idempotent: re-running must fix a half-finished move, not fail on the @@ -86,7 +161,16 @@ return new class extends Migration ->where('model_type', User::class)->where('model_id', $userId) ->update(['model_type' => Operator::class, 'model_id' => $operator->id]); - $this->removeUserRow($row); + // The old-id -> new-id map, built here while both are still in hand + // rather than reconstructed from email once the users row is gone: + // every column that has only ever held this operator's id follows + // them to their new home. + DB::table('vpn_peers')->where('user_id', $userId)->update(['user_id' => $operator->id]); + 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; } /** @@ -118,6 +202,45 @@ return new class extends Migration DB::table('users')->where('id', $row->id)->delete(); } + /** + * Drop the four OWNED_COLUMNS foreign keys — by column name, so it does + * not matter whether the existing constraint currently points at `users` + * or `operators`. That is what makes this safe to call on an + * already-migrated schema (RefreshDatabase re-running this file) as well + * as a fresh one. + * + * Guarded with hasForeignKey(): on real MariaDB, DDL is not transactional + * — an ALTER TABLE commits immediately, on its own, regardless of what + * DB::transaction() around it thinks is happening. That means a run that + * fails after this point (the customer-conflict abort below, or simply a + * dropped connection) leaves the constraint gone for good, and the next + * attempt at this migration has to cope with that rather than fail + * trying to drop something that is already dropped. SQLite tolerates a + * redundant drop silently; MariaDB does not, which is how this was found + * — dev's own admin account already had a VPN peer and a maintenance + * window, and a first, buggy version of this method failed exactly here. + */ + private function dropOwnedColumnForeignKeys(): void + { + foreach (self::OWNED_COLUMNS as ['table' => $table, 'column' => $column]) { + if (! Schema::hasForeignKey($table, [$column])) { + continue; + } + + Schema::table($table, fn (Blueprint $blueprint) => $blueprint->dropForeign([$column])); + } + } + + private function addOwnedColumnForeignKeys(string $to): void + { + foreach (self::OWNED_COLUMNS as ['table' => $table, 'column' => $column, 'onDelete' => $onDelete]) { + Schema::table($table, function (Blueprint $blueprint) use ($column, $to, $onDelete) { + $foreign = $blueprint->foreign($column)->references('id')->on($to); + $onDelete === 'restrict' ? $foreign->restrictOnDelete() : $foreign->nullOnDelete(); + }); + } + } + /** * `down()` recreates the `users` rows from `operators`. What it cannot bring * back is anything that only ever existed on the operator — `last_login_at` @@ -126,29 +249,36 @@ return new class extends Migration */ public function down(): void { - DB::transaction(function () { - app(PermissionRegistrar::class)->forgetCachedPermissions(); + app(PermissionRegistrar::class)->forgetCachedPermissions(); - foreach (DB::table('operators')->get() as $row) { - $userId = DB::table('users')->insertGetId([ - 'name' => $row->name, 'email' => $row->email, 'password' => $row->password, - 'two_factor_secret' => $row->two_factor_secret, - 'two_factor_recovery_codes' => $row->two_factor_recovery_codes, - 'two_factor_confirmed_at' => $row->two_factor_confirmed_at, - 'remember_token' => $row->remember_token, - 'is_admin' => true, - 'created_at' => $row->created_at, 'updated_at' => $row->updated_at, - ]); + $this->dropOwnedColumnForeignKeys(); - DB::table('model_has_roles') - ->where('model_type', Operator::class)->where('model_id', $row->id) - ->update(['model_type' => User::class, 'model_id' => $userId]); - } + foreach (DB::table('operators')->get() as $row) { + $userId = DB::table('users')->insertGetId([ + 'name' => $row->name, 'email' => $row->email, 'password' => $row->password, + 'two_factor_secret' => $row->two_factor_secret, + 'two_factor_recovery_codes' => $row->two_factor_recovery_codes, + 'two_factor_confirmed_at' => $row->two_factor_confirmed_at, + 'remember_token' => $row->remember_token, + 'is_admin' => true, + 'created_at' => $row->created_at, 'updated_at' => $row->updated_at, + ]); - DB::table('permissions')->where('guard_name', 'operator')->update(['guard_name' => 'web']); - DB::table('roles')->where('guard_name', 'operator')->update(['guard_name' => 'web']); + DB::table('model_has_roles') + ->where('model_type', Operator::class)->where('model_id', $row->id) + ->update(['model_type' => User::class, 'model_id' => $userId]); - app(PermissionRegistrar::class)->forgetCachedPermissions(); - }); + 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]); + DB::table('app_secrets')->where('updated_by', $row->id)->update(['updated_by' => $userId]); + } + + $this->addOwnedColumnForeignKeys('users'); + + DB::table('permissions')->where('guard_name', 'operator')->update(['guard_name' => 'web']); + DB::table('roles')->where('guard_name', 'operator')->update(['guard_name' => 'web']); + + app(PermissionRegistrar::class)->forgetCachedPermissions(); } }; diff --git a/database/migrations/2026_07_29_100001_point_operator_owned_columns_at_operators.php b/database/migrations/2026_07_29_100001_point_operator_owned_columns_at_operators.php deleted file mode 100644 index 92a441e..0000000 --- a/database/migrations/2026_07_29_100001_point_operator_owned_columns_at_operators.php +++ /dev/null @@ -1,66 +0,0 @@ -role('Admin')`, and Vpn::create() only ever writes - * the id of the operator's own account. - * - maintenance_windows.created_by (2026_07_25_140001) — who scheduled it; - * written by Admin\Maintenance from the operator guard. - * - app_secrets.updated_by (2026_07_27_060000) — who last rotated a stored - * credential; written by SecretVault::put(), which now takes an Operator. - * - * None of these is customer-facing. Once the previous migration moves - * operators out of `users`, storing an operator's id in a column still - * constrained to `users` fails at the database, not in application code. - * - * Left for a live server with existing operator-owned rows: their columns - * still hold the OLD users.id, and adding a constraint against `operators` - * below refuses to attach until those rows are re-pointed by hand (matched by - * the same email the previous migration carried operators across on). That - * failure is loud and safe — it blocks the migration rather than silently - * mis-attributing a row — and belongs to the pre-launch dev rehearsal the - * plan already calls for, not to a fresh test database, which never has rows - * in any of these tables at migration time. - */ -return new class extends Migration -{ - /** @var array */ - private const COLUMNS = [ - ['table' => 'vpn_peers', 'column' => 'user_id', 'onDelete' => 'restrict'], - ['table' => 'vpn_peers', 'column' => 'created_by', 'onDelete' => 'null'], - ['table' => 'maintenance_windows', 'column' => 'created_by', 'onDelete' => 'null'], - ['table' => 'app_secrets', 'column' => 'updated_by', 'onDelete' => 'null'], - ]; - - public function up(): void - { - $this->repoint('operators'); - } - - public function down(): void - { - $this->repoint('users'); - } - - private function repoint(string $to): void - { - foreach (self::COLUMNS as ['table' => $table, 'column' => $column, 'onDelete' => $onDelete]) { - Schema::table($table, function (Blueprint $blueprint) use ($column) { - $blueprint->dropForeign([$column]); - }); - - Schema::table($table, function (Blueprint $blueprint) use ($table, $column, $to, $onDelete) { - $foreign = $blueprint->foreign($column)->references('id')->on($to); - $onDelete === 'restrict' ? $foreign->restrictOnDelete() : $foreign->nullOnDelete(); - }); - } - } -}; diff --git a/database/seeders/DatabaseSeeder.php b/database/seeders/DatabaseSeeder.php index 31d0a4a..991814a 100644 --- a/database/seeders/DatabaseSeeder.php +++ b/database/seeders/DatabaseSeeder.php @@ -6,6 +6,7 @@ use App\Models\Customer; use App\Models\Datacenter; use App\Models\Host; use App\Models\Instance; +use App\Models\Operator; use App\Models\Order; use App\Models\ProvisioningRun; use App\Models\User; @@ -24,25 +25,25 @@ class DatabaseSeeder extends Seeder return; } - $admin = User::updateOrCreate( + $operator = Operator::updateOrCreate( ['email' => env('SEED_ADMIN_EMAIL', 'admin@clupilot.local')], [ 'name' => 'Admin', 'password' => Hash::make(env('SEED_ADMIN_PASSWORD', 'password')), - 'is_admin' => true, ], ); - if (! $admin->hasRole('Owner')) { - $admin->assignRole('Owner'); + if (! $operator->hasRole('Owner')) { + $operator->assignRole('Owner'); } // A plain customer to exercise the portal (and prove the admin gate). + // Never is_admin: that flag is dead, and the console account above is + // not a `users` row at all any more. User::updateOrCreate( ['email' => 'kunde@clupilot.local'], [ 'name' => 'Dr. S. Berger', 'password' => Hash::make(env('SEED_ADMIN_PASSWORD', 'password')), - 'is_admin' => false, ], ); diff --git a/tests/Feature/Admin/RbacMoveTest.php b/tests/Feature/Admin/RbacMoveTest.php index 580d785..79d8150 100644 --- a/tests/Feature/Admin/RbacMoveTest.php +++ b/tests/Feature/Admin/RbacMoveTest.php @@ -4,6 +4,7 @@ use App\Models\Customer; use App\Models\Operator; use App\Models\User; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Str; use Spatie\Permission\Models\Permission; use Spatie\Permission\Models\Role; @@ -74,3 +75,76 @@ it('refuses to delete a users row that has a customer on it', function () { expect($e->getMessage())->toContain('beides@clupilot.test'); } }); + +it('remaps vpn_peers and maintenance_windows ownership to the new operator, not just the role tables', function () { + // Simulates the live server this was written for: an operator with a VPN + // peer per device and at least one scheduled maintenance window. Both + // columns are foreign keys into `users` that have only ever held an + // operator's id — proving they end up on the new operators.id, and not + // null, is the only test that actually exercises the live case; a fresh + // database has no rows in either table for the migration to get wrong. + // + // A throwaway operator goes first, purely to consume operators.id=1: the + // in-memory test database restarts both the users and operators id + // sequences near 1 every test, so without this, netzwerk@clupilot.test's + // users.id and its own new operators.id would coincide — and a remap + // that was skipped entirely would still leave user_id/created_by + // "correct" by coincidence rather than by the migration doing its job. + // (Caught this exact way, by mutation: the first version of this test had + // no such guard and stayed green with the remap commented out.) + Operator::factory()->create(); // consumes operators.id — see the comment above. + + $hash = bcrypt('netzwerk-passwort'); + DB::table('users')->insert([ + 'name' => 'Netzwerk-Owner', 'email' => 'netzwerk@clupilot.test', 'password' => $hash, + 'is_admin' => true, 'created_at' => now(), 'updated_at' => now(), + ]); + $userId = DB::table('users')->where('email', 'netzwerk@clupilot.test')->value('id'); + DB::table('model_has_roles')->insert([ + 'role_id' => DB::table('roles')->where('name', 'Owner')->value('id'), + 'model_type' => User::class, 'model_id' => $userId, + ]); + + // RefreshDatabase already migrated once, so vpn_peers.user_id/created_by + // and maintenance_windows.created_by are already constrained against + // `operators` — the very thing this migration is supposed to arrange. + // Hand-building the LEGACY shape (a users.id sitting in those columns) + // means briefly turning that constraint off, exactly as it would have + // been before this migration ever ran on a real server. Not + // Schema::disableForeignKeyConstraints() — SQLite's `PRAGMA foreign_keys` + // is a documented no-op while a transaction is open, and RefreshDatabase + // always has one open. `defer_foreign_keys` is the pragma meant for + // exactly this: it can be toggled mid-transaction and only checks at + // commit — which this test's wrapping transaction never reaches. + DB::statement('PRAGMA defer_foreign_keys = ON'); + DB::table('vpn_peers')->insert([ + 'uuid' => (string) Str::uuid(), 'name' => 'Notebook', 'kind' => 'staff', + 'user_id' => $userId, 'created_by' => $userId, + 'public_key' => 'PUBKEYAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=', + 'allowed_ip' => '10.66.0.199', 'enabled' => true, 'present' => false, + 'created_at' => now(), 'updated_at' => now(), + ]); + DB::table('maintenance_windows')->insert([ + 'uuid' => (string) Str::uuid(), 'title' => 'Netz-Wartung', + 'starts_at' => now()->addDay(), 'ends_at' => now()->addDay()->addHours(2), + 'state' => 'draft', 'created_by' => $userId, + 'created_at' => now(), 'updated_at' => now(), + ]); + + (require base_path('database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php'))->up(); + + $operatorId = Operator::where('email', 'netzwerk@clupilot.test')->value('id'); + expect($operatorId)->not->toBeNull() + // The decoy operator above is what makes this a real assertion: it + // took operators.id 1, so netzwerk's own operator cannot be 1 too — + // the id genuinely has to have changed, not merely be numerically + // unchanged by coincidence. + ->and($operatorId)->not->toBe($userId); + + $peer = DB::table('vpn_peers')->where('name', 'Notebook')->first(); + expect($peer->user_id)->not->toBeNull()->and($peer->user_id)->toBe($operatorId) + ->and($peer->created_by)->not->toBeNull()->and($peer->created_by)->toBe($operatorId); + + $window = DB::table('maintenance_windows')->where('title', 'Netz-Wartung')->first(); + expect($window->created_by)->not->toBeNull()->and($window->created_by)->toBe($operatorId); +});