From db1dd71f3c6a99d7ebaeb7f32d29d148c453ae6a Mon Sep 17 00:00:00 2001
From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com>
Date: Fri, 28 Aug 2026 02:21:53 +0200
Subject: [PATCH] Stop an expired file locking a group shut for a scoped staff
 member

groupReachesNoFurther() asks whether anything shared with a group sits
outside the viewer's library. `f1b35cc9` established the shape of the
answer for deleted files: start from the live row, because "a deleted file
is not reach, because nobody can reach it".

An expired file is the same case. Membership grants nobody access to it --
File::scopeVisibleToClient ends in notExpired(), so it has left every
member's /my-files and the download answers 403 -- but it is equally gone
from files(), where its absence reads as "outside my library". The group
then locks for a scoped staff member: they cannot add a member, cannot
rename it, and cannot remove their own client again.

So the reach query skips expired files as it already skips deleted ones.
Expiry is reversible where deletion is not, and that needs no special
handling: the guard asks what is reachable at the moment somebody is added
or removed, and the file counts again the moment it stops being expired.

Not changed: File::scopeVisibleToClient, whose treatment of expiry was
settled deliberately in c8078f65. This is about what counts as reach, not
about what a scoped viewer may open.

Two tests, next to the deleted-file pair they mirror: the lockout, and the
half that must not soften -- a live out-of-reach file is still reach with
an expired sibling next to it. Without the fix the first goes red.
---
 .../Files/Access/StaffLibraryScope.php        | 10 ++++
 .../Groups/GroupMembershipScopeTest.php       | 52 +++++++++++++++++++
 2 files changed, 62 insertions(+)

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