Files
projectsend/app/Modules/Groups/Http/Controllers/MembershipRequestsController.php
T
denkfabrik-li 8a6543073b Group membership is a library boundary, not just a list
The four routes that edit a group's membership -- add and remove, web
and API -- contain no authorization call of any kind. `can:edit_groups`
in front of them is the whole of it, and a permission is not a boundary.

The authorization sweep looked at these and let them stand, on the
grounds that groups are installation-wide by design: GroupsController
::index lists every group unfiltered, so list and single-object access
agree, and there is no listing/direct-access mismatch to fix. That is
true, and it is the answer to the question of who may *see* a group.
This is a different question: what a write to one *does*.

Joining a group hands the new member everything shared with it. When
that member is one of a client-scoped staff member's own clients,
File::scopeVisibleToClient hands the same content straight back to them
-- that scope is what StaffLibraryScope::files() is built out of. So the
one write turns a file they get a 403 on into a file in their library,
and the download that follows is a 200. ResolvesShareTargets draws that
line on the sharing path through canAssignGroup(); nobody drew it on the
membership path, and canAssignGroup() is *derived from membership*, so
whoever may edit the list also decides what the list entitles them to.

StaffLibraryScope::allowsGroupMembership answers it directly instead of
through the derived predicate, which is the wrong tool here twice over.
Membership asks about reach, so it checks reach: the client must be one
this staff member holds, and the group must not already reach past their
library -- no file assigned to it, and no folder shared with it, outside
StaffLibraryScope. A group nothing has been shared with passes trivially,
which matters, because canAssignGroup() would have said no to a group
that has no members yet and left a scoped staff member unable to put the
first client into one they had just created.

The same write has a second door. MembershipRequestsController::approve
joins a client to a group with identical consequences, under
`approve_groups_memberships_requests`, and deny() decides about somebody
else's client and emails them about it. Both go through the same
boundary, answering 404 to match the guard already above approve().

