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