diff --git a/app/Modules/Files/Access/StaffLibraryScope.php b/app/Modules/Files/Access/StaffLibraryScope.php index de214d56..486075ff 100644 --- a/app/Modules/Files/Access/StaffLibraryScope.php +++ b/app/Modules/Files/Access/StaffLibraryScope.php @@ -268,6 +268,15 @@ class StaffLibraryScope * 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. + * + * An expired file is the same answer for the same reason. Membership + * in this group grants nobody access to it — File::scopeVisibleToClient + * ends in notExpired(), so it is gone from every member's /my-files and + * the download is refused — while its absence from files() otherwise + * reads as "outside my library" and locks the group exactly as a + * deleted file used to. Expiry is reversible where deletion is not, so + * the file counts as reach again the moment it does: this asks what is + * reachable now, at the moment somebody is added or removed. */ private function groupReachesNoFurther(User $user, Group $group): bool { @@ -282,6 +291,7 @@ class StaffLibraryScope $outside = File::query() ->whereIn('id', $assignedFiles) + ->notExpired() ->whereNotIn('id', $this->files($user)->select('id')) ->exists(); diff --git a/tests/Feature/Groups/GroupMembershipScopeTest.php b/tests/Feature/Groups/GroupMembershipScopeTest.php index 06507824..0aaf5fa5 100644 --- a/tests/Feature/Groups/GroupMembershipScopeTest.php +++ b/tests/Feature/Groups/GroupMembershipScopeTest.php @@ -325,6 +325,58 @@ test('a deleted file does not excuse a live one that is still out of reach', fun ->assertForbidden(); }); +// Expiry is the same answer for the same reason: membership grants nobody +// access to an expired file, because scopeVisibleToClient ends in +// notExpired(). It is only invisible to files(), which read as reach. +test('a group is not locked shut by a file that has since expired', function () { + $group = Group::query()->create(['name' => 'Seasonal', 'slug' => 'seasonal', 'public' => false]); + $group->members()->syncWithoutDetaching([$this->mine->id]); + + $file = uploadNamedFile($this->admin, 'summer-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(); + + $file->update(['expires_at' => now()->subDay()]); + + // Nobody in the group can reach it any more — the premise of the rule. + $this->actingAs($this->mine)->get("/files/{$file->id}/download")->assertForbidden(); + + $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 their own client out again, which is the part that reads + // worst: the rep could not undo their own membership change. + $this->actingAs($this->rep) + ->delete("/groups/{$group->id}/members/{$third->id}") + ->assertRedirect(); + + expect($group->members()->count())->toBe(2); +}); + +test('an expired file does not excuse a live one that is still out of reach', function () { + $expired = uploadNamedFile($this->admin, 'was-current'); + shareFileWithGroup($expired, $this->strangerGroup); + $expired->update(['expires_at' => now()->subDay()]); + + // $this->secret is live, shared with the same group and outside this + // rep's library — the guard still has to answer no. + $this->actingAs($this->rep) + ->post("/groups/{$this->strangerGroup->id}/members", ['user_id' => $this->mine->id]) + ->assertForbidden(); +}); + /** * The rep role ships without delete_groups, and these cases are about the