mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-17 17:15:08 +00:00
denkfabrik-li
b9f826282a
Every route that binds one client — the edit screen, the update, the
delete, the second-factor reset, the file browser — asks whether this
staff member may manage that client. POST users/convert/{user} binds one
too, and asks nothing about it.
AccountConversion::guardToStaff() says why it skips
StaffAccounts::guardTarget, and the reason is sound as far as it goes:
guardTarget asks "could the actor have granted the target's role", which
is meaningless of a client, and what limits a promotion is the role being
*granted* — enforced by the controller validating role_id against
assignableRoleIds(). That answers the question about the role. Nothing
answers the one about the target.
So a client-scoped staff member holding manage_users, edit_users and
edit_clients could promote any client on the installation. It is the
most far-reaching thing that can be done to a client account: the portal
access goes, the assignments that made them somebody's client go inert,
and they come out holding whatever staff role the actor picked from
their own list. The client is never told.
guardToStaff() now asks StaffLibraryScope::canAssignClient — the same
predicate ResolvesShareTargets uses to decide who a file may be shared
with, and true by construction for unscoped staff, so the ordinary
administrator path is untouched. 404 rather than 403, matching both the
isClient() check the controller makes on the way in and the answer the
clients routes give: a client this staff member may not manage should
not be distinguishable from one that is not there.
guardToClient() is unchanged. Its target is a staff account, guardTarget
is the right question to ask about one, and it was already being asked.
The account list on the converter screen is deliberately left as it is.
It runs under can:edit_users and shows every account of the chosen
direction, the same way every other staff surface that lists clients
shows all of them; narrowing a listing is a product decision, not this
fix. What changes is that the button on the row now refuses rather than
going through.
110 lines
4.3 KiB
PHP
110 lines
4.3 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
use App\Models\User;
|
|
use App\Modules\Audit\Action;
|
|
use App\Modules\Audit\ActivityLog;
|
|
use App\Modules\Groups\Models\Group;
|
|
use App\Modules\Identity\Models\Role;
|
|
use App\Modules\Identity\Models\RolePermission;
|
|
use App\Modules\Identity\Permissions\Permission;
|
|
use App\Modules\Identity\UserType;
|
|
use Illuminate\Support\Str;
|
|
|
|
/**
|
|
* Promoting a client to staff binds a client account, so it is inside
|
|
* the same boundary as every other route that binds one: the clients
|
|
* assigned to this staff member. guardToStaff() deliberately skips
|
|
* StaffAccounts::guardTarget, and the reason it gives is about the role
|
|
* being granted -- which leaves the target unasked about.
|
|
*/
|
|
beforeEach(function () {
|
|
$this->admin = User::factory()->create();
|
|
|
|
$this->role = Role::query()->create(['name' => 'Reps '.Str::random(6), 'client_scoped' => true]);
|
|
foreach ([Permission::ManageUsers, Permission::EditUsers, Permission::EditClients, Permission::CreateUsers] as $permission) {
|
|
RolePermission::query()->create(['role_id' => $this->role->id, 'permission' => $permission->value]);
|
|
}
|
|
|
|
$this->rep = User::factory()->create(['role_id' => $this->role->id]);
|
|
$this->mine = User::factory()->client()->create(['name' => 'Mine']);
|
|
$this->rep->assignedClients()->sync([$this->mine->id]);
|
|
|
|
$this->stranger = User::factory()->client()->create(['name' => 'Not Mine']);
|
|
});
|
|
|
|
test('a scoped staff member cannot promote a client outside their roster', function () {
|
|
$this->actingAs($this->rep)->post("/users/convert/{$this->stranger->id}", [
|
|
'direction' => 'to_staff',
|
|
'role_id' => $this->role->id,
|
|
'assigned_clients' => [],
|
|
])->assertNotFound();
|
|
|
|
$after = $this->stranger->fresh();
|
|
|
|
expect($after->type)->toBe(UserType::Client)
|
|
->and($after->role_id)->not->toBe($this->role->id);
|
|
});
|
|
|
|
test('everything a promotion would have done to a stranger client is left alone', function () {
|
|
$colleague = User::factory()->create(['role_id' => $this->role->id]);
|
|
$colleague->assignedClients()->sync([$this->stranger->id]);
|
|
|
|
$group = Group::query()->create(['name' => 'Theirs', 'slug' => 'theirs', 'public' => false]);
|
|
$group->members()->syncWithoutDetaching([$this->stranger->id]);
|
|
|
|
$this->actingAs($this->rep)->post("/users/convert/{$this->stranger->id}", [
|
|
'direction' => 'to_staff',
|
|
'role_id' => $this->role->id,
|
|
'assigned_clients' => [],
|
|
])->assertNotFound();
|
|
|
|
// A promotion clears every roster row pointing at the account and
|
|
// leaves its group memberships inert. Neither happened, and nothing
|
|
// was written to the log.
|
|
expect($colleague->assignedClients()->pluck('users.id')->all())->toBe([$this->stranger->id])
|
|
->and($group->members()->pluck('users.id')->all())->toBe([$this->stranger->id])
|
|
->and(ActivityLog::query()->where('action', Action::AccountConvertedToStaff->value)->count())->toBe(0);
|
|
});
|
|
|
|
test('the refusal does not distinguish a stranger client from one that is not there', function () {
|
|
$this->actingAs($this->rep)->post('/users/convert/999999', [
|
|
'direction' => 'to_staff',
|
|
'role_id' => $this->role->id,
|
|
])->assertNotFound();
|
|
});
|
|
|
|
test('a scoped staff member may still promote a client of their own', function () {
|
|
$this->actingAs($this->rep)->post("/users/convert/{$this->mine->id}", [
|
|
'direction' => 'to_staff',
|
|
'role_id' => $this->role->id,
|
|
'assigned_clients' => [],
|
|
])->assertSessionHasNoErrors();
|
|
|
|
$after = $this->mine->fresh();
|
|
|
|
expect($after->type)->toBe(UserType::Staff)
|
|
->and($after->role_id)->toBe($this->role->id);
|
|
});
|
|
|
|
test('unscoped staff promote any client, as before', function () {
|
|
$this->actingAs($this->admin)->post("/users/convert/{$this->stranger->id}", [
|
|
'direction' => 'to_staff',
|
|
'role_id' => $this->role->id,
|
|
'assigned_clients' => [],
|
|
])->assertSessionHasNoErrors();
|
|
|
|
expect($this->stranger->fresh()->type)->toBe(UserType::Staff);
|
|
});
|
|
|
|
test('the demotion direction keeps answering through guardTarget', function () {
|
|
$colleague = User::factory()->create();
|
|
|
|
$this->actingAs($this->rep)->post("/users/convert/{$colleague->id}", [
|
|
'direction' => 'to_client',
|
|
])->assertForbidden();
|
|
|
|
expect($colleague->fresh()->type)->toBe(UserType::Staff);
|
|
});
|