Filling the "Storage quota (MB)" field on the new-client form raised a
TypeError and the request died with a 500. Leaving it blank worked, which
is why it reached a release: that path goes through `null ?? 0`, and the
0 is an int.
The `integer` validation rule checks that a value looks like an integer.
It does not convert it. `$request->validate()` returns the raw input, so
the form field arrives as the string "2048" -- and the create form types
that field as a string in React, so it is a string even over JSON. Both
controllers declare strict_types, so handing it to
`ClientAccounts::create()`'s `int $storageQuotaMb` is a TypeError.
Fixed on both surfaces that call create(): the staff screen and
/api/v1/clients. The API twin had the same defect, reachable by sending
the quota as a quoted JSON value or a form-encoded body -- its own create
test only ever sent a JSON number.
Two more call sites had the same shape and are cast too, though nothing
sends them a string today: the share-link download cap and a comment's
reply_to. Both are safe only because a frontend file happens to call
Number() first, which is a fact about that file rather than anything the
signature guarantees. The null in each is preserved rather than collapsed
to 0 -- "no cap" is not a cap of zero.
`storage_quota_mb` is also cast on User and Invitation. The column is an
unsignedInteger and both docblocks already promise int; it is read
straight into provision()'s typed parameter when an invitation is
redeemed, and which type a driver hands back is not something that call
site should depend on.
Found on the new files-test rehearsal instance, on its first real use,
against the same build the whole fleet is running.
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.
file_comments.client_context_id is cascadeOnDelete, but users are
soft-deleted, so the cascade never fires: the column keeps pointing at a
row that is still there while the Eloquent relation resolves to null.
resolveClientContext branched on the relation, and a null context on a
Clients comment is the branch every client on the file reads -- so a
staff reply into one client's private thread became a circular to all of
them, with the canAssignClient check skipped on the way.
VisibleCommentScope says so in its own docblock: "A Clients comment
carrying client_context_id = C is never returned to any non-staff viewer
other than C ... A Clients comment with a null context is a staff message
to everyone on the file, and every client with access reads it."
Measured on main, with one file shared with two clients and the first of
them deleted after commenting:
column client_context_id 3
relation clientContext null
POST reply into her thread 201, stored with client_context_id null
read by the other client yes
Ask the column, and refuse when the account behind it is gone. There is
nobody left to answer, and the one outcome that must not follow from a
filled column is the broadcast, so this throws rather than falling
through to it.
authorName() had the same root cause from the other column: its docblock
claimed author_id cascades so there is no deleted author, and a deleted
client's comment was going out as "Anonymous" -- which is what a guest
comment looks like, and a guest comment is read by different rules. Guest
is now decided by author_id alone, the same question isFromGuest() asks,
and a trashed author is read with withTrashed(). Nothing comes back only
once the grace-period erasure has removed the row for real.
That read costs one query per comment whose author is trashed. Measured
on a ten-comment thread: 11 queries before, 21 after, against 20 for the
same thread with every author alive. Left as a lazy read rather than
eager-loading with withTrashed() at every call site, because the callers
would each have to remember it and the cost only applies to comments
whose author is gone.
Five tests, two measured red against the unfixed code (2 failed / 3
passed) -- one per column. The three that stay green either way are the
branches that must not move: a staff message with no context still
reaches everybody, a reply into a live client's thread still lands in
that thread alone, and a genuine guest comment is still anonymous.
Full suite passes (2053 passed / 2 skipped), PHPStan level 8 clean.
A managed installation's staff accounts were expected to arrive from
outside it, so users.manage was Community-only and /users, /roles and
their API twins answered 404 there. The platform side spent a long
document designing its way around that gate; opening it is cheaper than
routing around it, and more honest about where the knowledge sits.
The division that settles it is the one managed storage already uses. We
do not manage a tenant's files from outside — a bucket is provisioned, a
scoped credential handed over, and what goes in it is the tenant's
business. Seats are the same kind of thing. A platform knows how many
staff accounts it sold; it does not know whether Alice should be an
Account Manager, and it certainly does not know where her files go when
she leaves. Capacity is the platform's, occupancy is the tenant's, and
the cap belongs in an environment variable rather than in a closed
screen.
The capability stays in front of the routes rather than being deleted.
It is currently true in both editions, but it is the seam an edition
difference has to travel through, and removing it would mean inventing
one again later.
Seven test files asserted the old rule, which is the tests doing their
job. Most flip. Two needed a different example instead: EnsureCapability
and AbilityCapability were both using users.manage to stand for
"Community-only", so they now use storage.configure and manage_updates —
keys that still are.
Two rationales half-expired and say so rather than being quietly
rewritten. CommentAuthors gave two reasons for being a setting rather
than a permission; the first was that roles are uneditable on cloud,
which stopped being true here, and the second — that `Everyone` includes
anonymous visitors, who have no role to hold a key — was always the
stronger and is now the whole of it.
The seat cap this makes necessary is the next commit, not this one. On
its own this change lets a managed tenant create staff accounts without
limit, which is why the two belong in the same release.
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