mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-17 00:55:07 +00:00
main
23 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 ignacionelson
|
c21658f6f7 |
Let a client account expire on a date
Staff can give a client an expiry date on the create and edit screens, and through /api/v1/clients. When the date passes, the client is refused at sign-in and on their next request, and their API access ends too. Files and history stay, and a later date (or none) brings them back. Access is checked through one predicate, User::maySignIn(), at every door: sign-in, the web session, API tokens and the two-factor challenge. An hourly sweep also switches `active` off, so the list, its filter and seat counts agree. The sweep is not what enforces it, so a scheduler that is not running cannot keep an account open. An account cannot be active with a date that has passed. Reactivating an expired client needs a new date in the same save. The day-means-end-of-day-where-you-are rule moved out of FileExpiry into a shared DateInput, so file and account expiry read dates the same way. Requested by @Drardollan in #1310. |
||
|
ignacionelson
|
7c7ba7cd53 |
Stop a typed-in storage quota from 500ing when a client is created
Filling the "Storage quota (MB)" field on the new-client form raised a TypeError and the request died with a 500. Leaving it blank worked, which is why it reached a release: that path goes through `null ?? 0`, and the 0 is an int. The `integer` validation rule checks that a value looks like an integer. It does not convert it. `$request->validate()` returns the raw input, so the form field arrives as the string "2048" -- and the create form types that field as a string in React, so it is a string even over JSON. Both controllers declare strict_types, so handing it to `ClientAccounts::create()`'s `int $storageQuotaMb` is a TypeError. Fixed on both surfaces that call create(): the staff screen and /api/v1/clients. The API twin had the same defect, reachable by sending the quota as a quoted JSON value or a form-encoded body -- its own create test only ever sent a JSON number. Two more call sites had the same shape and are cast too, though nothing sends them a string today: the share-link download cap and a comment's reply_to. Both are safe only because a frontend file happens to call Number() first, which is a fact about that file rather than anything the signature guarantees. The null in each is preserved rather than collapsed to 0 -- "no cap" is not a cap of zero. `storage_quota_mb` is also cast on User and Invitation. The column is an unsignedInteger and both docblocks already promise int; it is read straight into provision()'s typed parameter when an invitation is redeemed, and which type a driver hands back is not something that call site should depend on. Found on the new files-test rehearsal instance, on its first real use, against the same build the whole fleet is running. |
||
|
ignacionelson
|
f06a3c7ab3 |
Put a new client account in front of the staff who administer clients
Invitations produced no in-app notification at all, and neither did self-registration: the whole Clients module raised none. The only admin-facing signal when an account appeared was an email to whatever raw addresses an operator typed into a setting -- addresses that need not correspond to any account in this installation, and that plenty of installations never fill in. An invitation could be accepted and nobody signed in would ever be told. So: one new type, client_registered, reaching the bell and /notifications. One type for both doors on purpose. A client arriving through the public form and one arriving through an invitation are the same event to the person being told -- an account now exists that did not -- and a second type would buy nothing, because preferences here govern email only, so it could not have been switched off separately anyway. Which door it came through is one click away in the activity log and on the invitations screen. In-app only, the reasoning client_uploaded already states: email for this event is sent separately to that address list, and routing it through Notifier's mail dispatch too would risk double-emailing any staff member who is also on it. Two things worth stating about who gets it. Recipients are resolved at the call site, because Notifier authorizes nothing by design -- its security contract is explicit that a broad query must never be handed to it. And a client-scoped staff member is deliberately not told: their whole view is the clients assigned to them, and a brand-new account is assigned to nobody, so it would link them to a screen they are refused. Which is also why the notification links to the clients list filtered to the address, and not to clients.edit: that route is gated by edit_clients while these recipients are chosen by manage_clients. A notification that refuses the person it was sent to is worse than one that lands a click short. Translated in all sixteen locales, and the redemption was driven through a real browser to see the row arrive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe |
||
|
ignacionelson
|
123ae68972 |
Give invitations their own place in the navigation
The "Invite client" button led to a history, which is not what it says. The tabs were a way of housing two things that had nowhere else to live, and now they do: Invitations is a sidebar entry between Custom fields and Groups, and the button goes to the form. That is also the shape every other list in this application already has -- Clients, Groups, Categories, Roles all sit in the sidebar with a "New X" button leading to their own create screen -- so the tabs were the odd one out rather than the pattern. Two URLs, each meaning one thing: /clients/invitations is the history, /clients/invitations/create is the form. Sending now returns to the history, where the invitation just sent is the first row. No badge on the sidebar entry, deliberately, unlike the two queues below it. Account requests and Membership requests count things waiting on somebody here; an outstanding invitation is waiting on the person who was invited. A number there would say "you have three things to do" about three things nobody in this installation can act on. Translations move with it: "History (:count pending)" was the tab label and is gone from all sixteen, and "Invite a client to share files with" comes back -- it was the form's description before the tabs took the heading, and had never been translated because it left the code in the same commit that would have reported it missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe |
||
|
ignacionelson
|
fb3fd1766c |
Turn the second tab into a history of every invitation, and open on it
The tab listed only what was still live, which cannot answer the question somebody actually arrives with: did we ever invite this person, and what happened? An invitation that was accepted, revoked or replaced by a newer one simply vanished from the screen, and the activity log was the only place left to look. So the tab is a history now -- every invitation ever sent, newest first, each carrying the state it ended in -- with a status filter for reading one slice of it. And it opens first, because arriving here the question is usually about what has already been sent, including to the person you were about to invite again. ?tab=send still goes straight to the form. Five states, and two of them needed deciding: - "Expired" is not a stored status and deliberately is not one: nothing writes it, a row becomes expired by the clock passing rather than by anybody acting, and storing it would need a scheduled task to stay true. Invitation::state() derives it, once, and both the badge and the filter read that -- two copies of the rule is how they start disagreeing about a row whose expiry passed a second ago. - "Replaced" is what superseded says to somebody who is not reading the source. It is a different fact from expired, and worth telling apart: one ran out, the other was retired by a newer invitation to the same address. The count in the tab label stays a count of live invitations rather than of the rows below. The history is mostly settled, and the number worth carrying in a label is the one that says whether anybody is still waiting -- which is also why it is counted over the table rather than the filtered page, so narrowing the list cannot change it. Revoke appears only on a row that still has something to revoke, and the expiry column is blank on a settled one: the date is still stored and still true, and printing it invites somebody to wonder what expires about an invitation that was accepted. The screen is called Invitations now, in the heading and the breadcrumb. With the history first it is no longer a form with a list under it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe |
||
|
ignacionelson
|
38187dcf1a |
Stop an expired invitation being renewed for ever
The expired page offers a "send me a new one" button that re-issues the invitation with no staff member involved. On its own that is reasonable -- the new link goes to the address on the invitation, never to whoever clicked, so holding a leaked URL gets nobody a working one, and it is the same shape as a password reset. What it spent was the operator's expiry window. A link could be renewed from a dead link, indefinitely, so a window set to 72 hours was only ever as short as the longest anybody bothered to wait. That matters in the case expiry is actually for: a link sitting somewhere it should not be -- a forwarded thread, a shared inbox, a mailbox that changed hands. So the chain gets a limit: three renewals, then a staff member has to send a new invitation. The count is carried forward on each renewal rather than stored per row, which is what makes it apply to the chain; a staff-sent invitation starts at zero, because sending one is somebody deciding to. A renewal beyond the limit answers in exactly the same words as a spent, unknown or revoked token, and sends nothing. Four situations, one sentence: telling them apart is how this door would become a way to learn which addresses an installation has invited. Renewals are now logged, which they were not -- sending and redeeming already were, and renewing was the one step that moved an invitation along with nobody behind it and left no trace. The limit bounds how many rows an anonymous door can write. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe |
||
|
ignacionelson
|
a502a26075 |
Let staff cancel an invitation nobody has used
An invitation could be sent and never taken back. There was no list of outstanding ones and no revoke, so the only way to withdraw a link sent to the wrong address was to let it expire -- and the expired page's own "send me a new one" button undoes exactly that, silently, for anybody still holding the link. The one cancel the feature had could be reversed by the person it was aimed at. So: a new STATUS_REVOKED, outside the pending() scope that both the redemption and the resend doors look through. A revoked link is dead to all three things a live one can do -- opening the form, redeeming it, and asking for a replacement -- and nothing but sending a fresh invitation brings it back. The list sits under the invite form rather than on a screen of its own, because the person who wants to cancel an invitation is the person who just sent one. It shows outstanding invitations only: pending, expired ones included. An expired invitation is not inert until it is revoked, so hiding it would hide the rows most worth a decision -- which is why they sort to the top, soonest expiry first. Revoking is gated by create_clients, the same authority as sending: whoever may invite somebody may take it back. It is logged, like sending and redeeming already were. The row is kept rather than deleted, for the reason a superseded one is kept -- the activity log names who invited this address and when, and that trail should still lead somewhere. Verified in a browser, not only in tests: the screen mounts, both rows render, and the expired one carries its badge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe |
||
|
ignacionelson
|
74d1de2d6e |
Say why an invitation was refused when the installation is full
Two halves of the same gap. Redemption always provisions with autoApprove: true, so it always meets SeatAllowance::guardClient(), which refuses on the `email` field -- and the redemption form's email input is read-only, never submitted, and had nowhere to render an error. The account was correctly not created and the person was told nothing at all: the form simply came back. The field now renders errors.email, which is where every other account form's refusal already lands. The other half is the button. "New client" has been seat-limited since the limit existed -- it goes dead with the reason beside it, rather than offering a form that cannot be submitted. "Invite client" sat next to it, live, on a full installation. Worse than the original complaint, because the refusal is met by the invited person rather than by the staff member who caused it. So the invite button is seat-limited too, and sending guards as well as redeeming. An outstanding invitation is still not a client and is still not counted as one -- the rule a pending account request follows, for the reason SeatAllowance spells out -- so this reserves nothing. It refuses to send a link a full installation could not honour, and redemption keeps its own guard, because the seat can be taken by somebody else in the days between. SeatLimitedAction's usage caption is now optional, and the invite button omits it. Two buttons governed by one limit, each captioned with the same sentence, reads as two limits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe |
||
|
ignacionelson
|
cd2ce960d3 |
Refuse an invitation whose address was taken while the link was live
An invitation stays live for days -- 72 hours by default -- and the address it names can be claimed in that window: staff got impatient and created the account by hand, or the person used the public registration form instead. Redemption never asked, so User::create() met the unique index on users.email and raised a QueryException. A 500, on the screen of somebody who had just chosen a password, having done nothing wrong. ClientProvisioning::addressIsFree() exists for exactly this, and its docblock says who must call it: the paths with no form to validate. LDAP asks. Redemption is the third such path and did not. It now refuses with a message that says what happened, rather than the generic "this invitation is no longer valid" the expired case uses. There is nothing to withhold here -- whoever holds the link already knows the address, because it is the one the invitation was sent to -- and being told to sign in instead is the only useful thing to say. The invitation stays pending rather than being retired. It is the account that resolved the situation, not the link, and a retired row would only make the second attempt read as expired. The address rule spans soft-deleted accounts, the same as it does everywhere else, so a deleted account still holds its address until erasure takes the row away. Both cases are tested. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe |
||
|
mash2k3
|
856c13b09c |
Invite a client to register instead of handing them a password (#1780)
Staff can now invite a specific address to register instead of typing a
password for somebody and finding a way to get it to them. The invited
person sets their own, the link is locked to the address it was sent to,
and an invitation always activates the account regardless of the
auto-approve setting -- naming an address is already the decision the
approval queue exists to make for one nobody named.
Two fixes ride along: outgoing mail now reads the installation's own site
name in its title, header and signature rather than the one baked into
config('app.name') at install time, and the CSRF cookie name is read per
request rather than captured once at load.
Follow-up work, tracked separately: an invitation cannot be cancelled --
there is no pending-invitations screen and no revoke, so letting one expire
is the only way to take it back, which the self-service resend button then
undoes. Redemption also needs the address-availability check every other
non-form caller of ClientProvisioning makes.
Thanks @mash2k3.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CPk8qAs38pudYGWwmGkYPe
|
||
|
ignacionelson
|
6ad26bb61e |
Hold an upload to the size it said it was sending
Reported by @ry2811 as GHSA-6jh6-gvj5-pv8v. A resumable upload declares its size, and that declaration is what store() weighs against the maximum file size and the client's storage quota. Only the assembled file was ever held to it. The parts in between were bounded one request at a time and never added up, so a client could declare one byte and then stream parts: ten thousand part numbers at twice a 20 MB part is about 400 GB, per session, and the number of sessions was not bounded either. None of it counted against anything, because nothing becomes a File row until the upload completes and ClientStorageUsage sums File rows. A client with a 1 MB quota could fill the volume and repeat. putPart()'s own comment described this defect and treated the per-part cap as the answer to it: "without a cap here the exposure is a day's worth of disk". A cap on one request bounds one request. The exposure was a day's worth of disk multiplied by however many requests somebody cared to make. Three limits, and each one exists because the other two do not cover it. A session may not stage more than it declared. The room for a part is claimed before the body is read — a body's length is not known until it has arrived, and by then it is on the disk being protected — and the write is then capped at exactly what was claimed, so an over-long body is cut off mid-stream as it always was, against a smaller number. The claim is a read and a conditional update under a per-session lock, the same shape complete() already uses: the protocol sends parts in parallel and how many is the client's choice, so an unlocked read lets every part in flight claim the same room, while an atomic claim alone refuses the honest parallel upload instead. Whatever the part really weighs is settled back afterwards, in a finally, or a client's own retries would exhaust a session with room to spare. Open sessions count against the quota at the size they declared. A quota measured against finished files alone is spent twice by opening sessions one after another — each is told there is room, because the ones before it have not finished. The cost is that an abandoned transfer holds its share until it is cancelled or swept, so the sweeper now runs hourly rather than daily: that gap is now somebody unable to upload, which it was not before. And a cap on open sessions, because for anyone with no quota to spend — staff, and clients on an installation that sets none — the session count is the only thing between a declared size and any multiple of it. Four tests fail on the unfixed code, and three existing ones had to change: they declared a tiny size and sent a large part deliberately, to reach the re-checks at complete(). That route is now closed at putPart(), so they reach those re-checks the way a real install would instead — the file-size limit or the quota moving while a long transfer is running, which is the reason complete() re-asks rather than trusting what store() decided. The staged-byte total is BIGINT UNSIGNED, and the suite runs SQLite, which has no unsigned integers. The first version of the bounds read `staged_bytes + :delta BETWEEN 0 AND size` and raised SQLSTATE 22003 on MySQL for any refund — in the comparison, so the bound written to prevent the underflow was the statement that underflowed. Every SQLite test passed on it. Both bounds are now arranged so the column is never inside a subtraction, and UploadSessionStagedBytesMysqlTest skips loudly unless the connection is MySQL. Verified against 8.4, as was the report itself: three sessions declaring one byte each put 6 MB on the volume of a client with a 1 MB quota before, and nothing at all after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNFU55Tkq6MuEQ73nbbBRx |
||
|
ignacionelson
|
62c763d04e |
Put a floor under a client quota nobody set
Setting::DefaultClientStorageQuotaMb defaults to 0, and 0 means
unlimited. That is the right default for somebody setting up their own
installation and the wrong one for an installation a platform operates
on other people's behalf: an account that arrived without an explicit
quota has no ceiling at all, and it does not have to be an account the
platform created.
So a platform may set a floor in the environment
(PROJECTSEND_PLATFORM_DEFAULT_CLIENT_QUOTA_MB), exactly as it sets the
seat caps, and for the same reason those are not settings: it is the
shape of what was sold rather than a preference the installation's
administrator is expressing. It applies only where the setting says
nothing, so an administrator who chose a number keeps it, and an install
with no platform behind it is unaffected.
ClientStorageUsage::defaultQuotaMb() is where the three sources resolve,
and every screen that presents the answer now reads it there:
- The client create and edit screens. The edit screen mirrors that
resolution client-side to draw the usage bar, so handed the raw
setting on a floored installation it computed an effective quota of
zero, printed "unlimited" and hid the bar entirely -- for a client
whose next upload was about to be rejected for exceeding a limit the
screen said did not exist.
- projectsend:status, which gains clients_can_register and
default_client_storage_quota_mb. Both defaults are the permissive
ones, both are invisible from outside, and a document reporting the
setting while uploads obeyed the floor would say the ceiling was
missing on an installation that has one.
The Client settings form deliberately still reads the raw setting: that
field is read and written back on save, so prefilling it with the floor
would write the platform's number into the setting as the
administrator's own choice, where it would outlive the floor.
|
||
|
ignacionelson
|
0a28e239d6 |
One home for what a client account is
Three surfaces create client accounts now: the staff screens, /api/v1/clients, and the platform control plane in the private package. Two of them held their own copy of the type, the role, the active flag, the "0 means inherit the site default" quota, the verified stamp, the activity entry and the seat guard — and the third could not have a copy at all, because a package cannot import a host class. ClientAccounts is that one definition, reached by name from outside. What stays with each caller is what genuinely differs: its validation, its response, its custom fields, and who is asking. Two things changed rather than moved: The seat cap is now checked inside create(), before anything is written, instead of at the top of each controller. That is what makes a leaked platform token an incident rather than an unbounded one — a guard that ran only where somebody remembered it is not a guard. email_verified_at is written with forceFill. It is deliberately absent from User::$fillable, so every client-creation path passed it into a mass assignment and lost it in silence. StaffAccounts already noted this and named the other paths; this closes the client half. |
||
|
ignacionelson
|
9c6f4df5bc |
Merge pull request #1736 from denkfabrik-li/fix/scoped-creator-keeps-client
A client-scoped staff member with create_clients created a client and lost it in the same request. guardTarget() answers 404 for anything off their roster, and StaffLibraryScope::clients() leaves it out of their list -- so the record existed, was logged, was welcomed by email, and was invisible to the person who made it. store() redirects to the edit page, which is exactly where they landed on a 404. The API twin had the same shape: a scoped token got a 404 from every route that binds the client it had just created. The new client is now attached to the creator's roster when the creator is client-scoped, on both sides. That is where a client they created belongs -- the roster is the same list assignedClients already uses for everything else they may reach. Unscoped creators gain nothing: they see every client already, and a roster entry would change what assignedClients means for them. Nothing is attached retroactively. The widening this involves is self-limited: the only thing added is an account the creator just made, which starts with no files, no folders and no group memberships, so assignableClientIds gains nothing to reach. Seats do not move either, since they are counted from active and account_requested. Verified before merging: 34 passed across both suites on the trial-merge, 2 failed / 32 passed with app/ reset. This is the busiest file set of the series -- it shares ClientsController with #1718 and Api/ClientsController plus the API test file with #1723 -- so the merged result was read rather than trusted: #1718's reassign_candidates gating and #1723's patchCustomFieldValues are both intact alongside it. scramble:export reproduces the committed docs/api/openapi.json byte for byte. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
2903a1da6d |
Merge pull request #1735 from denkfabrik-li/fix/editable-once-checkbox
ClientPortalCustomFields::save() writes '0' for an unticked checkbox, and filled('0') is true in Laravel. isLocked() asked whether anything is stored, so an editable_once checkbox locked itself 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 was the bug, and '0' is the absence of a decision in exactly the way null is for every other type.
A checkbox now locks on a stored '1' and nothing else. Every other type keeps filled(). What save() stores is unchanged -- '0' remains a recorded "no", as the API's client create also writes it -- and the behaviour after a real tick is unchanged too: the client still cannot untick it, and the test pinning that is untouched.
Verified before merging: 6 passed on the trial-merge, 1 failed / 5 passed with app/ reset. The editable-once text field test is green either way, which confines the change to checkboxes. The relaxation is safe because the lock is enforced on the write path and not only rendered: isLocked() gates rules(), which drops the field from validation, and save(), which skips it, so the ticked-to-unticked direction stays closed server-side.
Reported and fixed by @denkfabrik-li.
|
||
|
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
|
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
|
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. |
||
|
ignacionelson
|
9fc5042f4e |
Merge pull request #1688 from denkfabrik-li/fix/atomic-account-deletion
Delete an account and dispose of its content in one transaction Resolved the conflict with #1678 the way that PR's merge note predicted: the erasure stamp goes inside the new transaction, so a deletion that rolls back cannot leave a live account carrying a date on which it would be erased. |
||
|
ignacionelson
|
e7b5b6a757 |
Hold client records to the same boundary the rest of the library uses
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. |
||
|
denkfabrik-li
|
61c385e423 |
Delete an account and dispose of its content in one transaction
Deleting a staff or client account is two writes: soft-delete the account, then cascade or reassign the files and folders it owns. All four destroy() paths (Users + Clients, web + API) ran them one after the other with nothing tying them together. If the second write throws, the account is already gone but its content is not handled. The concrete way in is the reassign branch: validate() checks reassign_to_id with exists(active), but apply() re-resolves it with findOrFail() a moment later (AccountContentDeletion:108), so a target deactivated or deleted in between throws — leaving a soft-deleted account whose files still point at it, and a UserDeleted log for a deletion that did not finish. Wrap the delete()+apply() pair in a single DB::transaction() in each of the four destroy() methods. validate() and the authorization guards stay outside it: they are read-only and must be able to reject before anything is written. cascadeDelete()/reassignTo() already open their own transaction, which nests as a savepoint under this one, so the account soft-delete, its activity log, and the content work now commit or roll back together. Tests: a DeletedAccountContent double that reports content to handle and then throws while handling it (tests/Helpers.php) drives one test per destroy() endpoint asserting the account survives the failure and no UserDeleted entry is written; each goes red against the un-wrapped controller. |
||
|
ignacionelson
|
88c182cf3b |
Preview video, audio and PDF, not only images
v1 could preview four kinds of file in a modal — images, video, audio and PDF. v2 previewed only images, and not by decision: preview shipped as part of the image *thumbnail* work (1c68aa1), so "previewable" quietly became a synonym for "GD can decode it". FileThumbnailController::preview() gated on ThumbnailGenerator::SUPPORTED_MIME_TYPES, the frontend mirrored the same four types, and the dialog was a hardcoded <img>. Rather than widen that list — it drives pathFor(), extensionFor(), generate() and FileDiskCleanup, and a video reaching getimagesize() is a 500 — this separates the two questions. PreviewKind now answers "may these bytes be served inline, and what element renders them?", while ThumbnailGenerator keeps answering the narrower "can this app decode it itself?", which is what renditions, the cache and the watermark hook actually depend on. Image delegates to it so the two cannot drift. The allowlist stays a security boundary: mime_type is sniffed from the bytes, so text/html and image/svg+xml remain excluded, and PreviewKind is deliberately narrower than "formats a browser might cope with" — no quicktime, avi or matroska, because an embedded player for those shows a black rectangle. Those still download exactly as before. docs/security-audit-2026-08-05.md finding 1 recorded that adding application/pdf "should be a conscious decision". This is that decision, and three things were measured rather than assumed: - An <iframe sandbox> cannot be used. Chrome refuses to run its PDF viewer in a sandboxed frame at all (ERR_BLOCKED_BY_CLIENT, with or without allow-same-origin) — the attribute removes the feature, it does not harden it. - nginx's `Content-Security-Policy: sandbox; default-src 'none'` on /protected-files/ does work (a <video> frame lands in an opaque origin), but Chrome exempts its PDF viewer from it, so it is not what protects the PDF case. - What does is the allowlist plus the browser's own PDF sandbox, where PDF JavaScript has no DOM and no cookies. Range requests were verified end to end: 206 with a correct Content-Range, a byte-perfect file reassembled from three ranges, and a real browser seeking to 10s of a 20s clip. nginx drops the upstream Content-Length on the X-Accel path, so there is no collision. Two settings, both defaulting on so no installation loses what it has: clients_can_preview_files and public_listing_preview_enabled. Staff are never gated. The anonymous side needed a route of its own — there was no public preview endpoint — with its own throttle bucket, since a bare throttle: shares one counter across that whole block. A preview now logs at most one FilePreviewed per viewer per file per five minutes: a <video> turns one deliberate act into a long tail of Range requests, and a row each would bury the log. Also fixes a layout bug the tests could never catch. A portal file row was flex justify-between with three children — name, comment trigger, download — so the middle one settled wherever the name happened to end and the comment icon sat at a different place on every row. The name block now takes the slack and every action lives in one trailing group, with the comment trigger in a fixed-width slot so the icons form a column. And because half the previewable files have no thumbnail to click — a PDF, an mp3 and an mp4 all render as a generic icon — every row gains an explicit PreviewAction beside DownloadAction, matching whatever style that theme gives its download control. |
||
|
ignacionelson
|
6e47d76ba6 |
ProjectSend 2.0.0
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. |