Merge pull request #1754 from denkfabrik-li/fix/api-group-members-response-scope

Narrow the membership an API member write hands back

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')),
+        ]));
     }
 }

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]);