File::isExpired() documented the rule the whole application is supposed to follow: once past, the file is hidden from clients and the public site but staff keep full access. The second half is not true of a client-scoped staff member. StaffLibraryScope::buildFiles() builds their library as own uploads plus what each assigned client may see, and that second half runs through File::scopeVisibleToClient, which ends in notExpired() -- a client-side rule. So an expired file they held only through a client leaves their library and answers 403 on download, while their own expired upload stays and an unscoped administrator is unaffected. Api\FilesController stated it the same way, "Only the client branch of the visibility rules drops them", which reads as though a staff caller is unaffected when a client-scoped one is reached through that very branch.
This does not change that behaviour. c8078f65 weighed widening it and decided against, because scopeVisibleToClient is the single source of truth for client file access and the highest-stakes function to go changing for a dashboard widget, and relabelled the widget instead. That decision lived in a commit message and one widget's label; nothing in the code said it, and the docblock nearest the rule went on promising the opposite -- which is how the next person re-derives "staff keep full access" and widens the scope to match.
Documentation and characterisation only. isExpired() now states the boundary and why it is where it is, the API comment is corrected, and ExpiredFileStaffAccessTest pins all three cases.
Verified before merging: 3 passed on the trial-merge. The counter-check has to be inverted for a characterisation test -- these pass on unmodified main by construction, so the question is whether they fail when the boundary moves. Deleting the closing notExpired() from scopeVisibleToClient gives 1 failed / 2 passed, and it is the third case, the one carrying the decision, that falls. File.php overlaps #1726 and Api/FilesController.php overlaps #1727, both already merged, and both are intact in the merged tree. scramble:export reproduces the spec unchanged.
Reported and fixed by @denkfabrik-li.
File::isExpired() documents the rule the application is supposed to
follow: once past, the file is hidden from clients and the public site
"but staff keep full access to view, download, and manage it".
The second half is not true of a client-scoped staff member.
StaffLibraryScope::buildFiles() builds their library as their own uploads
union what each assigned client may see, and that second half runs
through File::scopeVisibleToClient, which ends in notExpired() -- a
client-side rule. Measured on main, with a rep holding one client and a
file the administrator uploaded and shared with that client:
before expiry in_library true GET .../download -> 200
after expiry in_library false GET .../download -> 403
the rep's own expired upload in_library true
an unscoped administrator, same expired file in_library true
Api\FilesController says it the same way -- "Only the client branch of
the visibility rules drops them" -- which reads as though a staff caller
is unaffected, when a client-scoped one is reached through that very
branch.
This does not change that behaviour. c8078f65 weighed exactly this and
decided against it: widening it 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. The widget was relabelled instead.
That decision lives in a commit message and in one widget's label.
Nothing in the code said it, and the docblock nearest the rule went on
promising the opposite -- which is how the next person re-derives "staff
keep full access" and widens something.
So both comments now state the boundary and why it is where it is, and
ExpiredFileStaffAccessTest makes it executable: an unscoped staff member
keeps an expired file, a client-scoped one keeps their own expired
upload, a client-scoped one loses a client's file when it expires.
Not changed: scopeVisibleToClient, StaffLibraryScope, and the
expired-files widget. If the boundary should move, that is a separate
conversation and a separate change.
Counter-check inverted, since these pass on unmodified main by
construction -- there is no behaviour fix for them to prove. What they
have to do is fail if the boundary moves, so the mutation is the widening
itself. Deleting the closing notExpired() call from scopeVisibleToClient
turns the file red, 1 failed / 2 passed, and it is the third case, the
one carrying the decision, that falls.
Suite 2108 passed / 2 skipped, 11416 assertions, PHPStan level 8 clean.
Measured on base 06c364d2, where main itself is 2105 / 2.
FileDiskCleanup wraps both deletions in one try. The first is the original
upload, on whatever disk the row names; the second is every cached
rendition, always on the local files disk. Storage::disk() throws outright
for a name with no configured driver -- which is the state the original's
disk is in whenever this fails at all -- so the catch swallowed it and the
renditions were never reached.
Nothing looks for them afterwards. OrphanFileScanner skips the rendition
directories on purpose (they are derived artifacts, never orphaned
uploads), so a file whose external disk had been removed or renamed kept
every cached copy of itself, indefinitely, on the disk that was working.
The two attempts are now separate, each with the same tolerance the class
was written for: a storage failure still never turns a delete click into a
500, and the warning is still the report.
While here, the comment in File::booted() that justifies deferring the
byte removal claimed "the worst case is bytes left on disk with no row,
which OrphanFileScanner already finds and reports". Not on this path: the
row is soft-deleted, and knownPaths() counts a trashed row's path as
claimed -- deliberately, so a scan never offers to double-adopt a file
still inside its erasure grace period. The comment now says what actually
happens.
One test: a file whose disk cannot be resolved loses its renditions. It
goes red without the fix, next to the existing test that the delete itself
still succeeds.
File::booted() removed the bytes the moment a row was deleted. For a
single file that is right. Two paths delete files inside a transaction,
though, and both delete many at once: FolderService::delete() takes a
folder's whole subtree, and DeletedAccountContent::cascadeDelete() takes
everything an account uploaded.
Anything that rolls either transaction back puts every row back while the
bytes are already gone. A transaction exists to make a set of writes
undoable, and removing the bytes was the one write in that set that
nothing can undo. The account path is the sharper one: since content
disposal is nested inside the caller's transaction, the write that fails
need not be in this code at all.
The two failure directions are not equal. Bytes gone with the rows
restored leaves rows pointing at nothing and no way back. Rows gone with
the bytes left leaves orphans on disk, which OrphanFileScanner already
exists to find. Defer to the recoverable one.
Three properties this relies on, all of them checked rather than assumed:
without a pending transaction the callback runs immediately, so a single
delete is unchanged; a savepoint committing inside a larger transaction
does not fire it, which is exactly the account case; and the connection
is the row's own rather than whichever is default.
detachOnDelete stays inside the transaction — it repairs the version
chain's pointers, which is database work that must roll back with
everything else.
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