mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
main
66 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
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
|
0671848bfa |
Read settings written before the columns they name existed
Reported by @apps3000 in #1770. Upgrading a container from 2.0 or 2.1 with external storage configured restart-loops, and says the database is unreachable while the database is fine. A row hydrated from the database does not get the model's column defaults — only a new model does. So a row written before external_storage_settings.provider existed reads that column as null, and the enum match in isConfigured() throws UnhandledMatchError. That would be a small bug anywhere else. It is not here, because PlatformServiceProvider::boot() reads these settings on every process boot, and boot happens before `artisan migrate` runs. During an upgrade the code is new and the schema is still old, so every artisan command in that window dies — including `projectsend:update`, the one that would have added the column. Reordering the entrypoint or using a lighter readiness probe does not help for that reason; the crash is in the bootstrap, not in the probe. current() now applies the model's declared defaults to any column the hydrated row does not have. That closes the window for every column with a default rather than for the one where it was found, and goes inert the moment the schema is current. The match in isConfigured() is left total on purpose: a default arm would swallow a real unhandled case, and the invariant it needs now holds at the one place the row is read. The probe's message is the other half. It boots the whole application, so it fails both when the database is absent and when the application cannot start, and it reported the second as the first — sending an operator off checking credentials that were never wrong. It now prints the error it actually hit and says which of the two it looks like. Verified end to end against a 2.1-shaped database: `artisan migrate` dies with UnhandledMatchError before the change and completes after it, leaving the row reading as S3 with its bucket intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmyH342d8MuW3pDuE9mbtS |
||
|
ignacionelson
|
eaba7ff633 |
Let an AWS-hosted install authenticate as its own IAM role
Requested by @ToMMy86 in #1773: an install running on ECS, EC2 or EKS already has a role attached, and making it also create an IAM user with a long-lived access key is both extra work and a worse security posture than the one AWS offers. The AWS SDK resolves credentials from its default provider chain whenever none is supplied, and Laravel's FilesystemManager already omits the `credentials` entry when the key and secret are empty — so the upload path needed almost nothing. What blocked it was ours: - `isConfigured()` demanded a key and a secret for S3, so a credential-less row was never "configured" and every upload silently stayed on the local disk. - `access_key` was `required_if:provider,s3` on both the save and the connection test. - `probeS3()` built an explicit `credentials` array, so Test connection would have failed even once uploads worked. An explicit `use_instance_role` column rather than "the key was left blank", because blank already means "keep the credential you have" on this form — neither the secret nor the GCS key file is ever sent back to the browser. Ticking it deletes the stored key and secret rather than leaving them in the row for the next database dump. Unchanged for everyone else: MinIO, Backblaze, Wasabi and any other S3-compatible service still authenticate with a key and secret, and the region is still required — the chain resolves credentials, not regions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmyH342d8MuW3pDuE9mbtS |
||
|
ignacionelson
|
e187513cdd |
Stop a mistyped CAPTCHA flag from switching the CAPTCHA off
`env()` recognises the words "true" and "false" and returns everything
else as the string it was — and every non-empty string is truthy in PHP.
So `(bool) env('PROJECTSEND_CAPTCHA_DISABLED')` read all of these as "yes,
disabled":
PROJECTSEND_CAPTCHA_DISABLED=no
PROJECTSEND_CAPTCHA_DISABLED=off
PROJECTSEND_CAPTCHA_DISABLED=fasle
An operator who meant to say no took the bot protection off their login
and registration forms and had nothing to tell them so — the setting
screen still shows the CAPTCHA configured, because this is the escape
hatch that runs ahead of it.
For most settings the cast is a shrug: somebody notices the feature is on
and fixes the line. It stops being a shrug when the wrong answer is the
unsafe one, and this is one of those. EnvFlag lists what counts as yes —
`true` and `1`, either case, either type — and reads everything else,
recognised or not, as no. A value typed as `disabled` turns nothing off:
a configuration mistake to be found rather than guessed at.
Found while fixing the same bug in a new cloud-modules flag, where the
unsafe direction was publishing a customer's files rather than dropping a
CAPTCHA. Two of the four remaining `(bool) env()` casts are left alone on
purpose: a wrong S3 path-style value breaks storage loudly, and the
migration tool's direct mode defaults to true anyway, so neither fails
into an unsafe state.
|
||
|
ignacionelson
|
b128b114b5 |
Make an announcement say who it is for
The first version refused clients outright. That was right for the only message that existed — a hosted instance telling its administrator about their plan — and it stopped being right the moment a message needed to reach the *clients* of a shared instance, where the administrator is the operator and the customers are client accounts. The unsafe fix would have been to drop the guard and let each listener check `isStaff`. The safe one is to make every caller say who it is talking to and have core enforce it: `show()` now takes a required `audience` with no default, and a message aimed elsewhere is dropped before it reaches the props. A listener that forgets therefore reaches nobody rather than everybody, which is the direction a mistake should fall. An unrecognised audience reaches nobody either, and is ignored rather than thrown — a listener aimed at the wrong people should show nothing, not break the page it was decorating. The old "a client is never shown one" test became "a message for staff reaches no client, even from a listener that never checks", which is the property that actually matters and the one the enforcement provides. Two more pin the other directions: a client message reaches clients and no staff, and an unknown audience reaches neither. cloud-modules declares `staff` for the free-plan band, and its test fake enforces the same rule, so a listener aimed at the wrong audience fails in the package's own suite rather than passing there and misbehaving in the host. |
||
|
ignacionelson
|
7c16733c16 |
Stop a managed instance being able to hide the project news
I shipped both daily calls as the same kind of thing — an operator's preference — and only one of them is. That was wrong in the direction that matters, because it handed a decision over rather than keeping it. An update notice on a hosted tenant is useless: they cannot act on it, the image is ours, and the screen that would show it is closed by capability. So that check does not run there at all, which is right and unchanged. News is the reverse. Announcements about the product are exactly what a hosted customer should be told, and a Cloud client with view_news sees that card today. One administrator switching it off for everybody on that instance is not a decision the platform meant to hand over — so on a managed instance the news now runs whatever any setting says, including a row left behind by an instance that used to be self-hosted. Capability::NewsConfigure, Community-only, and the thing it gates is the *choice* rather than the news. A self-hosted operator keeps the switch, because there nobody else decides what their installation reaches out for. An edition difference through the capability registry rather than an edition check, as everything here is. Gated in all three places rather than only the screen: the command ignores the setting without the capability, the controller neither sends nor reads the field, and the checkbox is absent. There is a test that a hand-crafted PATCH cannot do what the missing checkbox could not, and the guard is proved load-bearing — remove it and the managed-instance test goes red. The changelog and product highlights said "two switches" and now say what is actually true, including that neither appears on Cloud and why they are absent for opposite reasons. |
||
|
ignacionelson
|
d7d7acce85 |
Put the announcement behind the header icon too, from one source
A message worth showing was only on the dashboard, which means somebody who works in Files and Clients all day never meets it. It now also sits behind an icon next to the notification bell, and that is on every page. **One shared prop, not two.** "The same message in both places" is the requirement, and two props would have drifted the first time anybody edited one — so the hook moved out of DashboardController into HandleInertiaRequests, and the dashboard reads the same shared value the header does. The band and the dropdown also share the component that renders the words, for the same reason: the reliable way to keep two renderings identical is not to have two. Renamed with it. ResolvingDashboardCallout was accurate for about an hour and became a lie the moment it appeared somewhere else; it is ResolvingAnnouncement now, and the prop is `announcement`. Free to rename because nothing has shipped yet — the only other reference was cloud-modules', by string, updated alongside. The icon follows UpdateAvailableIcon beside it: absent entirely when there is nothing to say rather than a dead control, and a plain dot instead of a count, because there is only ever one of these and a "1" would invite somebody to look for the second. Two tests worth naming. One asserts the message reaches a page that is not the dashboard, which is the whole point of the addition. The other asserts a client is shown nothing even from a listener that sets it unconditionally — a client's header carries the bell too, and staff messages must not reach it however careless the listener. |
||
|
ignacionelson
|
334b11d562 |
Give packages a way into the sidebar and the top of the dashboard
Two seams, in the shape docs/extension-points-architecture.md settles on: a Laravel event with a mutable payload, dispatched unconditionally, and with nothing listening the documented default holds. A community installation gets an empty list and a null callout, which is exactly what it had before. ResolvingNavigationLinks exists because the sidebar is a hardcoded array in app-sidebar.tsx, so a package could not contribute to it at all — the nav entry was a separate manual edit every time a package grew a screen, and being manual it was forgotten more than once. Staff-only, decided in HandleInertiaRequests rather than trusted to each listener: these render in the administration area, and a client's portal shows their own files and nothing about the installation. There is a test that a listener adding unconditionally still reaches no client. ResolvingDashboardCallout is one band above the widget grid rather than a widget in it. The grid is a closed list of keys that dashboard.tsx renders one by one and each viewer arranges, so a message that mattered would sit wherever somebody dragged it, or under a fold, or switched off. One at a time, first listener wins: a dashboard that can accumulate banners accumulates them, and the second is what teaches people to skip the first. Core learns nothing about what either seam carries. Titles, URLs and copy all arrive from the listener, and that is not fastidiousness — the first caller is the hosted edition's link to its own customer portal and its pitch to free instances, which is commercial copy belonging to one offering and has no business sitting in the public repository because the sidebar happens to live here. An external link renders as a plain anchor opening in a new tab, never an Inertia <Link>: Link expects a page component back and another origin will not give it one, so it fails without saying so. It is also never marked active — nothing outside this app is the page you are on. |
||
|
ignacionelson
|
da1f432d87 |
Let an installation stop calling home, two different ways
Every instance reached projectsend.org twice a day and an operator could stop neither. The news feed had no switch of any kind — FetchNewsCommand went straight to the request, touching Settings only to write results back. The update check had one, but its default is on, and a managed fleet had been setting PROJECTSEND_CHECK_FOR_UPDATES=false for months against code that reads no such variable: check_for_updates is a database setting, so the environment never touched it and updates were enabled fleet-wide the whole time. They look like one problem and are two, which is why they are fixed differently. **The news feed gets a Setting**, its own key, default on. A Cloud client with view_news sees that card today — DashboardController gates it on the permission alone, with a comment saying in as many words that it is both editions and carries no capability. So switching it off is an operator's choice rather than an edition's, and it must stay reachable everywhere. Its own key rather than riding on check_for_updates because they are two different wants: "do not tell me about releases" and "do not show me the project's news" are asked separately, and an installation with no outbound access at all wants both. **The update check gets a capability guard**, ahead of the setting it already had, and deliberately not a Setting of its own. On a managed installation the result is unreachable rather than unwanted: the dashboard's System card and the update UI are both gated on Capability::SystemUpdates, which is Community-only, and the image is chosen by whoever provisioned the instance. A Setting would encode a fact about the edition as a preference — leaving it switchable back on per tenant, buying a nightly call for a number no screen can draw, and putting the reason in a provisioning script rather than beside the code. A self-hosted install holds the capability and loses nothing: its own setting still decides. Both guards return success rather than failure. A scheduled task that was asked not to run has not failed, and reporting it as one would put a red line in the scheduler history every night for an installation behaving exactly as configured. The news switch is on the General settings screen, outside the can_manage_updates block that hides the update toggle where the capability is absent — a setting only reachable by editing a database row is a row, not a switch. Seven tests, and the two that matter go red when either guard is removed. Sixteen locales translated in the same commit rather than left for the pass, since a release is close. |
||
|
ignacionelson
|
1e30e83f11 |
Stop projectsend:captcha-off claiming a success it did not have
The command writes Setting::CaptchaProvider = 'none'. On an installation using the platform's managed keys, Captcha::resolve() returns managedConfig() — read from config — before it ever looks at that setting, so the write lands somewhere nothing reads and every form stays protected. The command then printed "CAPTCHA is off". That is false in the worst direction: the person running this is locked out and debugging, and the message sends them away from the one thing that would have explained why they are still being challenged. It now says it changed nothing, and names PROJECTSEND_CAPTCHA_DISABLED, which is checked ahead of the key source and is therefore the only one of the two escape hatches that works on a managed installation. The docblock said those two were equivalent; they never were. Deliberately not gated behind captcha.configure. Gating it would take a self-hosted operator's way back in — the alternative being a hand-edited database row — to close something that on a managed installation does nothing anyway. Reaching it needs a shell in the container, which needs an RCE, at which point the CAPTCHA is not the problem. The command had no test at all. It has three now, including one that pins the ordering inside resolve(): if the environment check ever moves below the key source, a locked-out operator loses their last way in. |
||
|
ignacionelson
|
d32788e4a1 |
Put the CAPTCHA settings screen behind a capability
The screen is open in both editions and stays that way by default, so a self-hosted installation loses nothing: nobody else supplies its keys, and nobody else is affected by what it decides. What the key buys is the ability to take it away. A hosted fleet puts every tenant on one parent domain and one sending reputation, so an administrator who turns their own CAPTCHA off is spending everybody else's deliverability rather than only their own. That is not the shape LDAP and social login have, which is why those two stay ungated and this one does not. Gated all-or-nothing on the route, read included, exactly as Storage and Branding are. Per-field gating in the controller would not have closed it: switching the CAPTCHA off needs none of the gated fields — `provider: none` does it, and so does unticking the four per-form switches while leaving good keys in place — so the PATCH had to be closed too, and the middleware closes both verbs at once. Which keys the screen may offer is still the separate, narrower question Capability::CaptchaManagedKeys answers per field. An operator withdraws it by naming captcha.configure in PROJECTSEND_CAPABILITIES_DISABLED. Note that the key also joins the list `projectsend:status` and GET /api/v1/me report, which is additive — the OpenAPI document types capabilities as an untyped array, so nothing there needed regenerating. |
||
|
ignacionelson
|
6340b71dca |
Report recent usage and whether the scheduler is alive
projectsend:status could say what an installation holds and how many accounts it has, but nothing about whether anybody was using it. Adds a `usage` block -- downloads split staff/clients/anonymous, uploads, and five allowlisted action counts -- plus `activity.last_client_login_at`, `health.scheduler` and `health.failed_jobs_latest_at`. The scheduler is the one worth having on its own. `health.queues` catches a dead worker; nothing caught a dead scheduler, and its first symptom is not a stalled feature but an expired file that is still downloadable, because the job that was going to remove it stopped running weeks ago. Nothing about the installation looks wrong while that is true. `failed_jobs_latest_at` exists because the count beside it cannot say whether anything is wrong *now*, and reading it as though it could is a category error rather than a threshold wanting tuning. The table is swept daily, so the count spans a retention window -- one the installation chooses, and one that can be set to keep-forever by somebody who treats a failed job as evidence rather than debris. Two identical installations therefore report different numbers, and on a keep-forever one the count grows until any fixed threshold trips. A timestamp is independent of how long rows are kept: 27 failures whose newest is three weeks old is an installation that has been healthy for three weeks and has not been swept yet. `usage` is a rolling window with no lifetime totals, and that is a correctness decision rather than a presentational one: activity_log is never pruned, so a lifetime count over it gets slower every day of the installation's life while a windowed one stays flat. The window is emitted as `window_days` rather than left for the reader to assume. The actions are an allowlist, not a `group by action`. This document leaves the installation and Action gains cases most weeks, so an open group-by would ship new action names outward with nobody having decided they should go -- and some of them (account.erased, two_factor.reset) are somebody's compliance event, not a business metric. It is also ~30x cheaper: five keyed counts ride (action, created_at) while a group-by starts from created_at and reads rows. The scheduler's failure message and the queue exception text are omitted for the same reason; they are the fields here that can carry a path or a stack trace, and a count with a timestamp says "go and look", which is all a watcher is owed. The two indexes ship as a pair and the migration explains at length why. Measured at 2.1M rows: adding (action, created_at) alone fixes the windowed counts and takes last_staff_login_at -- already running hourly on every tenant -- from 0.63s to 7.7s, because the planner switches to it, still needs actor_type, and does a scattered primary-key lookup per row. With both, that query is answered from the index without reading a row at all (0.0004s) and the whole new usage block costs ~70ms. Also documents the keys as a contract, the way `capabilities` already is. This one fails worse: a renamed capability key breaks a comparison somebody is watching, a renamed usage key produces a chart that is silently empty, and nobody gets paged for a flat line. |
||
|
ignacionelson
|
07e7132747 |
Merge pull request #1757 from denkfabrik-li/fix/dont-flash-stored-secrets
Stop a rejected settings form flashing the credential it carried |
||
|
denkfabrik-li
|
35d68a792b |
Stop a rejected settings form flashing the credential it carried
When validation fails, Laravel flashes the request's input into the
session so the form can be repopulated. Its exclusion list is
current_password, password and password_confirmation -- written for the
login and password screens, and covering none of the credentials the
system settings screens take. `dontFlash` did not appear anywhere in this
repository.
So every one of these went into the session in clear the moment its form
was rejected:
secret ExternalStorageSettingsController (S3 secret access key)
key_file ExternalStorageSettingsController (GCS service account JSON)
bind_password LdapSettingsController
client_secret SocialLoginSettingsController, EmailSettingsController
secret_key CaptchaSettingsController
Each is stored with an `encrypted` cast, and config/session.php puts
sessions in the database with `encrypt => false` -- so the rejected save
wrote in clear into the same database the cast exists to protect.
The sharpest one is key_file. serviceAccountKeyRule() exists to catch a
paste that lost its last line, which makes "the request carrying a
service account private key" and "the request that fails validation" the
same request more often than not.
dontFlash() merges rather than replaces, so the framework's three stay.
The cost is that these five come back blank after a failed save. That is
already what they do after a successful one -- every screen here treats
them as write-only, and a blank means "keep what is stored" -- so the
behaviour is now the same either way instead of only on success.
Tests: one per field, each submitting a form that fails validation while
carrying a secret, then reading the old input back the way the form
would. All five fail against the unmodified bootstrap/app.php. A sixth
pins that the framework's own three are still excluded, and each
assertion checks a neighbouring non-secret field still comes back, so
this cannot pass by flashing nothing at all.
Note for the record: this is testable in the existing harness after all.
phpunit.xml sets SESSION_DRIVER=array, but old input is written to the
session whatever the driver backs it, so getOldInput() sees exactly what
a database session would have stored.
|
||
|
denkfabrik-li
|
9af0d643b1 |
Keep the mail and storage credentials out of the boot-config cache
MailConfigApplier and ExternalStorageConfigApplier read their settings through the `encrypted` casts -- decrypted -- and wrote the result into the cache store with rememberForever(). The SMTP password, the S3 secret access key and the whole GCS service account key file, private key included, went in as plain text under a key that never expires. The cache store encrypts nothing. On the store INSTALL.md documents for a manual install (CACHE_STORE=database) and config/cache.php defaults to, that is the `cache` table of the same database whose dump the `encrypted` cast exists to survive. On redis it is the redis dump. The rule already exists, two files away. MailOAuthConnection states it: Transports read this row fresh at send time -- tokens must never travel through the boot-config cache (see MailConfigApplier, which caches only readiness and the account address). MailConfigApplier's own cache-key comment says the same thing about the same array: what is deliberately NOT in the cached shape is tokens, because neither readiness nor an address is a credential. The SMTP password was in it anyway. SocialSettings::available() names both classes outright as making the mistake. So the credentials are read the way the tokens already are: from the row, at the point that uses them. The cached array keeps everything that is not a credential, and each applier reads its secret inside the branch that configures a transport -- an installation on OAuth, on cloud, or one that has never opened the Email or Storage screen reads nothing extra. BootSettingsCache grows a second entry point rather than the callers restating its rule. The cached read already survives a database with no tables, because booting must not require this application's own database; an uncached credential read on the same path needs exactly that guarantee and nothing else, since resolve() can hand back a warm "configured" from a database that has since stopped answering. Both cache keys are bumped, as their comments require on a shape change. Tests: five for the absence, two of them against the database cache store read as the raw rows an operator would find in a dump, since phpunit.xml runs the suite on the array store and the cache path was structurally invisible -- which is why GoogleCloudStorageTest could assert that the private key is not in the column while it sat in the cache. All five were run against the unfixed appliers and fail there. The three "still configures what it no longer caches" tests deliberately pass either way: they pin the behaviour the fix must not break. |
||
|
ignacionelson
|
81bb136e9e |
Merge pull request #1750 from denkfabrik-li/fix/mail-oauth-alarm-fires-once
RefreshMailOAuthTokensCommand is the daily refresh and, by its own docblock, the health check that goes with it: a delegated grant can die silently, and for a portal whose password-reset mails ride on this connection that must surface as a warning rather than as a support ticket weeks later. It decided whether to warn from last_error -- but last_error has a second writer. OAuthCodeFlowBroker::refresh() records a dead grant and notifies nobody, and freshAccessToken() reaches it from every send. So on an installation that is actually sending mail the send got there first, the command read the column as "already told them", and the warning never went out. last_error is cleared only by a successful refresh, which a dead grant never has, so it never went out later either. The alarm worked on installations that were not using the mailbox and failed on the ones that were. The anti-nag rule is not the problem and does not change: one notification per broken state is still all anybody gets. The problem is that one column was answering two questions, which the table's own comment describes -- "what the settings page's warning and the admin notification read". The warning wants "is this connection broken", and any writer may answer it, which is why the settings page turning red on a failed send is correct and stays. The notification wants "have the admins been told", and only the notifier can answer that. broken_notified_at is stamped when the command notifies, and the command asks that instead. It is cleared wherever last_error is cleared -- a successful refresh, a disconnect, a changed client id -- and those three sites now call clearFailure() rather than nulling two columns each, because a connection left healthy but still marked "already told them" would go quiet the next time it died, and a fourth caller is exactly how the first one happened. The send path still records the failure and still notifies nobody: a transport is not a place to decide who gets alarmed. Verified before merging: 27 passed on the merged tree, 2 failed / 25 passed with app/ reset and the migration and tests kept. The recovery test is green either way by design. This touches the same command and broker as #1739 and the follow-up to it, so the merged result was read rather than trusted: the refresh reporting sits in the try and the notify guard in the catch, they do not interact, and refreshSerially() re-reads the row before refreshing so the broken_notified_at the catch reads is the stored one -- while a stand-aside throws nothing and never reaches the catch at all. Note for the next release's upgrade notes: this adds a migration, so "nothing to do beyond dropping in the files" no longer holds. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
ff26fac9c5 |
Say when the scheduled mail refresh stood aside
#1739 put the nightly OAuth refresh under the same lock a send holds, which is right -- but standing aside for the lock holder still printed "Refreshed <provider> (<account>)". No token request was made, so the line describes something that did not happen, and scheduler output is read precisely by somebody trying to work out what did.
refreshSerially() now answers whether it refreshed, and the command says which of the two happened. Standing aside is a healthy outcome: somebody else is refreshing this very connection, which slides the token window just as well as doing it again would. It is just not a refresh, and it should not claim to be one.
The existing test for the stand-aside now asserts the output too, and it fails against the old message.
Same reasoning as
|
||
|
ignacionelson
|
a7e883ef70 |
Merge pull request #1739 from denkfabrik-li/fix/scheduled-mail-refresh-lock
OAuthCodeFlowBroker::freshAccessToken() serialises refreshes per connection, and its comment says why: both providers rotate the refresh token as they hand out a new access token, so a refresh token is good for exactly one use, and "a worker racing the nightly refresh command" means the slower one spends a token the faster one has already replaced. The provider answers that with invalid_grant, which is the same thing it says about a genuinely revoked grant -- last_error gets written, the settings page turns red, and every admin is told to re-consent a connection that was never broken. RefreshMailOAuthTokensCommand called refresh() directly, outside that lock: it was the racer the comment names rather than a party to the arrangement it describes, and the false alarm landed on the connection the daily run exists to protect. The command now goes through refreshSerially(), which takes the same lock -- named once, in one place, for both callers -- re-reads the row inside it, and refreshes. Unlike freshAccessToken() it refreshes a token that is still usable, which is the point of the daily run: a delegated refresh token dies of disuse and this keeps the window sliding. The lock is taken rather than waited for, unlike the send path: nobody is standing at a screen for a scheduled job, and a held lock means somebody is refreshing this very connection right now, which slides the window and establishes its health just as well. refresh() stays lock-free, because making it self-locking would deadlock the send path that already holds the lock. Verified before merging: 24 passed on the trial-merge, 1 failed / 23 passed with app/ reset. PHPStan level 8 clean across app/Modules/Platform/Mail. Adding a method to the MailOAuthBroker interface breaks nothing: OAuthCodeFlowBroker is its only implementer, and MailOAuthBrokers is a registry rather than an implementation. Known nit, fixed in a follow-up rather than here: when refreshSerially() stands aside because the lock is held, the command still prints "Refreshed <provider> (<account>)". Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
6b99e37d01 |
Merge pull request #1729 from denkfabrik-li/fix/api-surface-by-route
Two places asked "is this the API?", and each got it wrong in the opposite direction.
EnsureCapability asked $request->expectsJson(). Whether a feature exists in this installation's edition is a property of the installation, not of what the caller is willing to parse: the same capability-gated API route answered 403 capability_unavailable to Accept: application/json and a bare 404 to Accept: */*, which is curl's default, while routes/api.php promises the 403 in as many words. The mirror image was worse -- an Inertia visit to a capability-gated web screen accepts JSON, so it took the API branch and announced the feature by name, where the whole point of the 404 is that an unavailable feature is absent rather than teased.
ProblemDetails asked $request->is('api/*'). Two staff pages live under that prefix -- the API dashboard at /api and the OpenAPI reference at /api/docs, both from routes/web.php -- so a signed-out visitor to /api/docs got a 401 problem+json telling them to send a Bearer token instead of the login redirect every other page gives.
One question now, asked once, in App\Support\ApiSurface: under the API prefix, and not part of the web middleware group. The group is what actually separates the two surfaces -- sessions, cookies and CSRF on one side, tokens on the other -- and it keeps answering correctly for a future /api/v2 without being edited. An unmatched path has no route to ask and stays the API's answer, which is what the existing "a missing API route is a problem+json 404" test pins. EnsureStaff keeps its expectsJson() check: there the question really is about the caller.
Verified before merging: the discriminator was checked in the running application rather than assumed -- /api/docs and /api resolve to [web, auth, staff], /api/v1/files to [api, auth:sanctum, api-active, staff-token, token-can:...]. 12 passed on the trial-merge; with app/ reset and ApiSurface deleted, 3 failed / 9 passed, every new test and no old one. Wider suites green: tests/Feature/Api 239 passed, tests/Feature/Platform 485 passed. scramble:export reproduces main's docs/api/openapi.json byte for byte. Worth recording that the blast radius here is the shape of a refusal and never whether one happens: both call sites run after authentication and authorization.
Reported and fixed by @denkfabrik-li.
|
||
|
ignacionelson
|
f676e09bb2 |
Merge pull request #1728 from denkfabrik-li/fix/expiry-timezone-drift
The edit screen is given a file's expiry as a calendar date read back in the viewer's own zone -- deliberately, or "a file set to expire on the 12th reopens showing the 11th". Every save posts that date back, touched or not, and update() derived a fresh instant from it every time. So the expiry drifted by the difference between two people's zones on any other edit: a file set from Pacific/Auckland moved 19 hours later the moment somebody in Buenos Aires renamed it, and moved again on the next save from a third zone. A file could quietly outlive the expiry somebody set for it, through an edit that had nothing to do with expiry. The instant is now re-derived only when the posted date differs from the one the form was given, compared against the same string through a named pair: expiryDateFor() renders it, expiryInstant() reads it back, and the edit screen calls the render half so the two cannot drift apart. What a changed date means is unchanged -- still the end of that day in the zone of whoever changed it. bulkUpdate() needs nothing: its expiry is an explicit set / clear / no_change action, so an untouched expiry is never posted at all. Verified before merging: 22 passed on the trial-merge, 1 failed / 21 passed with app/ reset. The "a real change still lands in the editor's zone" and "clearing still clears" tests are green either way. Edge cases walked: a posted date against no stored expiry still sets it, and a posted null against a stored null leaves the column alone rather than writing. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
530f30606d |
Let a plan take a capability away, and split branding from white-labelling
Groundwork for moving Branding out of the private package. Two changes, both about who decides what an installation may do. An edition grants capabilities; an operator may now take some away, via PROJECTSEND_CAPABILITIES_DISABLED. Subtractive only, and that asymmetry is the whole design: a variable that could *add* would put the hosted edition's proprietary screens one line of .env away on every self-hosted install, which is not a gate at all. So the list is intersected with what the edition already allows and can only make the answer smaller. This is not the plan tier core has always refused to invent. There are still no billing tiers here to key off -- the objection config/api.php makes about rate limits stands. It is the operator stating a fact about this installation, exactly as PROJECTSEND_PLATFORM_MAX_STAFF_USERS does for seats: the platform knows what it sold, the installation is told and enforces. Unknown keys are ignored rather than fatal, because the variable outlives both the plan that wrote it and the release that named the key, and refusing to boot over a stale one would be an outage on upgrade day. The registry takes the list as a constructor argument rather than reading config itself, which keeps it a value object testable without an application -- the failure that surfaced it was a unit test with no container. And branding.customize is now both editions, with the white-label half split into attribution.hide, which stays Cloud-only. Dressing an installation in its own logo is not a hosted concern; taking ProjectSend's name off somebody's public pages is what a hosted customer pays for. The gate on the second is not the key but that the only code able to answer "hide it" ships in the private package, so flipping an edition variable buys nothing. EnsureCapabilityMiddlewareTest had to pick a new Cloud-only example for the second time -- branding after users.manage. It now uses storage.managed, and records what to ask if it ever needs a third. The code move itself is the next commit; nothing user-visible changes yet, because the screens still live in cloud-modules. |
||
|
ignacionelson
|
d62c62f788 |
Let an installation say which build it is
A version string is a decision somebody made. A commit is a fact, and the two come apart exactly when it matters: an image built from the tag and one built from the branch that tag sits on carry the same version and different code. The fleet spent a day reporting 2.2.0 from images that were not the released 2.2.0, and nothing inside any of them could have said so -- which is why 2.2.1 was cut for a control plane rather than for users. So every artifact now carries config/build.php, written by build-release.sh and never committed, and projectsend:status reports it as `build`: the commit, the ref it describes to, the channel and the build time. All four are null on a source checkout, because there is no such file there. That is the honest answer rather than a missing one -- "I was not built" and "I will not say" are different facts, and this file's whole null discipline exists because a reader that cannot tell them apart eventually acts on the wrong one. An empty string is treated as no answer for the same reason: a build step that ran and produced nothing must not read as "answered" to anything checking presence. |
||
|
denkfabrik-li
|
7be81d3586 |
Tell the admins the mailbox is dead, even when a send noticed first
The daily refresh doubles as the health check for a connected OAuth mailbox, and its own docblock says why that matters: a grant can die silently, "which for a portal whose password-reset mails ride on this connection must surface as a warning, not as a support ticket weeks later". It decided whether to warn by reading last_error -- but the send path writes that column too. OAuthCodeFlowBroker::refresh() records the failure and notifies nobody, and freshAccessToken() reaches it from every send. So on an installation that actually sends mail, the send lands first, the command reads the column as "already told them", and the warning never goes out. last_error is cleared only by a successful refresh, which a dead grant never has, so it never goes out again either. Measured on main, one dead grant, two orders: nobody sends, command first 1 notification, then quiet correct a password-reset mail first 0 ... 0 ... 0 never The alarm worked on installations that were not using the mailbox and failed on the ones that were. The anti-nag rule is not the problem and does not change. The problem is that last_error answers "is this broken", which any writer may set, while the command needs "have the admins been told", which only the notifier can. The table's own comment shows the conflation -- one column described as "what the settings page's warning and the admin notification read". So the notification gets its own column. broken_notified_at is stamped when the command notifies, and cleared wherever last_error is cleared: a successful refresh, a disconnect, a changed client id. The three call sites go through MailOAuthConnection::clearFailure() rather than nulling two columns each, because a connection left marked "already told them" while healthy would go quiet the next time it died -- the same bug in a new place. |
||
|
denkfabrik-li
|
02eafb473b |
Refresh a mailbox on the schedule under the lock a send would hold
freshAccessToken() serialises refreshes per connection, and its comment says why: both providers rotate the refresh token as they hand out an access token, so the token is good for exactly one use, and "a worker racing the nightly refresh command means the slower one spends a token the faster one has already replaced. The provider answers that with invalid_grant, which is the same thing it says about a genuinely revoked grant: last_error gets written, the settings page turns red, and every admin is told to go and re-consent a connection that was never broken." The nightly refresh command called refresh() directly, outside that lock. It was the racer the comment names, not a party to the arrangement it describes. It now goes through refreshSerially(), which takes the same lock -- named once, in one place, for both callers -- re-reads the row inside it, and refreshes. Unlike freshAccessToken() it refreshes a token that is still usable, which is the point of the daily run: a delegated refresh token dies of disuse and this keeps the window sliding. The lock is taken rather than waited for, unlike the send path. Nobody is standing at a screen for a scheduled job, and a held lock means somebody is refreshing this very connection right now -- which slides the window and establishes its health just as well as doing it again would. One test: with the lock held, the command sends no token request and leaves the connection untouched. Without the fix it spends the refresh token the holder is already spending. |
||
|
denkfabrik-li
|
640c5db591 |
Stop an expiry moving because somebody else saved the file
The edit form is given a file's expiry as a calendar date, read back in the viewer's own zone -- deliberately, so a file set to expire on the 12th does not reopen showing the 11th. Every save posts that date back, whether or not anybody touched it, and update() derived a fresh instant from it every time. So the expiry drifts by the difference between two people's zones on any other edit. A date set from Pacific/Auckland stores 2026-09-12T11:59:59Z; a colleague in UTC-3 opens the file, sees the same 12th, renames it, and the file now expires at 2026-09-13T06:59:59Z -- 19 hours later, with nobody having gone near the date. The instant is now re-derived only when the posted date differs from the one the form was given, compared against the same string through a named pair: expiryDateFor() renders it, expiryInstant() reads it back. The edit screen uses the same method it is compared against, so the two cannot drift apart. bulkUpdate() needs nothing: its expiry is an explicit set/clear/no_change action, so an untouched expiry is never posted in the first place. Three tests: the rename leaves the instant alone, a real change still lands in the editor's own zone, and clearing still clears. Without the fix the first goes red. |
||
|
denkfabrik-li
|
f424fe5365 |
Decide what is an API request from the route, not from the caller's headers
Two places asked "is this the API?" and got it wrong in opposite ways.
EnsureCapability asked $request->expectsJson(). Whether a feature exists
in this installation's edition is a property of the installation, not of
what the caller is willing to parse, so the same route answered
differently per header: `Accept: application/json` got the 403
`capability_unavailable` routes/api.php promises, `Accept: */*` -- curl's
default -- got a bare 404 `not_found`. The mirror image is worse: an
Inertia visit to a capability-gated *web* screen accepts JSON, so it got
403 with Laravel's default error body, naming the exception class, where
the point of the 404 is that an unavailable feature is absent rather than
teased.
ProblemDetails asked $request->is('api/*'). Two staff pages live under
that prefix -- the API dashboard at /api and the OpenAPI reference at
/api/docs, both registered in routes/web.php -- so a signed-out visitor to
either got 401 problem+json, "Send a valid API token in the Authorization
header as \"Bearer <token>\"", instead of the login redirect every other
page gives them.
Both now ask App\Support\ApiSurface: under the API prefix, and not part of
the `web` middleware group. The group is what actually separates the two
-- sessions and CSRF on one side, tokens on the other -- and it keeps
answering correctly for a future /api/v2 without being edited. An
unmatched path has no route to ask, which is the API's answer anyway: a
404 under its prefix is one it should describe in its own format, and the
existing test for that stays green.
Three tests, in the two files that already own these rules. Without the
fix all three go red.
|
||
|
ignacionelson
|
06c364d29a |
Report storage, health and what packages loaded in projectsend:status
Five more facts for whatever watches an installation from outside the container, and one seam so a package can add its own. Storage is the one that was about to be wrong. It is summed from the rows that record it, not measured on the volume: measuring the directory was correct until external storage went live and silently stopped being, since an upload that resolves to a bucket leaves nothing on disk to measure. A figure taken from the filesystem freezes while the account keeps filling, and on a managed installation that figure is what a customer is shown and billed against. `by_disk` splits the same sum by where the bytes went, which is the only way to see what is still sitting locally from before a cutover. Trashed files are excluded because they hold no bytes -- File's deleted hook takes them. Health is what a container cannot show from outside. A queue worker dying is invisible to anything watching the process: it is still up, and zips quietly stop building while mail stops going out. Same for a deploy whose migrations failed -- the application answers every request and is a schema behind. An unreachable queue reports null rather than zero, because an unreachable Redis is not an empty queue and reading the second as the first is how a dead worker looks healthy. The two-factor enforcement setting is echoed back the way EnforceTwoFactor reads it, fallback included: reporting a stricter rule than the middleware actually applies would be worse than reporting none. And ResolvingInstallationStatus, so a package can report what core cannot know. The managed storage backend and the version of the package providing it live in cloud-modules, which this repository must not reference, and a platform that writes eight environment variables only ever knows what it asked for. Those came apart once: a bucket provisioned, a token minted, every variable correct, and an image whose copy of the package predated the module that reads them. Files went to local disk with the configuration sitting perfectly right beside them. Two shapes are cast to objects deliberately. An empty PHP array encodes as [], so an installation with no packages -- or holding no files -- would answer a map-shaped field with a list, and a reader unmarshalling it breaks on the day it happens to be empty rather than the day it is written. There is a test for each. Requested by the ProjectSend Cloud control plane, whose storage figure stops growing the moment a tenant's uploads start reaching the bucket. |
||
|
Ignacio Nelson
|
58497ef776 |
Merge pull request #1716 from denkfabrik-li/fix/sole-administrator-self-deletion
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. |
||
|
Ignacio Nelson
|
b16d780ebe |
Merge pull request #1712 from denkfabrik-li/fix/storage-durability-dashboard-assertion
The test named for carrying the durability verdict to the system widget asserted only has('system'), and system is an unconditional key of the render array -- the controller's own comment beside storage_durability says as much. So the assertion could not fail.
Confirmed here by deleting the line that supplies the verdict: the new assertion fails with "Property [system.storage_durability] does not exist", where the old one stayed green.
Test-only, no application code.
Reported and fixed by @denkfabrik-li.
|
||
|
Ignacio Nelson
|
76f79d53a0 |
Merge pull request #1711 from denkfabrik-li/fix/update-tests-clear-compiled
Ten tests ran the real projectsend:update, which runs clear-compiled, which deletes bootstrap/cache/packages.php and services.php -- one copy for the whole checkout, shared by all eight workers of a parallel run. A worker booting in the window between that delete and its own rebuild reads an empty package manifest, registers no package service providers, and dies rendering the next page with "Target [Inertia\Ssr\Gateway] is not instantiable", in a file that has nothing to do with updates. Verified here rather than taken on trust: a probe running the real update inside a test on main deletes the manifests, exactly as described. The branch is green at 2066 with PHPStan clean, and touches no application code. The file already owned a double and explained why the artisan call is a seam; this extends it to the whole file and adds a test asserting the compiled caches survive. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
3f81dd5eab |
Merge pull request #1709 from denkfabrik-li/fix/seat-cap-approval-doors
Two doors onto the client seat cap did not ask it. Both update()
methods -- the edit screen and PATCH /api/v1/clients/{id} -- clear
account_requested when a pending client is activated, under a comment
saying that counts as approval, and approval is the moment a seat is
spent. So a managed installation sitting at its cap kept taking clients
on for as long as registrations arrived, and self-registration is open
to strangers, so the supply of pending rows is not the operator's to
control.
Verified rather than taken on trust: the two new door tests were run
against the unguarded controllers and fail there, and every place in
app/ that clears the flag was enumerated to check no third door was
missed. There is none -- the other six already ask, and a conversion
refuses a pending account outright rather than approving it sideways.
The guard sits inside the approval branch, so an installation at its cap
can still rename a client it already holds. That is pinned by a test of
its own.
Conflicted with tonight's seat work in SeatAllowanceTest, which had
added an import beside the one this adds. Resolved by keeping both;
suite green at 2065 and PHPStan clean after resolution.
Reported and fixed by @denkfabrik-li.
|
||
|
ignacionelson
|
73d93495c9 |
Report the last staff sign-in in projectsend:status
A platform can see that an installation is running. It cannot see
whether anybody is still using it, and the difference is what separates
a customer from an abandoned free instance holding a database.
So the status probe gains one field:
"activity": { "last_staff_login_at": "2026-08-24T21:13:32+00:00" }
Null means no staff account has ever signed in, and the key is emitted
either way. That is the whole care in this change: "they said never" and
"we got no answer" have to stay distinguishable, because collapsing them
is how a broken probe reads as a dormant fleet.
Only interactive sign-ins count. Laravel's Login event does not fire for
token authentication, so an integration polling every hour cannot make
an empty installation look busy -- which matters when the reading is
used to decide something.
Derived from the activity log rather than denormalised onto users. A
column would cost a migration, a listener change and a backfill to save
one indexed MAX() over a table with a handful of rows on exactly the
installations anybody asks this about. Nothing prunes the log, and
erasure anonymises entries rather than removing them -- actor_type
survives on purpose -- so the answer does not change when the person who
gave it is forgotten.
Requested by the ProjectSend Cloud control plane, which has no other way
to learn the date. Recorded in docs/api-todo.md as deliberately a
command rather than an endpoint, for the reason the command exists at
all: it observes, it does not accept instructions.
|
||
|
ignacionelson
|
13b56186f4 |
Say the seat limit before the form, not after it
On a managed installation with its staff seats full, /users/create opened as though there were room. You typed a name, an address and a password you had to invent, pressed Save, and the plan limit came back as a validation error under the email field -- which reads as a complaint about the address rather than a fact about the plan. A full installation is an ordinary state on a plan sold by the seat, so it is now stated up front. The list carries the seat position, the button goes dead once the last seat is taken and says why, and the create screen turns away anyone who reaches it by link or bookmark. The guard in store() is untouched: that is still the rule, this is only the door. The refusal is worded once, in SeatAllowance, and the screen is handed that sentence rather than writing its own -- two wordings of one limit is how somebody ends up believing there are two limits. `full` is derived there too, from the same comparison the guard refuses on, so a screen cannot disagree with it about the edge (used > limit, after an operator lowers a limit) and offer a button for a form that cannot be submitted. Clients get the same treatment: the cap exists there too, and reached it the same way. Self-hosted installations have no limit, so they are shown nothing about one. |
||
|
denkfabrik-li
|
28e18497b5 |
Refuse the last administrator deleting themselves, and keep setup shut
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. |
||
|
denkfabrik-li
|
9cc469b111 |
Make the storage durability dashboard test assert the verdict
The test named for carrying the verdict to the system widget only asserted that the 'system' key exists. It is an unconditional key of the Inertia::render array and is allowed to be null, and Inertia's has() is a key check, so the assertion held whether or not the verdict was in there. Deleting 'storage_durability' from DashboardController::systemInfo() left the file green. Substitute the class the way the rest of the file already does and assert the payload, as InstallationKindTest does for install_kind next door. |
||
|
denkfabrik-li
|
4469648d82 |
Stop the update tests emptying bootstrap/cache for every other worker
`UpdateWelcomeTest > staff who may not read system information are not
interrupted` fails on a parallel run roughly one time in six, with
BindingResolutionException: Target [Inertia\Ssr\Gateway] is not
instantiable
in a file that has nothing to do with updates. Run alone it is green
every time. The cause is not in that file.
`clear-compiled` deletes bootstrap/cache/packages.php and
bootstrap/cache/services.php. There is one of each for the whole
checkout, and `pest --parallel` gives eight worker processes the same
one. Instrumented over three full runs, the real command ran 12 times per
run -- 11 from UpdateCommandTest, 1 from StaleCodeNoticeTest -- and the
other workers observed the package manifest missing at boot 46 times.
What that costs is in PackageManifest::getManifest():
if (! is_file($this->manifestPath)) {
$this->build();
}
return $this->manifest = is_file($this->manifestPath) ?
$this->files->getRequire($this->manifestPath) : [];
A worker that loses the second is_file() to another worker's unlink gets
`[]`: no discovered packages, so no package service providers, so
Inertia's is never registered and `Inertia\Ssr\Gateway` is never bound.
The next page it renders dies in the compiled root view, where
`@inertia` resolves that interface. Any test in any file, whichever one
happened to be booting.
Both halves measured. Building the manifest with inertia-laravel in
`dont-discover` reproduces the reported failure exactly -- same test,
same exception, same frame (`app('Inertia\Ssr\Gateway')` from the
compiled app.blade.php). And 12 real `clear-compiled` calls per run is
the count above.
UpdateCommandTest already owns a double for this, and says why in its own
docblock: the artisan call is a seam. Nine of its tests and one in
StaleCodeNoticeTest simply do not use it. None of them asserts that a
command ran -- they assert EnsureSystemRoles, the settings writes, the
activity log and the welcome marker, and the double touches none of
those. So the seam now covers the file, through a beforeEach rather than
per test, because the next test added here should not have to know any of
this.
The double moves to tests/Support and its helper to tests/Helpers.php,
for the reason that file documents: Pest hands whole files to workers, so
a class declared in one test file does not exist for another.
Not changed: UpdateInstallation. `clear-compiled` belongs in a real
update. Also not changed: giving each worker its own bootstrap/cache
through APP_PACKAGES_CACHE and friends. That would make the destruction
cheap rather than remove it, and nothing in the suite needs those
commands to run at all.
One new test, on the files rather than on the recorded call list -- a
future double that forgot to intercept one command would still satisfy a
call-list assertion. Counter-checked: with the beforeEach removed it goes
red on both manifests being gone (1 failed / 22 passed).
Eight consecutive parallel runs green after the change; the manifests'
mtimes are untouched by a full run, where before they were rewritten
every time. Full suite passes (2049 passed / 2 skipped). PHPStan level 8
clean -- it analyses `app` only, so it does not cover this change.
Pre-existing and left alone: pint reports `ordered_imports` on
UpdateCommandTest.php. Its import block is misordered on main too.
|
||
|
denkfabrik-li
|
ab6e9eecf3 |
Ask the seat cap where a pending client is approved through edit()
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.
|
||
|
ignacionelson
|
d58e48301f |
Move the seat number to the end of the sentence
It read "limited to 1 staff accounts" -- the number sat directly in front of a countable noun, which is the message a free-tier customer meets the first time they try to add anybody. Adding plural forms would fix English and not much else. Polish, Czech and Russian inflect the noun by the number in front of it, on a three-way split that a two-form string cannot express, so ':count kont' cannot be right for every value however many variants it carries. Ending the sentence on the number means no language has to agree with it -- the same shape the other counted strings here already use. Both strings rewritten in all sixteen locales rather than left to the next translation pass, since the old key would otherwise go missing and block a build. Checked at 1 and at 25 in English, Spanish, German, Polish and Russian. |
||
|
ignacionelson
|
787e9ec189 |
Report version, edition, capabilities and seat usage as one probe
Asked for by the platform side, and the reason is better than convenience. Their reconciler's rule is that it observes an end state and never sends an instruction. `docker exec … php -r '…'` to reach a public method is an instruction with the caller's argv in it, however harmless the argv, and it would have been the first crack in that rule. A named command is an observation, the same kind of thing as reading a directory size. `--json` for a machine, plain lines for a person. Nothing here is a secret or a credential: every field is already visible to any signed-in administrator, which is what makes it safe to read from outside the container. The counts come from SeatAllowance — the code that refuses the account past the limit — rather than from a second query that agrees with it today. Two counts that merely agree diverge eventually, over an inactive account or a soft-deleted one, and the divergence reads as a billing fault rather than a counting one. Unlimited is emitted as null, with a test saying so, because the failure if a reader takes it for zero is a customer on the most expensive plan whose instance refuses to create a single client. The platform side independently landed the same care on the emitting end, omitting the variable rather than sending it empty. It also answers the question that started all of this. Diagnosing why a tenant ignored its bucket meant reaching into a container and calling app() by hand; `projectsend:status` now says which capabilities the edition grants, which is where that hunt began. |
||
|
ignacionelson
|
ac691387e8 |
Seed two-factor enforcement at provision, before the first account exists
The last of the three. Enforcement is a database setting defaulting to 'none', and on a managed installation the only writers are whoever administers it and the boot that creates them — so a policy meant to be on from the start had nowhere to be written. A control plane calling in afterwards leaves a window between the first account existing and the policy covering it, and the first account is the one with every permission. The entrypoint already seeds an account from the environment. This seeds the policy one line above it, so the administrator is born under the rule rather than ahead of it. There is a test for exactly that ordering, because the ordering is the whole point. Seeded, never overridden. A value that won on every boot would take the setting away from the person it belongs to — somebody who tightened it would find it loosened again by a restart. So it writes only when nothing has ever been stored, the same shape as `projectsend:admin --if-none`. Two things that would have been easy to get wrong, both pinned: 'none' is the enum's own default, so Settings::get() cannot tell "stored as none" from "never stored". Asking the accessor would have overwritten an administrator who deliberately chose it. The command asks the table. And it reads config rather than env() directly. `config:cache` stops .env being read at all, which is how TRUSTED_PROXIES came to have no effect on any web request while looking correct in the file. Deliberately not a general PROJECTSEND_SETTING_<KEY> mechanism. Every setting reachable from outside is one whose value depends on where you look, and the blast radius of getting that wrong is the settings table. One named key per setting that needs it. The three new variables are documented in config/projectsend.php and not in .env.example or the Docker Hub overview. Those two are written for somebody running one installation for themselves, and a seat cap is not a thing they have — FILES_WEB_SERVER_READABLE is in .env.example because a self-hoster on cPanel genuinely meets that problem. |
||
|
ignacionelson
|
463e86f82b |
Refuse an account past the seat count an operator sold
Opening user management on cloud (
|
||
|
ignacionelson
|
623ad686da |
Open user management on the cloud edition
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. |
||
|
ignacionelson
|
eda8091aef |
Serialise OAuth token refreshes, and don't offer Connect for an unsaved provider
Both fixes are follow-ups to the mail providers @denkfabrik-li added in #1679. A refresh token is good for exactly one use — Microsoft and Google both retire it as they issue the next one. Two queue workers finding the same expired access token would therefore both spend it, and the loser gets invalid_grant back. That is the same answer a revoked grant gives, so a healthy connection would be marked broken, painted red on the settings page and mailed to every admin. Refreshes now hold a per-connection lock and whoever waits re-reads the row, which normally means finding a token the winner already stored and not refreshing at all. The Connect button read the provider dropdown, but the flow it starts uses the saved provider. On an installation with both vendors registered, switching without saving would open the wrong consent screen. The dropdown now counts as an unsaved change like any other field, which also gives it the right "save first" hint for free. |
||
|
ignacionelson
|
1b3abd28b7 |
Merge branch 'main' into feature/oauth-mail-providers
Both sides added a .gitignore rule in the same place: this branch's exception for docs/email-oauth.md, and main's block for the local dev TLS material. Keep both. |
||
|
ignacionelson
|
daec0a877e |
Offer Google Cloud Storage as a storage backend
External storage meant S3 and nothing else, which is an odd hole for a product whose users are as likely to be standing on Google Cloud as on AWS — and paying to move bytes between two clouds to use this. The Storage screen now asks which provider first, and the answer decides which fields it shows, which it validates, and which driver the files_external disk resolves to. One disk, not two. files.disk is a stored column, so a third disk name would fragment the data model and make every $file->disk consumer know three names instead of two; the driver is swapped instead. A service account key gets its own encrypted column rather than sharing `secret`, because the two are validated, labelled and displayed differently and one column meaning two things is how that goes wrong later. Three things do not work by simply adding the adapter, and all three fail quietly: Laravel's temporaryUrl() looks for getTemporaryUrl() on the adapter, while League's GCS adapter names it temporaryUrl(), so without the registered callback every download and preview is a 500. The two SDKs spell the signing options differently, and an unrecognised one is dropped in silence — the symptom is a download named after the storage key, not an exception. GoogleCloudStorageDriver translates, so callers keep speaking one vocabulary, and the test asserts on the URL's contents rather than on "a redirect happened", which is what would let it regress. That callback is also re-bound to the FilesystemAdapter before it runs, so the translation is captured before registering rather than called as $this-> `provider` is validated with 'sometimes', not 'required': absent means S3, which is what every payload written before this choice meant, and stops a browser holding a stale bundle from failing to save on a field it cannot see. Verified in a browser as well as in tests — which is how the null provider on an unmigrated row was found, since the suite migrates and never sees that state. |
||
|
denkfabrik-li
|
8b59bb2a5a |
Move fakeIdToken() to the shared test helpers
It is used by both OAuth mail test files, which --parallel runs in separate processes — exactly the situation tests/Helpers.php exists for, as its own header explains. CI caught what a whole-suite serial run hides. |
||
|
denkfabrik-li
|
4eb8cf915a |
Add Google / Gmail as the second OAuth mail provider
Same delegated shape as the Microsoft 365 provider, through the same broker interface: the admin registers an OAuth client in Google Cloud Console, connects the Google account the installation should send as, and outgoing email goes through the Gmail API's messages.send as that account. The shared authorization-code machinery (exchange, refresh, token storage, id_token account detection, RFC 6749 failure telling a dead grant from a transient one) moves into an abstract OAuthCodeFlowBroker; the two vendor brokers keep only their endpoints, scopes and consent URL parameters. Google's quirks live where they belong: offline access with a forced consent screen (the only way Google issues a refresh token), and a refresh response that never re-sends one — the store keeps what it has. The settings screen needed no changes: the dropdown, the credential form and the connect flow all derive from the provider enum. |
||
|
denkfabrik-li
|
933eaa2ba4 |
Send mail through Microsoft Graph as an admin-connected mailbox
Adds "Microsoft 365 (OAuth)" to the Email settings provider dropdown. Selecting it swaps the SMTP form for an app registration (client id, secret, optional tenant) and a "Connect mailbox" flow: the admin signs into the mailbox the installation should send as, and outgoing email goes through Graph sendMail as that mailbox — no password, no app password, no SMTP AUTH, which Microsoft is winding down. Delegated flow on purpose: it needs no admin consent and works for work/school and personal accounts alike. Its one weakness — a grant can die silently behind a password reset or a Conditional Access change — is answered by a daily scheduled refresh that keeps the token alive and, on a dead grant, warns the settings admins once in-app and on the settings page instead of letting mail stop quietly. Tokens and the client secret live encrypted in their own row and are read fresh at send time, never through the boot-config cache. The stored SMTP transport survives a provider switch untouched. |
||
|
elibrachas
|
1aaab1bf66 |
Read TRUSTED_PROXIES late enough for it to be seen
The value was read with env() inside the withMiddleware closure in bootstrap/app.php. That closure runs when the HTTP kernel is resolved, which is before the dotenv bootstrapper reads .env — so on every web request env() returned null for anything set in .env, and the proxy was never trusted. It worked when the value came from a real environment variable, which is why the Docker compose path was fine and the manual install described in INSTALL.md, where we tell people to put it in .env, was not. Artisan bootstraps in the other order, so a check from the command line reported the setting as working the whole time. Behind a TLS-terminating proxy the consequence is not subtle. Laravel falls back to the connecting address and the plain scheme, builds every link and redirect with http:// while the browser is on https://, and marks the session cookie non-secure. The browser then declines to send that cookie to what it reads as a different, less secure origin, the session arrives empty, and the first write fails with a 419 that reads as "your session expired" — most often on the create-your-admin form, which is the first thing a new install submits. Afterwards each redirect leaves and re-enters over the wrong scheme, which is the random bounce back to the login screen people report as flakiness. Moved to config/trustedproxy.php, the key the framework's TrustProxies middleware already falls back to on its own. Config files load after dotenv, so the value is there whether it comes from .env or from the environment. This was also the only env() read outside config/, which means config:cache is no longer dangerous on this application. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |