Files
projectsend/app/Modules/Groups/Http/Controllers/GroupsController.php
T
denkfabrik-li eade690f73 Hold the group edit screen to the same library boundary as the rest
Every other group route asks StaffLibraryScope whether this viewer may
act on this group. GroupsController::update() and ::destroy() do, and so
do their API twins -- all four with abort_unless(allowsGroupChange, 404).
The two that read do not: edit() and Api\GroupsController::show() had no
boundary at all.

What they hand over is the membership, name and email per member, plus
the whole client roster of the installation as available_clients. So a
client-scoped staff member could open a group whose contents they cannot
see, read off every client on the installation, and only be refused when
they pressed save.

Two halves, because the leak has two shapes:

- The group itself. Reading it now asks the same reach question the write
  half asks, one step earlier, with the same 404 -- a group that reaches
  past the viewer's library is not theirs to open either.
- The lists inside it. Both narrow through StaffLibraryScope::clients(),
  the listing half of the rule this screen's buttons are already guarded
  with: allowsGroupMembership refuses removing a member outside the
  roster, and refuses adding a client outside it. Naming them anyway,
  with their address, is the mistake ClientsController made before
  clients() existed -- that method's own docblock says so.

The reach guard alone would not have been enough. A group nobody has
shared anything with reaches nowhere, so it stays open to everybody --
and it can still hold a stranger's client. That case is why the lists
narrow separately, and there is a test for it.

members_count is left whole on purpose: a size is not an identity, and it
is the same number the group listing already reports.

GroupResource's docblock claimed members are safe to expose because "the
group edit screen already shows [them] to anyone holding edit_groups".
That was a claim about a screen, and it stopped being true the moment the
screen narrowed. Reworded to say what now holds it up, and where.

Not changed: the group listing. It reports names and member counts, not
identities, and every button on it is guarded. Nor Api\GroupsController::
index(), for the same reason. Nor the API document -- scramble:export is
byte-identical, because GET /groups/{group} already documented a 404.

Four tests. Three measured red against the unguarded controllers (3
failed / 21 passed): the group cannot be opened at all, the edit screen
stops naming strangers, and the API twin narrows what it hands back. The
fourth -- an unscoped viewer keeps the whole roster and every member -- is
green either way and guards against the fix over-refusing.

Full suite passes (2052 passed / 2 skipped), PHPStan level 8 clean.
2026-08-28 01:19:36 +02:00

219 lines
8.7 KiB
PHP

