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

Make the RBAC move retry-safe and order-safe, and round-trip permissions 

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.
feat/operator-identity
nexxo 2026-07-28 13:31:34 +02:00
parent 6e81d6e93e
commit 54fb6deb03
3 changed files with 224 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
database/migrations/2026_07_25_133900_seed_roles_and_permissions.php
View File

@ -1,6 +1,5 @@
<?php
use App\Models\User;
use Illuminate\Database\Migrations\Migration;
use Spatie\Permission\Models\Permission;
use Spatie\Permission\Models\Role;
@ -45,14 +44,6 @@ return new class extends Migration
Role::findOrCreate($roleName, 'web')->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();
}

80
database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php
View File

@ -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<int, object>
*/
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]);

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

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