diff --git a/CHANGELOG.md b/CHANGELOG.md index bee7b17e..116a94f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,18 @@ when a version is cut. **Fixed** +- **A staff member limited to some clients can no longer see or change other people's groups.** The + groups list showed every group on the installation — name, description and member count — whatever + the viewer's roster, and a group that nothing had been shared with yet could be renamed, deleted or + made public by somebody with no relationship to any of its members. Making one public is the part + that mattered: whatever is shared with the group afterwards becomes reachable without signing in. + + *Who this affected:* only installations using a role with client scoping turned on. If every staff + role on your installation sees all clients, nothing changed for you. Groups holding at least one of + a scoped viewer's own clients stay visible and editable to them, exactly as before; groups holding + none of them are now hidden and refused. The API behaves the same way as the screens do. + + Reported by [@Drescargot](https://github.com/Drescargot). - **A public gallery no longer renders the same thumbnail several times at once.** The first visit to a page of large images started one full-size decode per thumbnail in parallel, which on a memory-limited server could exhaust it — and because a decode that dies writes nothing, the page diff --git a/app/Modules/Files/Access/StaffLibraryScope.php b/app/Modules/Files/Access/StaffLibraryScope.php index 46e1df47..c32a6b4e 100644 --- a/app/Modules/Files/Access/StaffLibraryScope.php +++ b/app/Modules/Files/Access/StaffLibraryScope.php @@ -159,15 +159,37 @@ class StaffLibraryScope return null; } - $clientIds = $this->assignableClientIds($user) ?? []; + return array_values($this->groups($user)->pluck('id')->map(fn ($id): int => (int) $id)->all()); + } - if ($clientIds === []) { - return []; + /** + * Every group this staff member may be told about, as a query. + * + * The listing half of assignableGroupIds(), and the same rule: a + * group counts as theirs because one of their clients is in it. The + * two were not the same code, and the listing simply had none — so + * `/groups` and `/api/v1/groups` showed a scoped staff member every + * group on the installation, name, description and member count, + * including groups whose every member was somebody else's client + * (GHSA-r3hg-3fxw-rcmr). + * + * Deliberately the *sharing* rule rather than the change rule below. + * A scoped staff member may already share a file with a mixed group, + * so its existence is not news to them; what they may not do is + * rename, publish or delete it. + * + * @return Builder + */ + public function groups(User $user): Builder + { + $query = Group::query(); + $clientIds = $this->assignableClientIds($user); + + if ($clientIds === null) { + return $query; } - return array_values(Group::query() - ->whereHas('members', fn (Builder $members) => $members->whereIn('users.id', $clientIds)) - ->pluck('id')->map(fn ($id): int => (int) $id)->all()); + return $query->whereHas('members', fn (Builder $members) => $members->whereIn('users.id', $clientIds)); } public function canAssignClient(User $user, User $client): bool @@ -249,7 +271,53 @@ class StaffLibraryScope */ public function allowsGroupChange(User $user, Group $group): bool { - return $this->groupReachesNoFurther($user, $group); + return $this->groupIsNotWhollySomebodyElses($user, $group) + && $this->groupReachesNoFurther($user, $group); + } + + /** + * Whether this group is somebody else's entirely — every member + * outside the staff member's roster, and none of theirs in it. + * + * The half allowsGroupChange() was missing. Reach answers "what would + * this group hand somebody", which is the right question for putting a + * client *into* it; it says nothing about who is already there. So a + * group with nothing shared with it yet passed the reach check + * vacuously, and a scoped staff member could rename it, delete it, or + * publish it — a group made entirely of clients they had never been + * assigned (GHSA-r3hg-3fxw-rcmr). + * + * **Not "every member is mine", which is the obvious reading and is + * wrong.** A mixed group has to stay changeable: GHSA-whmp-p9hv-r7j7 + * settled that a scoped staff member opens such a group's edit screen + * and is shown only their own clients in it, rather than being refused + * the screen. Requiring every member to be theirs turns that narrowing + * back into a 404 and undoes the earlier fix. What is left over — a + * mixed group whose shared content reaches past their library — is + * refused by groupReachesNoFurther() beside this, which is the check + * that has always covered it. + * + * **And deliberately not folded into groupReachesNoFurther() either.** + * That predicate is shared with allowsGroupMembership(), where a group + * nobody has joined must stay usable so its creator can put the first + * member in — the case that method's own docblock calls out. + * + * An empty group is nobody else's, so whoever just made it can still + * name it. + */ + private function groupIsNotWhollySomebodyElses(User $user, Group $group): bool + { + $clientIds = $this->assignableClientIds($user); + + if ($clientIds === null) { + return true; + } + + if (! $group->members()->exists()) { + return true; + } + + return $group->members()->whereIn('users.id', $clientIds)->exists(); } /** diff --git a/app/Modules/Groups/Http/Controllers/Api/GroupsController.php b/app/Modules/Groups/Http/Controllers/Api/GroupsController.php index 036c5783..b8d39127 100644 --- a/app/Modules/Groups/Http/Controllers/Api/GroupsController.php +++ b/app/Modules/Groups/Http/Controllers/Api/GroupsController.php @@ -41,7 +41,13 @@ class GroupsController extends Controller 'visibility' => ['nullable', Rule::in(['public', 'private'])], ]); - $query = Group::query()->withCount('members'); + $viewer = $request->user(); + assert($viewer !== null); + + // The API twin of the web listing's narrowing, and it has to be + // here rather than only there: the same disclosure through a token + // is the same disclosure (GHSA-r3hg-3fxw-rcmr). + $query = $this->scope->groups($viewer)->withCount('members'); if (($filters['search'] ?? null) !== null) { $search = $filters['search']; diff --git a/app/Modules/Groups/Http/Controllers/GroupsController.php b/app/Modules/Groups/Http/Controllers/GroupsController.php index 6f3b837a..e73b7be7 100644 --- a/app/Modules/Groups/Http/Controllers/GroupsController.php +++ b/app/Modules/Groups/Http/Controllers/GroupsController.php @@ -40,7 +40,14 @@ class GroupsController extends Controller 'visibility' => $validated['visibility'] ?? null, ]; - $groups = Group::query() + $viewer = $request->user(); + assert($viewer !== null); + + // Scoped, not Group::query(): a client-scoped staff member is told + // about a group because one of their clients is in it. Without + // this the listing showed every group on the installation, to a + // viewer who could reach nothing of theirs (GHSA-r3hg-3fxw-rcmr). + $groups = $this->scope->groups($viewer) ->withCount('members') ->when($filters['search'], fn (Builder $query, string $search) => $query->where(fn (Builder $q) => $q ->where('name', 'like', "%{$search}%") diff --git a/tests/Feature/Groups/GroupScopeDisclosureTest.php b/tests/Feature/Groups/GroupScopeDisclosureTest.php new file mode 100644 index 00000000..c78e5727 --- /dev/null +++ b/tests/Feature/Groups/GroupScopeDisclosureTest.php @@ -0,0 +1,203 @@ +admin = User::factory()->create(); + + $role = Role::query()->create(['name' => 'Reps '.Str::random(6), 'client_scoped' => true]); + foreach ([Permission::ManageGroups, Permission::EditGroups, Permission::DeleteGroups, Permission::CreateGroups] as $permission) { + RolePermission::query()->create(['role_id' => $role->id, 'permission' => $permission->value]); + } + + $this->rep = User::factory()->create(['role_id' => $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']); + + // The group at the centre of the report: somebody else's client is its + // only member, and nothing has been shared with it yet — so every + // "does this group reach past my library" check answers no, vacuously. + $this->theirs = Group::query()->create(['name' => 'Theirs Only', 'slug' => 'theirs-only', 'public' => false]); + $this->theirs->members()->syncWithoutDetaching([$this->stranger->id]); + + $this->ours = Group::query()->create(['name' => 'Ours', 'slug' => 'ours', 'public' => false]); + $this->ours->members()->syncWithoutDetaching([$this->mine->id]); +}); + +function listedGroupNames(User $viewer): array +{ + $names = []; + + test()->actingAs($viewer)->get('/groups')->assertOk()->assertInertia( + function (AssertableInertia $page) use (&$names) { + $names = collect($page->toArray()['props']['groups']['data'] ?? $page->toArray()['props']['groups']) + ->pluck('name')->all(); + }, + ); + + return $names; +} + +/* +|-------------------------------------------------------------------------- +| Being told it exists +|-------------------------------------------------------------------------- +*/ + +test('the listing hides a group made entirely of other people\'s clients', function () { + expect(listedGroupNames($this->rep))->not->toContain('Theirs Only'); +}); + +test('the listing still shows a group holding one of their own clients', function () { + expect(listedGroupNames($this->rep))->toContain('Ours'); +}); + +test('an unscoped administrator still sees every group', function () { + expect(listedGroupNames($this->admin))->toContain('Theirs Only')->toContain('Ours'); +}); + +test('the API listing hides it too', function () { + Sanctum::actingAs($this->rep, ['manage_groups']); + + $names = collect($this->getJson('/api/v1/groups')->assertOk()->json('data'))->pluck('name')->all(); + + expect($names)->not->toContain('Theirs Only')->toContain('Ours'); +}); + +/* +|-------------------------------------------------------------------------- +| Changing it +|-------------------------------------------------------------------------- +| +| The higher half of the report. Renaming, deleting or publishing a group +| lands on every member, and none of this group's members are theirs. +*/ + +test('they cannot open the edit form for it', function () { + $this->actingAs($this->rep)->get("/groups/{$this->theirs->id}")->assertNotFound(); +}); + +test('they cannot rename it', function () { + $this->actingAs($this->rep) + ->patch("/groups/{$this->theirs->id}", ['name' => 'Renamed', 'public' => false]) + ->assertNotFound(); + + expect($this->theirs->fresh()->name)->toBe('Theirs Only'); +}); + +test('they cannot publish it', function () { + // The consequence the report calls the serious one: a public group + // becomes an anonymous listing for whatever is shared with it later. + $this->actingAs($this->rep) + ->patch("/groups/{$this->theirs->id}", ['name' => 'Theirs Only', 'public' => true]) + ->assertNotFound(); + + expect($this->theirs->fresh()->public)->toBeFalse(); +}); + +test('they cannot delete it out from under its members', function () { + $this->actingAs($this->rep)->delete("/groups/{$this->theirs->id}")->assertNotFound(); + + expect(Group::query()->whereKey($this->theirs->id)->exists())->toBeTrue(); +}); + +test('the API refuses the same three', function () { + Sanctum::actingAs($this->rep, ['edit_groups', 'delete_groups']); + + $this->getJson("/api/v1/groups/{$this->theirs->id}")->assertNotFound(); + $this->patchJson("/api/v1/groups/{$this->theirs->id}", ['name' => 'Renamed'])->assertNotFound(); + $this->deleteJson("/api/v1/groups/{$this->theirs->id}")->assertNotFound(); + + expect($this->theirs->fresh()->name)->toBe('Theirs Only'); +}); + +/* +|-------------------------------------------------------------------------- +| What must keep working +|-------------------------------------------------------------------------- +| +| The pins. The fix suggested in the report puts the membership check +| inside groupReachesNoFurther(), which allowsGroupMembership() also +| calls — and that would have broken both of these. +*/ + +test('they can still rename a group of their own client', function () { + $this->actingAs($this->rep) + ->patch("/groups/{$this->ours->id}", ['name' => 'Ours Renamed', 'public' => false]) + ->assertRedirect(); + + expect($this->ours->fresh()->name)->toBe('Ours Renamed'); +}); + +test('they can still put the first member into a group they just made', function () { + // A group nobody has joined belongs to nobody, and this is the case + // StaffLibraryScope's own docblock says must keep working. + $fresh = Group::query()->create(['name' => 'Brand New', 'slug' => 'brand-new', 'public' => false]); + + $this->actingAs($this->rep) + ->post("/groups/{$fresh->id}/members", ['user_id' => $this->mine->id]) + ->assertRedirect(); + + expect($fresh->members()->pluck('users.id')->all())->toContain($this->mine->id); +}); + +test('they can still rename an empty group', function () { + $fresh = Group::query()->create(['name' => 'Brand New', 'slug' => 'brand-new', 'public' => false]); + + $this->actingAs($this->rep) + ->patch("/groups/{$fresh->id}", ['name' => 'Named At Last', 'public' => false]) + ->assertRedirect(); + + expect($fresh->fresh()->name)->toBe('Named At Last'); +}); + +test('a mixed group stays changeable, with its stranger unnamed', function () { + // The boundary between this report and GHSA-whmp-p9hv-r7j7. "Every + // member must be mine" is the obvious reading of the fix and it is + // wrong: it turns that advisory's narrowing back into a 404. A group + // holding one of their clients is theirs to work with; what protects + // the stranger in it is that they are never named, and that anything + // shared with the group beyond this viewer's library still refuses + // the change. + $mixed = Group::query()->create(['name' => 'Mixed', 'slug' => 'mixed', 'public' => false]); + $mixed->members()->syncWithoutDetaching([$this->mine->id, $this->stranger->id]); + + $this->actingAs($this->rep) + ->patch("/groups/{$mixed->id}", ['name' => 'Mixed Renamed', 'public' => false]) + ->assertRedirect(); + + expect($mixed->fresh()->name)->toBe('Mixed Renamed'); +}); + +test('an unscoped administrator can still change anything', function () { + $this->actingAs($this->admin) + ->patch("/groups/{$this->theirs->id}", ['name' => 'Admin Renamed', 'public' => false]) + ->assertRedirect(); + + expect($this->theirs->fresh()->name)->toBe('Admin Renamed'); +});