IRT-1786, IRT-1791: check stored passwords against policy at login

[dsvanstedt]

Closes the Core half of IRT-1786, part A of IRT-1791, and IRT-1798. The Web Admin half of IRT-1786 already merged as d70585a (BridgeLink-Web-UI nextgenhealthcare#720); everything here is Core.

The gap

Password requirements were enforced only when a password was set. doesPasswordMeetRequirements had exactly one production caller — the set-password path — and authorizeUser never called it. So when the defaults were hardened for 26.6.0 (minlength=8, one upper/lower/digit/special), every existing account kept its old password indefinitely, including passwords the server would now refuse to accept. Upgrading installs got the new policy in name only.

The existing expiration machinery does not cover this: it asks how old a password is, never whether it meets the rules, and it is dormant by default (password.expiration = 0).

What changed

Policy is evaluated at login. Login is the one point where the server holds the plaintext. authorizeUser now checks it against the current requirements and returns SUCCESS_GRACE_PERIOD when it fails, so the user is prompted rather than locked out and API clients keep working. Governed by password.enforceatlogin, default true.

The check passes a null user id deliberately. The checker runs its reuse-history rules when given a real id and a reuse policy is enabled, and a user's current password is by definition in their own credential history — a real id would report every compliant password as "reused" on every login, a violation the user cannot clear. Null also keeps the call free of database access, which matters because authorizeUser runs on every Basic auth REST request (MirthServlet:175), not just interactive logins.

admin is rejected by name. The hardened defaults already reject it for length and character class, so this is a message-quality change — the operator is told the actual problem instead of reading four composition failures. It lives in the checker, not the servlet, because the checker is on every path.

A server event is raised when a login is allowed but the password fails policy. Stored passwords are hashed, so there is no way to audit which accounts are affected — this event is the only way to find them. It is dispatched from UserServlet.login, not authorizeUser, so a non-compliant polling account does not write an event every 60 seconds.

Grace-period sessions can be confined. They were fully privileged, so the change-password prompt was client-side UX only and could be ignored over REST — step 1 of the disclosure PoC. The session is now marked at login (and per-request on the Basic auth path) and refused for anything outside the operations needed to complete the change. Extension endpoints authorize through their own method, so the check is in both; the bypass user is unaffected; the 403 carries an explanation instead of being bare.

Why the restriction ships off

password.restrictgracesessions defaults to false. It is a breaking change for automation — any account whose stored password fails the requirements starts getting 403 until rotated — and because passwords are hashed there is no way to identify those accounts before they log in. Note Fleet authenticates via POST /users/_login and caches a JSESSIONID, so it is on the session path and is not spared by any variant of the restriction.

Shipping it dormant gives operators one release in which the event above reveals affected accounts before the boundary that breaks them turns on. IRT-1791 AC4 therefore holds at the shipped defaults; enabling the flag deliberately overrides it. A follow-up should flip the default once operators have had a release to clean up.

Collateral fixes

• CLI accepted only plain SUCCESS, so a weak-password user was told "Could not login to server" — a working credential misreported as a bad one. This breaks at the shipped defaults, so it was not optional.
• Swing Administrator handled the grace period after building the main window (LoginPanel:567 vs :610). Under restriction every request made while loading would be refused before the user saw the prompt. The dialog now runs first, talking to the server directly since there is no Frame yet, and the login is abandoned if dismissed — which also makes it binding, where before Cancel left the user logged in having changed nothing.
• A fresh install hits both paths at once (the default password fails policy and it is the first login), so FirstLoginDialog no longer demands a password when one was already set during the same login.
• The MFA plugin returns its own LoginStatus rather than modifying the one it is given, which would silently drop the grace period. Restored when the plugin has finished and simply passed the login through.

Verification

Full ant -f mirth-build.xml green: 220 test classes, 0 failures, 0 errors. New tests mutation-checked — disabling the admin check fails 3, disabling the restriction fails 2.

• PasswordRequirementsCheckerTest (new, 7) — admin rejected in any case, exact-match not substring, defaults, property overrides
• MirthServletGracePeriodTest (new, 14) — restriction on/off, allowlisted operations, extension path, self-identification, and the IRT-1798 scoping cases below
• DefaultUserControllerTest (+3) — the reuse false-positive guard (AC5), non-compliant reporting, message format

Live against a dev server, both at shipped defaults and with the restriction enabled:

Check	Result
admin/admin login	SUCCESS_GRACE_PERIOD, "admin" is not allowed as a password listed first
Privileged call, defaults	200 session and Basic auth (AC4 holds)
Server event	Once per login; not raised per Basic-auth request
Privileged call, restriction on	403 with explanation, session and Basic auth
getCurrentUser / password change under restriction	200 / 204 — the user can still fix it
Same session after change	200, restriction lifted
Login with new password	plain SUCCESS; old password 401
Reuse policy on, compliant password	plain SUCCESS — no false "cannot reuse" (AC5)
Setting password back to admin	rejected, named first

Not in this PR

• password == username (IRT-1791 part B, AC8/AC9). Dropped by decision — it needs a username parameter threaded through the checker, UserController, UserServlet and UserServletInterface (a REST contract change) for a rule that is near-redundant under the shipped defaults: for password == username to be reachable the username itself must be 8+ chars with upper, lower, digit and special.
• Legacy Stripes webadmin treats SUCCESS_GRACE_PERIOD as plain success with no prompt, so its users would see failures with no explanation when the restriction is enabled. Documented in the property comment, not fixed.

Release note

password.enforceatlogin prompts non-compliant users at login starting this release. password.restrictgracesessions is available and off; enabling it will 403 non-compliant automation accounts, so rotate service-account passwords first.

---

Review round 1 — two blockers found and fixed

An adversarial review of the first three commits turned up three defects, fixed in 22ce0cda.

Blocker: the Swing client could not log in at all on a default install. MirthDialog.setVisible and dispose dereference PlatformUI.MIRTH_FRAME, which is only assigned inside new Mirth(client). Moving the change-password dialog ahead of that call — the entire point of the client change — meant it NPE'd on setVisible(true), and LoginPanel swallowed it as a generic connection error. Since the default admin password now fails policy, every fresh install would have hit this. Both calls are now null-guarded. Regression test: MirthDialogNoFrameTest (skips headless via Assume), which reproduces the exact NPE without the fix.

Blocker: a restricted session could not log out. logout and inactivityLogout are @DontCheckAuthorized, but call isUserAuthorized() purely to audit and ignore the answer — and that method now throws for a restricted session, aborting before session.invalidate(). A user who declined the change was left with a session they could neither use nor end, and the Swing abort path leaked it because it discards logout failures. Both operations are now in the allowlist; they only destroy the session. Regression test added.

Minor: the first-login register checkbox reset the password field to required, undoing the flag that prevents double-prompting.

Known limitations, deliberately accepted

• Two-leg MFA is not covered. The second leg (UserServlet:82) authenticates through the plugin and never reaches authorizeUser, so for those deployments enforceatlogin is a no-op — no grace status and no login event, which also means the discovery mechanism shows nothing for MFA users. Needs its own ticket.
• The restriction flag is per-session. A user with two concurrent sessions who changes their password in one leaves the other restricted until it logs out and back in. Fails closed.
• Extension operations are matched on the raw name. A plugin operation literally named checkUserPassword, getPasswordRequirements, getCurrentUser, logout, or inactivityLogout would pass the grace gate, though it still faces its own extension permission check. (updateUserPassword is no longer in that set — see round 2.)

Full build re-run after the fixes: 221 test classes, 0 failures.

---

Review round 2 — IRT-1798, privilege escalation through the password write

The confinement had a hole that made it worse than useless where it was enabled: a grace-restricted login could set any user's password and then log in as that user with a session under no restriction at all. Fixed in da979d1f.

The allowlist matched on operation name only. updateUserPassword was on it so a confined user could repair their own password, but nothing scoped it to their own account. Reproduced over Basic auth with no session at all:

GET  /api/channels                          403   (restricted, as expected)
PUT  /api/users/2/password  "Pwned!Pass1"   204   <-- should be 403
POST /api/users/_login  victim/Pwned!Pass1  200   SUCCESS -> fully privileged
GET  /api/channels  (as admin)              403   attacker still restricted

createUser was correctly refused throughout, so the allowlist mechanism itself was sound — the hole was specifically the unscoped write.

The fix. The allowlist is split in two. Operations that name no user stay unscoped: checkUserPassword (it takes only a plaintext string), getPasswordRequirements, getCurrentUser, logout, inactivityLogout. updateUserPassword moves to a self-only set and is permitted only when the request targets the current user. The target id is recorded in checkUserAuthorized(Integer, boolean), which already receives it from the @CheckAuthorizedUserId parameter and previously used it only for the "or it's yourself" fallback. A self-only operation that reaches the check without a resolved target is refused rather than allowed on the strength of its name — which also closes the extension-operation case noted above for this one operation.

Nothing changes at the shipped defaults: with password.restrictgracesessions = false a grace login keeps full privileges, including changing other users' passwords, on both the session and Basic auth paths.

Regression tests in MirthServletGracePeriodTest (7 -> 14): another user's id refused, own id still allowed, no-target refused, and the same cross-user write still permitted with the restriction off — each on both the session and the Basic auth path. Plus a check that updateUserPassword still carries @CheckAuthorizedUserId and that its paramName resolves to a real @Param: without the annotation the handler falls back to the no-argument overload, the target is unknown, and every grace-restricted user is refused their own password change, locking them out of the one operation that clears the restriction.

Fixture user ids sit deliberately outside the Integer cache, so the Integer-to-int comparison in isCurrentUser cannot silently degrade to reference identity and still pass.

Mutation-checked: reinstating updateUserPassword on the unscoped allowlist fails 3 of them, removing the annotation fails 1.

Re-verified live on a fresh install across both configurations — 31 checks, 0 failures, where the pre-fix branch scored 30/1 with the bypass as the single failure. Specifically confirmed: the cross-user write is 403 on both the session and Basic auth paths and the victim's password is genuinely unchanged; a restricted session can still set its own password (204) and the same session is unrestricted immediately afterward with no re-login; logout from a restricted session still returns 204 and the session is genuinely dead; the entire shipped-defaults phase is unaffected.

Correction to round 1

The round-1 note claiming that self-scoped operations annotated auditCurrentUser = false bypass the grace check, and that Web Admin depends on that to read the firstlogin preference, was wrong on both halves and has been removed. Observed: GET and PUT /users/1/preferences/firstlogin for self both return 403 under restriction, and Web Admin is blocked a step earlier at GET /users anyway.

The mechanism: UserServlet is constructed with initLogin = false, so currentUserId is still 0 when the auditCurrentUser = false branch evaluates isCurrentUser(userId) first. The self-check never matches and control always falls through to isUserAuthorized() and the grace check. Every @CheckAuthorizedUserId method in the codebase lives in UserServlet, so the exemption does not exist anywhere. It has been left non-existent rather than made real — nothing needs it, and relying on that initLogin accident would be fragile.

[dsvanstedt]

CI run 31752617577 failed on com.mirth.connect.donkey.test.ConnectorTests.testPollConnector:

junit.framework.AssertionFailedError: expected:<7> but was:<6>

Not from this PR — it is the known poll-timing flake that PR #193 (IRT-1788) fixes:

• This PR touches zero donkey files (git diff bridgelink_development...HEAD --name-only | grep donkey returns nothing).
• The assertion is an exact poll-count check. IRT-1788 commit 501fec4e, "wait for polls in testPollConnector instead of timing them", targets this exact test — and that fix is not on this branch, since fix(donkey): fix two flaky donkey tests (IRT-1788) #193 has not merged into bridgelink_development yet.
• The earlier run on this same branch (31748819094) passed, and two local full builds passed (221 test classes, 0 failures) with ConnectorTests green.

Re-running the failed job. Merging #193 first would remove the flake permanently rather than relying on a re-run.

[dsvanstedt]

Resolved. #193 was merged, and bridgelink_development is now merged into this branch (c51281fd2), so the poll-timing fix is present here rather than being worked around by a re-run.

CI green on the updated branch: run 31754951873 — build pass, 20m42s.

The merge was clean and brought across only .gitignore, ConnectorTests.java and TestUtils.java, none of which this PR touches. Supersedes my earlier "re-running the failed job" comment.

[dsvanstedt]

Closing this PR. The change has been re-landed on the 26.9.x development line with a net diff identical to this PR's — same files, same insertion and deletion counts — and is merged there, with CI green on JDK 17 and 21.

It will reach this repository with the 26.9.0 release. Nothing is lost.