SeatAllowance says a cap is only a cap if every door asks, and has a test
per door for that reason. Two doors do not ask.
The moment a seat is spent is the moment `account_requested` is cleared.
Five places do that. approve(), both store()s and ClientProvisioning ask
guardClient(); AccountConversion asks it through guardToClient(). The two
update()s -- web and API -- clear the flag with no guard at all, under a
comment that names exactly what they are doing:
// Activating a pending account through the edit screen counts as
// approval and clears the request flag.
Measured with clients: 0, one pending registration:
POST /account-requests/{id}/approve refused, flag still set
PATCH /clients/{id} active=true approved, clientUsed() 0 -> 1
PATCH /api/v1/clients/{id} active=true approved, clientUsed() 0 -> 1
A managed installation at its cap therefore keeps taking clients on, from
the edit screen or a PATCH, for as long as registrations keep arriving --
and self-registration is open to strangers, so the supply is not the
operator's to control.
Inside the branch, not above it. Above it, an installation sitting at its
cap could not rename a client it already holds, which would trade one
wrong refusal for another. There is a test pinning that.
The field is `active` rather than the default `email`: on this screen the
administrator is toggling `active`, and an error under the email field
would point at the wrong thing. approve() has no form of its own, so it
keeps the default.
Three tests, per door as the file's other eight are. The two door tests
were measured red against the unguarded controllers (2 failed / 18
passed). The third -- that editing an existing client still works at the
cap -- is green either way: it guards against the fix being written a
line too high, not against the bug.
Full suite passes (2051 passed / 2 skipped), PHPStan level 8 clean.
One thing worth knowing that this branch does not touch: on a parallel
run, `UpdateWelcomeTest > staff who may not read...` fails roughly one run
in six on untouched main, with `BindingResolutionException: Target
[Inertia\Ssr\Gateway] is not instantiable`. Measured over 24 baseline runs
before this change existed. It is not this fix, and it is not in scope
here, but it will start being visible as soon as the workflow parses
again.
Opening user management on cloud (623ad68) left a managed tenant able to
create staff accounts without limit. This is the other half, and the two
belong in the same release.
max_clients and max_staff_users are numbers the platform sells and does
not enforce — grep finds them only being passed to screens. The
application is the only process that can count against them, so it
accepts the number from the environment and refuses to exceed it. That is
not the same as inventing a plan tier, which is what config/api.php
declines to do when it will not key a rate limit off billing: nothing
here knows what a plan is.
## One definition
staffUsed() and clientUsed() are public and are what the guards read. A
control plane showing "2 of 3 used" from its own query, beside an
application refusing the fourth from a different one, disagrees
eventually — over an inactive account, or a deleted one — and the
disagreement reads as a billing fault rather than a counting one.
## What counts, and the consequences somebody has to explain
An inactive staff account occupies its seat. Excluding it would make
deactivation a way around the cap rather than a way to revoke access,
since reactivating is one click. The cost is an awkward incentive —
deactivating is the safe removal and keeps paying, deleting frees the
seat and asks what happens to the files — and it is better explained than
hidden.
A client awaiting approval does not. Self-registration is open to
strangers, and counting a pending request would let anybody exhaust a
paid limit from the outside, turning a pricing tier into an availability
control. The seat is spent at approval, which is where the guard sits.
A soft-deleted account frees its seat, though not its address —
AvailableEmailRule holds that until erasure. So a seat can be free while
re-adding the same person is still refused, which is the address rule
rather than this one.
## Eight doors, eight tests
There is no single User::create() to guard. StaffAccounts::create()
covers both staff controllers, but a promotion takes a staff seat without
creating anything, a demotion takes a client seat, ClientProvisioning
serves registration and LDAP and social sign-in alike, and approval turns
an uncounted request into a counted client.
A cap is only a cap if every door asks, so there is a test per door and
each was verified to fail without its guard — eight red, with the two
"must not change" cases green either way. DownloadAllowance's shape for
DownloadAllowance's reason: the failure mode is one of them quietly not
asking, invisible from everywhere except the door that forgot.
projectsend:admin is deliberately uncapped and has a test saying so. It
is the recovery path, and anyone who can run it can also edit the
environment the cap comes from.
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.
Let a deleted account's email address come back into use
Closes#1648, and with it the last open item of #1647's audit of unique
indexes on soft-deleting tables.
Resolved a trivial conflict in both ClientsControllers: this branch and
today's e7b5b6a each add a constructor property at the same line, so both
are kept. Nothing else overlapped.
Two things found by checking #1696 and #1699 -- open branches carrying
the same fixes I wrote this morning -- against what I actually shipped.
**topClientsByStorage was scoped with the wrong question.** 4b8220a
narrowed it with StaffLibraryScope::files(), which is right for the two
widgets that name files and wrong for the one that names clients: a
stranger client's upload can sit legitimately inside a scoped viewer's
library, shared with a group one of their own clients belongs to. So the
file was theirs to read and the uploader's name was not theirs to see.
Measured: "Stranger Client Ltd", on nobody's roster, ranked on a scoped
dashboard. assignableClientIds is what the widget is actually asking, and
it is what #1699 used. Their version was right and mine was not.
**The client guard is one method now, not eight copies.** #1696 wrote it
as a private guardTarget() rather than repeating viewer-resolve plus
abort at each site, which is better, and this is a change whose whole
argument is that a rule stated in many places drifts. Behaviour is
identical; the eight sites now read as one rule.
The published document reorders a 404 below a 422 on one path. Scramble
reads abort_unless out of a method body but not out of a helper it calls,
so the 404 now comes from route model binding instead of from the inline
abort -- same response, different position. #1701's body names this trap;
worth knowing it costs ordering and not content.
Credit where it is due: both come from denkfabrik-li's #1696 and #1699,
which were open while I was writing the same fixes. Those two are closed
against this and against e7b5b6a, 4b8220a and 67e9204.
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.
An account deleted by an administrator was soft-deleted with erase_after
null, so projectsend:purge-erasures — which filters on
whereNotNull('erase_after') — never reached it, and the unique index on
users.email kept the address reserved forever. Anyone re-creating the
account got "The email has already been taken", naming a conflict nothing
on any screen could show or clear (#1648).
Both halves of the issue's option 3:
Every deletion path now schedules the erasure. The stamp lives in
ErasureSchedule — self-deletion switched to it, and StaffAccounts::delete
(shared by the web screen and the API) and both client controllers call
it right before delete(). Same grace period, same purge, whoever deleted
the account. Deliberately no backfill for rows deleted before this
change: stamping them during an update would start a countdown to data
erasure that nobody chose at deletion time; the message below covers
them instead.
The staff creation paths swap unique:users,email for AvailableEmailRule,
which refuses exactly the same things but can explain the one refusal
the stock message can't: an address held by a deleted account now names
the date it becomes available, and one deleted before scheduling existed
points at projectsend:erase-account. A living account keeps the stock
message, and public registration keeps the stock rule — telling an
anonymous visitor the address belongs to a deleted account would confirm
it had an account here.
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.
Four create flows redirected to the new record's edit page on success,
but store is gated by create_* while the edit page is gated by edit_*,
and PermissionChecker has no create-implies-edit rule. A role holding
create_* without edit_* would create the record -- write, activity log
and notifications all run -- and then meet a 403 on the success
redirect, with no way to tell the action worked and every reason to
submit a duplicate. Categories is reachable with plain UI clicks, since
the sidebar shows it from create_categories alone.
Keep landing on the edit page for anyone who may edit, and divert only
those who can't -- to the create form, which shares store's own gate
and is therefore reachable by exactly whoever just created the record;
the success toast shows there. The index would not do: Clients/Groups
lists are gated by manage_*, which store itself does not require.
Implying edit_* from create_* would not do either -- edit has no
own/others split here, so it would silently hand a deliberately narrow
create-only role edit (two-factor reset included) on every existing
record.
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.
denkfabrik-li