Let a deleted account's email address come back into use
Closes#1648, and with it the last open item of #1647's audit of unique
indexes on soft-deleting tables.
Resolved a trivial conflict in both ClientsControllers: this branch and
today's e7b5b6a each add a constructor property at the same line, so both
are kept. Nothing else overlapped.
Two things found by checking #1696 and #1699 -- open branches carrying
the same fixes I wrote this morning -- against what I actually shipped.
**topClientsByStorage was scoped with the wrong question.** 4b8220a
narrowed it with StaffLibraryScope::files(), which is right for the two
widgets that name files and wrong for the one that names clients: a
stranger client's upload can sit legitimately inside a scoped viewer's
library, shared with a group one of their own clients belongs to. So the
file was theirs to read and the uploader's name was not theirs to see.
Measured: "Stranger Client Ltd", on nobody's roster, ranked on a scoped
dashboard. assignableClientIds is what the widget is actually asking, and
it is what #1699 used. Their version was right and mine was not.
**The client guard is one method now, not eight copies.** #1696 wrote it
as a private guardTarget() rather than repeating viewer-resolve plus
abort at each site, which is better, and this is a change whose whole
argument is that a rule stated in many places drifts. Behaviour is
identical; the eight sites now read as one rule.
The published document reorders a 404 below a 422 on one path. Scramble
reads abort_unless out of a method body but not out of a helper it calls,
so the 404 now comes from route model binding instead of from the inline
abort -- same response, different position. #1701's body names this trap;
worth knowing it costs ordering and not content.
Credit where it is due: both come from denkfabrik-li's #1696 and #1699,
which were open while I was writing the same fixes. Those two are closed
against this and against e7b5b6a, 4b8220a and 67e9204.
Closing the one thing 4b8220a left open, and the reason it was left: the
expired-files widget reads StaffLibraryScope::files(), and
File::scopeVisibleToClient ends in notExpired(), so a client-scoped
viewer sees only their own expired uploads and never a client's.
Widening that would mean a library query that keeps expired rows, and
scopeVisibleToClient is the single source of truth for client file
access -- the highest-stakes function to go changing for a dashboard
widget. So the boundary stays where it is and the widget stops
overstating itself.
That matters more here than on the two widgets beside it. "Largest
files" showing the largest files somebody can see is still true from
where they stand; a warning about what is due to be deleted, quietly
narrower than it looks, reads as "nothing to worry about" on behalf of
files it never looked at. So this one gets a `scoped` flag from the
server, a title of "Your expired files", a line saying clients' files
are not listed, and an empty state that says none of *your* uploads have
expired rather than that nothing has.
Retitled at the call site rather than in WIDGET_LABELS, because the same
widget means two different things to two viewers and only the server
knows which one is looking.
Checked in a browser for both, not just in the assertions: the scoped
dashboard renders "Your expired files / Files you uploaded. Your
clients' files are not listed here. / None of your uploads have
expired.", with no console errors, and an unscoped administrator's is
unchanged.
The last piece of the #1687 follow-up. BuildZipDownloadJob allows itself
an hour, every shipped topology runs exactly one worker, and everything
shares the default queue -- so one large archive delayed every
notification email queued behind it. The size cap and the
one-build-per-person rule bounded that in July; they did not remove it.
onQueue('zips') in the constructor rather than at the dispatch site, so a
second caller cannot forget it. Both images grow a worker for it:
compose.yaml gains worker-zips, supervisord gains [program:queue-zips],
and the existing worker in each narrows to --queue=default. --tries=1
there matches the job, which records its own failure rather than being
retried.
The part that needs care is the manual install. A worker whose command
still says plain `queue:work` consumes `default` only, so it would send
email happily and never finish a single zip, with nothing in any log
saying why. INSTALL.md's unit now reads --queue=default,zips -- one
worker watching both, which is right for most installations -- and says
what happens if you leave it off, with the two-worker split offered for
anyone who would rather keep the two kinds of work apart. CHANGELOG
carries it as an upgrade note, since it is something to do rather than
something that was done.
Verified in the dev stack rather than only in a test: dispatched a build
and watched worker-zips take it while the default worker stayed idle.
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.
A full parallel run failed once and passed on retry while I was doing the
#1703 follow-up. A flake is worse than a steady failure: it trains you to
re-run rather than look, and it quietly weakens every green run reported
beside it.
Upload parts are real files under storage_path('app/uploads-tmp/{session_id}'),
not a faked disk. Every parallel worker gets its own database, so session
ids restart at 1 in each of them, and two workers writing parts land in
the same directory. On top of that ChunkedUploadsTest's afterEach deleted
the whole tree rather than its own share, for everybody. Six test files
write parts, so this was reachable without anything I added.
The same collision exists inside one worker: RefreshDatabase rolls back,
so ids restart at 1 for every test, and a run that died before its
cleanup leaves parts sitting under the id the next test is about to
claim.
LocalPartStore now reads its root from config, defaulting to exactly
where it always was -- an installation with UPLOAD_PARTS_PATH unset
behaves identically. Tests\TestCase points it at a per-worker directory
and empties that directory per test, which closes the cross-worker, the
cross-run and the intra-worker versions together. ChunkedUploadsTest's
cleanup and its two directory assertions read the configured root rather
than the hardcoded path, so they can no longer reach into a neighbour.
Verified with eight consecutive parallel runs, green, and by watching the
per-worker directories appear separately (w1, w2, w4 … w14) rather than
one shared tree. The isolation itself cannot be asserted from inside a
single test; what a test can pin is the mechanism it rests on, so one
does: parts go where the configured root says.
Follow-up to #1703, which made `exists:folders,id` mean what its ten
readers already assumed. Two things it named and deliberately left.
**The chunked upload is two requests.** store()'s rule only ever sees the
first: POST /uploads records the resolved folder on the UploadSession and
complete() reads it back from the session rather than from the caller, so
deleting the folder while the bytes are in flight still files the
assembled file into it -- the same orphan state #1703 removes, reached by
a door a validation rule cannot watch. complete() now re-resolves through
Folder::query() and files at the root when the folder has gone.
Root rather than a refusal, because the two moments cost different
things. At store() nothing has been sent, so refusing is free and honest,
which is the call #1703 made. Here the bytes are already uploaded, and
discarding somebody's finished transfer over a folder that vanished
underneath them is the harsher of the two surprises. The file lands
somewhere they can see it and move it.
**The refusal now explains itself.** "The selected folder id is invalid"
says nothing when the answer is that the folder has been deleted -- and
that is the usual way to meet this rule, since a live id picked from a
list is how anybody gets here. It matters most on the chunked path, the
one place #1703 makes a previously-working request fail. A small
ValidationRule object carries the message, which keeps the single
definition Rules::folderId() exists for: a messages() array would have to
be repeated at all ten call sites, and rules meaning different things in
ten places is what went wrong in the first place.
One note for whoever writes the next test here. Upload parts live in
storage_path('app/uploads-tmp/{session_id}'), which is a real shared
directory rather than a faked disk, and each parallel worker's database
restarts session ids at 1 -- so two files writing parts on two workers
collide, and ChunkedUploadsTest's afterEach deletes the whole tree for
everybody. Six test files write parts today. These two cases live in
ChunkedUploadsTest rather than beside the rest of their subject so this
change does not add a seventh racer; the underlying isolation problem
predates it and is worth its own fix.
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.
The sweep after #1685 turned up the same leak two widgets further down
the same controller. largestFiles() and expiredFiles() already take the
viewer -- to decide whether their rows get links -- but queried with a
bare File::query(), so a client-scoped staff member's dashboard named
files belonging to clients they hold nothing of.
The note above largestFiles() says a link that 403s is accepted rather
than adding per-row scope checks. That reasoning is about the link. A row
that should not be there at all is a different problem, and the name is
the part that leaks: "Q3 delinquent accounts" says plenty without ever
being downloadable. Scoping the query is also cheaper than the per-row
check that note declined -- StaffLibraryScope builds a scoped user's
query once per request.
Reachable in the default configuration, unlike the last few of these: the
Client Manager role ships client-scoped and holds view_statistics.
topClientsByStorage() goes with them; it names clients rather than files,
which is the thing MembershipRequest::approvableBy and ActivityLogScope
already exist to keep inside a roster.
counters() and transferSeries() stay installation-wide, and now say so.
A total carries no names -- "417 files" tells a scoped viewer nothing
about whose they are -- and if that ever stops being the line, both move
together.
One consequence worth stating rather than discovering: scopeVisibleToClient
ends in notExpired(), so a scoped viewer's expired-files widget now lists
only their own expired uploads, not a client's. Safe, and under-inclusive
-- telling them about a file auto-delete is about to take needs a library
query that keeps expired rows, which is a boundary to decide rather than
to invent inside a leak fix.
#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.
#1685 fixed the dashboard rebuilding a log row by hand and dropping
`origin` from it. One layer down, the same method was skipping something
larger: it ran a bare ActivityLog::query(), so ActivityLogScope never
applied.
That scope exists for this exact case, and says so in its own docblock —
`view_actions_log` is not the whole answer for a client-scoped staff
member, because a log entry carries the subject's *name*. An unscoped log
reads out the name of every file in the installation, and who touched it,
to somebody who gets a 403 on the files themselves.
Measured before the fix, one client-scoped viewer with the permission:
/activity → []
/dashboard → Uploaded the file "Q3 delinquent accounts"
Same person, same permission, opposite answers. The activity page and the
download history both apply the scope; the dashboard was the one caller
that did not, which is the same shape of gap #1685 was about.
More reachable than it looks: the Client Manager system role ships with
`view_actions_log`, so this is the default configuration rather than
something an administrator has to build.
Two tests: a scoped viewer sees only the entry about a file in their
library, and an unscoped one still sees everything.
transferSeries() is left alone on purpose. It is unscoped too, but it
returns per-day counts with no names or subjects attached, which is a
different exposure and arguably not one at all.
#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.
approve() refuses a request that is not pending:
abort_unless($group !== null && $client !== null
&& $membershipRequest->status === MembershipRequest::STATUS_PENDING, 404);
deny(), one method below, checks nothing. Denying is not idempotent, so
repeating it is not a no-op:
- denied_at is stamped again, and that is what the client's re-request
cooldown counts from (MyGroupsController::inDenyCooldown). Repeating
the request keeps one client out of one group for as long as somebody
cares to keep asking, without a single new decision being made.
- a second GroupMembershipDenied entry goes into the activity log, for
a denial that did not happen.
- a second "your request was declined" mail goes to the client.
The queue lists only pending requests, so nothing on the screen offers
this; it takes asking for the route directly. It needs
approve_groups_memberships_requests, so it is not a stranger's move.
The guard is the same one, answering the same 404, placed where deny()
can reach it. deny() keeps tolerating a vanished group or client -- that
tolerance is deliberate and separate: the denied row persists for the
cooldown even when the group it named is gone, and index() already
filters those rows out with whereHas.
Not in this change: deny() writes the status, the log entry and the
notification without a shared transaction. approve() has exactly the same
shape, so fixing one alone would replace a symmetry with a difference,
and doing both means also deciding where the mail sits relative to the
commit -- which is the question #1691 answers for file bytes, and worth
answering on its own rather than inside a state-machine fix.
consumeRecoveryCode() reads the whole list, filters the used code out,
and writes the whole list back. Two requests that both read before
either writes each store their own copy, and the second write puts back
the code the first removed. So a spent code comes back, and the same
code offered twice is accepted twice -- while the method's first line
says "each code works exactly once".
Nobody gets in through this who was not already holding a valid code, so
it is a promise not being kept rather than a door standing open. The
promise is worth keeping anyway: it is the whole reason a printed sheet
of recovery codes can be crossed off, and it is what makes a code that
somebody watched being typed in stop working.
The decision now comes from the row as it stands, re-read under a lock
inside the transaction that writes it -- the shape SendNotificationDigest
already uses to claim the rows it is about to delete. A conditional
update, as in PublicShareController's downloads_count and the delivered_at
claim in #1692, is the other precedent in the tree, but the column is
`encrypted:array`: there is nothing in it a database can compare, so the
comparison has to happen after decryption, under something that holds the
row while it does.
The lock is what makes it atomic against a request arriving at the same
moment. The re-read is what makes the decision right, and it is the half
a test can show: SQLite ignores lockForUpdate, so the accompanying tests
pin the re-read and say so rather than claiming to prove the locking.
config/database.php runs MySQL or Postgres in production, and both honour
it. Saving through the caller's own instance keeps that instance in step
with the row, so a caller cannot go on to decide from a list the database
no longer has.
Folder uses SoftDeletes. The `exists` rule runs against the table, so a
folder in the trash passes it -- while every resolution that follows goes
through Folder::query(), which honours the soft delete and finds nothing.
Ten rules across five controllers rely on that check, and each one reads
it as "this folder exists".
Two of them then wrote the id anyway. Api\FilesController::store()
resolves the folder, hands the null to Folder::uploadableBy(), is told
yes -- correctly, that is the rule for a root upload -- and passes
$validated['folder_id'] to the write. FilesController::store() is the
same shape once #1694 gives it the guard. FilesController::update() and
its API twin write it straight through with nothing in between.
The result is a live file inside a deleted folder, which is a state
nothing else in the application produces: FolderService::delete() deletes
every file in the subtree along with it. The row is reachable by id, in
search and over the API, and missing from the listing its uploader would
look in.
Rules::folderId() makes the check mean what its readers assume, once,
where the reasoning can be written down -- the same argument slug() makes
for itself one method above. Every site takes it, so the file cannot end
up with two spellings of the same rule and no way to tell which is the
safe one.
What changes, path by path:
- POST /files, POST /api/v1/files, PATCH /files/{file} and
PATCH /api/v1/files/{file} refuse a folder in the trash instead of
writing its id. This is the fix.
- POST /uploads used to accept it and quietly file the upload at the
root -- its guard and its write already agreed, on null. It now says
so instead, which is what the other upload paths do.
- files/{file}/move, files/bulk-edit, folders, folders/{folder}/move
and the portal's my-folders already refused, through
StaffLibraryScope::folders() or Folder::scopeVisibleToClient(), both
of which drop trashed rows. They still refuse; the answer is now 422
naming folder_id rather than a bare 404. Those two guards are asking
a different question -- "is this folder yours" -- and they keep
asking it.
No live folder id behaves differently anywhere, and the root (a null
folder_id) is untouched.
The published API document is unchanged: `exists` renders the same either
way. Regenerated with php artisan scramble:export and byte-identical.
Every route that binds one client — the edit screen, the update, the
delete, the second-factor reset, the file browser — asks whether this
staff member may manage that client. POST users/convert/{user} binds one
too, and asks nothing about it.
AccountConversion::guardToStaff() says why it skips
StaffAccounts::guardTarget, and the reason is sound as far as it goes:
guardTarget asks "could the actor have granted the target's role", which
is meaningless of a client, and what limits a promotion is the role being
*granted* — enforced by the controller validating role_id against
assignableRoleIds(). That answers the question about the role. Nothing
answers the one about the target.
So a client-scoped staff member holding manage_users, edit_users and
edit_clients could promote any client on the installation. It is the
most far-reaching thing that can be done to a client account: the portal
access goes, the assignments that made them somebody's client go inert,
and they come out holding whatever staff role the actor picked from
their own list. The client is never told.
guardToStaff() now asks StaffLibraryScope::canAssignClient — the same
predicate ResolvesShareTargets uses to decide who a file may be shared
with, and true by construction for unscoped staff, so the ordinary
administrator path is untouched. 404 rather than 403, matching both the
isClient() check the controller makes on the way in and the answer the
clients routes give: a client this staff member may not manage should
not be distinguishable from one that is not there.
guardToClient() is unchanged. Its target is a staff account, guardTarget
is the right question to ask about one, and it was already being asked.
The account list on the converter screen is deliberately left as it is.
It runs under can:edit_users and shows every account of the chosen
direction, the same way every other staff surface that lists clients
shows all of them; narrowing a listing is a product decision, not this
fix. What changes is that the button on the row now refuses rather than
going through.
An account deleted by an administrator was soft-deleted with erase_after
null, so projectsend:purge-erasures — which filters on
whereNotNull('erase_after') — never reached it, and the unique index on
users.email kept the address reserved forever. Anyone re-creating the
account got "The email has already been taken", naming a conflict nothing
on any screen could show or clear (#1648).
Both halves of the issue's option 3:
Every deletion path now schedules the erasure. The stamp lives in
ErasureSchedule — self-deletion switched to it, and StaffAccounts::delete
(shared by the web screen and the API) and both client controllers call
it right before delete(). Same grace period, same purge, whoever deleted
the account. Deliberately no backfill for rows deleted before this
change: stamping them during an update would start a countdown to data
erasure that nobody chose at deletion time; the message below covers
them instead.
The staff creation paths swap unique:users,email for AvailableEmailRule,
which refuses exactly the same things but can explain the one refusal
the stock message can't: an address held by a deleted account now names
the date it becomes available, and one deleted before scheduling existed
points at projectsend:erase-account. A living account keeps the stock
message, and public registration keeps the stock rule — telling an
anonymous visitor the address belongs to a deleted account would confirm
it had an account here.
Follow-up to #1687, which made a zip build report failure honestly. Four
things it passed near, none of them regressions it introduced.
A zip has never had a size limit — only a cap of 10,000 files, which
bounds nothing that costs anything. Ten thousand spreadsheets zip in
seconds; two hundred videos is an hour of stream-copying and an archive
that fills the disk. Bytes are what a build actually costs, so the new
Settings → Downloads screen caps the total size instead, at 2 GB out of
the box. It is a setting rather than a constant because the safe figure
depends on free disk, on whether sources live on a remote disk, and on
the plan a hosted tenant is on — the file count stays fixed, since it is
a foot-gun rail and not a knob anybody needs. The controller measures
the selection at request time and names both numbers when it refuses;
the job measures again, because it re-derives the selection at run time
and a folder can grow while the job waits in the queue.
Every shipped topology runs exactly one queue worker, and everything
shares the default queue, so raising the job timeout to an hour handed
any signed-in person an hour of everyone else's notification mail. There
is now one build in progress per requester and a named throttle bucket
on the endpoint, which had neither. A pending row older than an hour is
treated as abandoned rather than in progress, so a worker killed hard
enough to skip failed() cannot lock somebody out for good. Giving zip
builds their own queue is the structural fix and wants its own change:
it touches compose, supervisord and the systemd unit in INSTALL.md, and
an install that upgrades without changing its worker command would stop
building zips silently.
zip_downloads.requested_by cascades on delete, so removing a user takes
their rows with it and strands every archive they built — invisible to a
purge that walks rows, and to OrphanFileScanner, which skips zips/ on
purpose. The purge now also sweeps files in zips/ that no row explains,
after a day's grace so a build in progress is never taken out from under
itself.
Two smaller things while in here. A build that failed because every file
had already hit its download limit said only that nothing was available,
and dropped the skipped list — the same distinction the store guard goes
out of its way to draw at request time. And a failed close() now logs
libzip's reason, which the @ silencing had been discarding: "the disk is
full" and "the source vanished" are different problems for whoever has
to fix one, while the requester still sees a message with no server
paths in it.
update() validated preferences.*.type as ['required', 'string'], so any
string at all became a row in notification_preferences. Nothing reads it
afterwards: emailEnabledFor() looks preferences up by a key the registry
knows, so a row under an unknown key is invisible for good.
It is not a way into somebody else's settings — user_id comes from the
session, never the payload — which is why this is validation rather than
authorization. The cost is a table that quietly accumulates rows nobody
can see, explain, or remove through the interface.
edit() already knew the answer. It filters the registry down to the types
that can email at all, by either route, and renders exactly those as
toggles. That list is now derived once and used by both halves, so what
the screen offers and what it accepts back cannot drift apart.
Rejecting a registered-but-unmailable key (client_uploaded is the one in
tree) is deliberate rather than incidental: FilesServiceProvider explains
that it has no mail companion on purpose, so a preference row for it
could never change what anybody receives.
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.
Every list in /api/v1 answers "what is there now". Nothing answered
"what happened", and for the two events people most want to act on there
was nowhere to look at all.
Sharing a file writes an assignment row and never touches the file, so no
amount of polling /files?updated_since= will ever show a share. A
download is recorded only in the activity log. So the most requested
automations for a file-sharing product — tell me when a client gets a
file, tell me when they open it — were not possible to build.
GET /api/v1/activity is one feed rather than one endpoint per event,
because the log already records every one of them and a caller filtering
by action gets whatever the application grows later without waiting for
us to expose it.
It reuses what already exists: view_actions_log is the permission the
activity screen uses, and ActivityLogScope narrows the rows the same way,
so a staff member limited to their assigned clients cannot read the whole
installation's log through a token when the screen would not show it.
Two deliberate limits. Class names never reach the wire — subject.type is
a stable public string, or moving a model between namespaces would be a
breaking change to a frozen contract. And no ip_address, though the
column exists and the screen shows it: a person looking at a log has
decided to look, where an integration streams every row to somebody
else's servers by default.
PollingQuery grew an optional column so it can walk a table that is
appended to rather than edited. The parameter stays updated_since
everywhere, because on an append-only log the two timestamps are the same
thing and one shape learned once is worth more than a second name.
BuildZipDownloadJob deferred every write to ZipArchive::close() but then
marked the row STATUS_READY regardless of the result:
- close() returns false when a source file was deleted between addFile()
and close() (a concurrent staff delete runs FileDiskCleanup at once) or
the disk filled up; the row went ready over an archive libzip never
wrote, and the download controller X-Accel-served a path that isn't there.
- An archive that ended up with no entries (every selected file removed or
its allowance spent before the queued job ran) is written as no file at
all by libzip, yet close() still returns true — again marked ready.
Check both the close() return and the added-entry count, and fail the row
(deleting any partial archive) when either says nothing was written.
The job also had no $tries/$timeout/failed(): a build of up to MAX_FILES
sources runs past the worker's default 60s timeout, and the kill skips the
catch, stranding the row as PENDING while the frontend polls forever. Give
it room, run it once, and add a failed() backstop that fails a row still
pending (leaving an already-resolved one alone).
Finally, purge leftover zips/{id}.zip* by row id: a killed build leaves a
partial archive and libzip temp file with no path recorded, so the path
field alone never cleaned them up.
The Recent activity widget rebuilt each log entry inline instead of going
through ActivityPresenter, and the inline copy dropped `origin`. On the
frontend "System" and "Anonymous" are both actor_name null and only
`origin` tells them apart, so every actorless entry -- public and
share-link downloads, anonymous comments -- rendered as "System ...".
Present the entries through the shared ActivityPresenter, the same
sentence-ready shape the activity page and detail panels already use, so
the dashboard cannot drift from them again.
The orphan scanner skips derived artifacts by path prefix, but the list
was a hard-coded ['thumbnails/', 'zips/'] that never learned about
'previews/'. ImageRendition::Preview caches under previews/ (and
previews/external/) on the local files disk, so every cached preview was
reported as an orphan: offered for import on the orphans screen, and
deleted by the purge command once past the grace period. An imported
preview also became a File row pointing at a path the rendition cache
owns -- destroyed the moment its source file was deleted or the cache
was flushed.
Derive the rendition prefixes from ImageRendition::cases() rather than
repeating them, so a future rendition can't be forgotten here the way
previews were; 'zips/' (the download-bundle job's) stays as it was.
Setting folder_id through update() is the same privileged reparent as
move() and bulkUpdate(), but only those two verified the target folder
was inside the caller's library (StaffLibraryScope::folders). update()
validated it only for existence, so a client-scoped staff member could
reparent an in-scope file into a folder shared with a client they are
not assigned to -- which File::scopeVisibleToClient then exposes to that
client, sidestepping the boundary the sharing endpoints enforce
(guardAssignable), and likewise into a public folder without
upload_public.
Apply the same scope->folders()->findOrFail() guard on both the web and
API update(), but only when folder_id actually changes, so re-saving a
file that already sits in an out-of-scope folder (reachable via a direct
client share) still works.
store() checks Setting::MaxFileSizeMb against the size the client declares
when it opens the session, and complete() re-checks the storage quota
against the real assembled byte count -- but nothing re-checked the size
limit itself. A client that declared a one-byte upload and then streamed
gigabytes of parts passed store()'s check and was never stopped, so the
configured limit (which store() applies to everyone, staff included) did
not hold for the resumable path that real uploads use.
Re-check the assembled byte count against MaxFileSizeMb in complete(),
cleaning up the assembled bytes and the session exactly as the quota
branch already does.
StaffAccounts::delete() soft-deletes the row. What happens to the files
and folders that account owns is a separate collaborator, and a caller
that stops at the first one leaves them pointing at an account that no
longer exists.
The docblock mentioned "the content-reassignment step" in passing, as
context for the return value, which is not the same as saying it is
required. Worth stating outright because the mistake hides: validate()
returns an empty array when the account owns nothing, so an account with
no files deletes perfectly through delete() alone, and keeps doing so
until somebody deletes a colleague who had actually done some work.
Found while reviewing a design that was about to call delete() on its
own.
An actor with no personal access token has always meant a browser, and
for as long as a session and a Sanctum token were the only two ways to
authenticate, that was true. It stops being true the moment anything else
can, and the failure is silent: the action gets recorded as a person
clicking, in the one table whose whole purpose is answering "did I do
that, or did something acting for me?"
Nothing misreports today — every call site that passes an explicit actor
is a browser request, an API request whose actor carries the token, or a
console command with no actor at all. This closes the trap before the AI
connector in cloud-modules walks into it.
ActivityOrigin is a closed enum, so core has to publish both the case and
the hook before a package can use either. ResolvingActivityOrigin is
asked only in the ambiguous case: a request carrying a token is the API
and a request with nobody signed in is public or system, and neither is
in any doubt, so neither is offered — one package must not be able to
quietly relabel how every integration's actions are attributed.
The person stays the actor. They authorised it, and a log naming the
assistant instead would lose the only fact that matters. What the
connector was called goes in api_token_name, beside a null token id,
because that column means a row in personal_access_tokens and this is not
one.
The new origin is kept out of the activity filter unless the edition can
actually produce it. A filter option that can only ever return nothing is
a feature dangled at an edition that does not have it, which is the one
thing the edition boundary exists not to do.
The API has had everything Zapier needs since it shipped — a bearer
token, an auth-test endpoint at /me, and list endpoints that return
newest-first with a stable id, which is exactly the shape a polling
trigger wants. What was missing was anyone saying so.
This is written for somebody wiring up a Zap, not for somebody writing
code, which is why it is a second document rather than a section of the
guide. Same reason it renders in-app rather than linking to GitHub: the
installations most likely to need it are the ones least likely to have
outbound internet access.
It says out loud the two things that will otherwise be discovered the
hard way — a token expires within a year and nothing renews it, and a
deletion cannot start a Zap because polling cannot see one.
Both fixes are follow-ups to the mail providers @denkfabrik-li added in #1679.
A refresh token is good for exactly one use — Microsoft and Google both
retire it as they issue the next one. Two queue workers finding the same
expired access token would therefore both spend it, and the loser gets
invalid_grant back. That is the same answer a revoked grant gives, so a
healthy connection would be marked broken, painted red on the settings
page and mailed to every admin. Refreshes now hold a per-connection lock
and whoever waits re-reads the row, which normally means finding a token
the winner already stored and not refreshing at all.
The Connect button read the provider dropdown, but the flow it starts
uses the saved provider. On an installation with both vendors registered,
switching without saving would open the wrong consent screen. The
dropdown now counts as an unsaved change like any other field, which also
gives it the right "save first" hint for free.
Both sides added a .gitignore rule in the same place: this branch's
exception for docs/email-oauth.md, and main's block for the local dev
TLS material. Keep both.
#1680 fixed the redirect rendered from an exception and said plainly
what it did not cover: EnsureSetupIsComplete, EnsureAccountIsActive and
EnforceTwoFactor answer before HandleInertiaRequests is ever entered, so
a response they return never unwinds through Inertia's 302 to 303
upgrade either. Same 405, reached a different way — an account
deactivated while its owner was part-way through a form, or one being
made to enrol in two-factor.
The rule now lives in one place rather than four. Three copies of "if
the method is PUT, PATCH or DELETE" is how the fourth caller gets it
wrong, and WriteSafeRedirect can carry the explanation of why 303 —
which is worth more than the three lines it replaces, because nothing
about a bare setStatusCode call says what a browser does with a 302.
PUT /timezone is the route the setup test uses: it is one of only two
writes a guest can reach and the only one that middleware does not
exempt, so the case is real rather than defensive. All three new tests
were run against the unfixed middleware and fail there.
Extends the work of @denkfabrik-li, who found the gap and wrote it down.
The failure message names the disk, which reads as a credentials problem
even when the real cause is a bucket name that was never changed — the
exact confusion produced by switching an existing S3 configuration over
to Google and leaving the old bucket in the field.
Logged rather than shown, because 'throw' => false means the reason is
already gone by the time this code runs, and because the message goes to
whoever was uploading. That can be a client, and a bucket name is not
theirs to see.
Two bugs a green suite could not find, both from pointing the
application at a real Google Cloud Storage bucket.
The adapter attaches a legacy per-object ACL to every write, and a
bucket with uniform bucket-level access — which our own setup
instructions require, and which Google recommends — refuses it:
"Cannot insert legacy ACL for an object when uniform bucket-level access
is enabled". So the default configuration could not write to the
recommended bucket. The library ships
UniformBucketLevelAccessVisibility for exactly this, and nothing is
lost by never setting an ACL: every object here is private and every
read is a signed URL.
The second is worse and was never about Google. Both file disks are
configured 'throw' => false, so a refused write returns false rather
than raising, and LocalPartStore ignored the return. The upload reported
success, the File row was written, and the bytes were nowhere — the
listing showed a file whose download could never work. An expired S3
credential did the same thing. It now checks, and the controller already
turns that into a validation error rather than a 500, so the person
uploading is told.
Verified against a live bucket with a key scoped to
roles/storage.objectAdmin: the probe lists, writes land, reads
round-trip byte for byte, and a signed URL comes back 200 carrying
"Informe año.pdf" intact through both the ASCII and RFC 8187 forms of
Content-Disposition.
Cloud instances are given a bucket rather than configuring one, which is
the counterpart of StorageConfigure above it rather than a contradiction
of it: one edition points itself at storage, the other is pointed.
Only the declaration lives here. The behaviour is in the private
cloud-modules package, the same division Branding already uses, and
without that package the capability is inert and files stay on local
disk — so a self-hosted installation that somehow holds it is unchanged.
External storage meant S3 and nothing else, which is an odd hole for a
product whose users are as likely to be standing on Google Cloud as on
AWS — and paying to move bytes between two clouds to use this. The
Storage screen now asks which provider first, and the answer decides
which fields it shows, which it validates, and which driver the
files_external disk resolves to.
One disk, not two. files.disk is a stored column, so a third disk name
would fragment the data model and make every $file->disk consumer know
three names instead of two; the driver is swapped instead. A service
account key gets its own encrypted column rather than sharing `secret`,
because the two are validated, labelled and displayed differently and
one column meaning two things is how that goes wrong later.
Three things do not work by simply adding the adapter, and all three
fail quietly:
Laravel's temporaryUrl() looks for getTemporaryUrl() on the adapter,
while League's GCS adapter names it temporaryUrl(), so without the
registered callback every download and preview is a 500.
The two SDKs spell the signing options differently, and an unrecognised
one is dropped in silence — the symptom is a download named after the
storage key, not an exception. GoogleCloudStorageDriver translates, so
callers keep speaking one vocabulary, and the test asserts on the URL's
contents rather than on "a redirect happened", which is what would let
it regress.
That callback is also re-bound to the FilesystemAdapter before it runs,
so the translation is captured before registering rather than called as
$this->
`provider` is validated with 'sometimes', not 'required': absent means
S3, which is what every payload written before this choice meant, and
stops a browser holding a stale bundle from failing to save on a field
it cannot see.
Verified in a browser as well as in tests — which is how the null
provider on an unmigrated row was found, since the suite migrates and
never sees that state.
Two routes still assumed every file sits on local disk, which stopped
being true the moment external storage was switched on. A share link
answered with X-Accel-Redirect whatever the file's disk said, pointing
nginx at a path it has nothing behind; a public listing built a
thumbnail from Storage::disk('files')->path(), which for an externally
stored file is a path nobody ever wrote. Both fail only for installs
using S3, and only on those two routes, so the same file downloading
correctly from the file manager made the share link look like the
broken thing rather than where the file lives.
Neither is a new rule. FileDownloadController and
FileThumbnailController already did it right, which is the actual
finding: the knowledge was sitting in a private method on one class and
inline in another, so the next caller could not inherit it and did not.
Both are now objects with one job.
StoredFileResponse replaces InlineFileResponse and grows an
attachment() alongside inline(), since the two differ only by
disposition. LocalSourceFile takes a closure rather than returning a
path: the version that returned one also left the caller to unlink it,
and both of those are exactly the mistakes made here.
The regression tests fail against the previous controllers — checked in
both directions rather than assumed.
Ignacio Nelson