Connecting a provider needed nothing but the session. Anyone holding one
could POST /settings/connected-accounts/google, follow the returned
Inertia::location(), sign in at the provider as *themselves*, and
completeLink() would bind their identity to the victim's account.
SocialAccount says what that row is:
This row *is* the authorization to sign in as that account.
So it is not a preference -- it is a credential, and one that outlives
every way the victim has of ending the session that created it. It
survives a password change, it survives Auth::logoutOtherDevices(), it
survives invalidating every session. Where a stolen session gives an
attacker access until it is noticed, this gives them an account.
routes/settings.php already makes exactly this argument, twenty lines
down, for the two-factor block and the API token routes:
a token outlives the session that minted it, so a stolen session must
not be enough to mint one
The link has that property too, and was the one thing on this screen
without the gate. Now it has it.
The gate goes on `connect`, not on the callback: starting the flow is what
writes the intent the callback completes, and the callback deliberately
sits outside every group so a provider sign-in works without a session.
Not changed, deliberately: `connected-accounts.destroy`. Disconnecting
removes a way in rather than adding one, and destroy() already refuses to
remove the last one ("This is the only way you can sign in. Set a password
first"). Putting it behind password.confirm would fall hardest on the
accounts a provider provisioned -- they hold a Str::password(64) nobody
has ever seen -- and leave them unable to disconnect anything at all.
There is a test pinning that it stays reachable.
Also not changed: the account owner still is not told. SocialLoginController
writes an activity log entry, and that sits behind `staff` +
can:view_actions_log, so a client never sees it. Notifying them is a real
gap and a separate change; this one closes the door rather than adding a
bell to it.
Tests: two that fail against the ungated route -- the redirect, and the
whole attack end to end with a stranger identity never binding. The
existing connect() helper now confirms the password, the way
enableTwoFactor() already did, so the rest of the file keeps exercising
the real gate rather than asserting around it.
/users/convert lists the accounts a conversion can be started from. For
the promotion direction those are clients, and the query asked only for
the type:
User::query()->where('type', UserType::Client)
The write beside it does not. AccountConversion::guardToStaff() ends with
abort_unless($this->library->canAssignClient($actor, $target), 404);
and says why: a promotion is the most far-reaching thing that can be done
to a client, so reaching one outside the actor's roster "through this door
and no other is not a rule, it is a gap".
The gap was on the way in. A client-scoped staff member holding
manage_users and edit_users was refused the promotion with a 404 -- the
refusal that is careful not to distinguish a stranger from an account that
is not there -- and then shown that same person's name, email, role,
status and consequence counts in the list the refusal came from,
searchable by name or address and paginated to the end.
StaffLibraryScope::clients() is canAssignClient()'s listing half, written
for this: "so a screen narrows by the same rule its buttons are guarded
with rather than restating it -- which is how ClientsController came to
list every client on the installation, name and email, to a viewer who
could reach nothing of theirs." The picker twenty lines below already went
through the same boundary via assignableClientIds().
Only the client direction is narrowed. The staff direction is left exactly
as it was: whoever may demote a staff member may see the staff roster, and
what limits a demotion is guardTarget() on the write, not the listing.
Tests: the listing half added to AccountConversionScopeTest, which until
now covered only the refusals. Two of the five fail against the unfixed
controller -- the stranger's address in the list, and reaching it by exact
search. The other three pin what must not change: the actor still sees
their own client, unscoped staff still see everybody, and the demotion
list still lists staff.
Two accounts reach the same reset with opposite needs, and it answered both by writing a hash and hoping.
A provider account is asked for something it cannot do. The Connected accounts screen refuses to release an account's last provider -- "Set a password first, then disconnect Google" -- and nothing set auth_source back to Local, so the screen went on asking for what had just been done, with no way out from inside the application. AuthSource already states the rule that closes it, for this case by name: a social account may later set a real password, and social only means the account came into existence without anybody choosing one. A reset by emailed token is where somebody chooses one, and the prop the screen reads is literally auth_source === Local under the name has_local_password.
A directory account is told something untrue. isDirectoryAccount() means the local hash is not consulted at all, so the same reset wrote a password that could never sign anybody in and reported success -- including when the directory it points at is gone, which is exactly the situation that sends somebody to a reset.
The reset now asks where the account's credentials live. social becomes Local, because the new password is the credential now. A directory account is refused, with the reason, and nothing about it moves -- writing Local there would not record something that had happened, it would take the account off its directory as a side effect of a password reset, which is an administrator's decision and already lives in AccountConversion with the password requirement and activity entry that belong to it. Everything else is byte for byte as before.
Verified before merging: 20 passed on the trial-merge, 2 failed / 18 passed with app/ reset, and the wider suites green -- tests/Feature/Auth 96 passed, tests/Feature/Identity 302 passed. Four properties were checked in the framework rather than argued. PasswordBroker::reset() calls validateReset() before the callback, so the refusal only reaches somebody holding a token emailed to that address and nothing is enumerable. It deletes the token after the callback, so a throw leaves the link usable. Every use of AuthSource::Local is in ConnectedAccountsController -- the has_local_password prop and the last-provider guard -- so the social-to-Local flip grants exactly the ability the screen instructs the user to obtain and nothing else, and no new login capability at all, since password login already worked for social accounts. And the check is isDirectoryAccount() rather than an auth_source comparison because LDAP is client-only, so staff are not refused; the test for that is green either way.
One new string is English only for now: "This account signs in through your directory, so its password is not set here."
Reported and fixed by @denkfabrik-li.
TwoFactorService::verify() asked Cache::has(), verified the code, 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 precisely what the replay guard exists to prevent, and the window an intercepted code has is the whole of its validity either side.
Cache::add() writes only if the key is absent, so of two requests carrying the same valid code exactly one gets true back, and has() is gone: a failed claim is "already used". Verification still runs first, so a wrong code never touches the cache and cannot burn the window for the code the person is about to type correctly. The 90-second claim, the key's shape, and the recovery codes are all unchanged.
Verified before merging: 11 passed on the trial-merge, 1 failed / 10 passed with app/ reset. The existing "a totp code cannot be replayed" test is green either way, because it covers the sequential case, which was never the problem. Being on the authentication path, the wider suites were run too: tests/Feature/Identity and tests/Feature/Auth together, 393 passed.
Cache::add() is only as atomic as the store under it, so every store an installation could realistically run was checked in the vendored framework rather than assumed: database (the default when CACHE_STORE is unset) decides on insertOrIgnore(...) > 0 against the cache table's primary key; redis and memcached have native atomic adds; and the file store takes an exclusive flock before it reads and writes.
Reported and fixed by @denkfabrik-li.
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.
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.
`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.
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.
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.
EnforceTwoFactor exempts by route name, and only the GET half of
confirm-password has one. routes/auth.php:95 names the form
`password.confirm`; :98 registers its submission with no name at all, and
Route::named() answers false for a null name.
So the loop the exemption exists to prevent is still there, one step
further along. With Setting::TwoFactorEnforcement set to staff, clients
or all, an un-enrolled account walks:
GET /dashboard -> two-factor.show
GET /system/settings/security -> two-factor.show
PATCH /system/settings/security -> two-factor.show
POST /settings/two-factor -> /confirm-password (RequirePassword)
GET /confirm-password -> 200, the form renders
POST /confirm-password -> two-factor.show <- not exempt
`auth.password_confirmed_at` is never written, so enrolling can never
start, and every route that is not on the exemption list stays shut --
including Settings -> Security, the one screen that could turn
enforcement back off. Logout is the only door left; recovery is CLI or
database access. It takes one administrator turning the setting on to
reach it, and it reaches every account on the installation at once,
including their own.
The fix is the name. `password.confirm*` then covers both halves of one
screen, matching `two-factor.*` in the same expression; the namespace
belongs entirely to a flow enrolment already depends on being reachable,
and the route table has nothing else under it -- `password.confirm` (GET)
and `password.confirm.store` (POST) are the two it reaches.
Exempting the submission grants nothing further. store() validates the
password, writes a session flag and redirects; the redirect it issues
enters this middleware like any other request, so Settings -> Security is
still answered with two-factor.show after confirming. What changes is
that enrolment can now be started.
Two tests, both measured red against the unfixed middleware: the password
confirmation sticks, and enrolment can be started afterwards (the secret
is written and the screen reports `pending`).
Also named the redirect the existing test settles for. `->assertRedirect()`
with no target passes on this middleware bouncing the request back to
two-factor.show, which is the shape that file exists to refuse. It is a
clarification rather than a guard -- that assertion is green either way,
since the redirect it sees comes from RequirePassword.
Full suite passes (2050 passed / 2 skipped), PHPStan level 8 clean.
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