This overturns something #1701 decided, so it should say so. That PR
closed the membership hole and left GroupsController::update and
destroy installation-wide on purpose, on the grounds that managing the
group object is a different question from managing who is in it.
What decides it is a measurement that was not in front of that decision.
An assignment to a group is how its members reach a file, so deleting a
group revokes that access for every member. Measured before this guard,
with a client-scoped role holding the group permissions:
stranger client can read the shared file true
PATCH /groups/{stranger group} 302, renamed
DELETE /groups/{stranger group} 302, group gone
stranger client can read the shared file false
So a staff member who may not add somebody to a group out of their reach
could delete it out from under the people already in it. That is not a
gentler version of the membership rule, it is a harder one, and the two
sitting on opposite sides of the same boundary was the odd part.
StaffLibraryScope::allowsGroupChange is the reach half of
allowsGroupMembership on its own, since no client appears in this
question -- one predicate, two callers, rather than a second statement of
it. Both surfaces take it, at 404, matching the membership guards.
A group that shares nothing beyond the actor's library still passes, so a
group they created or one holding their own clients stays theirs, and
unscoped staff are unaffected by construction.
The API document moves a 404 above a 422 on two paths. Both already
documented the 404 -- route model binding produced one -- and Scramble
orders responses by where they appear in the method, so the guard landing
before the validate() call is the whole of the change.
The other half of the sweep. ClientsController and its API twin checked
`abort_unless($client->isClient(), 404)` and nothing else -- a type
check, not a boundary, which is the phrase #1701 used about the group
membership routes for exactly the same reason.
Measured before the fix, with a client-scoped role holding the client
permissions:
GET /clients every client on the installation, name + email
GET /clients/{stranger} 200
PATCH /clients/{stranger} 302, name actually changed
DELETE /clients/{stranger} 302, client gone
The tell was one route over. ClientFilesController::index already draws
this line with StaffLibraryScope::canAssignClient and calls it "the same
boundary StaffLibraryScope enforces everywhere else in the library". Its
neighbours in the same family did not.
So the predicate is not new here. What is new is StaffLibraryScope::clients(),
the listing half of canAssignClient, so a screen narrows by the rule its
own buttons are guarded with instead of restating it -- restating it is
how this went wrong, and how the last four of these went wrong.
Eight actions take it: edit, update, destroy and the two-factor reset on
both surfaces, plus both listings. Answering 404 rather than 403, since a
client outside the roster should not be distinguishable from one that is
not there -- matching the isClient() guard already above it.
Account requests stay installation-wide on purpose: a self-registered
client who has not been approved belongs to nobody yet, so there is no
roster to narrow by and narrowing would empty the screen.
The published API document is unchanged -- both routes already documented
the 404 that the type check produced.
#1701 closed a real hole: group membership decides what a client reaches,
and through File::scopeVisibleToClient it decides what the staff member
holding that client reaches, so `edit_groups` alone was never a boundary.
The predicate it added asks whether everything shared with a group is
already inside the actor's library.
It asked by counting: pluck the group's assignment rows, count how many
of those ids the library query returns, and require the two to match. An
assignment row outlives the thing it points at — nothing clears them when
a file or folder is deleted — while files() and folders() exclude trashed
rows by construction. So one deleted file left a count that could never
balance again, and the group closed permanently: the scoped staff member
could no longer add their own client to it, or remove anybody from it,
with a 403 and nothing to explain it. Every group accumulates dead
assignments over time, so groups would have gone quiet one at a time.
Asked the other way round — is there anything live, shared with this
group, that is outside my library — the dead rows drop out by
construction, because the query starts from File/Folder rather than from
the assignment. That is also the truer question: a deleted file is not
reach, since nobody can reach it.
Three tests. A group stays usable after a file shared with it is deleted,
including removing a member; the same for a deleted folder assignment;
and the half that must not soften — a live file still out of reach is
still refused, deleted siblings or not.
#1698 moved the library boundary into FileCommentPolicy, where it
belongs, and said plainly what that cost: the moderation screen went
from 65 queries to 465 for a client-scoped moderator with five assigned
clients. Measured here, those numbers are exactly right.
The cost is not in asking. It is that StaffLibraryScope::files() rebuilds
its query every time, and building one runs four immediate lookups per
assigned client — the client's group ids, the same ids again inside
Folder::sharedFolderIds(), that method's own assignment lookup, and the
shared-folder get() in Folder::scopeVisibleToClient(). None of them
depend on the query being built. Gate resolves a fresh policy for every
check, so a listing paid for all of it once per row.
The built query is now memoised per user and handed back as a clone,
since every caller adds to it, and the scope is registered as `scoped`
rather than transient so the memo survives a request. Scoped rather than
a singleton on purpose: a long-lived queue worker keeps singletons
between jobs, and a library query built from one job's data has no
business answering the next one's question.
That is 465 queries down to 60 on the same page — below the 65 it cost
before #1698, because the memo also helps the callers that were already
asking repeatedly. FileVersions::sharedAudience(), which runs the same
helper twice per candidate while resolving notification recipients, gets
it for free.
So the answer to the question #1698 left open is neither of the two it
offered. can_delete stays a real question asked of the policy; nothing
restates the boundary; and the page is faster than it was before the
fix. Three tests: one user's query never answers another's, one caller's
constraints never follow the next, and the moderation screen does not
ask once per row.
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.
Client file sharing, rebuilt from the ground up: a private area per
client, resumable uploads, folders, groups and categories, sharing with
expiry dates and download limits, comments, file versions, an activity
log, a REST API, and sixteen languages.
This repository begins here. ProjectSend 2 was developed privately, and
that development history is not published — the previous generation
remains available, with its own history, at projectsend/legacy.
Free software under the GNU General Public License v2, or (at your
option) any later version.
ignacionelson