From 19ee9d9833e74b838bf642b5530a80eaaaf210e1 Mon Sep 17 00:00:00 2001
From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com>
Date: Fri, 28 Aug 2026 23:46:18 +0200
Subject: [PATCH] Narrow the membership an API member write hands back

Adding or removing a group member answered with the group, and loaded the
relation whole:

    return new GroupResource($group->loadCount('members')->load('members'));

GroupResource gives each member an id, a name and an email. So a
client-scoped staff member who added one of their own clients to a group
was handed, in the same response, the name and address of every other
client in it -- people they may not read anywhere else in the application,
and whom the group edit screen refuses to name for exactly that reason.
syncWithoutDetaching() makes the call idempotent, so the same request
returns the same list as often as it is sent.

The boundary is already written down. GroupResource's docblock:

  both narrow the list to the clients the viewer may act on, and the
  controller loading this relation is where that narrowing is applied

and Api\GroupsController::show() does it for the read of the same group,
noting that "it hands back the membership with addresses". Changing the
membership is not a reason to be told more than reading it is, so both
halves now narrow by the same query, through one private helper rather
than a third copy of it.

members_count is deliberately left whole, matching show(): a size is not
an identity, and it is the number the group listing already reports.

Nothing about who may perform the write changes -- StaffLibraryScope
::allowsGroupMembership() already decided that, and still does. This is
only what the answer is allowed to say.

Tests: added beside the existing "the API twin narrows the membership it
hands back", which covered the read half only. Both write tests fail
against the unfixed controller; the third pins that an unscoped token
still gets every member.
---
 .../Api/GroupMembersController.php            | 27 +++++++++-
 .../Groups/GroupMembershipScopeTest.php       | 49 +++++++++++++++++++
 2 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/app/Modules/Groups/Http/Controllers/Api/GroupMembersController.php b/app/Modules/Groups/Http/Controllers/Api/GroupMembersController.php
index b3576c73..dbb43939 100644
--- a/app/Modules/Groups/Http/Controllers/Api/GroupMembersController.php
+++ b/app/Modules/Groups/Http/Controllers/Api/GroupMembersController.php
@@ -11,6 +11,7 @@ use App\Modules\Audit\ActivityLogger;
 use App\Modules\Files\Access\StaffLibraryScope;
 use App\Modules\Groups\Http\Resources\Api\GroupResource;
 use App\Modules\Groups\Models\Group;
+use Illuminate\Database\Eloquent\Relations\BelongsToMany;
 use Illuminate\Http\Request;
 use Illuminate\Validation\ValidationException;
 
@@ -56,7 +57,7 @@ class GroupMembersController extends Controller
 
         $this->activity->log(Action::GroupMemberAdded, subject: $group, context: ['member' => $client->name]);
 
-        return new GroupResource($group->loadCount('members')->load('members'));
+        return $this->response($group, $actor);
     }
 
     public function destroy(Request $request, Group $group, User $member): GroupResource
@@ -72,6 +73,28 @@ class GroupMembersController extends Controller
 
         $this->activity->log(Action::GroupMemberRemoved, subject: $group, context: ['member' => $member->name]);
 
-        return new GroupResource($group->loadCount('members')->load('members'));
+        return $this->response($group, $actor);
+    }
+
+    /**
+     * The group as this actor may see it.
+     *
+     * GroupResource carries a name and an email per member, and its own
+     * docblock puts the boundary here: "the controller loading this
+     * relation is where that narrowing is applied". Api\GroupsController
+     * ::show() applies it for the read of the same group; changing the
+     * membership is not a reason to be told more than reading it, so both
+     * halves narrow by the same query.
+     *
+     * The count is deliberately not narrowed. members_count is the size of
+     * the group, which is a fact about the group rather than about who is
+     * in it, and the web screen shows the same total.
+     */
+    private function response(Group $group, User $actor): GroupResource
+    {
+        return new GroupResource($group->loadCount('members')->load([
+            'members' => fn (BelongsToMany $members) => $members
+                ->whereIn('users.id', $this->scope->clients($actor)->select('id')),
+        ]));
     }
 }
diff --git a/tests/Feature/Groups/GroupMembershipScopeTest.php b/tests/Feature/Groups/GroupMembershipScopeTest.php
index 239504b7..816521fa 100644
--- a/tests/Feature/Groups/GroupMembershipScopeTest.php
+++ b/tests/Feature/Groups/GroupMembershipScopeTest.php
@@ -600,6 +600,55 @@ test('the API twin narrows the membership it hands back', function () {
         ->and($data['members_count'])->toBe(2);
 });
 
+test('the API narrows the membership a member write hands back, the same way', function () {
+    // The read above is narrowed; changing the membership went through the
+    // same resource with the relation loaded whole, so the write handed
+    // back what the read refuses to.
+    $ours = Group::query()->create(['name' => 'Ours', 'slug' => 'ours', 'public' => false]);
+    $ours->members()->syncWithoutDetaching([$this->stranger->id]);
+
+    $token = $this->rep->createToken('t', [Permission::EditGroups->value])->plainTextToken;
+
+    $data = $this->withToken($token)
+        ->postJson("/api/v1/groups/{$ours->id}/members", ['user_id' => $this->mine->id])
+        ->assertOk()
+        ->json('data');
+
+    expect(array_column($data['members'], 'name'))->toBe(['Mine'])
+        ->and($data['members_count'])->toBe(2);
+});
+
+test('and on the way back out again', function () {
+    $ours = Group::query()->create(['name' => 'Ours', 'slug' => 'ours', 'public' => false]);
+    $second = User::factory()->client()->create(['name' => 'Also Mine']);
+    $this->rep->assignedClients()->sync([$this->mine->id, $second->id]);
+    $ours->members()->syncWithoutDetaching([$this->mine->id, $second->id, $this->stranger->id]);
+
+    $token = $this->rep->createToken('t', [Permission::EditGroups->value])->plainTextToken;
+
+    $data = $this->withToken($token)
+        ->deleteJson("/api/v1/groups/{$ours->id}/members/{$second->id}")
+        ->assertOk()
+        ->json('data');
+
+    expect(array_column($data['members'], 'name'))->toBe(['Mine'])
+        ->and($data['members_count'])->toBe(2);
+});
+
+test('an unscoped token keeps every member in the write response', function () {
+    $ours = Group::query()->create(['name' => 'Ours', 'slug' => 'ours', 'public' => false]);
+    $ours->members()->syncWithoutDetaching([$this->stranger->id]);
+
+    $token = $this->admin->createToken('t', [Permission::EditGroups->value])->plainTextToken;
+
+    $data = $this->withToken($token)
+        ->postJson("/api/v1/groups/{$ours->id}/members", ['user_id' => $this->mine->id])
+        ->assertOk()
+        ->json('data');
+
+    expect(array_column($data['members'], 'name'))->toBe(['Mine', 'Not Mine']);
+});
+
 test('an unscoped viewer keeps the whole roster and every member', function () {
     $ours = Group::query()->create(['name' => 'Ours', 'slug' => 'ours', 'public' => false]);
     $ours->members()->syncWithoutDetaching([$this->mine->id]);