<?php
declare(strict_types=1);
namespace App\Modules\Groups\Http\Controllers;
use App\Http\Controllers\Controller;
use App\Models\User;
use App\Modules\Audit\Action;
use App\Modules\Audit\ActivityLogger;
use App\Modules\Files\Access\StaffLibraryScope;
use App\Modules\Groups\Models\Group;
use App\Support\Pagination;
use App\Support\PublicUrl;
use App\Support\Rules;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Http\RedirectResponse;
use Illuminate\Http\Request;
use Illuminate\Validation\Rule;
use Inertia\Inertia;
use Inertia\Response;
class GroupsController extends Controller
{
public function __construct(
private readonly ActivityLogger $activity,
private readonly PublicUrl $publicUrl,
private readonly StaffLibraryScope $scope,
) {}
public function index(Request $request): Response
{
$validated = $request->validate([
'search' => ['nullable', 'string', 'max:255'],
'visibility' => ['nullable', Rule::in(['public', 'private'])],
]);
$filters = [
'search' => $validated['search'] ?? null,
'visibility' => $validated['visibility'] ?? null,
];
$groups = Group::query()
->withCount('members')
->when($filters['search'], fn (Builder $query, string $search) => $query->where(fn (Builder $q) => $q
->where('name', 'like', "%{$search}%")
->orWhere('description', 'like', "%{$search}%")))
->when($filters['visibility'], fn (Builder $query, string $visibility) => $query->where('public', $visibility === 'public'))
->orderBy('name')
->paginate(25)
->withQueryString()
->through(fn (Group $group): array => [
'id' => $group->id,
'name' => $group->name,
'description' => $group->description,
'public' => $group->public,
'members_count' => $group->members_count,
'public_url' => $group->public
? $this->publicUrl->for($group)
: null,
]);
return Inertia::render('groups/index', [
'groups' => $groups->items(),
'pagination' => Pagination::meta($groups),
'filters' => $filters,
]);
}
public function create(): Response
{
return Inertia::render('groups/create');
}
public function store(Request $request): RedirectResponse
{
$validated = $request->validate([
'name' => ['required', 'string', 'max:255'],
// The slug only matters (and is only shown) once a group is
// public — otherwise fall back to one derived from the name.
'slug' => Rules::slug('groups'),
'description' => ['nullable', 'string', 'max:2000'],
'public' => ['required', 'boolean'],
]);
$validated['slug'] = $validated['slug'] ?? '' ?: Group::uniqueSlugFrom($validated['name']);
$group = Group::query()->create($validated);
$this->activity->log(Action::GroupCreated, subject: $group);
if ($group->public) {
$this->activity->log(Action::GroupMadePublic, subject: $group, context: ['slug' => $group->slug]);
}
// Same create-without-edit rule as ClientsController::store().
$target = $request->user()?->can('edit_groups')
? redirect()->route('groups.edit', $group)
: redirect()->route('groups.create');
return $target->with('success', __('Group created.'));
}
public function edit(Request $request, Group $group): Response
{
$viewer = $request->user();
assert($viewer !== null);
// The same reach question update() and destroy() ask, asked one
// step earlier. Without it this was the one group route holding no
// library boundary at all: a scoped staff member could open a group
// whose contents they cannot see, read its membership off the
// screen, and only be refused on save.
abort_unless($this->scope->allowsGroupChange($viewer, $group), 404);
return Inertia::render('groups/edit', [
'group' => [
'id' => $group->id,
'name' => $group->name,
'slug' => $group->slug,
'description' => $group->description,
'public' => $group->public,
],
// Both lists narrow through StaffLibraryScope::clients(), which
// is the listing half of the rule this screen's buttons are
// already guarded with: a member outside the roster cannot be
// removed here (allowsGroupMembership refuses it), and a client
// outside it cannot be added. Naming them anyway, with their
// address, was the same mistake the client list made before
// that method existed. An unscoped viewer sees everything,
// unchanged.
'members' => $group->members()
->whereIn('users.id', $this->scope->clients($viewer)->select('id'))
->orderBy('name')
->get()
->map(fn (User $member): array => [
'id' => $member->id,
'name' => $member->name,
'email' => $member->email,
])->all(),
'available_clients' => $this->scope->clients($viewer)
->whereNotIn('id', $group->members()->pluck('users.id'))
->orderBy('name')
->get()
->map(fn (User $client): array => [
'id' => $client->id,
'name' => $client->name,
'email' => $client->email,
])->all(),
]);
}
public function update(Request $request, Group $group): RedirectResponse
{
$viewer = $request->user();
assert($viewer !== null);
// A group whose reach extends past this staff member's library is
// not theirs to change. #1701 drew this line for membership; the
// object itself needs it for the same reason and more sharply —
// an assignment to a group is how its members reach a file, so
// deleting one revokes that access for every member, including
// clients outside this person's roster. Measured before this
// guard: a scoped role deleted a stranger's group and the
// stranger's client stopped seeing the file it carried.
abort_unless($this->scope->allowsGroupChange($viewer, $group), 404);
$validated = $request->validate([
'name' => ['required', 'string', 'max:255'],
// The slug only matters (and is only shown) once a group is
// public — otherwise fall back to one derived from the name.
'slug' => Rules::slug('groups', $group->id),
'description' => ['nullable', 'string', 'max:2000'],
'public' => ['required', 'boolean'],
]);
// Omitting the field on an update leaves the current slug alone —
// it must not silently change just because the name did.
$validated['slug'] = ($validated['slug'] ?? '') ?: ($group->slug ?: Group::uniqueSlugFrom($validated['name'], $group->id));
$wasPublic = $group->public;
$group->update($validated);
$this->activity->log(Action::GroupUpdated, subject: $group);
if (! $wasPublic && $group->public) {
$this->activity->log(Action::GroupMadePublic, subject: $group, context: ['slug' => $group->slug]);
} elseif ($wasPublic && ! $group->public) {
$this->activity->log(Action::GroupMadePrivate, subject: $group);
}
return back()->with('success', __('Group updated.'));
}
public function destroy(Request $request, Group $group): RedirectResponse
{
$viewer = $request->user();
assert($viewer !== null);
// A group whose reach extends past this staff member's library is
// not theirs to change. #1701 drew this line for membership; the
// object itself needs it for the same reason and more sharply —
// an assignment to a group is how its members reach a file, so
// deleting one revokes that access for every member, including
// clients outside this person's roster. Measured before this
// guard: a scoped role deleted a stranger's group and the
// stranger's client stopped seeing the file it carried.
abort_unless($this->scope->allowsGroupChange($viewer, $group), 404);
$name = $group->name;
$group->delete();
$this->activity->log(Action::GroupDeleted, context: ['name' => $name]);
return redirect()->route('groups.index')->with('success', __('Group deleted.'));
}
}