From 495d314aa525ff553baa561699db5cd875ec4ebe Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 16:07:11 +0200 Subject: [PATCH] Preflight every customer conflict before this migration mutates anything --- ..._29_100000_move_rbac_to_operator_guard.php | 71 ++++++++++++++++--- tests/Feature/Admin/RbacMoveTest.php | 53 ++++++++++++++ 2 files changed, 113 insertions(+), 11 deletions(-) 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 460a655..1cb4b10 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 @@ -77,6 +77,19 @@ return new class extends Migration public function up(): void { + // Preflight, before a single UPDATE, ALTER TABLE or row copy below: + // walk the whole worklist and refuse up front if ANY account on it is + // also a customer's portal login, naming every conflicting address at + // once. The check used to live only inside removeUserRow(), reached + // after permissions, roles, ownership ids and the four OWNED_COLUMNS + // constraints had already moved — none of which is transactional on + // MariaDB (see the class docblock above), so that abort left a live + // server's currently-deployed (web-guard) code unable to authenticate + // anyone until a human resolved the conflict and re-ran. Nothing here + // needs any of that to have happened first: the worklist and the + // customer link are both already knowable. + $this->preflightCustomerConflicts(); + app(PermissionRegistrar::class)->forgetCachedPermissions(); DB::table('permissions')->where('guard_name', 'web')->update(['guard_name' => 'operator']); @@ -239,25 +252,61 @@ return new class extends Migration } /** - * The users row goes — whoever is in `operators` has no business in `users`. - * - * One exception, and it deletes nothing: a row that is also a customer's - * portal login means the same person is operator AND paying customer. Then - * it is not the row that is wrong but the assumption, and a silent delete - * would take billing data with it. - * + * Refuse up front, for every conflicting address at once, before this + * migration has changed anything. Shares isPortalLoginForCustomer() with + * removeUserRow() below rather than a second copy of the rule — one + * decision, checked twice. removeUserRow()'s own check stays in place as + * a backstop for a conflict created in the (short) window between this + * preflight and that row's own delete — a race, not a duplicated + * decision. + */ + private function preflightCustomerConflicts(): void + { + $conflicts = []; + + foreach ($this->operatorUserIds() as $userId) { + $row = DB::table('users')->where('id', $userId)->first(); + + if ($row !== null && $this->isPortalLoginForCustomer($row->id)) { + $conflicts[] = $row->email; + } + } + + if ($conflicts !== []) { + throw new RuntimeException( + 'Refusing to move: the following accounts are both an operator and a customer. ' + .'Resolve them by hand before migrating: '.implode(', ', $conflicts) + ); + } + } + + /** * `users` carries no `customer_id` of its own (checked against the real * schema) — the link runs the other way, `customers.user_id`, exactly the * column Customer::ensureUser() writes when it attaches a portal login. That * is the one place a users row is "also a customer"; seats and orders both * hang off `customers.id`, never off a user directly, so there is nothing - * further to check on this row once that link is clear. + * further to check once that link is clear. + */ + private function isPortalLoginForCustomer(int $userId): bool + { + return DB::table('customers')->where('user_id', $userId)->exists(); + } + + /** + * The users row goes — whoever is in `operators` has no business in `users`. + * + * One exception, and it deletes nothing: a row that is also a customer's + * portal login means the same person is operator AND paying customer. Then + * it is not the row that is wrong but the assumption, and a silent delete + * would take billing data with it. preflightCustomerConflicts() above + * already refused the whole migration if this were true when it started; + * reaching here with a conflict means one appeared in between — worth + * catching, not worth trusting to have already been caught. */ private function removeUserRow(object $row): void { - $isPortalLoginForCustomer = DB::table('customers')->where('user_id', $row->id)->exists(); - - if ($isPortalLoginForCustomer) { + if ($this->isPortalLoginForCustomer($row->id)) { throw new RuntimeException( "Refusing to move [{$row->email}]: this account is both an operator and a customer. " .'Resolve that by hand before migrating.' diff --git a/tests/Feature/Admin/RbacMoveTest.php b/tests/Feature/Admin/RbacMoveTest.php index 3a50311..de1cc8a 100644 --- a/tests/Feature/Admin/RbacMoveTest.php +++ b/tests/Feature/Admin/RbacMoveTest.php @@ -110,6 +110,59 @@ it('refuses to delete a users row that has a customer on it', function () { } }); +it('preflights every customer conflict before mutating anything, listing all of them at once', function () { + // The customer-conflict check used to live only inside removeUserRow(), + // reached after permissions, roles, ownership ids and the four + // OWNED_COLUMNS constraints had already moved — none of which is + // transactional on MariaDB (see the class docblock), so that abort left a + // live server's currently-deployed (web-guard) code unable to + // authenticate anyone until a human resolved the conflict by hand. Two + // conflicting accounts, not one: proves the preflight collects everyone + // before refusing, rather than an operator fixing one conflict, running + // the migration again, finding the next one, and so on. + $ownerRoleId = DB::table('roles')->where('name', 'Owner')->value('id'); + + $customerA = Customer::factory()->create(); + $userA = User::factory()->create(['email' => 'beides-a@clupilot.test']); + DB::table('customers')->where('id', $customerA->id)->update(['user_id' => $userA->id]); + + $customerB = Customer::factory()->create(); + $userB = User::factory()->create(['email' => 'beides-b@clupilot.test']); + DB::table('customers')->where('id', $customerB->id)->update(['user_id' => $userB->id]); + + DB::table('model_has_roles')->insert([ + ['role_id' => $ownerRoleId, 'model_type' => User::class, 'model_id' => $userA->id], + ['role_id' => $ownerRoleId, 'model_type' => User::class, 'model_id' => $userB->id], + ]); + + // Reset to the state up() actually starts from on a real server: nothing + // migrated yet. RefreshDatabase already moved these once, so without this + // reset "nothing was mutated" would read true no matter what this second + // run actually did. + DB::table('permissions')->where('guard_name', 'operator')->update(['guard_name' => 'web']); + DB::table('roles')->where('guard_name', 'operator')->update(['guard_name' => 'web']); + + 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-a@clupilot.test') + ->and($e->getMessage())->toContain('beides-b@clupilot.test'); + } + + // Nothing mutated: still exactly the pre-migration shape, not "moved and + // then rolled back" — there is nothing here to roll back on a real + // server, which is the whole reason this has to be checked up front. + expect(Permission::where('guard_name', 'web')->count())->toBe(17) + ->and(Permission::where('guard_name', 'operator')->count())->toBe(0) + ->and(Role::where('guard_name', 'web')->count())->toBe(6) + ->and(Role::where('guard_name', 'operator')->count())->toBe(0) + ->and(DB::table('users')->where('id', $userA->id)->exists())->toBeTrue() + ->and(DB::table('users')->where('id', $userB->id)->exists())->toBeTrue() + ->and(Operator::where('email', 'beides-a@clupilot.test')->exists())->toBeFalse() + ->and(Operator::where('email', 'beides-b@clupilot.test')->exists())->toBeFalse(); +}); + 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