diff --git a/app/Modules/Clients/Http/Controllers/ClientsController.php b/app/Modules/Clients/Http/Controllers/ClientsController.php index 08743e22..a4b1fe48 100644 --- a/app/Modules/Clients/Http/Controllers/ClientsController.php +++ b/app/Modules/Clients/Http/Controllers/ClientsController.php @@ -98,7 +98,12 @@ class ClientsController extends Controller 'clients' => $clients->items(), 'pagination' => Pagination::meta($clients), 'filters' => $filters, - 'reassign_candidates' => $this->accountDeletion->candidates(), + // Only for somebody who may actually reassign: the picker is + // part of the delete dialog, and React filtering it out of the + // page is not the same as it never being on the page. + 'reassign_candidates' => $viewer->can('delete_clients') + ? $this->accountDeletion->candidates($viewer) + : [], // Null on a self-hosted install: no limit, nothing to say. 'seats' => $this->seats->clientState(), ]); @@ -214,7 +219,9 @@ class ClientsController extends Controller ->where('user_id', $client->id) ->pluck('value', 'client_custom_field_id'), 'content' => $this->accountContent->summarize($client), - 'reassign_candidates' => $this->accountDeletion->candidates($client->id), + 'reassign_candidates' => $request->user()?->can('delete_clients') === true + ? $this->accountDeletion->candidates($request->user(), $client->id) + : [], ]); } diff --git a/app/Modules/Identity/AccountContentDeletion.php b/app/Modules/Identity/AccountContentDeletion.php index 2ae7aaa1..7ca149be 100644 --- a/app/Modules/Identity/AccountContentDeletion.php +++ b/app/Modules/Identity/AccountContentDeletion.php @@ -7,6 +7,7 @@ namespace App\Modules\Identity; use App\Models\User; use App\Modules\Audit\Action; use App\Modules\Audit\ActivityLogger; +use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Files\DeletedAccountContent; use App\Modules\Identity\Models\Role; use Illuminate\Database\Eloquent\Builder; @@ -32,20 +33,39 @@ class AccountContentDeletion public function __construct( private readonly DeletedAccountContent $content, private readonly ActivityLogger $activity, + private readonly StaffLibraryScope $scope, ) {} /** - * Every other active account, for the reassignment-target picker. - * $excludeId is omitted on index pages, where one candidate list is - * shared across every row and each row's own id is filtered out - * client-side instead. + * Every other active account this viewer may be shown, for the + * reassignment-target picker. $excludeId is omitted on index pages, + * where one candidate list is shared across every row and each row's + * own id is filtered out client-side instead. + * + * The client half is narrowed by StaffLibraryScope, the same rule that + * narrows the list this picker sits next to: a client-scoped staff + * member is not shown the name of somebody they can reach nothing of, + * and a picker is no more a reason to hand one over than a listing is. + * Staff accounts are not narrowed anywhere in the application and are + * not narrowed here. + * + * An unscoped viewer's list is unchanged — StaffLibraryScope::clients() + * returns every client for them. + * + * $viewer is null only where the picker is about the installation + * rather than about a screen: the erasure default in privacy settings + * is stored once for everybody, behind edit_settings, so narrowing it + * by whoever happens to be editing would store the wrong answer. * * @return array */ - public function candidates(?int $excludeId = null): array + public function candidates(?User $viewer, ?int $excludeId = null): array { return User::query() ->when($excludeId, fn (Builder $query, int $id) => $query->whereKeyNot($id)) + ->when($viewer, fn (Builder $query, User $for) => $query->where(fn (Builder $reachable) => $reachable + ->where('type', UserType::Staff) + ->orWhereIn('id', $this->scope->clients($for)->select('users.id')))) ->where('active', true) ->with('role') ->orderBy('name') diff --git a/app/Modules/Identity/Http/Controllers/UsersController.php b/app/Modules/Identity/Http/Controllers/UsersController.php index 2e3e0a46..174bf3c8 100644 --- a/app/Modules/Identity/Http/Controllers/UsersController.php +++ b/app/Modules/Identity/Http/Controllers/UsersController.php @@ -108,7 +108,11 @@ class UsersController extends Controller 'filters' => $filters, 'roles' => Role::query()->orderBy('name')->get(['id', 'name']) ->map(fn (Role $role): array => ['id' => $role->id, 'name' => $role->name])->all(), - 'reassign_candidates' => $this->accountDeletion->candidates(), + // Same rule as the clients list: the picker belongs to the + // delete dialog, so it is sent to whoever may open one. + 'reassign_candidates' => $this->actor()->can('delete_users') + ? $this->accountDeletion->candidates($this->actor()) + : [], // Null on a self-hosted install: no limit, nothing to say. 'seats' => $this->seats->staffState(), ]); @@ -184,7 +188,9 @@ class UsersController extends Controller && $this->accounts->isAdministratorRole($user->role_id) && $this->accounts->activeAdministratorCount() === 1, 'content' => $this->accountContent->summarize($user), - 'reassign_candidates' => $this->accountDeletion->candidates($user->id), + 'reassign_candidates' => $this->actor()->can('delete_users') + ? $this->accountDeletion->candidates($this->actor(), $user->id) + : [], // Read-only, deliberately: an administrator may see that an // integration exists and what it is allowed to do, but only // the owner can rename, re-scope or revoke it. See ApiTokens. diff --git a/app/Modules/Platform/Http/Controllers/PrivacySettingsController.php b/app/Modules/Platform/Http/Controllers/PrivacySettingsController.php index e21fdfc5..acc4af57 100644 --- a/app/Modules/Platform/Http/Controllers/PrivacySettingsController.php +++ b/app/Modules/Platform/Http/Controllers/PrivacySettingsController.php @@ -37,7 +37,10 @@ class PrivacySettingsController extends Controller 'account_erasure_grace_days' => $this->settings->get(Setting::AccountErasureGraceDays), 'account_erasure_content_action' => $this->settings->get(Setting::AccountErasureContentAction), 'account_erasure_reassign_to' => $this->settings->get(Setting::AccountErasureReassignTo), - 'reassign_candidates' => $this->accountDeletion->candidates(), + // Installation-wide on purpose: this is the default every + // erasure will use, stored once for everybody, and the page is + // already behind edit_settings. + 'reassign_candidates' => $this->accountDeletion->candidates(null), 'api_request_log_retention_days' => $this->settings->get(Setting::ApiRequestLogRetentionDays), 'discourage_search_indexing' => $this->settings->get(Setting::DiscourageSearchIndexing), ]); diff --git a/tests/Feature/Identity/AccountContentDeletionTest.php b/tests/Feature/Identity/AccountContentDeletionTest.php index 6681e54f..a2ba43ed 100644 --- a/tests/Feature/Identity/AccountContentDeletionTest.php +++ b/tests/Feature/Identity/AccountContentDeletionTest.php @@ -4,6 +4,9 @@ declare(strict_types=1); use App\Models\User; use App\Modules\Files\Models\File; +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; @@ -116,3 +119,81 @@ test('both screens offer the same reassignment candidates', function () { ->and(collect($fromUsers)->pluck('name'))->not->toContain('Inactive One') ->and(collect($fromClients)->pluck('name'))->not->toContain('Inactive One'); }); + +/* +|-------------------------------------------------------------------------- +| Who the picker is sent to, and what it may name +|-------------------------------------------------------------------------- +| +| The candidate list is the delete dialog's picker: every active account in +| the installation, by name and role. It sits on index pages next to a +| listing that is deliberately narrowed, and it was narrowed by nothing. +| +*/ + +/** @return array{0: User, 1: User, 2: User} viewer, their client, a stranger's client */ +function scopedClientManager(array $permissions): array +{ + $role = Role::query()->create(['name' => 'Scoped rep '.count($permissions), 'client_scoped' => true]); + foreach ($permissions as $permission) { + RolePermission::query()->create(['role_id' => $role->id, 'permission' => $permission]); + } + + $viewer = User::factory()->create(['role_id' => $role->id, 'name' => 'The Rep']); + $mine = User::factory()->client()->create(['name' => 'Mine']); + $viewer->assignedClients()->attach($mine->id); + + $stranger = User::factory()->client()->create(['name' => 'Stranger Ltd']); + + return [$viewer, $mine, $stranger]; +} + +test('the picker names no client the viewer may not see', function () { + [$viewer] = scopedClientManager([ + Permission::ManageClients->value, + Permission::DeleteClients->value, + ]); + + $props = $this->actingAs($viewer)->get('/clients')->assertOk()->viewData('page')['props']; + + $names = collect($props['reassign_candidates'])->pluck('name'); + + // Two lines above it, the listing itself is narrowed to their roster. + expect(collect($props['clients'])->pluck('name')->all())->toBe(['Mine']) + ->and($names)->toContain('Mine') + ->and($names)->not->toContain('Stranger Ltd'); +}); + +test('the picker is not sent at all to somebody who may not delete', function () { + [$viewer] = scopedClientManager([Permission::ManageClients->value]); + + $props = $this->actingAs($viewer)->get('/clients')->assertOk()->viewData('page')['props']; + + expect($props['reassign_candidates'])->toBe([]); +}); + +test('the same holds for the staff list', function () { + $role = Role::query()->create(['name' => 'Staff lister']); + RolePermission::query()->insert([ + ['role_id' => $role->id, 'permission' => Permission::ManageUsers->value], + ]); + $viewer = User::factory()->create(['role_id' => $role->id]); + + $props = $this->actingAs($viewer)->get('/users')->assertOk()->viewData('page')['props']; + + expect($props['reassign_candidates'])->toBe([]); +}); + +test('an unscoped administrator still gets every active account', function () { + // The half that must not narrow: nothing changes for the role that + // actually runs these screens. + $client = User::factory()->client()->create(['name' => 'Acme Ltd']); + $staff = User::factory()->role(SystemRole::ClientManager)->create(['name' => 'Sam Staff']); + + $props = $this->actingAs($this->admin)->get('/clients')->assertOk()->viewData('page')['props']; + + expect(collect($props['reassign_candidates'])->pluck('name')) + ->toContain('Acme Ltd') + ->toContain('Sam Staff') + ->toContain($this->admin->name); +});