Feature | Add Login UI MFA Flow#142
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
33d34df to
fc5deae
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
fc5deae to
3481ac9
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@resources/js/base_actions.js`:
- Around line 102-106: The issue is that params are unconditionally being added
to the URL query string via the url.query(params) call, which exposes sensitive
POST data like passwords, OTPs, and captchas in URLs and logs. Modify the code
to only add params to the query string for non-POST requests. Check the HTTP
method of the request before calling url.query(params) and only apply it when
the method is not POST, ensuring sensitive data in POST requests stays in the
request body and is never exposed in the URL.
In `@resources/js/login/components/email_error_actions.js`:
- Around line 22-42: The two Button components in this block are not respecting
the disableInput state, allowing users to trigger actions even when the UI
should be locked. Add the disabled prop set to disableInput on both buttons: the
button with onClick={emitOtpAction} and the button with
href={createAccountAction}. This will prevent users from clicking these recovery
action buttons when the input is disabled.
In `@resources/js/login/components/email_input_form.js`:
- Around line 51-55: Remove the use of dangerouslySetInnerHTML on the error
message element in the email error rendering section. Instead of using
dangerouslySetInnerHTML with the emailError property, render the emailError text
directly as a child of the paragraph element to prevent potential XSS
vulnerabilities. This applies to the conditional block where emailError is not
empty and the error paragraph is being displayed.
In `@resources/js/login/components/existing_account_actions.js`:
- Around line 34-37: The `Link` component with `disabled={disableInput}` prop
does not actually prevent navigation in MUI v4 when using the default anchor
element. To fix this, add an onClick handler to the Link component that prevents
the default navigation behavior when disableInput is true. The onClick handler
should call preventDefault() on the event whenever disableInput is true,
effectively blocking the navigation while the input is locked.
In `@resources/js/login/components/help_links.js`:
- Around line 18-21: The URL construction for forgotPasswordActionHref does not
account for existing query parameters in the forgotPasswordAction base URL. When
appending the email parameter, the code always uses `?email=...`, which will
create invalid URLs if forgotPasswordAction already contains a query string.
Modify the conditional block where forgotPasswordActionHref is assigned (when
userName exists) to check if forgotPasswordAction already contains a `?`
character. If it does, use `&` to append the email parameter; otherwise use `?`.
This ensures proper URL formatting regardless of whether the base URL already
has query parameters.
In `@resources/js/login/components/otp_input_form.js`:
- Around line 55-58: The error label paragraph element in the OTP input form is
using dangerouslySetInnerHTML to render the otpError variable, which creates an
XSS security vulnerability. Remove the dangerouslySetInnerHTML prop and instead
render the otpError directly as text content of the paragraph element so that
any HTML-like characters in the error message are displayed as plain text rather
than being interpreted as executable HTML.
- Line 1: Remove the useMemo hook to prevent stale captcha visibility state.
First, remove useMemo from the import statement at the top of the
otp_input_form.js file. Then, locate where useMemo is being used in the
component (likely wrapping captcha visibility logic) and unwrap it by removing
the useMemo function call while keeping the code it was wrapping. This ensures
the captcha visibility logic runs on every render and properly reflects the
current state of dependencies like loginAttempts, rather than relying on a stale
memoized result based only on the function reference.
- Line 49: The `hasErrored` prop being passed to the OtpInput component is not
supported in react-otp-input v3.1.1 and was removed in v3.0.0+. Remove the
`hasErrored={!otpError}` prop from the OtpInput component and instead use the
`renderInput` prop to handle error state styling. Pass the otpError state to the
custom input component through the `renderInput` prop so that the error styling
can be conditionally applied within the input component based on the parent
component's otpError state.
In `@resources/js/login/components/password_input_form.js`:
- Around line 85-88: The paragraph element with className styles.error_label is
rendering passwordError as HTML using dangerouslySetInnerHTML, which creates an
XSS vulnerability. Remove the dangerouslySetInnerHTML prop and instead render
passwordError directly as text content within the paragraph element to safely
display the error message without executing any embedded HTML or scripts.
In `@resources/js/login/components/recovery_code_form.js`:
- Around line 22-25: The handleBack function defined in the recovery_code_form
component is not connected to any UI element, so users cannot interact with it
to go back to the OTP screen. Add a button element in the form's JSX render that
calls the handleBack function on click. This button should be placed in a
logical location in the recovery form UI, likely near the form submission or as
a navigation element. Note that the same issue applies to the code around lines
65-72, so ensure both instances have the corresponding UI elements added to
enable the back navigation functionality.
- Around line 53-55: The recoveryError is being rendered as raw HTML using
dangerouslySetInnerHTML, which creates an XSS vulnerability if the error content
contains user-controlled or backend text. Replace the dangerouslySetInnerHTML
prop with direct text rendering by removing the
dangerouslySetInnerHTML={{__html: recoveryError}} attribute and instead render
recoveryError directly as plain text inside the paragraph element with the
error_label className. This will automatically escape any HTML special
characters and prevent potential XSS attacks.
In `@resources/js/login/components/two_factor_form.js`:
- Around line 91-93: The otpError variable is being rendered with
dangerouslySetInnerHTML which creates an XSS vulnerability since the value comes
from server responses. Remove the dangerouslySetInnerHTML attribute from the
error message paragraph element in the two_factor_form component and instead
render otpError as plain text content. If HTML formatting is required for the
error message, sanitize otpError using a proper HTML sanitization library before
using dangerouslySetInnerHTML.
In `@resources/js/login/login.js`:
- Around line 435-463: Add an early guard clause at the beginning of the
onVerify2FA method to prevent duplicate verification requests. Before processing
any validation logic or making the verify2FA API call, check if
this.state.disableInput is already true, and if so, return early from the
method. This will prevent rapid successive clicks or Enter key events from
triggering multiple concurrent API calls. Apply the same guard pattern to the
other verify handler mentioned in the comment (around lines 524-547).
- Around line 183-202: In the handleAuthenticateValidation method, the password
validation block that checks if user_password is empty is being executed
unconditionally for all auth flows. This causes the OTP flow validation to fail
even when a valid OTP code is provided, because the password check comes after
the OTP check and always requires a password. Wrap the password validation block
in a conditional statement that only executes when the auth flow is NOT
FLOW.OTP, so that OTP flow validation can complete successfully without
requiring a password field.
- Around line 220-241: The `default` case in the switch statement declares
`const redirect` without block scope, creating scope and TDZ issues. Wrap the
entire contents of the `default` case (from the `const redirect` declaration
through the closing of the if-else block that checks the redirect condition)
with curly braces to create proper block scope and isolate the variable
declaration from other switch cases.
In `@resources/views/auth/login.blade.php`:
- Around line 102-107: The line setting window.RESET_2FA_ENDPOINT is referencing
config.reset2faAction which is not defined in the template's configuration
object, causing the window variable to become undefined. Verify the template's
PHP configuration section where the config object is initialized and either add
the missing reset2faAction property with the appropriate endpoint URL, or
correct the reference to use the correct existing config key name that
corresponds to the reset 2FA endpoint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd2029b0-3aab-49c1-b3e1-2abdc066795e
📒 Files selected for processing (17)
package.jsonresources/js/base_actions.jsresources/js/login/actions.jsresources/js/login/components/email_error_actions.jsresources/js/login/components/email_input_form.jsresources/js/login/components/existing_account_actions.jsresources/js/login/components/help_links.jsresources/js/login/components/otp_help_links.jsresources/js/login/components/otp_input_form.jsresources/js/login/components/password_input_form.jsresources/js/login/components/recovery_code_form.jsresources/js/login/components/third_party_identity_providers.jsresources/js/login/components/two_factor_form.jsresources/js/login/constants.jsresources/js/login/login.jsresources/js/login/login.module.scssresources/views/auth/login.blade.php
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
3 similar comments
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
c54e492 to
e0b8519
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
|
|
||
|
|
||
| if (this.state.authFlow === FLOW.PASSWORD) { | ||
| this.handleAuthenticatePasswordFlow(form); |
There was a problem hiding this comment.
UX regression: password flow loses native password-manager integration.
Converting the password submission to AJAX (handleAuthenticatePasswordFlow via authenticateWithPassword/postRawRequestFull) means the browser never fires a native submit event on the <form>, which is what Chrome/1Password/etc. key their "save/update this password" prompt off of. Users who type or paste a password here won't be offered a save/update prompt, unlike every other flow in this same file (OTP, recovery, cancel), which still submit natively.
This wasn't flagged in the ticket (CU-86ba2zp3q) or any existing review comment - it's a usability downgrade, not a functional bug, but worth a deliberate call: was the AJAX conversion here intentional (e.g. to support the live in-SPA MFA transition), or just a side effect of refactoring the form components? If intentional, consider whether the live-transition benefit is worth losing autofill/save-password integration on the one flow where it matters most.
Suggested fix: let the password flow fall through to the native submit like the sibling flows already do (line 271's onAuthenticate), and drive the follow-up MFA screen via a redirect + session-flashed state instead - same mechanism this form already uses for every other flow and every other error.
postLogin()'s mfa_required response, and the display-strategy contract it depends on, bypassed $this->login_strategy entirely: every MFA response was Response::json(...) built by hand in the controller, ignoring OAuth2 display-strategy polymorphism (native vs page/popup/touch). Native OAuth2 clients (display=native) got JSON+200 with an ad hoc shape instead of the 412 + required_params/url/method contract every other login error already returns for that display mode. - ILoginStrategy::challengeRequired() / IDisplayResponseStrategy:: getChallengeRequiredResponse(): new methods, distinct from errorLogin() since a pending MFA challenge isn't a failed attempt. - DefaultLoginStrategy: identical bytes to before (200 + JSON) - zero behavior change for the plain IdP flow. - OAuth2LoginStrategy: rebuilds the auth_request from the memento (same pattern as errorLogin()) and delegates to DisplayResponseStrategyFactory. - DisplayResponseJsonStrategy (native): 412, matching its sibling getConsentResponse/getLoginResponse/getLoginErrorResponse methods. - DisplayResponseUserAgentStrategy (page/popup/touch): 200 JSON, same live in-SPA transition as the plain flow, since both render the same login.js. - ILoginStrategy::MFA_REQUIRED constant replaces the 'mfa_required' literal duplicated across three classes. Also closes a refresh-resilience gap PR #142's frontend already expected but the backend never delivered (its login.js constructor comment reads "Two-factor state (populated from the flash redirect...)"): postLogin() now flashes flow/mfa_method/otp_length/otp_lifetime to session on mfa_required so a page refresh mid-challenge restores the 2FA screen instead of dropping back to the password form. Cleared on successful verification/recovery and on session expiry; refreshed on resend2FA() (including method switches). New: OAuth2NativeMFALoginFlowTest exercises the real /oauth2/auth -> memento -> postLogin() path for display=native and asserts 412+mfa_required. TwoFactorLoginFlowTest gains coverage for the session-flash/clear behavior.
None of the three login strategies' cancelLogin() cleared any 2FA session state - not the pre-existing 2fa_pending_user_id/2fa_pending_at/2fa_remember keys, nor the flow/mfa_method/otp_length/otp_lifetime keys added for refresh-resilience. PR #142's Cancel button resets the client's React state immediately and fires cancelLogin() as a best-effort background call, so the broken UX was masked within the same tab - but a subsequent full page load within the challenge's 300s TTL (back button, reopened tab, direct /login navigation) would restore the 2FA screen for a challenge the user explicitly abandoned, and the stale OTP could still complete it. UserController::cancelLogin() now resolves the pending strategy via the mfa_method session key (when present) and clears its pending state before delegating to the login strategy, plus clears the UI-restoration keys via the existing clearMFAUISessionState() helper. New test proves the strongest form of the property: an OTP valid before cancel returns mfa_session_expired afterward, not just that some session keys are gone.
Ticket CU-86ba2zc6p's TESTS list requires: "OTP redeem is persisted only on commit; a failure inside the verify transaction rolls back the redeem." No such test existed anywhere in this branch or PR #142/#146 - the two closest existing tests (testOTPCodeRejectsReuseAfterSuccessfulVerification, testRecoveryCodeRejectsReuseAfterTransactionCommit) only prove the COMMIT path (a successful verification's redeem persists and blocks reuse), not that a FAILED verification's partial redeem rolls back. Pure test-coverage gap, no production fix needed - AuthService::verifyMFAChallenge() already wraps strategy->verifyChallenge() in tx_service->transaction(), and DoctrineTransactionService already rolls back and re-throws on failure. Confirmed the test has teeth: temporarily bypassing the transaction wrapper broke the pessimistic-lock acquisition inside verifyChallenge() (which requires an open transaction), proving the test environment genuinely depends on transactional context, not just coincidentally passing. testOTPRedeemRollsBackOnMidTransactionFailure wraps the real EmailOTPMFAChallengeStrategy in a test double that lets the genuine redeem happen, then throws immediately after - inside the same transaction. Asserts the OTP is refetched from the DB (post-rollback) still unredeemed.
e0b8519 to
1f2e9b6
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
webpack.common.js has no .jsx resolve extension configured, so the bare '../../shared/HTMLRender' import used by every login form component failed to resolve, breaking the build for this whole tree.
The backend login strategies (DefaultLoginStrategy, DisplayResponseUserAgentStrategy)
answer wrong-password and mfa_required with a 302 redirect plus flashed/persisted
session state, meant to be consumed by a native top-level form submit - the same
mechanism already used by the OTP and MFA screens. Converting the password step to
AJAX (postRawRequestFull) broke that contract: the hidden XHR redirect-follow GET
consumed the one-shot flash before the SPA could show it, silently dropping the
wrong-password message, resetting login_attempts (disabling the server-side captcha
escalation), and losing native password-manager save/update prompts.
Reverts PasswordInputForm to the same native-submit adapter OTPInputForm already
uses, and removes the now-dead AJAX path: handleAuthenticatePasswordFlow/Ok/Error,
authenticateWithPassword, window.FORM_ACTION_ENDPOINT, and the MFA_CHALLENGE_REQUIRED
constant (confirmed unused end-to-end - the server never emits mfa_required as JSON
to browser clients either, only via session state under the 'flow' key).
Also removes disabled={disableInput} from the password TextField and the
'remember' FormControlLabel. Under native submission, React's synchronous
setState(disableInput: true) inside the same onSubmit handler commits the
disabled attribute to the DOM before the browser constructs the form's
data set - and the HTML spec excludes disabled controls from that set.
The result was a silently dropped password field ('The password field is
required.', confirmed live against the backend). OTPInputForm was never
affected because it only disables its submit Button, never the field
carrying the actual submitted value - the fix here matches that pattern.
… crash HTMLRender uses JSX (<Component .../>) but never imported React. The project's babel-preset-react runs in classic mode (webpack.common.js), which compiles JSX to React.createElement(...) calls requiring React in scope per-module - importing it in a sibling file doesn't help, since webpack wraps each module in its own function scope. Every other component in this PR imports React; this one was missed. It went unnoticed while the file's own path (HTMLRender.jsx) failed to resolve at all; once that resolution bug was fixed, the runtime ReferenceError surfaced and crashed the whole login page on any render path that hits this component (confirmed live: 'ReferenceError: React is not defined', white-screen crash after password submit).
The 'cancel' route was GET-only (pre-dates this feature, never had a JS caller before). This PR's new cancelLogin() action POSTs to it, which 405'd silently (no .catch on the fire-and-forget call) - so UserController::cancelLogin()'s MFA cleanup (clearPendingState() + clearMFAUISessionState()) never ran. An OTP issued before Cancel stayed valid server-side despite the UI resetting to the password screen. Registers 'cancel' as POST + csrf, matching the sibling verify/recovery/resend routes (GET would work too - Laravel's CSRF middleware only checks unsafe verbs - but modeling a state-mutating action as GET risks a prefetcher/link-scanner silently cancelling a real pending session). Adds error handling to the previously fire-and-forget JS call, and updates TwoFactorLoginFlowTest's cancelLogin() test helper to POST with a CSRF token (it called the old GET route directly and would 405 otherwise). Verified live: POST /auth/login/cancel -> 200, and tests/TwoFactorLoginFlowTest.php passes in full (31 tests, 116 assertions).
…ects Root cause: verify2FA()/verify2FARecovery() returned login_strategy->postLogin()'s raw RedirectResponse directly to the XHR that called them. postLogin() always redirects to a same-origin URL (e.g. /oauth2/auth), but when the OAuth2 client already has consent on file, that endpoint's own consent-bypass branch (InteractiveGrantType::handle(), the has_former_consent + auto_approval case) issues the authorization code and redirects straight to the client's cross-origin redirect_uri - a hop the XHR was transparently trying to follow. No browser XHR/fetch can read a cross-origin redirect's response (confirmed against superagent's own source: lib/client.js, the browser build this project ships, has zero redirect-handling logic - only lib/node/index.js implements the .redirects(n) option, so that setting is a silent no-op in the browser). Worse, that same consent-bypass branch calls memento_service->forget() right after building the response, since the server considers the authorization complete - so the silently-failed XHR follow-through burns a real, delivered authorization code with no way for the frontend to recover it. handleMfaError()'s fallback (window.location.reload()) then finds the OAuth2 memento gone and lands the user on their own profile instead of resuming the flow - confirmed live end-to-end against a real oauth2_test_app client with a pre-existing consent record. Fix: verify2FA()/verify2FARecovery() now capture postLogin()'s redirect target and return it as JSON data (redirect_url) instead of a raw redirect. The frontend does a real window.location.href navigation to that same-origin URL - top-level navigations are never subject to CORS, so the browser completes any further hop (including the cross-origin one) natively, exactly as the original pre-MFA native-form-submit login flow always did. Cleanup: postRawRequestFull's finalUrl/status become unused by all three remaining callers (verify2FA, resend2FA, verifyRecoveryCode) once this lands, making it functionally identical to postRawRequest - removed and callers switched over. Also replaces the three remaining raw Response::json calls (HTTP_UNAUTHORIZED) in UserController with JsonResponses::unauthorized(), completing the same trait-based convention already used for the other status codes in this controller; the now-unused Symfony Response import (HttpResponse) is removed. Verified live against a real OAuth2 authorization_code flow (oauth2_test_app, consent already on file): the post-2FA redirect now correctly lands on the client's registered redirect_uri instead of the user's own profile page. tests/TwoFactorLoginFlowTest.php passes in full (31 tests, 120 assertions), updated to match the new 200+redirect_url contract on verify2FA/recovery success (six assertions across five tests); the three assertions covering postLogin()'s own native-submit paths (mfa_required, OTP-flow rejection, rate-limited retry) are untouched since postLogin() itself still redirects directly for those callers.
…creen Root cause was two-layered: 1. resetToPasswordFlow() reset authFlow to FLOW.PASSWORD but also cleared user_name/user_pic/user_fullname/user_verified in the same setState call. isPasswordFlow's render condition requires user_verified === true, so wiping it forced the render logic to showDefaultFlow (the email screen) regardless of authFlow being correct. 2. That alone wasn't sufficient: since the password step now submits as a native form POST (see the earlier native-submit fix), the mfa_required transition is a full page reload, not a client-side setState - the React app remounts from scratch and only recovers state the backend flashed to session. issueChallenge() (EmailOTPMFAChallengeStrategy/AbstractMFA ChallengeStrategy) only returns otp_length/otp_lifetime, so challengeRequired()'s session flash never carried username/user_fullname/ user_pic/user_verified in the first place - user_verified was already false the moment the 2FA screen first rendered, before Cancel was ever clicked. Fix #1 alone had nothing to preserve. Fix: postLogin()'s mfa_required branch now merges the same identity fields into the challengeRequired() payload that the AuthenticationException errorLogin() branch already flashes (same fields, same getters: username, user_fullname, user_pic, user_verified, user_is_active) - restoring the identity chip on the 2FA screen and giving resetToPasswordFlow() correct state to preserve. resetToPasswordFlow() no longer clears user_name/user_pic/ user_fullname/user_verified. Verified live: 2FA screen now shows the identity chip from first render: Cancel from the 2FA screen now returns directly to the password screen with the same user still identified, instead of resetting to the email-entry screen. tests/TwoFactorLoginFlowTest.php passes in full (31 tests, 120 assertions) - unaffected, since no test asserted the previously-missing identity fields.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
clearMFAUISessionState() only forgot flow/mfa_method/otp_length/otp_lifetime/ error_code. postLogin()'s challengeRequired() payload also persists username, user_fullname, user_pic, user_verified and user_is_active (needed to hydrate the React app on the initial post-redirect GET /login after an MFA challenge is issued), but those were never cleared - so they survived cancel, a successful verify, or a session-expiry indefinitely. On a shared browser session, the next visitor to hit /login would inherit the previous attempt's identity chip and skip straight to the password screen. Extend clearMFAUISessionState() to forget the same 5 keys, and extend testSuccessfulVerificationClearsUIState / testCancelClearsUIStateAndPendingChallenge to assert they're gone, matching the existing coverage for the other UI-state keys.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
handleDelete() (the login page's identity chip "x") reset client-side state but never called cancelLogin(), unlike the explicit "Cancel" link (resetToPasswordFlow()). During the 2fa/recovery screens this left the pending 2fa_pending_user_id session state and the issued OTP alive server-side until the session TTL, instead of being invalidated immediately like Cancel does. PR #142 review finding #1.
TwoFactorForm computed `expired` to show the countdown message but never used it to gate submission. A submit after expiry always fails server-side with mfa_verification_failed, which counts against the 2fa.rate:verify middleware's 3-attempt window - letting a user burn that budget on guaranteed-fail submits and get 429-locked out of the login flow entirely. Disable the VERIFY button and short-circuit handleSubmit (Enter-key defense in depth) once expired is true. PR #142 review finding #4.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
…resh The session stored otp_lifetime as a static duration, so any mid-challenge GET /login re-seeded the countdown with the FULL TTL - a user refreshing 4 minutes into a 5-minute challenge saw a fresh 5:00 countdown for a code the server would reject much sooner, letting them burn the 2fa.rate:verify attempt window (3 failures / 15 min lockout) on a code the UI claimed was still valid. issueChallenge() now also returns otp_issued_at taken from the OTP entity's created_at - the same source isAlive()/getRemainingLifetime() use server-side, so the countdown can never drift from the actual expiry check (a controller-side time() stamp would land after issuance and overstate the remaining window). The timestamp rides the same challengeRequired()/session mechanism as otp_length/otp_lifetime, is kept in sync on resend, cleared with the rest of the MFA UI state, and the blade seeds config.otpLifetime with max(0, lifetime - elapsed). RED verified before the fix: the new reproducer rendered the full TTL (600) instead of the remaining ~500. Full TwoFactorLoginFlowTest suite green: 32 tests, 140 assertions. PR #142 review finding LOW #1.
… code-entry UI The passwordless OTP expires exactly like the MFA one (same createOTPFromPayload infra) but its form gave no expiry feedback at all - the user only found out the code was dead after a full form POST the server rejected. emitOTP already returned otp_lifetime; the client just ignored it. Extract the duplicated code-entry cluster shared by TwoFactorForm and OTPInputForm into two reusable pieces: - use_otp_countdown.js: the 1s expiry ticker (reset via otpLifetime / codeVersion), lifted verbatim from TwoFactorForm. - otp_code_input.js: subtitle + OTP boxes + error + optional countdown (the ~25-line block both forms duplicated). OTPInputForm now shows the countdown and blocks submitting an expired code (same gating pattern as the MFA form). The countdown only renders when a fresh emitOTP happened in this page view (passwordlessLifetime state, null on restored views) - after a failed-submit reload the issuance time is unknown and showing a fresh full countdown would overstate the code's validity. Verified: babel parse on all five files + full yarn build (prod webpack) green. Net -14 lines including the new feature.
Both cancel paths (resetToPasswordFlow and handleDelete) reset the UI optimistically and fired cancelLogin() without handling failure beyond a console.error - on a network failure the server-side pending challenge silently survived until its 300s TTL while the UI told the user it was cancelled. Extract the duplicated call into a single cancelPendingLogin() helper that warns the user via the existing snackbar when the server-side invalidation fails, so they know the pending verification will only die by its own TTL. The optimistic reset is kept - the user asked to cancel, so returning control immediately stays correct. PR #142 review finding LOW #2.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
The 'resend email.' link in OTPHelpLinks had no throttle at all - each click fired a fresh emitOTP request immediately, unlike the MFA screen's 'resend code' link (TwoFactorForm), which already cools down for 30s. Mirror that exact pattern: OTPHelpLinks gains its own cooldown timer (useState/setInterval, same shape as TwoFactorForm's) and is also disabled while disableInput is true, matching the disableInput-gating convention already enforced elsewhere in this login flow. Promoted RESEND_COOLDOWN_SECONDS from a local constant in two_factor_form.js to constants.js so both forms share one value. Verified live in-browser (not just build): resend fires exactly one emitOTP request, the link disables and counts down (30s -> 1s), a click mid-cooldown fires zero additional requests, and the link re-enables with the countdown reset after expiry. Plan: docs/plans/2026-07-23-passwordless-otp-resend-cooldown.md, Task 1.
POST /auth/login/otp (emitOTP) had zero server-side throttle, unlike the MFA resend endpoint's 2fa.rate:resend. Reuses TwoFactorRateLimitMiddleware /TwoFactorRateLimitService via a new 'otp' action instead of duplicating a parallel middleware - the counting logic (cache-backed fixed window, 429 JSON shape) was already subject-agnostic; only the subject-resolution step needed a branch, since emitOTP() never writes any session state to key on (verified: zero Session::put calls in that method) unlike the session-keyed MFA actions. isRateLimited()/increment()/cacheKey() widen from int to string|int - source-compatible with both existing call sites (TwoFactorRateLimitMiddleware, UserController::postLogin()), which already pass an int. The otp subject is the submitted email, lowercased and trimmed - not just trimmed like postLogin()'s username normalization, which is safe only because it feeds a case-insensitive DB lookup before ever reaching a rate limiter. otp has no such lookup; the raw string IS the cache key, so trim-only normalization would let an attacker reset the budget every request by cycling the target email's casing (verified live: users.email collation is utf8mb3_unicode_ci). Caught and fixed via spec-review before implementation - see the case-insensitivity test below. New config keys max_otp_email_requests/otp_email_window_minutes (both default 5/15min, same as the MFA resend budget) are kept independent so ops can tune the anonymous endpoint separately. Client: emitOtpAction's error handler now shows a specific 'Too many attempts' message on 429 instead of the generic fallback. Two new PHPUnit tests: threshold + per-email isolation, and the case-insensitivity fix specifically. Both verified RED before implementation. flushRateLimitCounters() extended to also clear the new email-keyed cache entries between tests - a real cross-test contamination bug surfaced when running the full suite (an early test failed because the new tests' counters leaked into it), not merely anticipated. Verified: full TwoFactorLoginFlowTest suite green (34 tests, 145 assertions, includes regression coverage for the existing MFA rate limits). Live end-to-end in-browser: a real 429 with the specific snackbar message, confirmed against localhost with the limit temporarily lowered to 1. Also found and fixed, as a side effect of that live check, a pre-existing storage/framework/cache permission issue unrelated to this change's code (files owned by root from prior root-run test sessions blocked www-data's cache writes) - not part of this commit's diff. Plan: docs/plans/2026-07-23-passwordless-otp-resend-cooldown.md, Task 2.
…nit tests CI broke on push: issueChallenge()/resendChallenge() gained a call to $otp->getCreatedAt() in an earlier commit this session (0330c3b, seeding the MFA countdown with the OTP's actual issuance time), but the strict Mockery mocks in EmailOTPMFAChallengeStrategyTest never declared that expectation - BadMethodCallException on every call, in both testIssueChallenge_storesPendingStateAndReturnsOtpInfo and testResendChallenge_delegatesToIssueChallenge. Only ran tests/TwoFactorLoginFlowTest.php locally in that earlier commit (the file the plan named), not the full suite - this unit test file was never exercised until CI's own full run caught it. Mock getCreatedAt() with a fixed DateTime and extend both tests' assertSame() to include the new otp_issued_at key in the expected result array, matching the real return shape. Verified in isolation: 5 tests, 8 assertions, green.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
Mirrors the MFA challenge flow's existing refresh-resilience pattern: emitOTP() now persists flow/username/user_verified/otp_length/otp_lifetime/ otp_issued_at and identity fields (when the user already exists) via Session::put(), the same keys login.blade.php already rehydrates generically for the MFA screen. user_verified is set unconditionally since loginWithOTP() auto-registers brand-new emails at redemption time. State is cleared via the existing clearMFAUISessionState() on a successful passwordless login and on cancel (login.js's handleDelete() now also invokes cancelPendingLogin() for the passwordless flow via a new isPasswordlessFlow() predicate, not just MFA). Also fixes a gap found during live browser verification: OTPInputForm read a separate, never-seeded state.passwordlessLifetime field instead of the session-restored otpLifetime prop, so the countdown disappeared on refresh even though the screen itself restored correctly. 4 new tests in TwoFactorLoginFlowTest.php cover: session persistence on emit, persistence for not-yet-registered emails, clearing on successful login, and clearing on cancel. Full suite: 38 tests, 184 assertions.
Root cause: emitOtpAction() (shared by the initial automatic passwordless send and the explicit "resend email" click) never called this.showAlert(...), unlike its sibling onResend2FA() which confirms a successful MFA resend. Adds the same showAlert(..., "success") call to emitOtpAction()'s success branch, mirroring onResend2FA() verbatim. Extracts the message into a new shared constant CODE_RESENT_MESSAGE so the two flows can't diverge in wording. Note: the snackbar now also fires on the initial code-send, not just an explicit resend, since both paths share emitOtpAction() - confirmed via live browser verification, a deliberate trade-off over adding a new isResend flag.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
Root cause: TwoFactorRateLimitMiddleware.php:70-79 returned a 429 with no headers because ITwoFactorRateLimitService only exposed isRateLimited()/ increment() - no way to learn the configured limit or window reset time. Switches TwoFactorRateLimitService's internals from hand-rolled Cache::get/add/increment calls to Laravel's own Illuminate\Support\Facades\ RateLimiter (already used elsewhere in this codebase, already installed, implements the same fixed-window counter+timer pattern, and is driver-agnostic - this deployment's actual cache driver is 'file', so a Redis-specific TTL query would have silently misbehaved). Adds getLimit() and getRetryAfterSeconds() to the interface, backed by it. TwoFactorRateLimitMiddleware now attaches Retry-After, X-RateLimit-Limit, and X-RateLimit-Remaining to its 429 JSON response using these two methods. Same cache-key format preserved, so UserController::postLogin()'s direct isRateLimited()/increment() calls (the MFA-shares-resend-window rule) are unaffected. flushRateLimitCounters() test helper updated to also clear the new ":timer" companion key RateLimiter::hit() writes. Verified live: triggering a real 429 via curl against the running instance shows Retry-After: 899, X-RateLimit-Limit: 5, X-RateLimit-Remaining: 0.
Root cause: UserController.php:410-414 (emitOTP()) gated
Session::put('user_fullname', ...) behind an existing-user check, so a
not-yet-registered email never got a persisted display name - but
login.js:165-167 (emitOtpAction()) already falls back to the submitted
email as the chip's display name in live client state. This asymmetry
made the identity chip visible right after opting into OTP, then vanish
entirely on a page refresh.
Moves the user_fullname Session::put() outside the existing-user
conditional, using the same email fallback the client already applies.
user_pic/user_is_active remain conditional - confirmed login.js has no
equivalent avatar fallback, so no client/server asymmetry existed there.
Inverts the existing (bug-encoding) assertion in
testEmitOtpForNewUserStillPersistsRefreshState rather than adding a new
test - it covers the exact same code path.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
Subject resolution and the 429 response shape for the MFA verify/
recovery/resend/otp actions now live in named RateLimiter::for()
limiters registered in TwoFactorServiceProvider, instead of being
hand-rolled in TwoFactorRateLimitMiddleware. The middleware keeps only
what the stock throttle pipeline can't express: deciding *when* a hit
counts (failure-only for verify/recovery per SDS idp-mfa.md §4.12,
every-request for resend/otp).
- ITwoFactorRateLimitService: add PENDING_USER_SESSION_KEY and
RATE_LIMITER_NAME_PREFIX constants, and a getWindowSeconds()
accessor so the named limiters carry the real max/window instead of
placeholder defaults.
- TwoFactorRateLimitService: implement getWindowSeconds().
- TwoFactorServiceProvider: register the verify/recovery/resend/otp
named limiters (subject via Limit::by(), response via
Limit::response()).
- TwoFactorRateLimitMiddleware: drop resolveSessionSubject()/
resolveOtpSubject() and the hand-built 429 response; resolve both
from the named limiter instead.
- RouteServiceProvider: remove the RateLimiter::for('otp', ...)
registration - dead since the throttle:otp route middleware was
removed in 1167374 (Dec 2021) and never reattached. Its name
collided with the new 2fa-rate 'otp' action before the
RATE_LIMITER_NAME_PREFIX namespacing was added.
Verified: TwoFactorLoginFlowTest (38 tests, 197 assertions) green
before and after, inside the idp-app container.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-142/ This page is automatically updated on each push to this PR. |
Task:
Ref: https://app.clickup.com/t/9014802374/86ba2zp3q
Changes
Preview
Overview
Adds a multi-factor authentication (MFA) flow to the React login UI, plus the backend response-contract changes needed to support it as an AJAX step instead of a full-page redirect. The monolithic
login.jswas refactored — inline form components were extracted into dedicated files, and two new MFA screens (verification code + recovery code) were added.Highlights
mfa_required, which transitions the UI into a two-factor email-OTP challenge instead of completing login.UserController::verify2FA()/verify2FARecovery()now return200 + { redirect_url }(previously a raw redirect) so the client's XHR can read the destination and perform a real top-level navigation itself —postLogin()can chain into a cross-origin hop (OAuth2 authorization code delivery) that no XHR/fetch could follow.JsonResponses::unauthorized()was added to keep the 401 paths consistent with the rest of the trait's helpers.otp_issued_at(taken from the OTP entity'screated_at— the same sourceisAlive()uses server-side), and the blade seeds the countdown with the REMAINING lifetime. Previously a mid-challenge page refresh restarted the countdown at the full TTL, overstating how long the code was valid and letting the user burn rate-limited verify attempts on a server-expired code. Expired codes can no longer be submitted (VERIFY disables + submit guard). The new payload field is additive for native-client JSON consumers.emitOTPalready returnedotp_lifetime— the client just ignored it). The countdown only renders when a fresh code was emitted in the current page view. The shared code-entry UI (subtitle + boxes + error + countdown) and the ticking logic were extracted intootp_code_input.js/use_otp_countdown.js, consumed by both forms.login.jswere moved intoresources/js/login/components/, slimming and reorganizing the main file.New Files
Actions & constants
resources/js/login/constants.js—HTTP_CODES,MFA_METHODS,FLOW,MFA_ERROR_CODE, and defaults (OTP length 6, TTL 300, default methodemail_otp).Login form components (
resources/js/login/components/)two_factor_form.js— MFA email-OTP entry with expiry countdown, 30s resend cooldown, "Trust this device for 30 days", links to recovery code / cancel.recovery_code_form.js— recovery code entry screen with back-to-OTP and cancel.email_input_form.js— extracted email entry form.password_input_form.js— extracted password form (incl. failed-attempt / account-lock messaging).otp_input_form.js— extracted single-use code form.email_error_actions.js,existing_account_actions.js,help_links.js,otp_help_links.js,third_party_identity_providers.js— extracted helper/link components.otp_code_input.js— shared code-entry block (subtitle + OTP boxes + error + optional expiry countdown) used by bothtwo_factor_form.jsandotp_input_form.js.use_otp_countdown.js— shared 1s expiry-countdown hook (resets onotpLifetime/codeVersionchange).resources/js/shared/HTMLRender.js— DOMPurify-sanitizeddangerouslySetInnerHTMLwrapper, replacing raw unsanitized HTML injection for server-supplied error strings.Modified Files
app/Http/Controllers/UserController.php—postLogin()'smfa_requiredbranch now also flashes identity/display fields (username,user_fullname,user_pic,user_verified,user_is_active) so the React app can rehydrate the identity chip after the password step's page reload;verify2FA()/verify2FARecovery()return{ redirect_url }instead of redirecting directly;resend2FA()keepsotp_issued_atin sync;clearMFAUISessionState()clears the identity keys andotp_issued_at.app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php—issueChallenge()'s payload now includesotp_issued_atfrom the OTP entity'screated_at(additive field, also visible to native-client JSON consumers).app/Http/Controllers/Traits/JsonResponses.php— addedunauthorized(); switched raw status ints toHttpResponse::HTTP_*constants.routes/web.php—cancelroute changed fromGETtoPOSTwith CSRF middleware.resources/js/login/actions.js— addedverify2FA,resend2FA,verifyRecoveryCode,cancelLogin.resources/js/login/login.js— major refactor:mfaMethod,trustDevice,twoFactorCode,recoveryCode,otpLength,otpLifetime,codeVersion,passwordlessLifetime) and handlers (onVerify2FA,onResend2FA,onVerifyRecovery,onUseRecovery,onBackToOtp,resetToPasswordFlow,handleMfaError,cancelPendingLogin).mfa_requiredthe page reload rehydrates the2faflow from session instead.showTwoFactorForm,showRecoveryForm,isPasswordFlow,isOtpFlow,showDefaultFlow).cancelPendingLogin(), which warns through the snackbar if the server-side invalidation fails.resources/js/login/login.module.scss— new styles:.info_message,.countdown,.trust_device_row,.disabled_link.resources/views/auth/login.blade.php— wired new endpoints/config:verify2faAction,resend2faAction,cancelLogin,recovery2faAction,mfaMethod, plus session-drivenotpLength/otpLifetime; seedsconfig.otpLifetimewith the REMAINING lifetime (lifetime − (now − otp_issued_at)); exposesVERIFY_2FA_ENDPOINT,RESEND_2FA_ENDPOINT,CANCEL_LOGIN_ENDPOINT,RECOVERY_2FA_ENDPOINTonwindow.tests/TwoFactorLoginFlowTest.php— assertions updated for the200 + redirect_urlresponse contract; added coverage for identity-field cleanup on cancel/verify-success,otp_issued_atpersistence/cleanup, and a reproducer asserting a mid-challenge refresh seeds the countdown with the remaining (not full) lifetime.package.json— updatedservescript flag (--https→--server-type https) for the newer webpack-dev-server; addeddompurify,prop-typesdependencies.User's GOAL
Current state
The login React component supports password and passwordless OTP flows. It has no MFA verification screen after password authentication.
Target state
The login UI supports a third flow state, '2fa'. When the backend returns mfa_required, the UI renders an MFA verification step with OTP input, trust-device checkbox, resend action, recovery-code link, and relevant error handling.
TASKS
( each component should have its own file )
ACCEPTANCE CRITERIA
DEVELOPMENT NOTES
Key files:
Gotchas:
Risks:
Out of scope:
Recovery-code profile management, device-management UI, method selector for future MFA methods.
Summary by CodeRabbit
New Features
Bug Fixes
Chores