🏗️ PUT-1745: enforce the forced password change on workspace seats - #3737
Conversation
Local validationEngine: mysql 8 · The migration appliesProvisioning now writes both the flag and the expiry: The gate — the phase 2 finding, closedThat finding was: a seat signs in on its temporary password, self-serves email confirmation, and reaches everything with ⚠ Testing this needs care, because the email gate masks the password gate. Straight after provisioning, So the seat's email was confirmed first, leaving the new clause as the only thing standing: Compare the same four routes in phase 2: Sign-in itself still succeeds, which is the design — the member is in, and can do nothing until they choose a password. The endpoint that clears it is reachable through the gateThe trap this ticket names: a gate refusing everything would also refuse the way out. Both the flag and the expiry clear. The seat then has the product: The 409 that was previously unreachableBecause the flag was never cleared, The reset route, and its audit rowThe audit trail, read from the table: No credential in any row, and Full suite 7766 passed | 26 skipped, typecheck clean. Not exercised here: the 24-hour expiry lapsing in real time. |
3dafab4 to
c342b17
Compare
c342b17 to
a1f32a6
Compare
|
Rebased onto One collision worth naming: those commits took migration Verified per commit rather than at the tip — each of the 9 typechecks on its own, so no intermediate commit is broken. Full suite 7883 passed | 26 skipped. |
a1f32a6 to
78ae619
Compare
78ae619 to
973d24e
Compare
Terminology: "workspace" is now "team", everywhereThe feature said Applied to every commit rather than added on top, so each PR shows its own code already named correctly and no intermediate commit references a name a later one changes. All 13 commits still typecheck individually. The contract-bearing renamesThese were free to change now and expensive after release, which is why they went in this pass:
Plus Scoped, and checked"workspace" means other things in this repo. These were deliberately left alone, and verified untouched:
Audited three ways: every changed line mentions team or workspace (no collateral edits), no stutters or mangled words ( One thing not renamedThe git branch names still say |
973d24e to
1ed9fb2
Compare
1ed9fb2 to
a06cb6d
Compare
a06cb6d to
c22ea9a
Compare
`user.requires_password_change` shipped with the team columns but nothing enforced it and nothing ever cleared it, so a provisioned seat kept its administrator-issued password indefinitely and `reissueCredential`'s "already activated" 409 was unreachable. Adds the fourth clause to `assertVerifiedAccount`, the only place a verification gate may live -- WebDAV builds its own actor and calls that function directly, so a second implementation would bypass it the way the phone and card gates once were bypassed. A gate that refuses everything also refuses the endpoint that clears it, so `/user-protected/change-password` opts out with `allowUnconfirmed`. That widens the route: an account pending email, phone or card verification can now change its password, which it could not before. The caller is authenticated and proves the current password, so this is benign, but it is a behaviour change to a shared route. Also here, because the gate is worthless without them: - change-password and the recovery-token path clear the flag, and record an `activate` entry when the account is a seat. - Reset takes a live account back with a fresh credential, capped at 20 per day and audited as `reset_member_password` with no credential in the row. Re-issue is audited the same way; it stays closed once a seat has chosen its own password. - An issued credential expires after 24h (new `temp_password_expires_at` column, three dialects) and login refuses it after that, so an unused reset dies instead of becoming a standing credential. - 2FA is untouched by a reset, so a reset alone is not takeover.
c22ea9a to
b7421ac
Compare
1 participant
Closes PUT-1745. Stacked on #3735 (phase 4).
The gate this ticket owns
user.requires_password_changeshipped in PUT-1699 and has been written but never enforced. Provisioning sets it;reissueCredentialreads it as the "has this seat activated" test.gates.tschecksrequires_email_confirmation,requires_phone_verificationandrequires_card_verification— there was no fourth check.Verified live during phase 2 validation, and it is worse than "the flag does nothing":
The flag was also only ever written as
1, never cleared, anywhere insrc/backend. So two things followed: the temporary password the administrator generated stayed valid forever, andreissueCredential's "already activated" 409 was unreachable — an administrator could mint a working credential for any seat, indefinitely.What this adds
The fourth clause in
assertVerifiedAccount, and nowhere else. Its docblock records that WebDAV once bypassed the phone and card gates by dispatching off a singlerouter.use; a second implementation repeats a documented mistake.The clearing path, because the gate alone would have locked every seat out permanently.
handleChangePasswordand thepass_recovery_tokenpath both clear the flag and the expiry, and record anactivateaudit row.allowUnconfirmed: trueon/user-protected/change-password— the obvious trap is that a gate refusing everything also refuses the endpoint that clears it.⚠ That flag is coarser than the ticket's wording: it disables the whole
requireVerifiedAccountgate on that route, so a pending phone or card verification is also let through, not just email. Benign for the same reason — the caller is authenticated and proving their current password — but broader than described, and it changes a shared route.A 24-hour expiry on the temporary password (
user.temp_password_expires_at, three dialects:0080/mysql_mig_35/postgres_mig_24), so an unused reset dies rather than becoming a standing credential. Login refuses an expired one withtemporary_password_expired.POST /teams/:uid/members/:username/password-reset— 20/day, audited asreset_member_passwordwith no credential in the row, drops the member's sessions, leaves 2FA alone.reissueCredentialnow writes an audit row too, and its 409 is reachable.New limit documented in
rate-limits-and-quotas.md, same change.Why this is on its own
It is the only phase 5 change that touches shared platform code, and it does so twice:
gates.tsruns on every authenticated request for every account on the platform, and there is a three-dialect migration. Both want isolated review and an isolated revert. The member-facing half is the PR stacked on this one.Verification
Typecheck clean. Full backend suite 7765 passed | 26 skipped.
Falsified — each piece broken, the failing tests confirmed, then restored:
assertVerifiedAccounthandleChangePasswordallowUnconfirmedon change-password#issueTemporaryPasswordrecordPasswordSelfChangeisTemporaryPasswordExpiredOne test deliberately passes when its neighbour breaks: dropping the expiry write does not fail the HTTP login-expiry test, because that test writes the expiry itself. That split is intended — one test proves the write, the other proves enforcement — and each fails when its own code is broken.
Decisions worth a reviewer's eye
#suspend. Easy to remove.reissueandresetshare thereset_member_passwordaudit action (reissue carriesreason: 'reissue'). From the member's side both mean the administrator holds a working credential.requires_password_changeissmallinton postgres while the other threerequires_*flags areboolean— an inconsistency inherited from PUT-1699. It is therefore not inUSER_BOOLEAN_COLUMNS; writes stay0/1. Worth a follow-up to align the column type.Not done here
The forced-change screen naming who reset it and when is GUI, so phase 6. The 403 body carries no actor or timestamp today; if the screen needs them, that is a follow-up on this route.