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 f6f3c8c..460a655 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 @@ -123,30 +123,46 @@ return new class extends Migration } /** - * Every `users` row that currently holds a console role and has not yet - * been carried across to `operators`. + * Every `users` row that currently holds a console role, a directly + * granted console permission, or both — 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 + * Spatie allows a permission to be granted straight to a model with no + * role in between. Reading only model_has_roles missed exactly that + * case: an account like that kept working right up until this migration + * flips the permission's own guard to `operator` a few lines above, at + * which point it silently lost console access while staying behind as a + * portal user — nothing downstream ever noticed, because carryAcross() + * was never asked to look at it. + * + * Both queries keep `orderBy('model_id')`, and the merged, deduplicated + * result is sorted again in PHP — the ordering matters regardless of + * which table a given id came from: 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. + * MariaDB happens to satisfy the model_has_roles 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') + $viaRoles = DB::table('model_has_roles') ->where('model_type', User::class) ->orderBy('model_id') - ->pluck('model_id') - ->unique() - ->all(); + ->pluck('model_id'); + + $viaPermissions = DB::table('model_has_permissions') + ->where('model_type', User::class) + ->orderBy('model_id') + ->pluck('model_id'); + + return $viaRoles->merge($viaPermissions)->unique()->sort()->values()->all(); } /** diff --git a/tests/Feature/Admin/RbacMoveTest.php b/tests/Feature/Admin/RbacMoveTest.php index 96ca58c..3a50311 100644 --- a/tests/Feature/Admin/RbacMoveTest.php +++ b/tests/Feature/Admin/RbacMoveTest.php @@ -56,6 +56,39 @@ it('carries an existing operator user across with password and two-factor intact ->and(DB::table('users')->where('email', 'alt@clupilot.test')->exists())->toBeFalse(); }); +it('carries across an operator who holds a permission directly and no role at all', function () { + // operatorUserIds() used to look only at model_has_roles. A pre-existing + // console user granted console.view straight through model_has_permissions + // — never through any role — was invisible to that worklist. Meanwhile the + // permission's own guard is switched to `operator` a few lines above the + // worklist loop, so an account like this would silently lose console + // access while staying behind as a portal user, forever. + $hash = bcrypt('direkt-passwort'); + $userId = DB::table('users')->insertGetId([ + 'name' => 'Direkt', 'email' => 'direkt@clupilot.test', 'password' => $hash, + 'is_admin' => true, 'created_at' => now(), 'updated_at' => now(), + ]); + $permissionId = DB::table('permissions')->where('name', 'console.view')->value('id'); + DB::table('model_has_permissions')->insert([ + 'permission_id' => $permissionId, 'model_type' => User::class, 'model_id' => $userId, + ]); + // Deliberately no model_has_roles row for this user. + + (require base_path('database/migrations/2026_07_29_100000_move_rbac_to_operator_guard.php'))->up(); + + $operator = Operator::where('email', 'direkt@clupilot.test')->first(); + + expect($operator)->not->toBeNull() + ->and($operator->roles()->count())->toBe(0) + ->and($operator->can('console.view'))->toBeTrue() + ->and(DB::table('model_has_permissions') + ->where('model_type', Operator::class)->where('model_id', $operator->id) + ->where('permission_id', $permissionId)->exists())->toBeTrue() + // Carried, and gone from `users` like any other operator — not left + // behind as a still-working portal login. + ->and(DB::table('users')->where('email', 'direkt@clupilot.test')->exists())->toBeFalse(); +}); + it('refuses to delete a users row that has a customer on it', function () { // `users` has no `customer_id` column (checked against the real schema) — // the link runs the other way, `customers.user_id`, exactly what