mirror of
https://github.com/projectsend/projectsend.git
synced 2026-10-03 21:03:17 +00:00
docs/readme-projectsend-cloud
213 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 ignacionelson
|
530f30606d |
Let a plan take a capability away, and split branding from white-labelling
Groundwork for moving Branding out of the private package. Two changes, both about who decides what an installation may do. An edition grants capabilities; an operator may now take some away, via PROJECTSEND_CAPABILITIES_DISABLED. Subtractive only, and that asymmetry is the whole design: a variable that could *add* would put the hosted edition's proprietary screens one line of .env away on every self-hosted install, which is not a gate at all. So the list is intersected with what the edition already allows and can only make the answer smaller. This is not the plan tier core has always refused to invent. There are still no billing tiers here to key off -- the objection config/api.php makes about rate limits stands. It is the operator stating a fact about this installation, exactly as PROJECTSEND_PLATFORM_MAX_STAFF_USERS does for seats: the platform knows what it sold, the installation is told and enforces. Unknown keys are ignored rather than fatal, because the variable outlives both the plan that wrote it and the release that named the key, and refusing to boot over a stale one would be an outage on upgrade day. The registry takes the list as a constructor argument rather than reading config itself, which keeps it a value object testable without an application -- the failure that surfaced it was a unit test with no container. And branding.customize is now both editions, with the white-label half split into attribution.hide, which stays Cloud-only. Dressing an installation in its own logo is not a hosted concern; taking ProjectSend's name off somebody's public pages is what a hosted customer pays for. The gate on the second is not the key but that the only code able to answer "hide it" ships in the private package, so flipping an edition variable buys nothing. EnsureCapabilityMiddlewareTest had to pick a new Cloud-only example for the second time -- branding after users.manage. It now uses storage.managed, and records what to ask if it ever needs a third. The code move itself is the next commit; nothing user-visible changes yet, because the screens still live in cloud-modules. |
||
|
ignacionelson
|
d62c62f788 |
Let an installation say which build it is
A version string is a decision somebody made. A commit is a fact, and the two come apart exactly when it matters: an image built from the tag and one built from the branch that tag sits on carry the same version and different code. The fleet spent a day reporting 2.2.0 from images that were not the released 2.2.0, and nothing inside any of them could have said so -- which is why 2.2.1 was cut for a control plane rather than for users. So every artifact now carries config/build.php, written by build-release.sh and never committed, and projectsend:status reports it as `build`: the commit, the ref it describes to, the channel and the build time. All four are null on a source checkout, because there is no such file there. That is the honest answer rather than a missing one -- "I was not built" and "I will not say" are different facts, and this file's whole null discipline exists because a reader that cannot tell them apart eventually acts on the wrong one. An empty string is treated as no answer for the same reason: a build step that ran and produced nothing must not read as "answered" to anything checking presence. |
||
|
denkfabrik-li
|
7be81d3586 |
Tell the admins the mailbox is dead, even when a send noticed first
The daily refresh doubles as the health check for a connected OAuth mailbox, and its own docblock says why that matters: a grant can die silently, "which for a portal whose password-reset mails ride on this connection must surface as a warning, not as a support ticket weeks later". It decided whether to warn by reading last_error -- but the send path writes that column too. OAuthCodeFlowBroker::refresh() records the failure and notifies nobody, and freshAccessToken() reaches it from every send. So on an installation that actually sends mail, the send lands first, the command reads the column as "already told them", and the warning never goes out. last_error is cleared only by a successful refresh, which a dead grant never has, so it never goes out again either. Measured on main, one dead grant, two orders: nobody sends, command first 1 notification, then quiet correct a password-reset mail first 0 ... 0 ... 0 never The alarm worked on installations that were not using the mailbox and failed on the ones that were. The anti-nag rule is not the problem and does not change. The problem is that last_error answers "is this broken", which any writer may set, while the command needs "have the admins been told", which only the notifier can. The table's own comment shows the conflation -- one column described as "what the settings page's warning and the admin notification read". So the notification gets its own column. broken_notified_at is stamped when the command notifies, and cleared wherever last_error is cleared: a successful refresh, a disconnect, a changed client id. The three call sites go through MailOAuthConnection::clearFailure() rather than nulling two columns each, because a connection left marked "already told them" while healthy would go quiet the next time it died -- the same bug in a new place. |
||
|
denkfabrik-li
|
92f50fdb85 |
Read a comment's author even after the account is deleted
`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. |
||
|
denkfabrik-li
|
27c289a4d6 |
Let a password reset know where the account's credentials live
Two accounts reach the same reset with opposite needs, and it treated both as "write a hash and hope". A provider-created account is told, on the Connected accounts screen, to "set a password first, then disconnect Google" -- and doing it changed nothing, because nothing ever set auth_source back to Local. AccountConversion is the only writer, and that is an administrator. So the screen went on asking for something that had already been done, and the person could not release their last provider without help. AuthSource states the rule that closes this: `social` means the account came into existence without anybody choosing a password, and, in as many words, "a social account may later set a real password". A reset by emailed token is where somebody does. The screen's has_local_password prop is literally auth_source === Local, so the write is what completes the sentence it prints. A directory account is the opposite case and gets the opposite answer. isDirectoryAccount() means the local hash is not consulted at all, so the reset reported success and left the person with a password that cannot sign them in -- including when the directory it points at is gone, which is exactly when somebody reaches for a reset. It is refused now, with the reason, and nothing about the account moves: taking one off its directory is an administrator's decision through AccountConversion, not a side effect of a reset. The refusal sits where the token has already been validated, not where the link is asked for. That endpoint answers "A reset link will be sent if the account exists" to everybody on purpose, and refusing there would tell a stranger both that an address is an account and how it signs in. Throwing before the write also leaves the token unspent, since PasswordBroker deletes it after the callback returns. |
||
|
denkfabrik-li
|
5e60d2ef88 |
Say what expiry does to a client-scoped staff member's library
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.
|
||
|
denkfabrik-li
|
21cae2acb1 |
Stop a version link telling people about a file they already had
FileVersions::link() resolves its notification audience before the merge,
and says why:
RESOLVED BEFORE THE MERGE, and the ordering is the whole dedupe:
these are the people who could already see both files, so anyone the
merge below is about to reach for the first time is excluded here and
gets file_shared from FileSharing::assign() instead. Resolve it
afterwards and every newly-added client receives two emails about one
action.
The merge then undoes it. moveAssignmentsToRoot() hands every one of the
revision's targets to FileSharing::assign(), under a comment claiming
that firstOrCreate makes a target the root already has a no-op. It makes
the assignment row idempotent; the three side effects under it --
activity entry, in-app notification, digest -- run unconditionally.
Measured on main:
client already holds the root and the revision, then both are linked
file_shared (Report) <- wrong, they have had it all along
file_new_version (Report v2) <- right
assignment rows on the root: 1
client holds only the revision, then both are linked
file_shared (Report) <- right, the merge does hand it over
Two notifications for one action, for exactly the people the early
resolve was meant to protect.
So a target the root already holds is skipped rather than handed to
assign(). Nobody is gaining access in that case, and the activity entry
would be as untrue as the notification. copyAssignmentsFrom() directly
below already states that rule for its own case, which is why it inserts
directly instead of going through FileSharing. Both stale comments are
corrected with it.
Not changed: FileSharing::assign() itself, and so the behaviour
ShareNotificationsTest pins -- re-posting an existing assignment through
the share endpoint still notifies again. That test names the condition
for ever changing it, "it should stop being sent for both at once", and
that is a decision about files and folders together. This is narrower: a
version merge is not somebody choosing to share again, and it already
had a stated intent to send exactly one notification.
Three cases in ShareNotificationsTest -- the target already on the root,
the target gaining it, and a group already on the root. Reverting
FileVersions alone leaves 2 failed / 8 passed in that file; the middle
case passes without the fix, because it guards against skipping too much
rather than against the duplicate notice.
Suite 2108 passed / 2 skipped, 11415 assertions, PHPStan level 8 clean.
Measured on base
|
||
|
denkfabrik-li
|
fc5651faad |
Check the read half of the redirect rule at every door, not one
Three middleware answer before HandleInertiaRequests and so have to repeat its 302→303 upgrade themselves: EnsureSetupIsComplete, EnsureUserIsActive and EnforceTwoFactor. This file has a write case for each, and the rule has a second half -- a read still gets a plain 302, because a 303 there would be an upgrade nobody asked for. That half was checked once, on the deactivation door, under a name that said otherwise: "leaves a read alone in every one of those cases". The setup door and the two-factor door were not covered at all, so a change that upgraded reads at either of them would have gone through with the suite green and this test's name still claiming it would not. Both are covered now, as a dataset with one case per door. The setup case reads a guest-reachable GET for the same reason the write case posts to /timezone: anything behind `auth` is answered by the guest redirect before EnsureSetupIsComplete sees it. No production code changes; today all three doors answer a read with 302, which is what the new cases assert. Demonstrated by mutation rather than reversion: making EnsureSetupIsComplete upgrade every redirect to 303 fails this file (1 failed / 8 passed) and passes the old one (7 passed). |
||
|
denkfabrik-li
|
b838036a9a |
Set the directory permission Flysystem actually reads
FILES_WEB_SERVER_READABLE asks for 0755 on the directories a download has to be traversed through, and asks for it from a key that is never consulted. FilesystemManager::createLocalDriver passes `directory_visibility ?? visibility ?? private` to PortableVisibilityConverter::fromArray() as the default visibility for directories. This disk sets `visibility` to public two lines above, and no `directory_visibility`, so directories are public and the converter reads `dir.public`. The configuration names only `dir.private`. The mode is 0755 regardless, because 0755 is Flysystem's default for a public directory -- the right answer from the wrong place. Adding `directory_visibility` to this disk, or a change to that default, is all it would take for the flag to stop doing what it says. Measured on main, with the flag on: dir.private 0755 → 0750 directory stays 0755 (nothing reads it) dir.public 0755 → 0750 directory becomes 0750 (this is the key) Both are named now, so the intent survives either way round. FilePermissionsTest could not have caught this, because it was not testing this configuration. filesDiskWith() restated the shipped branch inline, verbatim down to the 0755, so it went on passing against its own copy however the real one changed. It now requires config/filesystems.php and replaces only the root, which is what makes the mutation above visible to it. Two more things in the same helper, both about the suite rather than the subject: the scratch root is per worker now (Tests\TestCase does the same for upload parts, and eight workers sharing one directory means one worker's afterEach deletes another's tree mid-test), and it is cleared before each test as well as after, so a killed run does not poison the next one. |
||
|
denkfabrik-li
|
02eafb473b |
Refresh a mailbox on the schedule under the lock a send would hold
freshAccessToken() serialises refreshes per connection, and its comment says why: both providers rotate the refresh token as they hand out an access token, so the token is good for exactly one use, and "a worker racing the nightly refresh command means the slower one spends a token the faster one has already replaced. The provider answers that with invalid_grant, which is the same thing it says about a genuinely revoked grant: last_error gets written, the settings page turns red, and every admin is told to go and re-consent a connection that was never broken." The nightly refresh command called refresh() directly, outside that lock. It was the racer the comment names, not a party to the arrangement it describes. It now goes through refreshSerially(), which takes the same lock -- named once, in one place, for both callers -- re-reads the row inside it, and refreshes. Unlike freshAccessToken() it refreshes a token that is still usable, which is the point of the daily run: a delegated refresh token dies of disuse and this keeps the window sliding. The lock is taken rather than waited for, unlike the send path. Nobody is standing at a screen for a scheduled job, and a held lock means somebody is refreshing this very connection right now -- which slides the window and establishes its health just as well as doing it again would. One test: with the lock held, the command sends no token request and leaves the connection untouched. Without the fix it spends the refresh token the holder is already spending. |
||
|
denkfabrik-li
|
674781e57a |
Claim a TOTP code atomically instead of checking then writing
verify() asked Cache::has(), verified, then Cache::put(). Between the read and the write the key is free, so two requests carrying the same code could both be told yes -- which is exactly what the replay guard exists to prevent, and the window an intercepted code has is the whole of its validity either side. The claim is now the answer: Cache::add() writes only if the key is absent, so of two requests carrying the same valid code exactly one gets true back. That is the same mechanism, for the same reason, as the preview log's debounce -- "Cache::add is the whole mechanism: it writes only if the key is absent ... without a read-then-write race between two of them". Verification still happens first, so a wrong code never touches the cache and cannot burn the window for the code the person is about to type correctly. One test, modelling the interleaving it is about: the winner's claim has landed, and the loser's has() answers from before that write. Without the fix the loser is signed in. |
||
|
denkfabrik-li
|
a1773cad5e |
Count a shared folder's contents as reach, not just the folder
groupReachesNoFurther() asks whether anything shared with a group sits outside the viewer's library. Its docblock says the folder half covers "the folders whose subtrees it can browse". It compares the folder ids the assignment names and stops there. A folder shared with a group hands its members the whole subtree -- File::scopeVisibleToClient matches on folder placement, and a folder is visible to a client when it or an ancestor is shared with them. So the guard passed on a subtree it had never looked into. Measured on main: a scoped rep's own folder, a subfolder somebody else created inside it, and that person's file in the subfolder. parent in the rep's library true subfolder in it false the file in it false add their own client to a group holding the parent 302, allowed the client can then reach the file true And because files() is "own uploads plus everything my clients can see", the file lands in the rep's own library on the next request. That is the widening this guard exists to refuse -- the first test in the file is called "a scoped staff member cannot widen their own library through a group". The folder half now walks each assigned folder's subtree, and the files inside it are checked too: a folder can be in the library while a file in it is not, since somebody else's upload into a folder this rep owns is neither their own nor their clients'. Expired files are skipped for the reason the deleted ones are -- membership grants nobody access to one. Three tests: the subfolder case, the stranger-file case, and a subtree wholly inside the library, which stays manageable. The first two go red without the fix. |
||
|
denkfabrik-li
|
17fc9ff4cb |
Compare the transfers window against the column's own timezone
resolveTransferRange() builds every boundary in the viewer's zone, which is right and deliberate: "last week" should end when their evening does. Its docblock then claims the instants "compare against the UTC column directly". They do not. The query builder formats a Carbon in whatever zone the object carries and drops the offset, so the viewer's midnight arrives at the database as a UTC string. For Asia/Tokyo, measured: the instant the window really starts 2026-08-21T15:00:00+00:00 what the query asked for 2026-08-22 00:00:00 Nine hours at each end, in the same direction: the first nine hours of the viewer's window are missing from the chart, and the last nine hours of somebody else's day are counted into it. Every zone east or west of UTC gets a chart that is quietly wrong at both edges, which is worse than one that is obviously wrong. The comparison now converts; the day cursor a few lines below does not, because that half genuinely is about the viewer's calendar and is what puts an evening upload on the right bar. One test, in Asia/Tokyo, with an upload in the first hour of the viewer's window. Without the fix it is missing from the chart. |
||
|
denkfabrik-li
|
776d3d99f4 |
Put a client on the roster of the scoped staff member who created them
A client-scoped staff member with create_clients creates a client and loses it immediately. guardTarget() answers 404 for anything off their roster, and StaffLibraryScope::clients() leaves it out of their list -- so the record exists, is logged, is welcomed by email, and is invisible to the person who made it. store() redirects to the edit page, which is where they land: POST /clients → 302 → /clients/4 on_creator_roster → false GET /clients/4/edit → 404 clients listed → ["Mine"] the new client is not there Their own roster is where a client they created belongs, so it is attached there. An unscoped creator gains nothing: they see every client already, and a roster entry would change what assignedClients means for them. The API twin does the same, for the same reason -- a scoped token gets a 404 from every route that binds the client it just created. Three tests: the scoped creator can open and list the client, an unscoped creator gains no roster entry, and the API twin behaves like the web. The first and third go red without the fix. |
||
|
denkfabrik-li
|
9ddd39c41d |
Refuse to provision over a deleted account's address instead of crashing
The unique index on `email` spans soft-deleted rows -- AvailableEmailRule is built on exactly that, so a deleted account keeps its address until erasure removes the row. The registration form learns this from validation. The machine paths have no form: a directory or an identity provider hands over an address and provision() inserts it. Measured on main, a client deleted last week signing in through a provider that may auto-provision: GET /auth/google/callback → 500 (QueryException, unique constraint) Same shape through LDAP at POST /login. Nothing is created, nothing is signed in, and what the person meets is a server error. Both provisioners now ask ClientProvisioning::addressIsFree() first and refuse. The social flow already has a refusal for an identity it cannot provision -- "There is no account here for that address." -- which is also all a stranger should learn: whether an address was once an account here is not the provider's to publish. The LDAP flow falls through to the ordinary failed sign-in. Deliberately not resurrecting the deleted account. Restoring one because a directory says the address exists is a decision for a person, not a side effect of somebody logging in. Two tests, one per path: the sign-in is refused, nothing is created, and the trashed row is still trashed. Both go red without the fix. |
||
|
denkfabrik-li
|
19c449ee20 |
Stop an editable-once checkbox locking before anybody ticks it
save() writes '0' for an unticked checkbox, and filled('0') is true in
Laravel -- so isLocked(), which asks whether anything is stored, locked
the field the first time the client saved the page it sits on, whatever
they had chosen. A box they never ticked could then never be ticked, and
the one edit the setting promises was spent on a decision they had not
made.
A text field left empty stores null and stays open. That asymmetry is the
bug: '0' is the absence of a decision, which is what null means for every
other type.
So a checkbox locks on a stored '1' and nothing else. Everything else is
unchanged, including the existing case of a client ticking the box and
then being unable to untick it.
Two tests: an unrelated save leaves the box open and the tick that follows
still lands and locks it; and an editable-once text field behaves exactly
as before. Without the fix the first goes red.
|
||
|
denkfabrik-li
|
4b998cda92 |
Fail a zip build without handing the requester the server's reason
The write-failure branch already draws the line and says why: "What the requester sees stays generic: a libzip string means nothing to them and can name a server path. An operator needs the opposite ... so the reason goes to the log instead." Thirty-seven lines below it, the catch-all around the whole build stored $e->getMessage() in the row the requester polls. Measured, a client asking for an archive of a file on a disk that is no longer configured was told: "Disk [a-disk-that-is-not-configured] does not have a configured driver." The reason now goes to the log with the exception class, and the row carries the same kind of sentence fail() already uses. Second, the temp files. tempnam() creates the file, and $tempFiles[] was appended only after the copy had finished -- so every throw in between (a disk that will not resolve, a stream that will not open) left a zip-src- file in the system temp directory that nothing ever removes. It is now registered the moment it exists. Third, in the same method: the copy itself was unchecked. A copy that stops early is a truncated member added to the archive as though it were the file, so the build reports ready and the recipient gets something that opens and is wrong. Both the copy and the fclose that flushes it are checked now, and both handles close on every path. Two tests: the failure message names nothing about the server, and a build that throws mid-copy leaves no temp file behind. Both go red without the fix. |
||
|
denkfabrik-li
|
4164678ebc |
Delete a file's renditions even when its own disk cannot be resolved
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. |
||
|
denkfabrik-li
|
fc758c701a |
Write a rendition through a temporary file, and never serve an empty one
Both thumbnail routes treat "the file exists" as "the rendition is cached", and nothing ever invalidates one: RenderedImageCache::flush() runs on ImageRenderingChanged, which no core code raises. Whatever is at the path is what every later viewer gets. ThumbnailGenerator encoded straight onto that path. A render that died partway -- a full volume, a killed worker -- left a half-written file that was then served as the rendition for good, and two requests rendering the same file at once encoded into one path together. It now writes beside the destination and renames into place. rename() within a directory is atomic and replaces what is there, so the path is either the previous rendition or a complete new one, and the loser of a race leaves a whole image rather than a mixture of two. The temporary file is removed on the way out either way. The read side gets the other half: an empty file is not a rendition, so both routes replace one rather than serve it. Writing through a temporary file means this state can no longer be created here, but an installation that ran an older version can already have it on disk, and nothing else will ever clear it. Three tests: an empty rendition is replaced on the signed-in route and on the public one, and a successful render leaves nothing half-written behind. Without the fix the first two go red; the third is about the fix's own temporary file and passes either way. |
||
|
denkfabrik-li
|
250e8664d3 |
Stop a client PATCH clearing custom fields it never mentioned
update() states the rule eighteen lines above the bug: "PATCH semantics, unlike the web form which always submits every field: an absent key means 'leave alone', not 'clear'." Every column obeys it. The custom fields did not, because they went through create()'s pass, which walks every field there is and writes null for the ones the request did not carry. Two fields filled, a PATCH naming one: status → 200 named field → "Robin" the other one → null (was "ATU12345678") Nothing says so in the response, and there is no other copy of the value. The write pass is now shared but entered two ways: create() keeps writing every field, since a new client has no values and a checkbox nobody ticked is a recorded "no"; update() writes only the fields the request named. Three tests: the untouched field survives, a named empty value still clears, and create still records every field. Without the fix the first goes red. |
||
|
denkfabrik-li
|
640c5db591 |
Stop an expiry moving because somebody else saved the file
The edit form is given a file's expiry as a calendar date, read back in the viewer's own zone -- deliberately, so a file set to expire on the 12th does not reopen showing the 11th. Every save posts that date back, whether or not anybody touched it, and update() derived a fresh instant from it every time. So the expiry drifts by the difference between two people's zones on any other edit. A date set from Pacific/Auckland stores 2026-09-12T11:59:59Z; a colleague in UTC-3 opens the file, sees the same 12th, renames it, and the file now expires at 2026-09-13T06:59:59Z -- 19 hours later, with nobody having gone near the date. The instant is now re-derived only when the posted date differs from the one the form was given, compared against the same string through a named pair: expiryDateFor() renders it, expiryInstant() reads it back. The edit screen uses the same method it is compared against, so the two cannot drift apart. bulkUpdate() needs nothing: its expiry is an explicit set/clear/no_change action, so an untouched expiry is never posted in the first place. Three tests: the rename leaves the instant alone, a real change still lands in the editor's own zone, and clearing still clears. Without the fix the first goes red. |
||
|
denkfabrik-li
|
e1cd010f9d |
Give an API expiry date the same meaning the web gives it
FilesController::expiryInstant exists because a calendar day ends where the person naming it lives: the web form posts a bare YYYY-MM-DD, and storing that as it arrives would cut a file off at midnight UTC -- "expires on the 12th" ending partway through the 11th for anyone in the Americas. The API takes the same field, validates it as a date, and stores it raw: web → 2026-09-12T23:59:59+00:00 (end of the day, as the docblock means) API → 2026-09-12T00:00:00+00:00 (raw) Same value, same field, same file, two meanings -- and the earlier of the two is a file that dies at the start of the day it was promised. A bare date now means the end of that day in the caller's timezone, as it does on the web. A value carrying a time is unchanged: it is an instant the caller named on purpose, the API can express one and a date input cannot. The endpoint's docblock says both, so the OpenAPI document does too. Three tests: the day, the timestamp, and clearing. Without the fix the first goes red. |
||
|
denkfabrik-li
|
c2dd2c758a |
Debounce the public preview log the way the signed-in one already is
FileThumbnailController::preview() writes at most one FilePreviewed row per viewer per file per five minutes, because a browser turns one video into a long tail of Range requests against the same URL. Its docblock names the anonymous route as the place the same act happens without an account -- and that route logs unconditionally. Measured: five requests for the same public file, five PublicFilePreviewed rows, against one for the signed-in twin. One visitor watching one clip buries the public half of the activity log, which is also the half an operator reads to see what the outside world is doing. The window is now a shared PreviewLog, next to PreviewKind, which the two preview routes already share for the same reason. Keying is unchanged for a signed-in viewer; an anonymous one has no account to key on, so the request IP stands in -- the same substitute the API's rate limiter makes for an unauthenticated caller. It is a cache key with a five-minute life and never reaches the log, which keeps its own decision about recording an IP (ActivityLogger::shouldRecordIp, Setting::DownloadIpLogging). Three tests: the replay is one row, two visitors are two rows, and the window is per file. Without the fix the first goes red. |
||
|
denkfabrik-li
|
9d4b096c19 |
Narrow the reassignment picker to what a viewer may see
`reassign_candidates` is the delete dialog's picker: every active account
in the installation, by name and by role label. The same list is shared
on the clients index, the users index, both edit screens and privacy
settings, and it was narrowed by nothing.
Two lines above it on the clients index sits the listing itself, narrowed
through `scope->clients($viewer)` with a comment saying why: "a
client-scoped staff member is not shown the name and email of somebody
they can reach nothing of". The picker beside it handed over every client
in the installation, plus every staff account and its role name. The
filter by `can('delete_clients')` happens in React, which decides what is
rendered, not what is sent.
So the client half of the candidate list goes through the same
StaffLibraryScope as the listing, and each screen sends the picker only to
a viewer holding the delete permission it exists for. Staff accounts are
not narrowed -- they are not narrowed anywhere else either -- and an
unscoped viewer's list is unchanged, because StaffLibraryScope::clients()
returns every client for them.
Privacy settings keeps the whole installation on purpose: that picker sets
the erasure default stored once for everybody, behind edit_settings, so
narrowing it by whoever happens to be editing would store the wrong
answer. The parameter is nullable for that one caller, and the docblock
says so.
Four tests. Without the fix three go red; the fourth is the guard that an
administrator still sees every active account.
|
||
|
denkfabrik-li
|
763777d282 |
Say which permission a bulk edit was actually missing
Two different things stop a selected file being changed, and bulkUpdate()
reported both as the first one.
Files dropped by the Gate::allows('update') filter are ones the staff
member may not edit at all. A file that survives the filter and still
changes nothing is a different case: it was editable, and every field they
asked to change was one their role does not let them set -- expiry,
download limit, categories, each behind its own permission, exactly as the
single-file editor treats them.
Measured with edit_files but without set_file_expiration_date, three files
they own, expiry the only change: "0 of 3 selected files were updated. The
rest were skipped because you don't have permission to edit them." They
own all three and editing is precisely what they may do, so the sentence is
both wrong and unactionable.
The two cases now have their own sentences. The existing string is kept
for the case it describes -- every skip a file they may not edit -- so its
sixteen translations stay in use. The new one covers a field permission,
and covers a mixture of both reasons, since "permission to make those
changes" is true either way.
The new key is English only; a locale without it falls back to English,
which is a translated-but-wrong sentence traded for an untranslated
correct one.
Three tests: each reason on its own, and the mixture. Without the fix the
first and third go red.
|
||
|
denkfabrik-li
|
f424fe5365 |
Decide what is an API request from the route, not from the caller's headers
Two places asked "is this the API?" and got it wrong in opposite ways.
EnsureCapability asked $request->expectsJson(). Whether a feature exists
in this installation's edition is a property of the installation, not of
what the caller is willing to parse, so the same route answered
differently per header: `Accept: application/json` got the 403
`capability_unavailable` routes/api.php promises, `Accept: */*` -- curl's
default -- got a bare 404 `not_found`. The mirror image is worse: an
Inertia visit to a capability-gated *web* screen accepts JSON, so it got
403 with Laravel's default error body, naming the exception class, where
the point of the 404 is that an unavailable feature is absent rather than
teased.
ProblemDetails asked $request->is('api/*'). Two staff pages live under
that prefix -- the API dashboard at /api and the OpenAPI reference at
/api/docs, both registered in routes/web.php -- so a signed-out visitor to
either got 401 problem+json, "Send a valid API token in the Authorization
header as \"Bearer <token>\"", instead of the login redirect every other
page gives them.
Both now ask App\Support\ApiSurface: under the API prefix, and not part of
the `web` middleware group. The group is what actually separates the two
-- sessions and CSRF on one side, tokens on the other -- and it keeps
answering correctly for a future /api/v2 without being edited. An
unmatched path has no route to ask, which is the API's answer anyway: a
404 under its prefix is one it should describe in its own format, and the
existing test for that stays green.
Three tests, in the two files that already own these rules. Without the
fix all three go red.
|
||
|
denkfabrik-li
|
cd8da6a117 |
Name the quota a client is actually held to when an upload is refused
Both chunked-upload quota checks resolve the limit through ClientStorageUsage::quotaBytes(), which falls back to the site default when a client has no quota of their own -- and then print `$user->storage_quota_mb` in the rejection. For every client who was never given an explicit quota that column is 0, so the message reads "This upload would exceed your storage quota of 0 MB." at the one moment somebody is trying to find out what their limit is. The API's single-request upload already prints `$this->storageUsage->quotaMb($user)` for the same sentence (Api/FilesController.php:208). The two chunked copies now do the same. Three tests: the inherited default is named at session creation and again at completion, and a client with a quota of their own still sees their own number. Without the fix the first two go red, the third stays green. |
||
|
denkfabrik-li
|
db1dd71f3c |
Stop an expired file locking a group shut for a scoped staff member
groupReachesNoFurther() asks whether anything shared with a group sits
outside the viewer's library. `f1b35cc9` established the shape of the
answer for deleted files: start from the live row, because "a deleted file
is not reach, because nobody can reach it".
An expired file is the same case. Membership grants nobody access to it --
File::scopeVisibleToClient ends in notExpired(), so it has left every
member's /my-files and the download answers 403 -- but it is equally gone
from files(), where its absence reads as "outside my library". The group
then locks for a scoped staff member: they cannot add a member, cannot
rename it, and cannot remove their own client again.
So the reach query skips expired files as it already skips deleted ones.
Expiry is reversible where deletion is not, and that needs no special
handling: the guard asks what is reachable at the moment somebody is added
or removed, and the file counts again the moment it stops being expired.
Not changed: File::scopeVisibleToClient, whose treatment of expiry was
settled deliberately in
|
||
|
denkfabrik-li
|
c8de16101f |
Gate the comment moderation surfaces on reading, not just on the library
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.
|
||
|
denkfabrik-li
|
cb53120779 |
Show the portal dashboard the files a client can actually open
clientDashboard() restates the assignment half of File::scopeVisibleToClient in a whereHas of its own. The scope is the single source of truth for client file access and ends in notExpired(), which the copy leaves off, so the two disagree in both directions. Over: an expired file stays counted and keeps its name on the dashboard after /my-files has stopped listing it and the download answers 403. Under: everything that reaches a client another way is missing -- a file inside a folder shared with them, a file they uploaded through the portal themselves, and a revision, which owns no assignment row at all and inherits its original's recipients through SharingIdentity. Replaced by the scope itself, which is what /my-files runs. The existing test for the page is unchanged and still passes: a directly assigned, unexpired file counts exactly as before. Two tests, one for each direction. Without the fix both go red. |
||
|
denkfabrik-li
|
84e9f6e2fe |
Scope the API dashboard's recent actions to what the viewer may read
ApiUsage::recentActions() is the only ActivityLog query outside ActivityLogger and AccountEraser that does not run through ActivityLogScope::apply(). Its whole boundary is view_actions_log -- the permission whose own scope class says, in as many words, that it "is not the whole answer for a client-scoped staff member". The Client Manager system role is client_scoped and ships with that permission, so this is the default configuration. Such a viewer opening /api?all=1 reads the fifteen most recent API log rows for the entire installation, each with its subject_name: the names of files and clients they get a 403 on. /activity, the download history and the dashboard's recent-activity widget all narrow the same rows; the API dashboard was missed. The scope is applied on both sides of the install-wide branch. The own-actor filter for the narrow view already stays inside what the scope allows, and a boundary that exists in only one arm of an `if` is one refactor away from not existing. Three tests: the scoped viewer sees only the entry about a file in their library, their own actions stay whole even when the subject is outside it, and an unscoped viewer's feed is unchanged. Without the fix the first goes red; the other two are green either way and guard against narrowing too far. |
||
|
ignacionelson
|
06c364d29a |
Report storage, health and what packages loaded in projectsend:status
Five more facts for whatever watches an installation from outside the container, and one seam so a package can add its own. Storage is the one that was about to be wrong. It is summed from the rows that record it, not measured on the volume: measuring the directory was correct until external storage went live and silently stopped being, since an upload that resolves to a bucket leaves nothing on disk to measure. A figure taken from the filesystem freezes while the account keeps filling, and on a managed installation that figure is what a customer is shown and billed against. `by_disk` splits the same sum by where the bytes went, which is the only way to see what is still sitting locally from before a cutover. Trashed files are excluded because they hold no bytes -- File's deleted hook takes them. Health is what a container cannot show from outside. A queue worker dying is invisible to anything watching the process: it is still up, and zips quietly stop building while mail stops going out. Same for a deploy whose migrations failed -- the application answers every request and is a schema behind. An unreachable queue reports null rather than zero, because an unreachable Redis is not an empty queue and reading the second as the first is how a dead worker looks healthy. The two-factor enforcement setting is echoed back the way EnforceTwoFactor reads it, fallback included: reporting a stricter rule than the middleware actually applies would be worse than reporting none. And ResolvingInstallationStatus, so a package can report what core cannot know. The managed storage backend and the version of the package providing it live in cloud-modules, which this repository must not reference, and a platform that writes eight environment variables only ever knows what it asked for. Those came apart once: a bucket provisioned, a token minted, every variable correct, and an image whose copy of the package predated the module that reads them. Files went to local disk with the configuration sitting perfectly right beside them. Two shapes are cast to objects deliberately. An empty PHP array encodes as [], so an installation with no packages -- or holding no files -- would answer a map-shaped field with a list, and a reader unmarshalling it breaks on the day it happens to be empty rather than the day it is written. There is a test for each. Requested by the ProjectSend Cloud control plane, whose storage figure stops growing the moment a tenant's uploads start reaching the bucket. |
||
|
Ignacio Nelson
|
046be36861 |
Merge pull request #1710 from denkfabrik-li/fix/folder-delete-file-authority
FoldersController::destroy() authorized delete on the folder and nothing else, while FolderService::delete() soft-deletes every file in the subtree and File's deleted hook takes the bytes off disk. So a staff member refused a file one route over could destroy it by deleting the folder around it -- permission and library boundary both unasked. MyFoldersController::destroy() already draws this line for the client half of the same cascade, and says why: owning the folder is not authority over content someone else put in it. This is the staff half of that sentence. Verified before merging: the four bug tests fail on main and pass here, and the SQL predicate was read line by line against FilePolicy::delete -- it is a faithful negation, including the null-uploader case and the short-circuit for an unscoped viewer holding both delete permissions. Membership of the check is one COUNT, not a policy call per file. Suite at 2099, PHPStan clean. Behaviour change, deliberately accepted: a folder delete that used to succeed now refuses, naming how many files are in the way. The likely case is somebody who owns a folder another account uploaded into. The alternative is irreversible loss of files the same person is refused individually. Not taken: deleting what the actor may and keeping the rest. Half a tree is worse than either answer. Naming the blocking files would be friendlier than counting them and is worth doing later -- the list has to hide any file the viewer cannot see, which is its own small design question. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
4a35c25894 |
Merge pull request #1717 from denkfabrik-li/fix/deleted-client-comment-context
file_comments.client_context_id is cascadeOnDelete, but users are soft-deleted, so the cascade never fires: the column goes on pointing at a row that is still there while the relation resolves to null. resolveClientContext() branched on the relation, so "this is Alice's conversation" read as "this has no conversation" -- and a null context on a clients comment is the branch every client on the file reads. A staff reply into a departed client's private thread became a circular, and canAssignClient() was skipped on the way. That is the invariant docs/feature-comments.md calls the rule everything hangs off: a clients comment carrying client_context_id = C is never returned to any non-staff viewer other than C, because one customer learning another exists is worse than leaking a comment's text. Verified before merging: both new tests are red on main and green here, and the three that must not move stay green either way. Suite at 2093, PHPStan clean. The second half is the same root cause through the other column. authorName() read a deleted client's comment as "Anonymous", which is what a visitor's comment looks like -- and a visitor's comment is governed by different rules, so the two must not be able to look the same. Whether the author is a visitor is now decided by author_id alone, the question isFromGuest() already asks. Accepted consequence: a soft-deleted client's name is visible on their old comments during the erasure grace period, where it previously read as Anonymous. It goes for good when erasure removes the row. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
58497ef776 |
Merge pull request #1716 from denkfabrik-li/fix/sole-administrator-self-deletion
ProfileController::destroy() validated the current password and soft-deleted, without asking guardLastAdministrator() -- the rule the other four doors ask, at the one door where the account being removed is certainly signed in. The sole administrator could empty their own installation, and EnsureSetupIsComplete, which asks exists() and so skips trashed rows, then handed the first-run setup form to whoever loaded the page next. That form creates an active System Administrator, unauthenticated. Verified before merging: on main the sole administrator's self-deletion succeeds and setup reopens; both new tests are red there and green here. Suite at 2088, PHPStan clean. Two locks, because one of these questions is asked at five doors and the other at one. The guard closes the door. And "has this installation been set up" stops meaning "does it have a working administrator right now" -- a trashed staff row is still evidence that setup happened, counted now in both the middleware and SetupController::setupIsComplete(), which have to agree or the result is a redirect loop or an open form. Worth recording: erasure force-deletes a self-deleted account after its grace period, so the second lock would expire on its own. It does not matter because the first lock stops the installation reaching that state, but a future change to either should know the other is not permanent. An installation that has already lost its last administrator now finds setup shut. That is the point: recovery is php artisan projectsend:admin, which is also how every unattended container installs itself. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
d751314196 |
Merge pull request #1715 from denkfabrik-li/fix/zip-duplicate-entries
The job walked the loose file ids and then every selected folder's subtree, adding whatever each pass found. A selection reaching the same file both ways got it twice: two copies of the same bytes, a total_size inflated by the repeat -- which is what the size cap is checked against -- and a file limited to a single download handed over in three copies while the log recorded one, because delivery logs per contained file and DownloadAllowance counts those records. Verified before merging: the three new tests fail on main and pass here. Suite at 2082, PHPStan clean. Two halves, because one fix does not cover both shapes. The added-ids list becomes a map keyed by id and the folder pass skips what is already in, before the per-file re-checks, so a duplicate does not spend an allowance twice either. And a folder sitting inside another selected folder is dropped before either is walked, which also settles which path the surviving entry keeps rather than leaving it to row order. One measured cost, accepted: the pruning compares every selected folder with every other. The pathological case -- ten thousand sibling folders, the selection cap -- benchmarks at around twenty seconds of CPU, in a background worker, on a selection that would take far longer to compress. A sort-by-path-length version would be cheaper if it ever matters. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
00d118559d |
Merge pull request #1714 from denkfabrik-li/fix/group-edit-library-scope
Every group route asked StaffLibraryScope whether this viewer may act on this group except the two that read it. So a client-scoped staff member could open the edit screen of a group they cannot change, read its membership with addresses, and get the whole client roster in available_clients besides. The API twin returned the same membership. Verified before merging: the three new tests fail on main and pass here. Two things checked beyond the report -- group membership is edited through separate, already-guarded routes, so narrowing the displayed list cannot remove anybody on save; and scramble:export regenerates byte-identical, as claimed. Suite at 2078, PHPStan clean. The fix has two halves because one guard does not cover both shapes. Reading the group now asks the same reach question the write half asks. And both lists narrow through StaffLibraryScope::clients(), because a group nobody has shared anything with reaches nowhere, stays open to everybody, and can still hold a stranger's client. Unscoped viewers are unaffected: clients() returns the whole roster for them and allowsGroupChange() is true by construction. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
abaca20261 |
Merge pull request #1713 from denkfabrik-li/fix/api-self-deactivation-boolean
The validation rule accepts 0 and "0" as well as false and does not cast, so a strict comparison against the validated array let two of the three spellings past the self-deactivation guard -- and the model's own boolean cast then stored exactly the value the guard had just decided was not a deactivation.
Reproduced on main before merging: {"active": false} is refused, {"active": 0} and {"active": "0"} both return 200 and switch the account off. Green on the branch, suite at 2074, PHPStan clean.
The fix reads the flag once with Request::boolean() and gives that same value to the guard and to the write -- the rule RolesController::guardScopeRemoval already documents for the same reason. Validation is unchanged, so the accepted inputs are the same; one of them just stops meaning two different things on its way through the method.
Follow-up for the release: this is a caller-visible change (200 to 422) and wants a line in api-changelog.md.
Reported and fixed by @denkfabrik-li.
|
||
|
Ignacio Nelson
|
b16d780ebe |
Merge pull request #1712 from denkfabrik-li/fix/storage-durability-dashboard-assertion
The test named for carrying the durability verdict to the system widget asserted only has('system'), and system is an unconditional key of the render array -- the controller's own comment beside storage_durability says as much. So the assertion could not fail.
Confirmed here by deleting the line that supplies the verdict: the new assertion fails with "Property [system.storage_durability] does not exist", where the old one stayed green.
Test-only, no application code.
Reported and fixed by @denkfabrik-li.
|
||
|
Ignacio Nelson
|
602c7bed94 |
Merge pull request #1708 from denkfabrik-li/fix/confirm-password-under-enforcement
EnforceTwoFactor exempts by route name, and only the GET half of the confirm-password screen had one -- Route::named() answers false for a null name, so the submission was never exempt. Enrolling requires password confirmation, so with enforcement on nobody could enrol at all: the form rendered, its POST was redirected to two-factor.show, auth.password_confirmed_at was never written, and every account on the installation was left with logout as its only working route. Including the administrator who turned the setting on. Reproduced on main before merging: POST /confirm-password redirects to /settings/two-factor and the session flag stays unset. The widened pattern was checked against the route table -- password.confirm* reaches password.confirm and the newly named password.confirm.store and nothing else; password.reset, password.store and the rest are not under that prefix. Exempting the submission grants nothing further, since every other route stays bounced and store() still validates the password. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
76f79d53a0 |
Merge pull request #1711 from denkfabrik-li/fix/update-tests-clear-compiled
Ten tests ran the real projectsend:update, which runs clear-compiled, which deletes bootstrap/cache/packages.php and services.php -- one copy for the whole checkout, shared by all eight workers of a parallel run. A worker booting in the window between that delete and its own rebuild reads an empty package manifest, registers no package service providers, and dies rendering the next page with "Target [Inertia\Ssr\Gateway] is not instantiable", in a file that has nothing to do with updates. Verified here rather than taken on trust: a probe running the real update inside a test on main deletes the manifests, exactly as described. The branch is green at 2066 with PHPStan clean, and touches no application code. The file already owned a double and explained why the artisan call is a seam; this extends it to the whole file and adds a test asserting the compiled caches survive. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
3f81dd5eab |
Merge pull request #1709 from denkfabrik-li/fix/seat-cap-approval-doors
Two doors onto the client seat cap did not ask it. Both update()
methods -- the edit screen and PATCH /api/v1/clients/{id} -- clear
account_requested when a pending client is activated, under a comment
saying that counts as approval, and approval is the moment a seat is
spent. So a managed installation sitting at its cap kept taking clients
on for as long as registrations arrived, and self-registration is open
to strangers, so the supply of pending rows is not the operator's to
control.
Verified rather than taken on trust: the two new door tests were run
against the unguarded controllers and fail there, and every place in
app/ that clears the flag was enumerated to check no third door was
missed. There is none -- the other six already ask, and a conversion
refuses a pending account outright rather than approving it sideways.
The guard sits inside the approval branch, so an installation at its cap
can still rename a client it already holds. That is pinned by a test of
its own.
Conflicted with tonight's seat work in SeatAllowanceTest, which had
added an import beside the one this adds. Resolved by keeping both;
suite green at 2065 and PHPStan clean after resolution.
Reported and fixed by @denkfabrik-li.
|
||
|
ignacionelson
|
73d93495c9 |
Report the last staff sign-in in projectsend:status
A platform can see that an installation is running. It cannot see
whether anybody is still using it, and the difference is what separates
a customer from an abandoned free instance holding a database.
So the status probe gains one field:
"activity": { "last_staff_login_at": "2026-08-24T21:13:32+00:00" }
Null means no staff account has ever signed in, and the key is emitted
either way. That is the whole care in this change: "they said never" and
"we got no answer" have to stay distinguishable, because collapsing them
is how a broken probe reads as a dormant fleet.
Only interactive sign-ins count. Laravel's Login event does not fire for
token authentication, so an integration polling every hour cannot make
an empty installation look busy -- which matters when the reading is
used to decide something.
Derived from the activity log rather than denormalised onto users. A
column would cost a migration, a listener change and a backfill to save
one indexed MAX() over a table with a handful of rows on exactly the
installations anybody asks this about. Nothing prunes the log, and
erasure anonymises entries rather than removing them -- actor_type
survives on purpose -- so the answer does not change when the person who
gave it is forgotten.
Requested by the ProjectSend Cloud control plane, which has no other way
to learn the date. Recorded in docs/api-todo.md as deliberately a
command rather than an endpoint, for the reason the command exists at
all: it observes, it does not accept instructions.
|
||
|
ignacionelson
|
13b56186f4 |
Say the seat limit before the form, not after it
On a managed installation with its staff seats full, /users/create opened as though there were room. You typed a name, an address and a password you had to invent, pressed Save, and the plan limit came back as a validation error under the email field -- which reads as a complaint about the address rather than a fact about the plan. A full installation is an ordinary state on a plan sold by the seat, so it is now stated up front. The list carries the seat position, the button goes dead once the last seat is taken and says why, and the create screen turns away anyone who reaches it by link or bookmark. The guard in store() is untouched: that is still the rule, this is only the door. The refusal is worded once, in SeatAllowance, and the screen is handed that sentence rather than writing its own -- two wordings of one limit is how somebody ends up believing there are two limits. `full` is derived there too, from the same comparison the guard refuses on, so a screen cannot disagree with it about the edge (used > limit, after an operator lowers a limit) and offer a button for a form that cannot be submitted. Clients get the same treatment: the cap exists there too, and reached it the same way. Self-hosted installations have no limit, so they are shown nothing about one. |
||
|
denkfabrik-li
|
e272f19045 |
Keep a private reply private after the client is deleted
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. |
||
|
denkfabrik-li
|
28e18497b5 |
Refuse the last administrator deleting themselves, and keep setup shut
ProfileController::destroy() validates current_password and soft-deletes. It never asks StaffAccounts::guardLastAdministrator(), and every other door does: Staff update(), guardDeletable(), and both directions of the role conversion. This is the one door where the account being removed is certainly signed in. An installation with a single administrator therefore had a button that emptied it. Measured on main: DELETE /settings/profile 302, the account is gone live staff rows 0 (the row is trashed, not removed) anonymous GET / 302 -> /setup anonymous POST /setup a new active System Administrator EnsureSetupIsComplete asks ->exists(), which excludes trashed rows, and routes/web.php registers GET and POST setup with no auth and no guest middleware -- correctly, since a fresh installation has nobody to authenticate. SetupController::store() re-checks the same condition, so both halves agreed with each other and both were wrong once the last staff row was trashed. Two locks, because one of them is asked at five doors and the other at one. First: destroy() now asks guardLastAdministrator(), the same call with the same message as everywhere else. An administrator with a colleague still goes, a non-administrator staff member still goes, and a client still closes their own account. Second: "has this installation been set up" is not the same question as "does it have a working administrator right now", and only the first one belongs in EnsureSetupIsComplete. A trashed staff row is still evidence that setup happened, so it now counts -- in the middleware and in SetupController::setupIsComplete(), which have to agree or the result is either a redirect loop or an open form. That second lock holds even if a future door forgets the first one. Measured with the guard bypassed entirely and the row trashed directly: GET / answers with the login screen and POST /setup creates nothing. Worth stating plainly: an installation that has already lost its last administrator will now find setup shut rather than open. That is the point -- the recovery path for it is `php artisan projectsend:admin`, which is also how every unattended container installs itself, not a form that anybody on the internet can reach. Six tests, two measured red against the unfixed code (2 failed / 4 passed) -- one per lock. The other four are the boundaries: a colleague present, a staff member who is not an administrator, a client, and a genuinely fresh installation that must still reach setup. Two existing tests needed saying more clearly rather than changing: ProfileUpdateTest's deletion cases now create a second administrator, so that what they assert is self-deletion and not this new refusal; and GettingStartedTest's "fresh installation" cases forceDelete rather than delete, because a soft-deleted staff row is no longer a fresh installation -- which is the whole of the second lock. Full suite passes (2054 passed / 2 skipped), PHPStan level 8 clean. |
||
|
denkfabrik-li
|
b44c6bf098 |
Add a file to a zip once, however many ways the selection reaches it
BuildZipDownloadJob walks the loose file ids and then every selected
folder's subtree, and adds whatever each pass finds. A selection can
reach the same file from more than one of them, and nothing noticed:
file_ids [f], folder_ids [Reports]
-> ['report.pdf', 'Reports/report.pdf']
file_ids [f], folder_ids [Reports, Reports/Q1]
-> three entries, file_count 3, total_size three times the file
Two copies of the same bytes in one archive, and total_size is what the
size cap is checked against, so a selection could also be refused for a
weight it does not have.
The one that costs more than bandwidth is delivery. It logs one
FileDownloaded per contained file, and DownloadAllowance counts those
records -- so a file limited to a single download left in three copies
while the log recorded one. Measured: three entries, one record.
Two causes, so two halves.
`$added` is now keyed by id instead of being appended to a list, and the
folder pass skips a file already in the archive. A lookup rather than a
scan because the selection cap is 10000 sources. The loose pass runs
first, so a file picked both ways sits under its loose name; either
answer is defensible, but it has to be the same one every run.
And a selected folder inside another selected folder is dropped before
either is walked. Zipping both would reach every file in the inner one
twice, and which path the surviving entry ended up under would be decided
by the order the rows came back in. Keeping the outer folder keeps the
fuller path -- Reports/Q1/report.pdf rather than Q1/report.pdf.
Containment is decided on the materialized path, so it is one comparison
per pair with no queries: a folder's path starts with an ancestor's
subtreePathPrefix(), and both end in '/', so /5/ cannot match /50/.
Not changed: the per-file re-checks inside the folder pass. Visibility
and the download allowance are still re-derived per file, and the skip
happens before them, so a duplicate never spends an allowance twice
either. Nor the selection endpoint -- a caller may send whatever
selection they like, and the job is where it is resolved.
Four tests. Three measured red against the unfixed job (3 failed / 32
passed): the loose-plus-folder case, the nested-folder case, and the
three-way case asserted through delivery rather than through the archive.
The fourth -- two selected folders that merely share a name are both
zipped -- is green either way and guards the pruning against being about
names rather than containment.
Full suite passes (2052 passed / 2 skipped), PHPStan level 8 clean.
|
||
|
denkfabrik-li
|
eade690f73 |
Hold the group edit screen to the same library boundary as the rest
Every other group route asks StaffLibraryScope whether this viewer may
act on this group. GroupsController::update() and ::destroy() do, and so
do their API twins -- all four with abort_unless(allowsGroupChange, 404).
The two that read do not: edit() and Api\GroupsController::show() had no
boundary at all.
What they hand over is the membership, name and email per member, plus
the whole client roster of the installation as available_clients. So a
client-scoped staff member could open a group whose contents they cannot
see, read off every client on the installation, and only be refused when
they pressed save.
Two halves, because the leak has two shapes:
- The group itself. Reading it now asks the same reach question the write
half asks, one step earlier, with the same 404 -- a group that reaches
past the viewer's library is not theirs to open either.
- The lists inside it. Both narrow through StaffLibraryScope::clients(),
the listing half of the rule this screen's buttons are already guarded
with: allowsGroupMembership refuses removing a member outside the
roster, and refuses adding a client outside it. Naming them anyway,
with their address, is the mistake ClientsController made before
clients() existed -- that method's own docblock says so.
The reach guard alone would not have been enough. A group nobody has
shared anything with reaches nowhere, so it stays open to everybody --
and it can still hold a stranger's client. That case is why the lists
narrow separately, and there is a test for it.
members_count is left whole on purpose: a size is not an identity, and it
is the same number the group listing already reports.
GroupResource's docblock claimed members are safe to expose because "the
group edit screen already shows [them] to anyone holding edit_groups".
That was a claim about a screen, and it stopped being true the moment the
screen narrowed. Reworded to say what now holds it up, and where.
Not changed: the group listing. It reports names and member counts, not
identities, and every button on it is guarded. Nor Api\GroupsController::
index(), for the same reason. Nor the API document -- scramble:export is
byte-identical, because GET /groups/{group} already documented a 404.
Four tests. Three measured red against the unguarded controllers (3
failed / 21 passed): the group cannot be opened at all, the edit screen
stops naming strangers, and the API twin narrows what it hands back. The
fourth -- an unscoped viewer keeps the whole roster and every member -- is
green either way and guards against the fix over-refusing.
Full suite passes (2052 passed / 2 skipped), PHPStan level 8 clean.
|
||
|
denkfabrik-li
|
3e15237f90 |
Refuse self-deactivation over the API however the boolean is written
Api\UsersController::update() compares the validated value strictly:
if ($user->is($actor) && ($validated['active'] ?? true) === false) {
The `boolean` rule accepts 0 and "0" as well as false, and it does not
cast. `0 === false` is false, so the refusal never fires -- and the
model's own `boolean` cast then stores as false exactly the value the
guard had just decided was not a deactivation.
Measured against main, with a second administrator present so that
guardLastAdministrator is not what answers:
{"active": false} -> 422, still active
{"active": 0} -> 200, active is now false
{"active": "0"} -> 200, active is now false
The method's own docblock says it is "Refused with a 422 if the change
would leave the installation with no active administrator, or if you
would be deactivating yourself", and the web screen does refuse. This is
the API half of that sentence.
RolesController::guardScopeRemoval documents the rule this breaks, in the
same words: callers resolve the flag with Request::boolean() and hand the
same value to the guard and to the write, deliberately, because reading
the validated array and comparing it strictly "would let a request
through here that the model's `boolean` cast then stores as false anyway
-- the guard and the write disagreeing about one value is exactly the
shape this guard exists to prevent".
So read it once, with Request::boolean(), and give that one value to both.
Not changed: the validation rule. It stays `boolean`, so the accepted
inputs are the same as before -- what changes is that one of them stops
meaning two different things on its way through. Nor anything about
deactivating somebody else: all three forms still work, and there are
tests saying so.
Six cases from two datasets. Two measured red against the unfixed
controller (2 failed / 4 passed): 0 and "0" on yourself. `false` was
already refused, and the three "somebody else" cases are green either way
-- they guard against the fix over-refusing, not against the bug.
Full suite passes (2054 passed / 2 skipped), PHPStan level 8 clean.
|
||
|
denkfabrik-li
|
9cc469b111 |
Make the storage durability dashboard test assert the verdict
The test named for carrying the verdict to the system widget only asserted that the 'system' key exists. It is an unconditional key of the Inertia::render array and is allowed to be null, and Inertia's has() is a key check, so the assertion held whether or not the verdict was in there. Deleting 'storage_durability' from DashboardController::systemInfo() left the file green. Substitute the class the way the rest of the file already does and assert the payload, as InstallationKindTest does for install_kind next door. |