diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fc8affc..35e713c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -201,6 +201,20 @@ a version is cut. (found, diagnosed and fixed by [@denkfabrik-li](https://github.com/denkfabrik-li) in [#1683](https://github.com/projectsend/projectsend/pull/1683)) +- **Group membership now respects a limited role's boundary.** A staff role can be limited to its + own assigned clients. Adding somebody to a group, or taking them out, checked only that the person + held the "edit groups" permission — not that the group was any of their business. Because joining a + group hands the new member everything shared with it, someone with a limited role could put one of + their own clients into any group on the installation and, through that client, reach files they + had been refused a moment earlier. Approving or denying a membership request was the same write + through a second door, and the requests screen listed every pending request by name and email, + including clients outside the viewer's roster. All of it is now held to the same boundary the rest + of the library uses, and the sidebar count agrees with the screen behind it. No role that ships + with ProjectSend combines the two permissions this needed, so reaching it took a custom role. + Nothing changes for an administrator or any unrestricted role. + (found, diagnosed and fixed by [@denkfabrik-li](https://github.com/denkfabrik-li) in + [#1701](https://github.com/projectsend/projectsend/pull/1701)) + ## 2.1.0 — 18 August 2026 Updating, mostly. ProjectSend now tells you when there is a new version, ends an update somewhere diff --git a/app/Modules/Files/Access/StaffLibraryScope.php b/app/Modules/Files/Access/StaffLibraryScope.php index defe0a8a..486e1d8b 100644 --- a/app/Modules/Files/Access/StaffLibraryScope.php +++ b/app/Modules/Files/Access/StaffLibraryScope.php @@ -219,6 +219,18 @@ class StaffLibraryScope * Whether everything shared with this group is already inside the * user's library — files assigned to it, and the folders whose * subtrees it can browse. + * + * Asked as "is anything shared with this group outside my library", + * rather than by counting assignment rows against library rows. An + * assignment outlives the thing it points at: nothing clears these + * rows when a file or folder is deleted, and a deleted one can never + * appear in files()/folders(), which exclude trashed rows. Counting + * therefore never balanced again, and the group became permanently + * unmanageable for a scoped staff member — including for their own + * clients, and including removing somebody. Starting from the live + * row rather than from the assignment ignores the dead ones by + * construction, which is also the right answer: a deleted file is + * not reach, because nobody can reach it. */ private function groupReachesNoFurther(User $user, Group $group): bool { @@ -228,19 +240,24 @@ class StaffLibraryScope $morph = $group->getMorphClass(); - $fileIds = FileAssignment::query() - ->where('assignable_type', $morph)->where('assignable_id', $group->id) - ->pluck('file_id')->unique(); + $assignedFiles = FileAssignment::query()->select('file_id') + ->where('assignable_type', $morph)->where('assignable_id', $group->id); - if ($fileIds->isNotEmpty() && $this->files($user)->whereIn('id', $fileIds)->count() !== $fileIds->count()) { + $outside = File::query() + ->whereIn('id', $assignedFiles) + ->whereNotIn('id', $this->files($user)->select('id')) + ->exists(); + + if ($outside) { return false; } - $folderIds = FolderAssignment::query() - ->where('assignable_type', $morph)->where('assignable_id', $group->id) - ->pluck('folder_id')->unique(); + $assignedFolders = FolderAssignment::query()->select('folder_id') + ->where('assignable_type', $morph)->where('assignable_id', $group->id); - return $folderIds->isEmpty() - || $this->folders($user)->whereIn('id', $folderIds)->count() === $folderIds->count(); + return ! Folder::query() + ->whereIn('id', $assignedFolders) + ->whereNotIn('id', $this->folders($user)->select('id')) + ->exists(); } } diff --git a/tests/Feature/Groups/GroupMembershipScopeTest.php b/tests/Feature/Groups/GroupMembershipScopeTest.php index 9e4c686c..3587d968 100644 --- a/tests/Feature/Groups/GroupMembershipScopeTest.php +++ b/tests/Feature/Groups/GroupMembershipScopeTest.php @@ -245,3 +245,80 @@ test('a folder shared with a group counts as reach too', function () { expect($bare->members()->count())->toBe(0); }); + +// An assignment row outlives the file it points at — nothing clears them +// on delete — and a trashed file can never appear in files(). Asking +// "is anything outside my library" from the live row rather than counting +// assignment rows is what keeps a group usable after somebody deletes a +// file that was once shared with it. +test('a group is not locked shut by a file that has since been deleted', function () { + $group = Group::query()->create(['name' => 'Newsletter', 'slug' => 'newsletter', 'public' => false]); + $group->members()->syncWithoutDetaching([$this->mine->id]); + + // Shared with the group, and reachable by this rep because their own + // client is a member — so the group is theirs to manage. + $file = uploadNamedFile($this->admin, 'seasonal-offer'); + shareFileWithGroup($file, $group); + + $second = User::factory()->client()->create(['name' => 'Also Mine']); + $this->rep->assignedClients()->attach($second->id); + + $this->actingAs($this->rep) + ->post("/groups/{$group->id}/members", ['user_id' => $second->id]) + ->assertRedirect(); + + // The uploader deletes it. The assignment row stays behind. + $file->delete(); + + $third = User::factory()->client()->create(['name' => 'Mine Too']); + $this->rep->assignedClients()->attach($third->id); + + $this->actingAs($this->rep) + ->post("/groups/{$group->id}/members", ['user_id' => $third->id]) + ->assertRedirect(); + + expect($group->members()->count())->toBe(3); + + // And taking somebody out again still works, which the count form + // also blocked. + $this->actingAs($this->rep) + ->delete("/groups/{$group->id}/members/{$third->id}") + ->assertRedirect(); + + expect($group->members()->count())->toBe(2); +}); + +test('a deleted folder assignment does not lock a group either', function () { + $group = Group::query()->create(['name' => 'Bulletin', 'slug' => 'bulletin', 'public' => false]); + $group->members()->syncWithoutDetaching([$this->mine->id]); + + $folder = app(FolderService::class)->create('Seasonal', null); + FolderAssignment::query()->create([ + 'folder_id' => $folder->id, + 'assignable_type' => $group->getMorphClass(), + 'assignable_id' => $group->id, + ]); + + $folder->delete(); + + $second = User::factory()->client()->create(['name' => 'Second']); + $this->rep->assignedClients()->attach($second->id); + + $this->actingAs($this->rep) + ->post("/groups/{$group->id}/members", ['user_id' => $second->id]) + ->assertRedirect(); + + expect($group->members()->count())->toBe(2); +}); + +// The half that must not soften: a live file outside the library is still +// reach, deleted siblings or not. +test('a deleted file does not excuse a live one that is still out of reach', function () { + $dead = uploadNamedFile($this->admin, 'was-shared'); + shareFileWithGroup($dead, $this->strangerGroup); + $dead->delete(); + + $this->actingAs($this->rep) + ->post("/groups/{$this->strangerGroup->id}/members", ['user_id' => $this->mine->id]) + ->assertForbidden(); +});