Narrow the reassignment picker to what a viewer may see

`reassign_candidates` is the delete dialog's picker: every active account
in the installation, by name and by role label. The same list is shared
on the clients index, the users index, both edit screens and privacy
settings, and it was narrowed by nothing.

Two lines above it on the clients index sits the listing itself, narrowed
through `scope->clients($viewer)` with a comment saying why: "a
client-scoped staff member is not shown the name and email of somebody
they can reach nothing of". The picker beside it handed over every client
in the installation, plus every staff account and its role name. The
filter by `can('delete_clients')` happens in React, which decides what is
rendered, not what is sent.

So the client half of the candidate list goes through the same
StaffLibraryScope as the listing, and each screen sends the picker only to
a viewer holding the delete permission it exists for. Staff accounts are
not narrowed -- they are not narrowed anywhere else either -- and an
unscoped viewer's list is unchanged, because StaffLibraryScope::clients()
returns every client for them.

Privacy settings keeps the whole installation on purpose: that picker sets
the erasure default stored once for everybody, behind edit_settings, so
narrowing it by whoever happens to be editing would store the wrong
answer. The parameter is nullable for that one caller, and the docblock
says so.

Four tests. Without the fix three go red; the fourth is the guard that an
administrator still sees every active account.

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)
+                : [],
         ]);
     }
 

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<int, array{id: int, name: string, role: string}>
      */
-    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')

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.

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),
         ]);

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);
+});