mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
706ebf6166
StaffAccounts opens with the rule: "Nobody hands out authority they do not hold ... that turns one permission into every permission and makes the rest of the matrix decorative." mayGrant() enforces it for a role's permissions, guardTarget() applies the same test to an existing account, and RolesController::guardGrantablePermissions() names the attack in full -- a non-administrator holding manage_users minting a role that carries more than they do, and then holding it. A role carries one more thing, and it is the larger one. `client_scoped` decides whether the role reaches the clients assigned to its holder or the whole library, which is the boundary StaffLibraryScope, ActivityLogScope and every listing in the application are built around. Nothing weighed it. store() and update() wrote the flag straight from the request, and mayGrant() looked only at permissions -- so `manage_users` on a client-scoped role was enough to take the limit off that role and keep working, or to mint a role without one and move into it. Either way the next request read the whole library, and the `assigned_clients` roster that #1697 protects stopped meaning anything for that account. Both halves of the existing pair get the missing clause: - guardScopeRemoval() in RolesController refuses a client-scoped actor who creates a role without the limit, or takes the limit off one that has it. Phrased as "removes the limit" rather than "is not limited", so only what this request changes is weighed -- the same reasoning guardGrantablePermissions() gives for looking at the diff. Editing an already-unlimited role's permissions is not this actor lifting a limit. Both writers resolve the flag with Request::boolean() and hand that same value to the guard and to the write: the `boolean` validation rule accepts "0" and 0 as well as false and validates without casting, so reading the validated array and comparing it strictly would leave this guard and the model's own `boolean` cast disagreeing about one value -- which is the shape the guard exists to prevent. - mayGrant() refuses a client-scoped actor granting a role that is not client-scoped, which closes assigning an existing one. It reaches both surfaces at once: assignableRoleIds() validates role_id on the web and API staff forms and on the account converter, assignableRoles() fills the pickers, and guardTarget() covers the account itself. Administrators are unaffected -- mayGrant() returns early for them, and an administrator role is never client-scoped. Unscoped staff are unaffected: the clause is conditioned on the actor's own scope, so a non-administrator with manage_users and no limit creates, edits and grants exactly as before. The seeded roles are untouched; update() already refused to move the flag on a system role, which is why the stock Client Manager was never the way in. Two changes a client-scoped holder of manage_users will notice, both following from mayGrant(): - the role picker on the staff form and the account converter now offers only client-scoped roles, rather than offering one the request behind it would refuse; - editing or deleting a staff account whose role is not client-scoped now answers 403, through guardTarget(), on the same "if you could not grant their role you have no business editing that account" rule that already applied to permissions. The roles API is read-only (GET /roles is the whole surface), so this half has no API twin to mirror; the account half is covered above.
248 lines
9.8 KiB
PHP
248 lines
9.8 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
use App\Models\User;
|
|
use App\Modules\Files\Access\StaffLibraryScope;
|
|
use App\Modules\Identity\Models\Role;
|
|
use App\Modules\Identity\Models\RolePermission;
|
|
use App\Modules\Identity\Permissions\Permission;
|
|
use App\Modules\Identity\Permissions\SystemRole;
|
|
use Illuminate\Support\Facades\Storage;
|
|
use Illuminate\Support\Str;
|
|
use Inertia\Testing\AssertableInertia;
|
|
|
|
/**
|
|
* A role's client_scoped flag is the boundary a limited staff member
|
|
* works inside. StaffAccounts already refuses to let anybody grant
|
|
* permissions they do not hold; this is the same rule for the flag that
|
|
* decides how much of the library the role reaches.
|
|
*/
|
|
beforeEach(function () {
|
|
Storage::fake('files');
|
|
$this->admin = User::factory()->create();
|
|
|
|
$this->scopedRole = Role::query()->create(['name' => 'Reps '.Str::random(6), 'client_scoped' => true]);
|
|
foreach ([Permission::ManageUsers, Permission::EditUsers, Permission::CreateUsers] as $permission) {
|
|
RolePermission::query()->create(['role_id' => $this->scopedRole->id, 'permission' => $permission->value]);
|
|
}
|
|
|
|
$this->rep = User::factory()->create(['role_id' => $this->scopedRole->id]);
|
|
$this->mine = User::factory()->client()->create();
|
|
$this->rep->assignedClients()->sync([$this->mine->id]);
|
|
|
|
// Somebody else's file: the thing the boundary is holding back.
|
|
$this->secret = uploadNamedFile($this->admin, 'not-theirs');
|
|
});
|
|
|
|
function unscopedRoleWith(array $permissions): Role
|
|
{
|
|
$role = Role::query()->create(['name' => 'Wide '.Str::random(6), 'client_scoped' => false]);
|
|
foreach ($permissions as $permission) {
|
|
RolePermission::query()->create(['role_id' => $role->id, 'permission' => $permission->value]);
|
|
}
|
|
|
|
return $role;
|
|
}
|
|
|
|
function stillScoped(User $rep): bool
|
|
{
|
|
$rep->refresh()->unsetRelation('role');
|
|
|
|
return $rep->isClientScoped();
|
|
}
|
|
|
|
test('a scoped staff member cannot lift the limit off their own role', function () {
|
|
$this->actingAs($this->rep)->patch("/roles/{$this->scopedRole->id}", [
|
|
'name' => $this->scopedRole->name,
|
|
'client_scoped' => false,
|
|
'permissions' => [Permission::ManageUsers->value, Permission::EditUsers->value, Permission::CreateUsers->value],
|
|
])->assertSessionHasErrors('client_scoped');
|
|
|
|
expect($this->scopedRole->fresh()->client_scoped)->toBeTrue()
|
|
->and(stillScoped($this->rep))->toBeTrue()
|
|
->and(app(StaffLibraryScope::class)->files($this->rep)->pluck('id')->all())
|
|
->not->toContain($this->secret->id);
|
|
});
|
|
|
|
test('a scoped staff member cannot mint a role without the limit', function () {
|
|
$this->actingAs($this->rep)->post('/roles', [
|
|
'name' => 'Escape Hatch',
|
|
'client_scoped' => false,
|
|
'permissions' => [Permission::ManageUsers->value],
|
|
])->assertSessionHasErrors('client_scoped');
|
|
|
|
expect(Role::query()->where('name', 'Escape Hatch')->exists())->toBeFalse();
|
|
});
|
|
|
|
/**
|
|
* The `boolean` validation rule accepts "0" and 0 as well as false, and
|
|
* validates without casting -- so a guard that compares the validated
|
|
* value strictly against false sees a string, waves the request through,
|
|
* and the model's own `boolean` cast then writes the flag as false. Both
|
|
* writers read the flag with Request::boolean() for that reason, and
|
|
* these two pin it: the same two requests as above, spelled the other
|
|
* way, have to be refused the same way.
|
|
*/
|
|
test('the limit cannot be dropped by spelling false as "0"', function () {
|
|
$this->actingAs($this->rep)->patch("/roles/{$this->scopedRole->id}", [
|
|
'name' => $this->scopedRole->name,
|
|
'client_scoped' => '0',
|
|
'permissions' => [Permission::ManageUsers->value, Permission::EditUsers->value, Permission::CreateUsers->value],
|
|
])->assertSessionHasErrors('client_scoped');
|
|
|
|
expect($this->scopedRole->fresh()->client_scoped)->toBeTrue()
|
|
->and(stillScoped($this->rep))->toBeTrue()
|
|
->and(app(StaffLibraryScope::class)->files($this->rep)->pluck('id')->all())
|
|
->not->toContain($this->secret->id);
|
|
|
|
// And through the JSON door, where it arrives as an integer.
|
|
$this->actingAs($this->rep)->patchJson("/roles/{$this->scopedRole->id}", [
|
|
'name' => $this->scopedRole->name,
|
|
'client_scoped' => 0,
|
|
'permissions' => [Permission::ManageUsers->value, Permission::EditUsers->value, Permission::CreateUsers->value],
|
|
])->assertStatus(422);
|
|
|
|
expect($this->scopedRole->fresh()->client_scoped)->toBeTrue();
|
|
});
|
|
|
|
test('a role without the limit cannot be minted by spelling false as "0"', function () {
|
|
$this->actingAs($this->rep)->post('/roles', [
|
|
'name' => 'Escape Hatch Zero',
|
|
'client_scoped' => '0',
|
|
'permissions' => [Permission::ManageUsers->value],
|
|
])->assertSessionHasErrors('client_scoped');
|
|
|
|
expect(Role::query()->where('name', 'Escape Hatch Zero')->exists())->toBeFalse();
|
|
});
|
|
|
|
test('a scoped staff member cannot move into an unscoped role that already exists', function () {
|
|
$wide = unscopedRoleWith([Permission::ManageUsers, Permission::EditUsers, Permission::CreateUsers]);
|
|
|
|
$this->actingAs($this->rep)->patch("/users/{$this->rep->id}", [
|
|
'name' => $this->rep->name,
|
|
'email' => $this->rep->email,
|
|
'role_id' => $wide->id,
|
|
'active' => true,
|
|
])->assertSessionHasErrors('role_id');
|
|
|
|
expect(stillScoped($this->rep))->toBeTrue()
|
|
->and(app(StaffLibraryScope::class)->files($this->rep)->pluck('id')->all())
|
|
->not->toContain($this->secret->id);
|
|
});
|
|
|
|
test('a scoped staff member cannot hand an unscoped role to anybody else either', function () {
|
|
$wide = unscopedRoleWith([Permission::ManageUsers]);
|
|
|
|
$this->actingAs($this->rep)->post('/users', [
|
|
'name' => 'New Rep',
|
|
'email' => 'new-rep@example.test',
|
|
'role_id' => $wide->id,
|
|
'password' => 'a-strong-password-1',
|
|
'password_confirmation' => 'a-strong-password-1',
|
|
])->assertSessionHasErrors('role_id');
|
|
|
|
expect(User::query()->where('email', 'new-rep@example.test')->exists())->toBeFalse();
|
|
});
|
|
|
|
test('the staff form stops offering a role the request behind it would refuse', function () {
|
|
$wide = unscopedRoleWith([Permission::ManageUsers]);
|
|
|
|
$this->actingAs($this->rep)->get('/users/create')->assertInertia(
|
|
fn (AssertableInertia $page) => $page->where('roles', function ($roles) use ($wide) {
|
|
$ids = collect($roles)->pluck('id')->all();
|
|
|
|
expect($ids)->toContain($this->scopedRole->id)->not->toContain($wide->id);
|
|
|
|
return true;
|
|
})
|
|
);
|
|
});
|
|
|
|
test('a scoped staff member has no business editing an unscoped colleague', function () {
|
|
$wide = unscopedRoleWith([Permission::ManageUsers]);
|
|
$colleague = User::factory()->create(['role_id' => $wide->id]);
|
|
|
|
$this->actingAs($this->rep)->patch("/users/{$colleague->id}", [
|
|
'name' => 'Renamed',
|
|
'email' => $colleague->email,
|
|
'role_id' => $wide->id,
|
|
'active' => true,
|
|
])->assertForbidden();
|
|
|
|
expect($colleague->fresh()->name)->not->toBe('Renamed');
|
|
});
|
|
|
|
test('the API surface refuses the same move', function () {
|
|
$wide = unscopedRoleWith([Permission::ManageUsers, Permission::EditUsers, Permission::CreateUsers]);
|
|
$token = $this->rep->createToken('t', [Permission::ManageUsers->value, Permission::EditUsers->value])->plainTextToken;
|
|
|
|
$this->withToken($token)
|
|
->patchJson("/api/v1/users/{$this->rep->id}", ['role_id' => $wide->id])
|
|
->assertStatus(422);
|
|
|
|
expect(stillScoped($this->rep))->toBeTrue();
|
|
});
|
|
|
|
test('a scoped staff member may still create and edit a role inside their own limit', function () {
|
|
$this->actingAs($this->rep)->post('/roles', [
|
|
'name' => 'Junior Rep',
|
|
'client_scoped' => true,
|
|
'permissions' => [Permission::ManageUsers->value],
|
|
])->assertSessionHasNoErrors();
|
|
|
|
$junior = Role::query()->where('name', 'Junior Rep')->firstOrFail();
|
|
expect($junior->client_scoped)->toBeTrue();
|
|
|
|
$this->actingAs($this->rep)->patch("/roles/{$junior->id}", [
|
|
'name' => 'Junior Rep',
|
|
'client_scoped' => true,
|
|
'permissions' => [Permission::ManageUsers->value, Permission::EditUsers->value],
|
|
])->assertSessionHasNoErrors();
|
|
|
|
expect($junior->fresh()->permissions()->pluck('permission')->all())
|
|
->toContain(Permission::EditUsers->value);
|
|
});
|
|
|
|
test('an unscoped staff member with the same permissions is unaffected', function () {
|
|
$wideRole = unscopedRoleWith([Permission::ManageUsers, Permission::EditUsers, Permission::CreateUsers]);
|
|
$manager = User::factory()->create(['role_id' => $wideRole->id]);
|
|
|
|
$this->actingAs($manager)->post('/roles', [
|
|
'name' => 'Another Wide One',
|
|
'client_scoped' => false,
|
|
'permissions' => [Permission::ManageUsers->value],
|
|
])->assertSessionHasNoErrors();
|
|
|
|
expect(Role::query()->where('name', 'Another Wide One')->value('client_scoped'))->toBeFalse();
|
|
|
|
$this->actingAs($manager)->patch("/users/{$manager->id}", [
|
|
'name' => $manager->name,
|
|
'email' => $manager->email,
|
|
'role_id' => $wideRole->id,
|
|
'active' => true,
|
|
])->assertSessionHasNoErrors();
|
|
});
|
|
|
|
test('an administrator is unaffected', function () {
|
|
$this->actingAs($this->admin)->patch("/roles/{$this->scopedRole->id}", [
|
|
'name' => $this->scopedRole->name,
|
|
'client_scoped' => false,
|
|
'permissions' => [Permission::ManageUsers->value],
|
|
])->assertSessionHasNoErrors();
|
|
|
|
expect($this->scopedRole->fresh()->client_scoped)->toBeFalse();
|
|
});
|
|
|
|
test('the seeded Client Manager role keeps its flag, as it always did', function () {
|
|
$clientManager = Role::query()->where('name', SystemRole::ClientManager->value)->firstOrFail();
|
|
|
|
$this->actingAs($this->rep)->patch("/roles/{$clientManager->id}", [
|
|
'name' => $clientManager->name,
|
|
'client_scoped' => false,
|
|
'permissions' => [],
|
|
]);
|
|
|
|
expect($clientManager->fresh()->client_scoped)->toBeTrue();
|
|
});
|