Nobody hands out reach they do not hold either
Two resolutions against branches that landed first. #1678 and this one
each add a constructor property and an import to StaffAccounts, so both
are kept. And #1702's merge note called this one exactly: its
"converting an account to staff cannot hand out clients either" case
promoted a stranger client, which #1702 now refuses at 404 before
validation runs. Pointed at a client the actor holds, as that note
proposed, so the request reaches the assigned_clients rule the case is
actually about.
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.
StaffAccounts opens with the rule: "Nobody hands out authority they do
not hold ... that turns one permission into every permission and makes
the rest of the matrix decorative." mayGrant() enforces it for a role's
permissions, guardTarget() applies the same test to an existing account,
and RolesController::guardGrantablePermissions() names the attack in
full -- a non-administrator holding manage_users minting a role that
carries more than they do, and then holding it.
A role carries one more thing, and it is the larger one. `client_scoped`
decides whether the role reaches the clients assigned to its holder or
the whole library, which is the boundary StaffLibraryScope,
ActivityLogScope and every listing in the application are built around.
Nothing weighed it. store() and update() wrote the flag straight from
the request, and mayGrant() looked only at permissions -- so
`manage_users` on a client-scoped role was enough to take the limit off
that role and keep working, or to mint a role without one and move into
it. Either way the next request read the whole library, and the
`assigned_clients` roster that #1697 protects stopped meaning anything
for that account.
Both halves of the existing pair get the missing clause:
- guardScopeRemoval() in RolesController refuses a client-scoped actor
who creates a role without the limit, or takes the limit off one
that has it. Phrased as "removes the limit" rather than "is not
limited", so only what this request changes is weighed -- the same
reasoning guardGrantablePermissions() gives for looking at the diff.
Editing an already-unlimited role's permissions is not this actor
lifting a limit. Both writers resolve the flag with
Request::boolean() and hand that same value to the guard and to the
write: the `boolean` validation rule accepts "0" and 0 as well as
false and validates without casting, so reading the validated array
and comparing it strictly would leave this guard and the model's own
`boolean` cast disagreeing about one value -- which is the shape the
guard exists to prevent.
- mayGrant() refuses a client-scoped actor granting a role that is not
client-scoped, which closes assigning an existing one. It reaches
both surfaces at once: assignableRoleIds() validates role_id on the
web and API staff forms and on the account converter,
assignableRoles() fills the pickers, and guardTarget() covers the
account itself.
Administrators are unaffected -- mayGrant() returns early for them, and
an administrator role is never client-scoped. Unscoped staff are
unaffected: the clause is conditioned on the actor's own scope, so a
non-administrator with manage_users and no limit creates, edits and
grants exactly as before. The seeded roles are untouched; update()
already refused to move the flag on a system role, which is why the
stock Client Manager was never the way in.
Two changes a client-scoped holder of manage_users will notice, both
following from mayGrant():
- the role picker on the staff form and the account converter now
offers only client-scoped roles, rather than offering one the
request behind it would refuse;
- editing or deleting a staff account whose role is not client-scoped
now answers 403, through guardTarget(), on the same "if you could
not grant their role you have no business editing that account"
rule that already applied to permissions.
The roles API is read-only (GET /roles is the whole surface), so this
half has no API twin to mirror; the account half is covered above.
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.
StaffAccounts opens with the rule for roles: "Nobody hands out authority
they do not hold … that turns one permission into every permission and
makes the rest of the matrix decorative." mayGrant enforces it, and
guardTarget applies the same test to an existing account.
The client roster never got the same treatment. `assigned_clients` was
validated as `exists:users,id where type = client` and passed straight to
syncAssignedClients, with nothing anywhere asking whether the actor holds
the clients they are handing out. Assigning a client is not a label: it
is that client's whole library, given to whoever is on the other end.
The case that matters is the actor's own account. guardTarget returns
immediately when the target is the actor — editing your own name and
email is not a question of authority — so a client-scoped staff member
with edit_users could PATCH their own id with every client on the
installation and read the whole library from then on.
assignableClientIds() answers the roster question the way
assignableRoleIds() answers the role one, and the five places that accept
`assigned_clients` — users store and update on both surfaces, and the
account conversion — validate against it. It returns the full roster for
an unrestricted actor, so every caller validates against one list instead
of composing a conditional rule; that list is already client-typed, which
is why one rule replaces the exists() and the type filter together.
The two pickers that offer the roster are narrowed to the same list, so a
form no longer offers a client the request behind it will refuse.
syncAssignedClients is untouched: which ids stick to which role is its
decision and it was never the problem.
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.
Setup ended by handing somebody a login form and an empty dashboard.
Everything this application can do was one menu away, and which menu was
theirs to discover.
The first time the administrator signs in to a new installation they now
land on a short ordered list of what is worth doing first — add a client,
upload a file, group the people who get the same things, choose how the
file lists and the email look, point it at a mail server, add the team,
check the scheduler — each a link straight to the screen that does it.
The list is filtered twice, and both filters matter. By permission,
because a link that answers 403 is worse than no link. And by edition:
a managed installation is not sent off to configure a mail server
somebody else runs, to create staff accounts that are not its to create,
or to check a scheduler it does not host. Those three drop out on Cloud
and the other five remain.
Two steps tick themselves, because the database can answer them: a client
exists, a file exists. Nothing else is checkable without guessing — a
theme that was never changed looks exactly like one chosen deliberately —
and a tick meaning "we assume so" is worse than no tick.
The invitation to the Discord is at the very bottom, after the list.
Somebody who has just installed this came with a job in mind, and opening
with a social invitation is the fastest way to lose them.
The marker is raised where a first administrator comes into existence —
the setup screen and `projectsend:admin`, so a container provisioned from
environment variables is welcomed too — and it is false by default, so an
installation that updates into this feature is not congratulated on an
install it finished a year ago.
RedirectToWhatsNew becomes RedirectToGreeting and answers for both: they
are the same interruption, and a second middleware on the same route
would have to know about the first to avoid arguing with it. Installing
wins; release notes for a version you never ran are the wrong greeting.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Ignacio Nelson