boban
/
CluPilotCloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Move the value remap and constraint swap before the delete, and fix the seeder

feat/operator-identity
nexxo 2026-07-28 11:00:09 +02:00
parent 87c49de6e5
commit f54d00b3e7
4 changed files with 241 additions and 102 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

192
database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php
View File

@ -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<int, array{table: string, column: string, onDelete: string}> */
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();
}
};

66
database/migrations/2026_07_29_100001_point_operator_owned_columns_at_operators.php
View File

@ -1,66 +0,0 @@
<?php
use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
/**
* A gap the guard-switch brief did not name: three columns are foreign keys
* into `users` and hold nothing but operator ids.
*
* - vpn_peers.user_id / .created_by (2026_07_25_200000, _210000) — a staff
* VPN access. VpnPeerFactory's own default owner is
* `Operator::factory()->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<int, array{table: string, column: string, onDelete: string}> */
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();
});
}
}
};

11
database/seeders/DatabaseSeeder.php
View File

@ -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,
],
);

74
tests/Feature/Admin/RbacMoveTest.php
View File

@ -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);
});