A managed installation's staff accounts were expected to arrive from
outside it, so users.manage was Community-only and /users, /roles and
their API twins answered 404 there. The platform side spent a long
document designing its way around that gate; opening it is cheaper than
routing around it, and more honest about where the knowledge sits.
The division that settles it is the one managed storage already uses. We
do not manage a tenant's files from outside — a bucket is provisioned, a
scoped credential handed over, and what goes in it is the tenant's
business. Seats are the same kind of thing. A platform knows how many
staff accounts it sold; it does not know whether Alice should be an
Account Manager, and it certainly does not know where her files go when
she leaves. Capacity is the platform's, occupancy is the tenant's, and
the cap belongs in an environment variable rather than in a closed
screen.
The capability stays in front of the routes rather than being deleted.
It is currently true in both editions, but it is the seam an edition
difference has to travel through, and removing it would mean inventing
one again later.
Seven test files asserted the old rule, which is the tests doing their
job. Most flip. Two needed a different example instead: EnsureCapability
and AbilityCapability were both using users.manage to stand for
"Community-only", so they now use storage.configure and manage_updates —
keys that still are.
Two rationales half-expired and say so rather than being quietly
rewritten. CommentAuthors gave two reasons for being a setting rather
than a permission; the first was that roles are uneditable on cloud,
which stopped being true here, and the second — that `Everyone` includes
anonymous visitors, who have no role to hold a key — was always the
stronger and is now the whole of it.
The seat cap this makes necessary is the next commit, not this one. On
its own this change lets a managed tenant create staff accounts without
limit, which is why the two belong in the same release.
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.
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.
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.
consumeRecoveryCode() reads the whole list, filters the used code out,
and writes the whole list back. Two requests that both read before
either writes each store their own copy, and the second write puts back
the code the first removed. So a spent code comes back, and the same
code offered twice is accepted twice -- while the method's first line
says "each code works exactly once".
Nobody gets in through this who was not already holding a valid code, so
it is a promise not being kept rather than a door standing open. The
promise is worth keeping anyway: it is the whole reason a printed sheet
of recovery codes can be crossed off, and it is what makes a code that
somebody watched being typed in stop working.
The decision now comes from the row as it stands, re-read under a lock
inside the transaction that writes it -- the shape SendNotificationDigest
already uses to claim the rows it is about to delete. A conditional
update, as in PublicShareController's downloads_count and the delivered_at
claim in #1692, is the other precedent in the tree, but the column is
`encrypted:array`: there is nothing in it a database can compare, so the
comparison has to happen after decryption, under something that holds the
row while it does.
The lock is what makes it atomic against a request arriving at the same
moment. The re-read is what makes the decision right, and it is the half
a test can show: SQLite ignores lockForUpdate, so the accompanying tests
pin the re-read and say so rather than claiming to prove the locking.
config/database.php runs MySQL or Postgres in production, and both honour
it. Saving through the caller's own instance keeps that instance in step
with the row, so a caller cannot go on to decide from a list the database
no longer has.
Every route that binds one client — the edit screen, the update, the
delete, the second-factor reset, the file browser — asks whether this
staff member may manage that client. POST users/convert/{user} binds one
too, and asks nothing about it.
AccountConversion::guardToStaff() says why it skips
StaffAccounts::guardTarget, and the reason is sound as far as it goes:
guardTarget asks "could the actor have granted the target's role", which
is meaningless of a client, and what limits a promotion is the role being
*granted* — enforced by the controller validating role_id against
assignableRoleIds(). That answers the question about the role. Nothing
answers the one about the target.
So a client-scoped staff member holding manage_users, edit_users and
edit_clients could promote any client on the installation. It is the
most far-reaching thing that can be done to a client account: the portal
access goes, the assignments that made them somebody's client go inert,
and they come out holding whatever staff role the actor picked from
their own list. The client is never told.
guardToStaff() now asks StaffLibraryScope::canAssignClient — the same
predicate ResolvesShareTargets uses to decide who a file may be shared
with, and true by construction for unscoped staff, so the ordinary
administrator path is untouched. 404 rather than 403, matching both the
isClient() check the controller makes on the way in and the answer the
clients routes give: a client this staff member may not manage should
not be distinguishable from one that is not there.
guardToClient() is unchanged. Its target is a staff account, guardTarget
is the right question to ask about one, and it was already being asked.
The account list on the converter screen is deliberately left as it is.
It runs under can:edit_users and shows every account of the chosen
direction, the same way every other staff surface that lists clients
shows all of them; narrowing a listing is a product decision, not this
fix. What changes is that the button on the row now refuses rather than
going through.
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.
Client file sharing, rebuilt from the ground up: a private area per
client, resumable uploads, folders, groups and categories, sharing with
expiry dates and download limits, comments, file versions, an activity
log, a REST API, and sixteen languages.
This repository begins here. ProjectSend 2 was developed privately, and
that development history is not published — the previous generation
remains available, with its own history, at projectsend/legacy.
Free software under the GNU General Public License v2, or (at your
option) any later version.
ignacionelson