file_comments.author_id is cascadeOnDelete and the cascade never fires, because a user is soft-deleted. The row behind a deleted commenter is still there and the column still points at it -- the relation just would not hand it over, and every caller then had to invent a meaning for the absence. They invented different ones: the author type became "guest" on the moderation screen and on the file's own thread, and "client" in the API, each printed beside a name that stayed correct, so one row said "Dana Staff" and "guest" at the same time. The author filter and the name search stopped matching the comment altogether, which is the worse half: a moderator filtering for staff comments did not see a staff comment sitting in front of them, and nothing about that looks like a missing row.
This is the author half of #1717, and DeletedClientThreadTest's docblock already described both columns. FileComment::authorName() was the one place that reached past the relation by hand, which is why the names were right while everything beside them was wrong.
The relation is fixed rather than the five call sites: author() reads a deleted account, and the API resource, the author filter and the name search then need no change at all, because they were already asking the right question of a relation that would not answer it. The two authorType() copies now ask author_id, which after the relation fix answers the same either way -- written that way because "no author row means guest" is exactly the reading that produced the bug.
Verified before merging: tests/Feature/Comments at 186 passed on the trial-merge, 5 failed / 1 passed with app/ reset. The survivor is the guest guard, green either way, which is what says this did not simply relabel everything as staff. scramble:export reproduces the spec byte for byte.
No visibility widens, and that was checked rather than taken on trust: every decision point in VisibleCommentScope and FileCommentPolicy compares author_id directly, five sites, none through the relation. No new field is exposed either -- the API resource reads only id, name and type from the author, and name already went through authorName()'s withTrashed lookup. Deleting an account still takes its comments with it when the grace period ends, since author_id is cascadeOnDelete.
Reported and fixed by @denkfabrik-li.
`author_id` is cascadeOnDelete and the cascade never fires, because users
are soft-deleted: the row behind a deleted commenter is still there and
the column still points at it. The plain relation handed back null
anyway, and every caller invented its own meaning for that absence.
Measured on main, one staff member's staff-only comment, before and after
the account is deleted:
/comments screen Dana Staff / staff -> Dana Staff / guest
the file's own thread Dana Staff / staff -> Dana Staff / guest
GET /api/v1/.../comments type staff -> type client
filter author_type=staff 1 row -> 0 rows
search "Dana" 1 row -> 0 rows
unfiltered 1 row -> 1 row
Three surfaces, three different wrong answers, each next to a name that
stayed correct -- so a row can read "Dana Staff" and "guest" at once. A
moderator filtering for staff comments does not see a staff comment that
is sitting in the list in front of them.
This is the author half of what #1717 fixed for client_context_id, and
DeletedClientThreadTest's docblock already describes both columns.
The fix is the relation, not the five call sites: author() reads a
deleted account, which is what authorName() already reached for by hand.
The resource, the filter and the search then need no change at all. The
two authorType() copies now ask author_id rather than the relation --
which after this answers the same either way, and is the rule
isFromGuest() and authorName() already follow.
Nothing that decides who may read a comment goes through this relation.
VisibleCommentScope and FileCommentPolicy both compare author_id
directly, so no visibility widens.
FilePolicy::view() has two halves for staff: one of the three file keys
(upload / edit_files / edit_others_files), AND StaffLibraryScope. Every
comment surface that spans files narrowed by the library half alone.
A role holding moderate_comments and no file key therefore got a 403 on
every file in the installation while reading every comment written about
them on /comments: the text, staff-only notes, the client name a
Clients-visibility comment carries, and a visitor's IP address. The API
queue answered the same way, and approving through it hands the body back
in the response, so it was a reading door as well as a writing one.
The class says this is not supposed to happen -- across()'s own docblock
("a moderation screen is not a way around the visibility model"), the
route comment on /comments ("the list itself is still narrowed by
VisibleCommentScope, so holding the permission does not widen what a
viewer may read"), and routes/api.php ("reading and writing a comment is
gated by 'may see this file', the same three keys the file endpoints
use"). FileCommentPolicy::view() enforces it for a single comment, by
running the file's own gate first. Only the cross-file queries did not.
So they now take their files from ViewableFileScope, which is
FilePolicy::view() expressed as a query, instead of from StaffLibraryScope,
which is only its second half: across(), pendingTotal() and the API's
pending list. The permission half moves into a named method on that class,
since three modules now ask the same question.
FileCommentPolicy::moderate() gets it too, in both forms. Its row form is
otherwise unchanged -- the library check still runs by file id, so a
comment on a soft-deleted file behaves exactly as before.
No system role changes behaviour: Account Manager and System Administrator
are the two that ship with moderate_comments, and both hold upload. What
changes is a hand-built role that holds moderation and nothing else.
Seven tests. Without the fix, five go red; the other two are the premise
(that the viewer really is refused the file itself) and the guard that a
moderator who may read files still moderates the whole installation.
docs/api/openapi.json regenerated for the one changed description.
VisibleCommentScope says at the top of the class that its callers must
already have established that the viewer may see the file. The public
listing's comment endpoint establishes only the guest half of that — the
file is reachable without an account — and then hands $request->user()
straight to the authenticated reading.
For anyone the file's own gate would refuse, that reading is far too
wide. A staff account outside its library, or one holding no file
permission at all, read the file's staff-only notes; a client the file
was never shared with read the messages staff addressed to that file's
clients. Both get a 403 from GET /files/{file}/comments and needed only
to ask the public URL instead.
The endpoint now asks the file's own gate which reading applies. A reader
it admits sees no less than before. A reader it refuses gets what a
visitor gets, widened by their own comments — which is what this
controller has always promised them, and all it promised. The
held-comment rule moves into a method both readings share rather than
being restated.
The same root reaches PATCH and DELETE /comments/{comment}, which bind a
comment rather than a file and so never authorized `view` on it either.
They answer with the thread, and now with the one the file's gate allows.
Refusing them outright would be wrong: somebody who commented through
the public page is exactly the person entitled to edit their own words.
FileCommentPolicy::moderate() asked only whether somebody is staff and
holds moderate_comments. It never weighed the file the comment sits on,
and delete() returns true the moment moderate() does — so a client-scoped
moderator could delete any comment on the installation by naming its id.
Three call sites already knew this and wrote the boundary out by hand,
each with its own abort_unless($library->allowsFile(...), 403) after the
gate. The two that did not are FileCommentsController::destroy(), web and
API: both bind a comment directly, so nothing earlier in the request
establishes that the viewer may see its file.
The intent was documented in three places and enforced in none of them by
the policy — StaffLibraryScope says "the policies consult allowsFile() so
direct access respects the same boundary", VisibleCommentScope says "a
moderation screen is not a way around the visibility model". Put the rule
where those docblocks already say it lives.
moderate() now takes the comment when there is one. Named against the
class it still answers the coarser "does this user moderate at all",
which is what the queue's gate and the affordances ask. Membership is
tested by file id, so a file soft-deleted out from under its comments is
not in a scoped moderator's library either.
The author branch of delete() is deliberately untouched: deleting your
own words inside the edit window is not moderation, and a client is not
client-scoped in StaffLibraryScope's sense.
Approving through the API now derives its 403 from Gate::authorize rather
than the removed abort_unless, so the committed OpenAPI document gains
the shared AuthorizationException ref in place of an inline "An error"
schema — the shape nine of the other twelve documented 403s already use.
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