The queue and its sidebar badge are narrowed to the clients the viewer
holds, through one scope on the model that both read -- the rule the
comment badge in HandleInertiaRequests already states two branches down
("a client-scoped staff member is not shown a number they cannot act
on"), and the reason VisibleCommentScope owns its own pendingTotal()
rather than leaving the middleware to count for itself. Each row carries
the client's name and email, so an unnarrowed queue was also handing
those over for clients outside the roster. Unscoped staff still see every
pending request.

That narrowing is on the client, not on the group: whether a group is
reachable depends on what is shared with it, which is not a question to
ask row by row in a listing. A scoped viewer may therefore still be
shown a request they would be refused on -- one of their own clients
asking to join a group out of their reach. The names were the part that
leaked.

Unscoped staff are unaffected throughout -- both halves of the predicate
are true for them by construction. No seeded role reaches this: Client
Manager is the only client-scoped role that ships, and it holds no group
permissions, so a custom role is needed to get here at all.

The published API document gains a 403 on both member routes.
Regenerated with php artisan scramble:export; Scramble reads abort_unless
out of the method body but not out of a private helper, which is why the
guard is written out at each of the four call sites rather than shared.
2026-08-26 08:49:48 +02:00

149 lines
5.8 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\Modules\Groups\Models\MembershipRequest;
use App\Modules\Groups\Notifications\GroupMembershipDeniedNotification;
use App\Modules\Notifications\Notifier;
use App\Modules\Platform\Settings\Setting;
use App\Modules\Platform\Settings\Settings;
use App\Support\Pagination;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Http\RedirectResponse;
use Illuminate\Http\Request;
use Inertia\Inertia;
use Inertia\Response;
/**
* The staff queue for group membership requests: approve joins the
* client to the group, deny discards the request. Both survive in the
* activity log.
*/
class MembershipRequestsController extends Controller
{
public function __construct(
private readonly ActivityLogger $activity,
private readonly Settings $settings,
private readonly Notifier $notifier,
private readonly StaffLibraryScope $scope,
) {}
public function index(Request $request): Response
{
$validated = $request->validate([
'search' => ['nullable', 'string', 'max:255'],
]);
$filters = ['search' => $validated['search'] ?? null];
$viewer = $request->user();
assert($viewer !== null);
$requests = MembershipRequest::query()
->pending()
// A request whose client or group vanished is dead weight; excluding
// it in SQL (not after fetching) keeps pagination counts honest.
->whereHas('user')
->whereHas('group')
// Narrowed the way the buttons on each row now are — see
// MembershipRequest::scopeApprovableBy, which the sidebar badge
// reads too so the number and this screen agree.
->approvableBy($viewer)
->with(['group', 'user'])
->when($filters['search'], fn (Builder $query, string $search) => $query->where(fn (Builder $q) => $q
->whereHas('user', fn (Builder $u) => $u->where('name', 'like', "%{$search}%")->orWhere('email', 'like', "%{$search}%"))
->orWhereHas('group', fn (Builder $g) => $g->where('name', 'like', "%{$search}%"))))
->orderBy('created_at')
->paginate(25)
->withQueryString()
->through(fn (MembershipRequest $membershipRequest): array => [
'id' => $membershipRequest->id,
'client_name' => $membershipRequest->user?->name,
'client_email' => $membershipRequest->user?->email,
'group_name' => $membershipRequest->group?->name,
'created_at' => $membershipRequest->created_at?->toIso8601String(),
]);
return Inertia::render('groups/membership-requests', [
'requests' => $requests->items(),
'pagination' => Pagination::meta($requests),
'filters' => $filters,
]);
}
public function approve(Request $request, MembershipRequest $membershipRequest): RedirectResponse
{
$group = $membershipRequest->group;
$client = $membershipRequest->user;
abort_unless($group !== null && $client !== null && $membershipRequest->status === MembershipRequest::STATUS_PENDING, 404);
$this->guardRequest($request, $group, $client);
$group->members()->syncWithoutDetaching([$client->id]);
$membershipRequest->delete();
$this->activity->log(Action::GroupMembershipApproved, subject: $group, context: ['member' => $client->name]);
$this->notifier->send('group.membership_approved', [$client], subject: $group, data: ['groupName' => $group->name]);
return back()->with('success', __('Membership request approved.'));
}
public function deny(Request $request, MembershipRequest $membershipRequest): RedirectResponse
{
$group = $membershipRequest->group;
$client = $membershipRequest->user;
if ($group !== null && $client !== null) {
$this->guardRequest($request, $group, $client);
}
// The denied row persists: the client sees the outcome, and it
// enforces the re-request cooldown.
$membershipRequest->forceFill([
'status' => MembershipRequest::STATUS_DENIED,
'denied_at' => now(),
])->save();
if ($group !== null && $client !== null) {
$this->activity->log(Action::GroupMembershipDenied, subject: $group, context: ['member' => $client->name]);
if ($this->settings->get(Setting::EmailNotificationsEnabled) === true) {
$client->notify(new GroupMembershipDeniedNotification($group->name));
}
}
return back()->with('success', __('Membership request denied.'));
}
/**
* Approving a request is GroupMembersController::store by another
* door: it joins a client to a group, with the same consequence for
* what that client -- and any staff member holding them -- can reach
* afterwards. Denying one is a decision about somebody's client, and
* emails them about it. Both belong inside the same boundary, and
* `approve_groups_memberships_requests` in front of the route is a
* permission, not one.
*
* 404 rather than 403, matching the guard immediately above it in
* approve(): a request this staff member may not act on should not
* be distinguishable from one that is not there.
*/
private function guardRequest(Request $request, Group $group, User $client): void
{
$viewer = $request->user();
assert($viewer !== null);
abort_unless($this->scope->allowsGroupMembership($viewer, $group, $client), 404);
}
}