diff --git a/CHANGELOG.md b/CHANGELOG.md index a5290e6f..2bcd9acb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,28 @@ Anything under **Upgrade notes** is something you have to do, not something we d This section collects changes as they land; the release process turns it into a numbered entry when a version is cut. +**Closed holes in who can see what** + +- A staff member limited to their own assigned clients could read the names of other clients out of + file details. Sharing means a file can reach somebody through one client while it was uploaded by + another, or while it is also shared with another. That is normal and the file is theirs to open — + but the uploader's name, the other recipient's name, and both their ID numbers were being sent + along with it, on the library list, the file's edit page, the details panel, the per-client file + list, and the matching API responses. A group holding none of their clients was named the same + way. The file list could also be filtered by uploader, which answered "does this client of yours + share files with that client of mine" without naming anybody. + + Those names are now left out for a limited staff member, and the uploader filter no longer answers + for a client they are not assigned to. Administrators and any unrestricted role see exactly what + they saw before. + + **Who this affected.** Only installations using the Client Manager role, or a custom role with + "Limit to assigned clients" switched on, and only where files are shared with more than one client + or through groups. No files, downloads or credentials were reachable this way — a file belonging + to a client outside the roster was refused before, and still is. + + Reported by [@Noorkhalel](https://github.com/Noorkhalel) (GHSA-whmp-p9hv-r7j7). + ## 2.3.0 — 1 September 2026 If you run ProjectSend on Apache or LiteSpeed, this is the release to take. It installed fine on diff --git a/app/Modules/Files/Access/ClientIdentityScope.php b/app/Modules/Files/Access/ClientIdentityScope.php new file mode 100644 index 00000000..535b317f --- /dev/null +++ b/app/Modules/Files/Access/ClientIdentityScope.php @@ -0,0 +1,227 @@ +|null> + */ + private array $clientIds = []; + + /** @var array|null> */ + private array $groupIds = []; + + public function __construct(private readonly StaffLibraryScope $scope) {} + + /** + * Whether $viewer may be told that $subject exists, and what they are + * called. + * + * A null subject is permitted: there is no identity to leak, and every + * caller here is reading an optional relation. + */ + public function permits(?User $viewer, ?User $subject): bool + { + if ($subject === null) { + return true; + } + + if (! $subject->isClient()) { + return true; + } + + if ($viewer === null) { + return false; + } + + if ($viewer->is($subject)) { + return true; + } + + $ids = $this->identifiableClientIds($viewer); + + return $ids === null || in_array($subject->id, $ids, true); + } + + /** + * The same question about a client known only by id — used where a + * caller has a foreign key rather than a loaded model. + * + * An id that belongs to nobody, or to a staff member, is permitted: + * there is no client identity behind it to protect. + */ + public function permitsClientId(?User $viewer, ?int $id): bool + { + if ($id === null) { + return true; + } + + return $this->permits($viewer, User::query()->find($id)); + } + + /** + * Whether $viewer may be told a group exists. + * + * A group is a list of clients wearing one name, so naming one to + * somebody who may reach none of its members says the same thing + * naming a client would. The set is StaffLibraryScope's + * assignableGroupIds — every group holding at least one of the + * viewer's own clients. + */ + public function permitsGroupId(?User $viewer, ?int $id): bool + { + if ($id === null) { + return true; + } + + if ($viewer === null) { + return false; + } + + $ids = $this->identifiableGroupIds($viewer); + + return $ids === null || in_array($id, $ids, true); + } + + /** + * A client's name, or null when this viewer may not be told it. + * + * Null rather than a placeholder on purpose: every consumer of these + * fields already renders "no uploader recorded" for a null, because a + * deleted account leaves one behind. Inventing a "Hidden" string would + * be a new thing for sixteen locales to translate and would itself + * announce that there is somebody there to hide. + */ + public function nameOf(?User $viewer, ?User $subject): ?string + { + return $this->permits($viewer, $subject) ? $subject?->name : null; + } + + /** + * Drop the entries this viewer may not be told about from a list of + * id/name pairs describing clients. + * + * @param list $pairs + * @return list + */ + public function filterClientPairs(?User $viewer, array $pairs): array + { + if ($this->identifiableClientIds($viewer) === null) { + return $pairs; + } + + return array_values(array_filter( + $pairs, + fn (array $pair): bool => $this->permitsClientId($viewer, $pair['id']), + )); + } + + /** + * @param list $pairs + * @return list + */ + public function filterGroupPairs(?User $viewer, array $pairs): array + { + if ($this->identifiableGroupIds($viewer) === null) { + return $pairs; + } + + return array_values(array_filter( + $pairs, + fn (array $pair): bool => $this->permitsGroupId($viewer, $pair['id']), + )); + } + + /** + * Both halves of a `shares` payload at once, since the two lists are + * always filtered together. + * + * @param array{clients: list, groups: list} $shares + * @return array{clients: list, groups: list} + */ + public function filterShares(?User $viewer, array $shares): array + { + return [ + 'clients' => $this->filterClientPairs($viewer, $shares['clients']), + 'groups' => $this->filterGroupPairs($viewer, $shares['groups']), + ]; + } + + /** + * Whether this viewer is narrowed at all. Callers use it to skip + * per-row work for the common unscoped case. + */ + public function isNarrowed(?User $viewer): bool + { + return $viewer === null || $this->identifiableClientIds($viewer) !== null; + } + + /** + * @return list|null + */ + private function identifiableClientIds(?User $viewer): ?array + { + if ($viewer === null) { + return []; + } + + // Deliberately the same set as "who may I share with". A client on + // the roster is one this viewer already works with by name; a + // client off it is one they have no business knowing exists. + return $this->clientIds[$viewer->id] ??= $this->scope->assignableClientIds($viewer); + } + + /** + * @return list|null + */ + private function identifiableGroupIds(?User $viewer): ?array + { + if ($viewer === null) { + return []; + } + + return $this->groupIds[$viewer->id] ??= $this->scope->assignableGroupIds($viewer); + } +} diff --git a/app/Modules/Files/Access/ShareTargets.php b/app/Modules/Files/Access/ShareTargets.php index 9a616b52..767eb005 100644 --- a/app/Modules/Files/Access/ShareTargets.php +++ b/app/Modules/Files/Access/ShareTargets.php @@ -28,13 +28,25 @@ use Illuminate\Support\Collection; */ class ShareTargets { - public function __construct(private readonly StaffLibraryScope $scope) {} + public function __construct( + private readonly StaffLibraryScope $scope, + private readonly ClientIdentityScope $identity, + ) {} /** * The clients and groups a subject is already shared with, as id/name * pairs. Neutral keys, so callers can nest it ('shares' on the details * panel) or flatten it (the edit pages' assigned_* props). * + * **This is the unfiltered truth, and it is not what a screen should + * show.** Everyone a file is really in front of is the right answer for + * deciding something — VisibleCommentScope resolves notification + * recipients from it, and a recipient left out of that list is one who + * never hears about a message addressed to them. It is the wrong answer + * for telling somebody, because a client-scoped viewer may hold a file + * that is also shared with a client they have no business knowing + * exists. Anything rendering these names wants assignedFor() below. + * * @return array{clients: list, groups: list} */ public function assigned(File|Folder $subject): array @@ -47,6 +59,17 @@ class ShareTargets ]; } + /** + * assigned(), narrowed to the recipients this viewer may be told + * about. The display half of the pair — see the warning above. + * + * @return array{clients: list, groups: list} + */ + public function assignedFor(File|Folder $subject, ?User $viewer): array + { + return $this->identity->filterShares($viewer, $this->assigned($subject)); + } + /** * The assigned lists plus everything still available to share with, * narrowed to what this viewer is allowed to reach. @@ -76,7 +99,12 @@ class ShareTargets ->orderBy('name') ->get(); - $assigned = $this->assigned($subject); + // assignedFor, not assigned: an edit page listing a recipient this + // viewer may not identify would both name them and offer a control + // for a share the viewer cannot otherwise reach. available_* below + // was already narrowed this way; assigned_* was not, which is the + // asymmetry that made the whole panel a roster listing. + $assigned = $this->assignedFor($subject, $viewer); return [ 'assigned_clients' => $assigned['clients'], diff --git a/app/Modules/Files/FilesServiceProvider.php b/app/Modules/Files/FilesServiceProvider.php index 0087077a..c11bc02f 100644 --- a/app/Modules/Files/FilesServiceProvider.php +++ b/app/Modules/Files/FilesServiceProvider.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Modules\Files; +use App\Modules\Files\Access\ClientIdentityScope; use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Files\Models\File; use App\Modules\Files\Models\Folder; @@ -30,6 +31,10 @@ class FilesServiceProvider extends ServiceProvider // reached twice. Scoped rather than a singleton so a long-lived // queue worker starts each job with an empty memo. $this->app->scoped(StaffLibraryScope::class); + + // Same lifetime, same reason: the identity rule memoises a roster + // per viewer and the file listings ask it once per row. + $this->app->scoped(ClientIdentityScope::class); } public function boot(): void diff --git a/app/Modules/Files/Http/Controllers/Api/FilesController.php b/app/Modules/Files/Http/Controllers/Api/FilesController.php index bece639c..4db97c8a 100644 --- a/app/Modules/Files/Http/Controllers/Api/FilesController.php +++ b/app/Modules/Files/Http/Controllers/Api/FilesController.php @@ -12,6 +12,7 @@ use App\Modules\Audit\ActivityLogger; use App\Modules\Clients\ClientStorageUsage; use App\Modules\Comments\CommentingRules; use App\Modules\Comments\CommentScope; +use App\Modules\Files\Access\ClientIdentityScope; use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Files\Access\ViewableFileScope; use App\Modules\Files\DownloadLimitScope; @@ -62,6 +63,7 @@ class FilesController extends Controller private readonly ActivityLogger $activity, private readonly CommentingRules $commenting, private readonly StaffLibraryScope $scope, + private readonly ClientIdentityScope $identity, private readonly TimezoneRegistry $timezones, ) {} @@ -114,6 +116,17 @@ class FilesController extends Controller } if (array_key_exists('uploaded_by', $filters) && $filters['uploaded_by'] !== null) { + // A filter is a question, and this one asks "did client N put + // anything into my library". Answered plainly it is an oracle: + // a client-scoped caller could walk the id space and learn + // which clients off their roster share files with clients on + // it, without ever reading a name. So an id this caller may + // not identify matches nothing — indistinguishable from a + // client who has uploaded nothing, which is the point. + if (! $this->identity->permitsClientId($user, (int) $filters['uploaded_by'])) { + $query->whereRaw('1 = 0'); + } + $query->where('files.uploaded_by', $filters['uploaded_by']); } diff --git a/app/Modules/Files/Http/Controllers/ClientFilesController.php b/app/Modules/Files/Http/Controllers/ClientFilesController.php index 888f8be9..099a162b 100644 --- a/app/Modules/Files/Http/Controllers/ClientFilesController.php +++ b/app/Modules/Files/Http/Controllers/ClientFilesController.php @@ -6,6 +6,7 @@ namespace App\Modules\Files\Http\Controllers; use App\Http\Controllers\Controller; use App\Models\User; +use App\Modules\Files\Access\ClientIdentityScope; use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Files\Models\Category; use App\Modules\Files\Models\File; @@ -28,6 +29,7 @@ class ClientFilesController extends Controller { public function __construct( private readonly StaffLibraryScope $scope, + private readonly ClientIdentityScope $identity, ) {} public function index(Request $request, User $client): Response @@ -66,7 +68,11 @@ class ClientFilesController extends Controller 'size' => $file->size, 'created_at' => $file->created_at?->toIso8601String(), 'uploaded_by_client' => $file->uploaded_by === $client->id, - 'uploader' => $file->uploader?->name, + // Being allowed to browse this client's files does not + // extend to the other clients who shared files with them: + // a file reaches this listing through the client in the + // URL, and its uploader can be somebody else entirely. + 'uploader' => $this->identity->nameOf($viewer, $file->uploader), 'downloads_count' => $file->downloads_count, 'can_download' => Gate::forUser($viewer)->allows('view', $file), 'categories' => $file->categories->map(fn (Category $category): array => [ diff --git a/app/Modules/Files/Http/Controllers/FileDetailsController.php b/app/Modules/Files/Http/Controllers/FileDetailsController.php index 9f11431a..18aa8b0d 100644 --- a/app/Modules/Files/Http/Controllers/FileDetailsController.php +++ b/app/Modules/Files/Http/Controllers/FileDetailsController.php @@ -11,6 +11,7 @@ use App\Modules\Audit\ActivityLog; use App\Modules\Audit\ActivityPresenter; use App\Modules\Audit\DownloadPresenter; use App\Modules\Comments\CommentingRules; +use App\Modules\Files\Access\ClientIdentityScope; use App\Modules\Files\Access\DownloadAllowance; use App\Modules\Files\Access\ShareTargets; use App\Modules\Files\DownloadLimitScope; @@ -79,6 +80,7 @@ class FileDetailsController extends Controller private readonly ActivityPresenter $presenter, private readonly DownloadPresenter $downloadPresenter, private readonly ShareTargets $shareTargets, + private readonly ClientIdentityScope $identity, private readonly CommentingRules $commenting, private readonly FileVersionLinks $versionLinks, private readonly DownloadAllowance $allowance, @@ -100,7 +102,10 @@ class FileDetailsController extends Controller 'size' => $file->size, 'mime_type' => $file->mime_type, 'checksum' => $file->checksum, - 'uploader' => $file->uploader?->name, + // Null when the uploader is a client this viewer may not + // be told about, which reads the same as an uploader whose + // account has since been deleted. + 'uploader' => $this->identity->nameOf($viewer, $file->uploader), 'folder' => $file->folder?->only('id', 'name'), 'categories' => $file->categories()->orderBy('name')->get() ->map(fn (Category $category): array => ['id' => $category->id, 'name' => $category->name, 'color' => $category->color]) @@ -140,7 +145,7 @@ class FileDetailsController extends Controller // Resolved from the chain root for a revision (ShareTargets // does that), so this names who really has the file. The panel // says where those recipients are set. - 'shares' => $this->shareTargets->assigned($file), + 'shares' => $this->shareTargets->assignedFor($file, $viewer), 'sharing_root' => $file->isRevision() ? File::query()->find($file->sharingOwnerId())?->only('id', 'name') : null, @@ -368,7 +373,7 @@ class FileDetailsController extends Controller 'name' => $folder->name, 'files_count' => $folder->files()->count(), 'children_count' => $folder->children()->count(), - 'creator' => $folder->creator?->name, + 'creator' => $this->identity->nameOf($viewer, $folder->creator), 'created_at' => $folder->created_at?->toIso8601String(), 'open_url' => route('files.index', ['folder' => $folder->id], false), // Read-only here, same as a file's shares — sharing (and every @@ -377,7 +382,7 @@ class FileDetailsController extends Controller 'edit_url' => route('folders.share', $folder, false), 'can_update' => Gate::forUser($viewer)->allows('update', $folder), 'can_view_activity' => $viewer->can('view_actions_log'), - 'shares' => $this->shareTargets->assigned($folder), + 'shares' => $this->shareTargets->assignedFor($folder, $viewer), ]); } diff --git a/app/Modules/Files/Http/Controllers/FilesController.php b/app/Modules/Files/Http/Controllers/FilesController.php index 9abfa380..90127a94 100644 --- a/app/Modules/Files/Http/Controllers/FilesController.php +++ b/app/Modules/Files/Http/Controllers/FilesController.php @@ -10,6 +10,7 @@ use App\Modules\Audit\Action; use App\Modules\Audit\ActivityLogger; use App\Modules\Comments\CommentingRules; use App\Modules\Comments\CommentScope; +use App\Modules\Files\Access\ClientIdentityScope; use App\Modules\Files\Access\ShareTargets; use App\Modules\Files\Access\StaffLibraryScope; use App\Modules\Files\DownloadLimitScope; @@ -49,6 +50,7 @@ class FilesController extends Controller private readonly StaffLibraryScope $scope, private readonly PublicUrl $publicUrl, private readonly ShareTargets $shareTargets, + private readonly ClientIdentityScope $identity, private readonly CommentingRules $commenting, private readonly FileVersions $versions, private readonly FileVersionLinks $versionLinks, @@ -164,7 +166,7 @@ class FilesController extends Controller 'original_name' => $file->original_name, 'size' => $file->size, 'mime_type' => $file->mime_type, - 'uploader' => $file->uploader?->name, + 'uploader' => $this->identity->nameOf($viewer, $file->uploader), 'folder_id' => $file->folder_id, 'public' => $file->public, 'commentable' => $file->commentable, diff --git a/app/Modules/Files/Http/Controllers/FoldersController.php b/app/Modules/Files/Http/Controllers/FoldersController.php index 262ea306..815d4047 100644 --- a/app/Modules/Files/Http/Controllers/FoldersController.php +++ b/app/Modules/Files/Http/Controllers/FoldersController.php @@ -10,6 +10,7 @@ use App\Modules\Audit\Action; use App\Modules\Audit\ActivityLogger; use App\Modules\Comments\Access\VisibleCommentScope; use App\Modules\Comments\CommentingRules; +use App\Modules\Files\Access\ClientIdentityScope; use App\Modules\Files\Access\DownloadAllowance; use App\Modules\Files\Access\ShareTargets; use App\Modules\Files\Access\StaffLibraryScope; @@ -54,6 +55,7 @@ class FoldersController extends Controller private readonly ActivityLogger $activity, private readonly PublicUrl $publicUrl, private readonly ShareTargets $shareTargets, + private readonly ClientIdentityScope $identity, private readonly BreadcrumbBuilder $breadcrumbs, private readonly CommentingRules $commenting, private readonly VisibleCommentScope $comments, @@ -240,7 +242,11 @@ class FoldersController extends Controller 'original_name' => $file->original_name, 'mime_type' => $file->mime_type, 'size' => $file->size, - 'uploader' => $file->uploader ? [ + // The whole block goes, not just the name: type and role + // describe the same person, and "a client uploaded this" on a + // row whose uploader is off this viewer's roster narrows who + // it could be just as effectively as naming them. + 'uploader' => ($file->uploader !== null && $this->identity->permits($user, $file->uploader)) ? [ 'name' => $file->uploader->name, 'type' => $file->uploader->type->value, 'role' => $file->uploader->role?->name, diff --git a/app/Modules/Files/Http/Resources/Api/FileResource.php b/app/Modules/Files/Http/Resources/Api/FileResource.php index 5dafe710..2c3fdd8f 100644 --- a/app/Modules/Files/Http/Resources/Api/FileResource.php +++ b/app/Modules/Files/Http/Resources/Api/FileResource.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Modules\Files\Http\Resources\Api; +use App\Modules\Files\Access\ClientIdentityScope; use App\Modules\Files\DownloadLimitScope; use App\Modules\Files\Models\File; use App\Modules\Files\Models\FileAssignment; @@ -26,6 +27,23 @@ use Illuminate\Http\Resources\Json\JsonResource; * - `checksum` is included deliberately, since verifying an integration's * own download is a real use case, and it reveals nothing about * location. + * + * Two fields are narrowed to the caller: the uploader and the assignment + * list both name clients, and a client-scoped account may hold a file whose + * uploader or co-recipients are clients off their own roster — the file is + * theirs to read, those names are not theirs to see. ClientIdentityScope is + * the rule; a name dropped here is dropped to null or out of the list, and + * an unscoped account is unaffected. + * + * That narrowing happens here rather than in the controllers, which is the opposite of how the version counterparts are + * handled a few files over — and deliberately so. Whether a counterpart may + * be named is a set-shaped question with a query to express it, so it is + * asked once in the caller's eager load. Whether a client may be named is a + * per-row check against the viewer's roster with no query to fold it into, + * and this resource is built at eight call sites across four controllers, + * two of them re-loading `assignments.assignable` after a write. Asking at + * the point of serialisation is the only version of this rule that cannot + * be forgotten by the ninth caller. */ class FileResource extends JsonResource { @@ -34,6 +52,15 @@ class FileResource extends JsonResource */ public function toArray(Request $request): array { + $viewer = $request->user(); + $identity = app(ClientIdentityScope::class); + + // The morph class rather than ::class, matching ShareTargets: with + // a morph map registered the two disagree, and this line now + // decides which roster an entry is checked against, so getting it + // wrong would mean checking a group id against the client list. + $groupMorph = (new Group)->getMorphClass(); + return [ 'id' => $this->id, 'name' => $this->name, @@ -96,11 +123,16 @@ class FileResource extends JsonResource ]), // Name only. The uploader is a user record; their email address - // is not part of what "this file exists" needs to say. - 'uploaded_by' => $this->whenLoaded('uploader', fn (): ?array => $this->uploader === null ? null : [ - 'id' => $this->uploader->id, - 'name' => $this->uploader->name, - ]), + // is not part of what "this file exists" needs to say. Null + // when the uploader is a client the token's owner is not + // scoped to; an unscoped account always gets the name. + 'uploaded_by' => $this->whenLoaded( + 'uploader', + fn (): ?array => $identity->permits($viewer, $this->uploader) && $this->uploader !== null ? [ + 'id' => $this->uploader->id, + 'name' => $this->uploader->name, + ] : null, + ), 'categories' => $this->whenLoaded('categories', fn (): array => $this->categories ->map(fn ($category): array => [ @@ -109,15 +141,22 @@ class FileResource extends JsonResource ]) ->all()), + // Who the file is shared with, as far as this caller is + // concerned: a recipient the token's owner is not scoped to is + // left out rather than returned without a name. 'assignments' => $this->whenLoaded('assignments', fn (): array => $this->assignments + ->filter(fn (FileAssignment $assignment): bool => $assignment->assignable_type === $groupMorph + ? $identity->permitsGroupId($viewer, (int) $assignment->assignable_id) + : $identity->permitsClientId($viewer, (int) $assignment->assignable_id)) ->map(fn (FileAssignment $assignment): array => [ - 'type' => $assignment->assignable_type === Group::class ? 'group' : 'client', + 'type' => $assignment->assignable_type === $groupMorph ? 'group' : 'client', 'id' => $assignment->assignable_id, // getAttribute() rather than ->name: the relation is a // MorphTo over User|Group, so the property is only // knowable at runtime. Both targets carry a name. 'name' => $assignment->assignable?->getAttribute('name'), ]) + ->values() ->all()), 'links' => [ diff --git a/docs/api/openapi.json b/docs/api/openapi.json index 0ae74a9d..2bffd2b1 100644 --- a/docs/api/openapi.json +++ b/docs/api/openapi.json @@ -4075,7 +4075,7 @@ "object", "null" ], - "description": "Name only. The uploader is a user record; their email address\nis not part of what \"this file exists\" needs to say.", + "description": "Name only. The uploader is a user record; their email address\nis not part of what \"this file exists\" needs to say. Null\nwhen the uploader is a client the token's owner is not\nscoped to; an unscoped account always gets the name.", "properties": { "id": { "type": "integer" @@ -4109,6 +4109,7 @@ }, "assignments": { "type": "array", + "description": "Who the file is shared with, as far as this caller is\nconcerned: a recipient the token's owner is not scoped to is\nleft out rather than returned without a name.", "items": { "type": "object", "properties": { diff --git a/tests/Feature/Files/ClientIdentityScopeTest.php b/tests/Feature/Files/ClientIdentityScopeTest.php new file mode 100644 index 00000000..0b2c00d9 --- /dev/null +++ b/tests/Feature/Files/ClientIdentityScopeTest.php @@ -0,0 +1,291 @@ +admin = User::factory()->create(); + $this->onRoster = User::factory()->client()->create(['name' => 'Roster Client']); + $this->offRoster = User::factory()->client()->create(['name' => 'Offroster Client']); + + $this->manager = User::factory()->role(SystemRole::ClientManager)->create(); + $this->manager->assignedClients()->sync([$this->onRoster->id]); + + $this->token = $this->manager->createToken('t', [Permission::Upload->value])->plainTextToken; +}); + +/** A file the manager may read, uploaded by a client they may not identify. */ +function fileFromStranger(): File +{ + $file = File::factory()->create([ + 'uploaded_by' => test()->offRoster->id, + 'name' => 'shared-onward', + ]); + shareFileWith($file, test()->onRoster); + + return $file; +} + +/** + * A client-scoped staff member holding wider permissions than the built-in + * Client Manager — the roles that can open a colleague's file for editing + * and browse a client's own library. Scoping and permissions are separate + * axes, and the leak is a property of the scoping. + * + * @param list $permissions + */ +function scopedStaffWith(array $permissions): User +{ + $role = Role::query()->create(['name' => 'Scoped '.uniqid(), 'client_scoped' => true]); + + foreach ($permissions as $permission) { + RolePermission::query()->create(['role_id' => $role->id, 'permission' => $permission]); + } + + $staff = User::factory()->create(['role_id' => $role->id]); + $staff->assignedClients()->sync([test()->onRoster->id]); + + return $staff; +} + +/** A file the manager may read that is also shared with a stranger client. */ +function fileWithStrangerCoRecipient(): File +{ + $file = File::factory()->create(['uploaded_by' => test()->admin->id, 'name' => 'shared-both']); + shareFileWith($file, test()->onRoster); + shareFileWith($file, test()->offRoster); + + return $file; +} + +test('the file boundary itself is unchanged: a stranger-only file is still refused', function () { + $file = File::factory()->create(['uploaded_by' => $this->offRoster->id]); + shareFileWith($file, $this->offRoster); + + $this->withToken($this->token)->getJson("/api/v1/files/{$file->id}")->assertForbidden(); +}); + +test('the file boundary itself is unchanged on the web', function () { + $file = File::factory()->create(['uploaded_by' => $this->offRoster->id]); + shareFileWith($file, $this->offRoster); + + $this->actingAs($this->manager)->get("/files/{$file->id}/details")->assertForbidden(); +}); + +test('the API does not name a stranger uploader of a file the caller may read', function () { + $file = fileFromStranger(); + + $show = $this->withToken($this->token)->getJson("/api/v1/files/{$file->id}")->assertOk(); + $index = $this->withToken($this->token)->getJson('/api/v1/files')->assertOk(); + + expect($show->getContent())->not->toContain('Offroster Client') + ->and($index->getContent())->not->toContain('Offroster Client') + ->and($show->json('data.uploaded_by'))->toBeNull() + // The file is still readable — this narrows the answer, it does + // not withdraw it. + ->and($show->json('data.id'))->toBe($file->id); +}); + +test('the API does not list a stranger co-recipient', function () { + $file = fileWithStrangerCoRecipient(); + + $show = $this->withToken($this->token)->getJson("/api/v1/files/{$file->id}")->assertOk(); + + expect($show->getContent())->not->toContain('Offroster Client') + ->and($show->json('data.assignments'))->toHaveCount(1) + ->and($show->json('data.assignments.0.name'))->toBe('Roster Client'); +}); + +test('a stranger co-recipient is not named in the reply to a write', function () { + $file = fileWithStrangerCoRecipient(); + + // The assignment endpoints re-load assignments.assignable and hand the + // result straight back, which is a second serialisation path — and one + // that a fix applied only to the read controllers would have missed. + // The 200 is asserted on purpose: without it this passes on a 403, + // whose body names nobody either. + $writer = scopedStaffWith([Permission::Upload->value, Permission::EditFiles->value, Permission::EditOthersFiles->value]); + $token = $writer->createToken('w', [Permission::EditFiles->value, Permission::EditOthersFiles->value])->plainTextToken; + + $response = $this->withToken($token) + ->postJson("/api/v1/files/{$file->id}/assignments", ['type' => 'client', 'id' => $this->onRoster->id]) + ->assertOk(); + + expect($response->getContent())->not->toContain('Offroster Client') + ->and($response->json('data.assignments'))->toHaveCount(1); +}); + +test('uploaded_by cannot be used to probe for a client the caller may not identify', function () { + fileFromStranger(); + + $probe = $this->withToken($this->token)->getJson("/api/v1/files?uploaded_by={$this->offRoster->id}")->assertOk(); + + expect($probe->json('data'))->toBeEmpty(); +}); + +test('uploaded_by still filters by a client on the roster', function () { + $own = File::factory()->create(['uploaded_by' => $this->onRoster->id, 'name' => 'theirs']); + shareFileWith($own, $this->onRoster); + fileFromStranger(); + + $hit = $this->withToken($this->token)->getJson("/api/v1/files?uploaded_by={$this->onRoster->id}")->assertOk(); + + expect($hit->json('data'))->toHaveCount(1) + ->and($hit->json('data.0.id'))->toBe($own->id); +}); + +test('uploaded_by still filters by a staff member', function () { + $file = fileWithStrangerCoRecipient(); + + $hit = $this->withToken($this->token)->getJson("/api/v1/files?uploaded_by={$this->admin->id}")->assertOk(); + + expect($hit->json('data'))->toHaveCount(1) + ->and($hit->json('data.0.id'))->toBe($file->id); +}); + +test('the details panel names neither a stranger uploader nor a stranger recipient', function () { + $stranger = fileFromStranger(); + $both = fileWithStrangerCoRecipient(); + + $one = $this->actingAs($this->manager)->get("/files/{$stranger->id}/details")->assertOk(); + $two = $this->actingAs($this->manager)->get("/files/{$both->id}/details")->assertOk(); + + expect($one->getContent())->not->toContain('Offroster Client') + ->and($one->json('uploader'))->toBeNull() + ->and($two->getContent())->not->toContain('Offroster Client') + ->and($two->json('shares.clients'))->toHaveCount(1); +}); + +test('the library listing does not describe a stranger uploader', function () { + fileFromStranger(); + + $body = $this->actingAs($this->manager)->get('/files')->assertOk()->getContent(); + + // Not the name, and not the "a client uploaded this" shape either. + expect($body)->not->toContain('Offroster Client'); +}); + +test('the edit page does not name a stranger uploader or recipient', function () { + $file = fileWithStrangerCoRecipient(); + $file->update(['uploaded_by' => $this->offRoster->id]); + + $editor = scopedStaffWith([Permission::Upload->value, Permission::EditFiles->value, Permission::EditOthersFiles->value]); + + // route() rather than a built path: File binds by slug, so an id in + // the URL is a 404 rather than the page under test. + $body = $this->actingAs($editor)->get(route('files.edit', $file))->assertOk()->getContent(); + + expect($body)->not->toContain('Offroster Client'); +}); + +test('the per-client file listing does not name a stranger uploader', function () { + fileFromStranger(); + + $browser = scopedStaffWith([Permission::Upload->value, Permission::EditClients->value]); + + $body = $this->actingAs($browser) + ->get("/clients/{$this->onRoster->id}/files")->assertOk()->getContent(); + + expect($body)->not->toContain('Offroster Client'); +}); + +test('a group holding none of the viewer clients is not named', function () { + $group = Group::query()->create(['name' => 'Offroster Group']); + $group->members()->sync([$this->offRoster->id]); + + $file = File::factory()->create(['uploaded_by' => $this->admin->id]); + shareFileWith($file, $this->onRoster); + shareFileWithGroup($file, $group); + + $api = $this->withToken($this->token)->getJson("/api/v1/files/{$file->id}")->assertOk(); + + expect($api->getContent())->not->toContain('Offroster Group') + ->and($api->json('data.assignments'))->toHaveCount(1); +}); + +test('a group holding none of the viewer clients is not named on the web', function () { + $group = Group::query()->create(['name' => 'Offroster Group']); + $group->members()->sync([$this->offRoster->id]); + + $file = File::factory()->create(['uploaded_by' => $this->admin->id]); + shareFileWith($file, $this->onRoster); + shareFileWithGroup($file, $group); + + $details = $this->actingAs($this->manager)->get("/files/{$file->id}/details")->assertOk(); + + expect($details->getContent())->not->toContain('Offroster Group') + ->and($details->json('shares.groups'))->toBeEmpty(); +}); + +test('an unscoped administrator still sees every name', function () { + $file = fileWithStrangerCoRecipient(); + $file->update(['uploaded_by' => $this->offRoster->id]); + + $token = $this->admin->createToken('a', [Permission::Upload->value])->plainTextToken; + + $api = $this->withToken($token)->getJson("/api/v1/files/{$file->id}")->assertOk(); + + expect($api->json('data.uploaded_by.name'))->toBe('Offroster Client') + ->and($api->json('data.assignments'))->toHaveCount(2); +}); + +test('an unscoped administrator still sees every name on the web', function () { + $file = fileWithStrangerCoRecipient(); + $file->update(['uploaded_by' => $this->offRoster->id]); + + $details = $this->actingAs($this->admin)->get("/files/{$file->id}/details")->assertOk(); + + expect($details->json('uploader'))->toBe('Offroster Client') + ->and($details->json('shares.clients'))->toHaveCount(2); +}); + +test('a scoped viewer is still told about their own roster and their own uploads', function () { + $mine = File::factory()->create(['uploaded_by' => $this->manager->id, 'name' => 'mine']); + shareFileWith($mine, $this->onRoster); + + $api = $this->withToken($this->token)->getJson("/api/v1/files/{$mine->id}")->assertOk(); + + expect($api->json('data.uploaded_by.name'))->toBe($this->manager->name) + ->and($api->json('data.assignments.0.name'))->toBe('Roster Client'); +}); + +test('the rule itself: staff are never hidden, strangers always are', function () { + $identity = app(ClientIdentityScope::class); + + expect($identity->permits($this->manager, $this->admin))->toBeTrue() + ->and($identity->permits($this->manager, $this->onRoster))->toBeTrue() + ->and($identity->permits($this->manager, $this->offRoster))->toBeFalse() + // A client may always be told who they themselves are. + ->and($identity->permits($this->offRoster, $this->offRoster))->toBeTrue() + // An unscoped viewer is narrowed by nothing. + ->and($identity->permits($this->admin, $this->offRoster))->toBeTrue() + ->and($identity->isNarrowed($this->admin))->toBeFalse() + ->and($identity->isNarrowed($this->manager))->toBeTrue() + // No viewer at all is the closed case, not the open one. + ->and($identity->permits(null, $this->offRoster))->toBeFalse() + ->and($identity->permits(null, $this->admin))->toBeTrue(); +});