Feature | Add Device Trust Service by matiasperrone-exo · Pull Request #133 · OpenStackweb/openstackid

matiasperrone-exo · 2026-05-21T17:27:56Z

Task:

Ref: https://app.clickup.com/t/86ba0nah4

Blocked by:

PR: Feature | MFA Challenge Strategy Pattern (Interface, Abstract, Factory, EmailOTP) #129

Changes:

New service layer (`app/Services/Auth/`)

IDeviceTrustService.php — Interface defining the contract: trustDevice, isDeviceTrusted, removeTrustedDevices, generateDeviceIdentifier.
DeviceTrustService.php — Implementation:
- trustDevice: generates a 128-char random hex token, hashes it with SHA-256, persists a UserTrustedDevice record with configurable expiry (default 30 days), returns the raw token
  for cookie storage.
- isDeviceTrusted: hashes the cookie token, looks up the record, returns false if not found / revoked / expired; updates last_seen_at on a valid hit.
- removeTrustedDevices: bulk-revokes all devices for a user (sets is_revoked = true).
TwoFactorServiceProvider.php — Deferred service provider that binds IDeviceTrustService → DeviceTrustService as a singleton; registered in config/app.php.

Repository changes (`IUserTrustedDeviceRepository` / `DoctrineUserTrustedDeviceRepository`)

Two new methods added:

getByUserAndDeviceIdentifier — looks up a device by user + hashed identifier with no revoked/expiry filter (used by the service to check all states).
revokeAllForUser — bulk DQL UPDATE setting is_revoked = true for all devices belonging to a user.

Entity updates (`UserTrustedDevice`)

Constructor now initializes last_seen_at to now().
Added isExpired() helper (compares expires_at against now).
Fixed targetEntity reference from FQCN string to User::class.

Config

config/two_factor.php — Added device_trust_lifetime_days (default 30) and cookie_name (default 'device_trust_token') settings, driven by env vars.

Tests (`tests/DeviceTrustServiceTest.php`)

264-line unit test suite using Mockery covering:

isDeviceTrusted: null/empty cookie, unknown token, revoked device, expired device, valid device, last_seen_at update.
trustDevice: returns 128-char hex token, stores SHA-256 hash (not raw token), persists exactly one record.
removeTrustedDevices: delegates to revokeAllForUser.
generateDeviceIdentifier: SHA-256 correctness.

Requested GOAL

Current state

No device trust mechanism exists. Users would need to complete 2FA on every single login, even from the same browser and device they used yesterday.

Target state

IDeviceTrustService and its implementation allow the system to remember trusted devices via a secure cookie mechanism. A SHA-256 hash of a cryptographically random token is stored
in the user_trusted_devices table with a configurable TTL (default 30 days). The raw token is stored in a long-lived HttpOnly cookie , On subsequent logins, the cookie token is hashed and compared against stored records to bypass 2FA.

TASKS

Create IDeviceTrustService interface with methods: isDeviceTrusted(User, ?string cookieToken): bool, trustDevice(User, string userAgent, string ipAddress): string (returns raw cookie token), removeTrustedDevices(User): void, generateDeviceIdentifier(string token): string (returns SHA-256 hash)
Implement DeviceTrustService: isDeviceTrusted() hashes the cookie token via generateDeviceIdentifier(), queries IUserTrustedDeviceRepository for a matching non-revoked, non-expired record for the user, updates last_seen_at on match.
trustDevice() generates a 64-byte random token via bin2hex(random_bytes(32)), stores SHA-256 hash in user_trusted_devices with TTL from config, extracts User-Agent for device_name, returns raw token.
removeTrustedDevices() revokes all devices for the user (sets is_revoked=true).
Implement IUserTrustedDeviceRepository::getByUserAndDeviceIdentifier() query method
Register IDeviceTrustService in TwoFactorServiceProvider
Write unit tests for isDeviceTrusted() with: valid trusted device, expired device, revoked device, no cookie, wrong cookie
Write unit tests for trustDevice(): verify token generation, hash storage, and record creation
Write unit tests for removeTrustedDevices(): verify all devices for user are revoked
DeviceTrustService must depend on IUserTrustedDeviceRepository, not EntityManager directly.
trustDevice() must instantiate a UserTrustedDevice entity, associate it with the User, set device_identifier, device_name, ip_address, user_agent, trusted_at, expires_at, last_seen_at, is_revoked=false, then persist it through the repository.
isDeviceTrusted() must call IUserTrustedDeviceRepository::getByUserAndDeviceIdentifier($user, $deviceIdentifier) and only return true for a non-revoked, non-expired record.
On successful match, isDeviceTrusted() must update last_seen_at and persist the change.
removeTrustedDevices() must call a repository method that revokes all trusted devices for the user.

ACCEPTANCE CRITERIA

trustDevice() returns a 128-character hex string (64 bytes as hex)
The stored device_identifier is a SHA-256 hash of the returned token (not the raw token itself)
isDeviceTrusted() returns true when a matching non-revoked, non-expired record exists
isDeviceTrusted() returns false for expired records (expires_at < now)
isDeviceTrusted() returns false for revoked records (is_revoked = true)
isDeviceTrusted() returns false when cookieToken is null or empty
isDeviceTrusted() updates last_seen_at on a valid match
removeTrustedDevices() sets is_revoked=true on all records for the user
DeviceTrustService does not query Doctrine directly; all UserTrustedDevice access goes through IUserTrustedDeviceRepository.
trustDevice() creates exactly one UserTrustedDevice record per call.
The raw cookie token is returned to the caller but is never stored in UserTrustedDevice.
UserTrustedDevice::device_identifier stores hash('sha256', $rawToken).
isDeviceTrusted() updates UserTrustedDevice::last_seen_at when a valid device is found.
Device trust TTL defaults to 30 days, configurable via config('two_factor.device_trust_lifetime_days')
cookie name is configuravle via a config('two_factor.cookie_name')
All unit tests pass

coderabbitai · 2026-05-21T17:28:03Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e28d845d-6e91-4af7-85bf-c17ceaade84e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/mfa-device-trust-service

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-05-21T17:29:08Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-133/

This page is automatically updated on each push to this PR.

github-actions · 2026-05-21T17:44:18Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-133/

This page is automatically updated on each push to this PR.

github-actions · 2026-05-21T20:20:51Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-133/

This page is automatically updated on each push to this PR.

github-actions · 2026-05-21T20:57:55Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-133/

This page is automatically updated on each push to this PR.

github-actions · 2026-05-21T21:27:02Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-133/

This page is automatically updated on each push to this PR.

github-actions · 2026-05-21T21:41:47Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-133/

This page is automatically updated on each push to this PR.

github-actions · 2026-05-21T22:07:08Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-133/

This page is automatically updated on each push to this PR.

caseylocker

LGTM. All Clickup tasks/requirements are met. One suggestion which is @smarcet call:

tests/DeviceTrustServiceTest.php:29 extends BrowserKitTestCase, whose setUp() runs a full Redis flush, doctrine migrations, and TestSeeder on every test method
(tests/BrowserKitTestCase.php:36-56). This suite is pure Mockery and needs none of that — it pays heavy CI cost per method and risks the pattern getting copied as more 2FA service tests land.

Suggest extending Tests\TestCase (tests/TestCase.php:19) instead. That still boots Laravel so config('two_factor.device_trust_lifetime_days') resolves in
testTrustDeviceSetsExpiresAtFromConfig, but skips the migrate + seed.

While you're in there, three small tightening opportunities on the same file:

:72 — shouldReceive('getByUserAndDeviceIdentifier') has no with(...); add ->with($user, hash('sha256', 'unknowntoken')) so the test proves the lookup uses the SHA-256 of the cookie, not the raw token.
:118 — shouldReceive('add')->once() doesn't verify the $sync=true second arg. Add ->with(Mockery::type(UserTrustedDevice::class), true) so a regression that drops the sync flag is caught.
:230 — $lifetimeDays = (int) config(...) makes the assertion self-referential (passes for any config value, including 0). Use Config::set('two_factor.device_trust_lifetime_days', 45)at the top of the test and assert45` literally.

github-actions · 2026-05-29T16:34:46Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-133/

This page is automatically updated on each push to this PR.

github-actions · 2026-06-01T15:24:31Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-133/

This page is automatically updated on each push to this PR.

github-actions · 2026-06-08T20:49:33Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-133/

This page is automatically updated on each push to this PR.

* feat: Two-Factor Audit Service * Feature | MFAGateService (Two-Factor Gate Decision Service) (#135) * feat: MFAGateService (Two-Factor Gate Decision Service) * Feature | UserController MFA Integration, Device Trust Cookie Management, Audit Wiring, and 2FA Rate Limiting (#136) * feat: UserController MFA Integration, Device Trust Cookie Management, Audit Wiring, and 2FA Rate Limiting * chore: Add PR's requested changed * chore: Add PR's requested changes * Add TWO_FACTOR_ENABLED global kill-switch to MFA gate MFAGateService::requiresChallenge() had no master on/off switch, contradicting the SDS idp-mfa.md §10.1 rollout plan, which requires being able to instantly revert to password-only login without a code rollback if something goes wrong post-deploy. config/two_factor.php gains an 'enabled' key (env TWO_FACTOR_ENABLED, default true) checked first in requiresChallenge(), short-circuiting before any per-user or device-trust evaluation. * Route MFA challenge responses through login_strategy, not hardcoded JSON 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. * Clear pending MFA challenge and UI-restoration state on cancelLogin() 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. * Block passwordless MFA bypass; make challengeRequired self-contained Two related fixes to the MFA login flow: 1. Passwordless (flow=otp) login never checked shouldRequire2FA(), so an enforced-2FA user could bypass MFA entirely via emitOTP() + postLogin with flow=otp instead of flow=password (SDS idp-mfa.md §7.4 / Open Question #3 explicitly treats passwordless as single-factor). Now throws AuthenticationException before loginWithOTP(), reusing the existing errorLogin() redirect+flash path - the OTP form still submits as a native form POST, so this needed no new response contract. 2. challengeRequired()'s redirect-based implementations (DefaultLoginStrategy, DisplayResponseUserAgentStrategy) previously ignored the $params they received, silently depending on the caller having already flashed otp_length/otp_lifetime to session - an implicit contract that would silently break for any other caller. Both now flash their own $params (persistent, not one-shot, so it survives repeated refreshes) and set error_code, mirroring what DisplayResponseJsonStrategy already sends native clients in JSON. clearMFAUISessionState() now clears error_code too. The '2fa' flow value moves from a new ILoginStrategy constant to IAuthService::AuthenticationFlowMFA, alongside its siblings AuthenticationFlowPassword/AuthenticationFlowPasswordless - all three are the same session 'flow' enum (already flashed together in the AuthenticationException catch block), so splitting the third value into a different interface would have been inconsistent. New test: OAuth2NativeMFALoginFlowTest gains a non-native (page/popup/ touch) case proving the 302+session-flash contract, alongside the existing native 412+JSON case. TwoFactorLoginFlowTest covers the passwordless-bypass rejection (including that it still reuses errorLogin(), not a new JSON contract) and the error_code flash/clear. * Rate-limit the initial MFA challenge issuance in postLogin() The '2fa.rate' middleware could never gate postLogin()'s initial OTP issuance: its before-phase reads 2fa_pending_user_id from session to know which user to throttle, but that key is only written by issueChallenge() - inside the very request that would need throttling. A user with valid credentials could repeatedly POST to the plain login route and trigger unlimited email-OTP sends, bypassing the 5-per-15-minute resend cap entirely (SDS idp-mfa.md §4.12 explicitly requires the initial issuance to share the same 2fa_rate:resend:{user_id} window as resend()). Extracted the cache-key/window logic that lived only in TwoFactorRateLimitMiddleware into ITwoFactorRateLimitService / TwoFactorRateLimitService (same pattern as DeviceTrustService / TwoFactorAuditService / MFAGateService, registered in TwoFactorServiceProvider), so both the middleware (verify/recovery/resend routes) and UserController::postLogin() (initial issuance, now knows the user id post-validateCredentials()) share one source of truth instead of duplicating cache-key construction. postLogin() checks isRateLimited() before issuing a challenge and calls increment() after a successful issue. The rejection throws AuthenticationException, reusing the existing catch block's errorLogin() redirect+flash path - consistent with challengeRequired() already being redirect-based, since the password form still submits as a native form POST. resend2FA()/verify2FA()/verifyRecoveryCode() stay JSON+429 via the middleware, unaffected, since those are AJAX-only endpoints. New test proves postLogin() and resend() share the same window: after max_otp_requests postLogin() calls, the next one is rejected. * Fix op_browser_state ordering bug in AuthService::loginUser() Investigated the "session fixation" finding from the PR review (SDS idp-mfa.md §9.3 asks for a test proving 2fa_pending_user_id cannot be injected). Traced actual runtime behavior via debug instrumentation before writing a fix, since pattern-matching "no explicit Session::regenerate() call" as a vulnerability turned out to be wrong. Laravel's SessionGuard::login() (invoked via Auth::login(), already called unconditionally at the end of loginUser()) already calls $session->migrate(true) internally - the session-fixation window was already closed by the framework, with no code change needed for that property specifically. An added test asserting this (comparing session ID before/after login) passed identically with or without any fix, proving it was a false positive caused by this test harness resetting the session ID between $this->action() calls regardless of production behavior - that test was written and then discarded rather than kept for false confidence. What IS real, found via the same investigation: PrincipalService::register() (called by loginUser() before this fix) hashes the CURRENT session ID into op_browser_state, used for OIDC Session Management (check-session iframe). Since register() ran BEFORE Auth::login(), its hash was computed from a session ID that Auth::login()'s own migrate(true) was about to invalidate moments later - any relying party polling the check-session iframe would see a value that no longer matched what the server would recompute, incorrectly signaling a session change. Fix: call Auth::login() first, then principal_service->clear()/register() after, so the hash uses the final, stable post-login session ID. No new Session::regenerate() call needed - Auth::login() already provides one. New tests: - AuthServiceLoginUserTest (unit, Mockery-alias facades, same pattern as AuthServiceLogoutTest): asserts the call order directly. - TwoFactorLoginFlowTest::testCompletedMFALoginKeepsOPBrowserStateConsistentWithSessionId (integration): proves op_browser_state matches a freshly-computed hash of the post-login session ID end-to-end through the real MFA verify flow. Confirmed failing against the pre-fix ordering, passing after. * Add test proving OTP redeem rolls back on mid-transaction failure 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. * Make verify2FARecovery audit logging best-effort EventRecoveryUsed was logged unguarded after loginUser() and clearPendingState(), so an audit-sink failure at that point propagated to the outer catch(Exception) and returned a 500 to a user who was already authenticated with an already-burned recovery code — the account's last-resort login path. Mirrors the same best-effort try/catch already applied to verify2FA()'s EventChallengeSucceeded audit call. Adds testRecoveryAuditFailureDoesNotBlockLogin, the recovery-path analogue of testAuditFailureDoesNotBlockLogin, reproducing the 500 before the fix and asserting a 302 + established session after it. * Add real concurrent-connection tests for OTP/recovery-code row locks testOTPCodeRejectsReuseAfterSuccessfulVerification and testRecoveryCodeRejectsReuseAfterTransactionCommit only prove sequential reuse is rejected after a transaction commits. Neither exercises the actual property refreshExclusiveLock() exists for: blocking a second, concurrent request from redeeming the same unredeemed OTP or recovery code while the first request's transaction still holds the row. Adds two tests that open a genuinely independent physical DB connection (verified via differing MySQL CONNECTION_ID()) and prove FOR UPDATE from that connection is blocked (lock wait timeout) while EmailOTPMFAChallengeStrategy/AbstractMFAChallengeStrategy's production refreshExclusiveLock() call holds the row. Verified the assertion is non-vacuous by temporarily disabling the lock call and confirming the test fails as expected, then restoring it. * Guard all MFA audit-log calls against Throwable, not just Exception Best-effort audit logging around the MFA flows only caught Exception, which misses Error subtypes (TypeError, ArgumentCountError, etc.). An Error escaping any of these would still turn a clean response into an uncaught 500 or, worse for the two failure-path calls, drop the error_code the rate-limit middleware keys its failure counter on (TwoFactorRateLimitMiddleware::isFailure() only sees the JSON body of whatever response actually gets returned). Applies the codebase's existing convention for this exact situation (see app/Audit/AuditLoggerFactory.php, TrackRequestMiddleware.php) to all 7 best-effort audit/device-trust sites in this controller: - postLogin(): initial challenge issuance audit log (was unguarded) - verify2FA(): failure-path audit log (was unguarded) - verify2FA(): queueDeviceTrustCookie() call (was catch(Exception)) - verify2FA(): success-path audit log (was catch(Exception)) - verify2FARecovery(): failure-path audit log (was unguarded) - verify2FARecovery(): success-path audit log (was catch(Exception)) - resend2FA(): challenge-reissue audit log (was unguarded) Verified: full Two Factor Authentication Test Suite (83 tests, 241 assertions) passes unchanged. Co-Authored-By: Claude <noreply@anthropic.com> * Honor the global 2FA kill-switch in User::shouldRequire2FA() The passwordless-login guard called shouldRequire2FA() directly, which ignored config('two_factor.enabled'), so an enforced admin stayed blocked from passwordless login even with the kill-switch off (SDS idp-mfa.md rollout, section 10.1). Move the enabled check into shouldRequire2FA() as the single source of truth shared by both the MFA gate and the passwordless guard, and drop the now-redundant check in MFAGateService. * Feature | Add Login UI MFA Flow (#142) * feat: Add Login UI MFA Flow * fix: rename HTMLRender.jsx to .js so webpack can resolve it 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. * fix: revert password submit to native form POST 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. * fix: add missing React import in HTMLRender to prevent ReferenceError 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). * fix: cancel login now invalidates the pending MFA challenge server-side 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). * fix: stop the 2FA verify XHR from following cross-origin OAuth2 redirects 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. * fix: cancel and session-expiry now correctly return to the password screen 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. * fix: clear identity fields from session on MFA cancel/verify-success 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. * fix: invalidate pending MFA challenge when identity chip is cleared 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. * fix: block submitting an expired MFA code 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. * fix: seed the MFA countdown with the remaining OTP lifetime after refresh 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. * feat: add expiry countdown to the passwordless OTP form, dedup shared 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. * fix: surface cancelLogin failures instead of swallowing them in console 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. * feat: add 30s resend cooldown to the passwordless OTP screen 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. * feat: rate-limit the passwordless OTP issuance endpoint server-side 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. * fix: mock EmailOTPMFAChallengeStrategy's new getCreatedAt() call in unit tests CI broke on push: issueChallenge()/resendChallenge() gained a call to $otp->getCreatedAt() in an earlier commit this session (0330c3be, 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. * feat: passwordless OTP screen survives browser refresh 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. * fix: show success snackbar when passwordless OTP code is (re)sent 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. * fix: set Retry-After/X-RateLimit-* headers on 2FA rate-limit 429s 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. * fix: persist identity chip fallback for new passwordless-OTP users 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. * Refactor 2FA rate limiting to use RateLimiter::for() named limiters 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 1167374c (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. * Feat/fe testing infrastructure (#144) * feat: first tests Signed-off-by: romanetar <roman_ag@hotmail.com> * feat: add testing infrastructure for login MFA flow and E2E suite Signed-off-by: romanetar <roman_ag@hotmail.com> * feat: add testing infrastructure for login MFA flow and E2E suite Signed-off-by: romanetar <roman_ag@hotmail.com> * test: isolate login.spec.ts in CI, fix MFA mock route ordering Comment out login-mfa-flow.spec.ts and register.spec.ts so CI runs login.spec.ts alone to verify it now passes without account lockout interference. Also fix the MFA beforeEach mock: fulfill() must run before unroute(), otherwise Playwright auto-resolves the in-flight route on unroute and the later fulfill() throws "Route is already handled" - which was letting the real POST through with a wrong password and locking out test@test.com. * test: re-enable MFA and registration e2e suites login.spec.ts verified green in isolation; re-enable the MFA flow suite (route-ordering fix already applied) and the registration suite now that the account-lockout cascade is gone. Signed-off-by: romanetar <roman_ag@hotmail.com> * fix: align MFA e2e/JS tests with PR #142's native-form-POST mechanism PR #142 reverted the password login step from AJAX back to a native form POST + server redirect/session flow (commit 0eca371c), removing handleAuthenticatePasswordFlow/Ok/Error, authenticateWithPassword, and the MFA_CHALLENGE_REQUIRED constant. The tests added by this branch were written against the old AJAX contract and needed to be realigned. - tests/js/login/login.mfa.test.js: remove the handleAuthenticatePasswordOk describe block - it tested a client-side AJAX handler that no longer exists in login.js. - tests/e2e/tests/auth/login-mfa-flow.spec.ts: - beforeEach no longer mocks the password POST as JSON; it performs a real native login against a real MFA-enforced account, matching how postLogin() actually issues a challenge (redirect + session state). - Each TS-* test now uses its own seeded MFA user (mfa-ts-NNN@test.com) instead of sharing one fixed account - a real challenge issuance counts against two_factor.rate_limit.max_otp_requests, so 8 tests sharing one account exhausted the limit before the suite finished. - Fixed VERIFY_URL/RESEND_URL/RECOVERY_URL/CANCEL_URL glob patterns to end with '**': postRawRequest() appends every param as a query string in addition to the body, so the exact-suffix glob never matched and silently left every route mock inert (requests were hitting the real backend instead). - TS-004/TS-007: resetToPasswordFlow() keeps the verified identity and returns to the password step (authFlow: FLOW.PASSWORD) - it does not clear user_name/user_verified. Both tests asserted the email step was shown instead, contradicting their own titles and the function's name. - TS-002: widened the post-verify assertion timeout - onVerify2FA() always assigns window.location.href on success, so even a same-URL mock response occasionally triggers a real navigation that raced the original 1s timeout. - .github/workflows/{pull_request,push}_frontend_tests.yml: seed the 8 mfa-ts-NNN@test.com accounts alongside the existing test@test.com / e2e@test.com fixtures. - .gitignore: add /test-results/ (Playwright's screenshot/video/trace output directory) - only /tests/e2e/report/ was previously ignored. Verified: 40/40 PHP (TwoFactorLoginFlowTest), 23/23 Jest, 13/13 Playwright e2e, stable across repeated runs via `docker compose --profile e2e run --rm playwright npx playwright test`. * feat: add e2e coverage for the OAuth2 authorization code flow Adds tests/e2e/tests/oauth2/auth-code-flow.spec.ts, exercising the full authorization code grant end to end - including the memento (pending OAuth2 request) surviving a real MFA detour, consent-bypass for a returning user, and MFA-skip for a trusted device: - unauthenticated /oauth2/auth redirects to login (memento serialized). - full flow: real login -> real MFA challenge -> real OTP -> consent screen for the correct client -> Accept -> authorization code -> code exchanged at the token endpoint for a real access_token. - returning user with prior consent: a second /oauth2/auth for the same client+scope skips the consent screen entirely and redirects straight to redirect_uri (InteractiveGrantType::handle()'s has_former_consent + auto_approval branch). - trusted device: checking "Trust this device" during MFA sets the Secure device_trust_token cookie; logging out and logging back in then skips the MFA challenge entirely. Infrastructure needed to drive this for real (no mocks): - app/Console/Commands/GetLatestOtp.php (idp:get-latest-otp {email}): prints the newest not-yet-redeemed OTP for a user, since the mailer queues via Redis and there is no catchable local mailbox to read the code from. Registered in app/Console/Kernel.php. - tests/e2e/utils/otp.ts: reads that OTP from the test runner - directly via `php artisan` when reachable in-process (CI, host dev), or via `docker exec idp-app php artisan ...` when running against the dockerized stack (APP_URL points at nginx). - docker-compose/playwright/Dockerfile + docker-compose.yml: the playwright service now builds this image (adds the Docker CLI on top of the stock Playwright image) and mounts /var/run/docker.sock so the above `docker exec` path works from inside that container. Scoped to the e2e profile only. - The suite works around two config('app.url')-vs-actual-origin mismatches (e.g. app.url=http://localhost but this suite runs against http://nginx in the docker-compose e2e profile - cookies are domain-scoped, so following the server's literal absolute redirect/ form-action URLs client-side would drop the session): verify2FA's redirect_url, the consent form's action, and the password step's postLogin() redirect are all replayed via page.request (shares the page's cookies) instead of trusting the browser/client-side JS to follow them unassisted. - .github/workflows/{pull_request,push}_frontend_tests.yml: seed mfa-oauth2-consent@test.com and mfa-oauth2-trust@test.com alongside the existing mfa-oauth2@test.com fixture. Known environment limitation (not a bug): the trusted-device assertion requires a "potentially trustworthy origin" for the Secure cookie to persist - true for http://localhost (host dev, and CI, which already uses APP_URL=http://localhost:8001) but not for the docker-compose e2e profile's http://nginx, where browsers silently drop the cookie. Verified: 16/17 e2e via `docker compose --profile e2e run --rm playwright npx playwright test` (the trusted-device test is the one expected miss, per the above), 4/4 in tests/e2e/tests/oauth2/ via host (`npx playwright test`), 40/40 PHP (TwoFactorLoginFlowTest), 23/23 Jest. * fix: seed the e2e OAuth2 test client without depending on TestSeeder CI was red: tests/e2e/tests/oauth2/auth-code-flow.spec.ts authorizes against a client_id that only exists as a side effect of database/seeds/TestSeeder.php, which is wired ONLY into PHPUnit's BrowserKitTestCase ($this->seed('TestSeeder')) - never into `php artisan db:seed`, which is all the CI workflow runs. On a genuinely fresh database the client_id never resolves, so InteractiveGrantType::handle() throws InvalidClientException before ever reaching the "redirect to login" branch, and the very first oauth2 test ("unauthenticated request redirects to login") gets a 400 error page instead of a redirect - exactly what the failing CI run showed. Local testing never caught this because the long-lived docker-compose dev database already had TestSeeder's fixtures from past PHPUnit runs. TestSeeder itself is not a safe fix for CI: its run() truncates users/groups/oauth2_client (and otp/consent/session-adjacent tables) before reseeding its own fixed set - correct for PHPUnit's isolated test lifecycle, destructive against the same shared database this workflow also seeds idp:create-super-admin/idp:create-raw-user users into. - app/Console/Commands/CreateOAuth2TestClient.php (idp:create-oauth2-test-client): idempotent, additive-only - creates just the one confidential client (same client_id/secret/redirect_uri the e2e suite already uses) plus a dedicated owner user (the consent screen's getDeveloperEmail() dereferences the owner unconditionally - an ownerless client 500s as soon as a real login reaches /accounts/user/consent) and grants it the 'profile' scope. Registered in app/Console/Kernel.php. - .github/workflows/{pull_request,push}_frontend_tests.yml: run the new command alongside the existing user fixtures. Verified: 16/17 e2e via `docker compose --profile e2e run --rm playwright npx playwright test` (the 17th, trusted-device, is the pre-existing environment-only miss - Secure cookies don't persist over http://nginx), 40/40 PHP, 23/23 Jest. * feat: recovery code management (#146) * feat: recovery code management Signed-off-by: romanetar <roman_ag@hotmail.com> * fix: add missing postRawRequestFull to base_actions.js profile/actions.js imports postRawRequestFull for the new enableTwoFactor and regenerateRecoveryCodes flows, but it was never exported, causing a runtime TypeError on both actions. Falling back to postRawRequest is unsafe here since it copies params into the URL query string, which would leak current_password into access logs. * fix: reject enableTwoFactor when 2FA is already enabled enable2FA() had no already-enrolled guard, so a second POST to /2fa/enable for an enrolled user silently regenerated recovery codes with no password confirmation, bypassing the password-gated rotation flow required by CU-86ba2zp66 and sds/idp-mfa.md §4.10.3. * refactor: move 2FA enrollment orchestration into RecoveryCodeService The transaction plus enable2FA + repository->add + code generation lived in UserApiController, breaking the thin-controllers/fat-services convention and diverging from the regenerateRecoveryCodes path, which already delegates to the service. UserApiController::enableTwoFactor now only validates input and calls RecoveryCodeService::enableTwoFactorAndGenerateCodes. * fix: normalize recovery code server-side before hash check Hash::check() compared the raw submitted code against the dash-less uppercase hash, so the "strip separators + uppercase" contract was only enforced by the login.js client. Any other consumer submitting a code exactly as displayed (XXXX-XXXX) would fail verification on this lockout-critical path. Apply the same normalization in AbstractMFAChallengeStrategy::verifyRecoveryCode() before Hash::check. * feat: warn on low recovery codes after MFA recovery login CU-86ba2zp66 / sds/idp-mfa.md §4.10.3, §4.11 step 5 require a dismissable low-code warning after a successful MFA login, but it was only wired into the profile page - a user who burns codes at login never saw it unless they happened to visit their profile. verify2FARecovery now returns recovery_codes_remaining and the configured low threshold; login.js holds the post-login redirect and shows a dismissable banner when the count is low, before navigating away. The sessionStorage dismissal key is shared with the profile page's RecoveryCodesPanel via a new shared module so dismissing in either place suppresses it everywhere for the rest of the session. * test: cover recovery-code redemption, re-enrollment, and the real request layer Three gaps mapped to the riskiest parts of this PR were unpinned: 1. Nothing proved a code returned as XXXX-XXXX actually redeems through AbstractMFAChallengeStrategy::verifyRecoveryCode() - the hash is of the dash-less string, so the generate->display->redeem contract (including the dash normalization) was untested. 2. enableTwoFactor()'s already-enrolled guard (412) had no regression test. 3. Every JS test mocked profile/actions, so nothing exercised the real request layer - exactly where the missing postRawRequestFull export lived. tests/js/profile/actions.test.js only stubs the transport (superagent) and calls the real enableTwoFactor/regenerateRecoveryCodes; verified it reproduces the original "postRawRequestFull is not a function" TypeError when that export is removed. * fix: fix CI failures from the recovery-code round-trip test and dash normalization 1. testDisplayedRecoveryCodeRedeemsThroughVerifyRecoveryCode called AbstractMFAChallengeStrategy::verifyRecoveryCode() directly, but it takes a PESSIMISTIC_WRITE row lock that requires an open transaction (Doctrine\ORM\TransactionRequiredException in CI). Route it through IAuthService::verifyMFARecoveryCode(), like the real login flow, which wraps the call in a transaction. 2. Several pre-existing test fixtures hashed a "plain" recovery code with a literal "-" baked in (e.g. 'RECOVERY-REUSE-TX-' . uniqid()) and then submitted that same string for verification. The dash normalization added earlier in this PR strips separators from the submitted code before Hash::check(), so a hash made from a dash-containing string can never match its own normalized submission - a real generated code never contains a dash in its raw/hashed form, only in its display formatting. Fixed the 7 affected fixtures across TwoFactorLoginFlowTest and AbstractMFAChallengeStrategyTest to drop the literal dash. * fix: remove nested transaction in enableTwoFactorAndGenerateCodes enableTwoFactorAndGenerateCodes() wrapped enable2FA()/user persist in one transaction() call while also calling generateRecoveryCodes(), which opens its own. DoctrineTransactionService::transaction() closes the entity manager and connection on failure, so an inner failure could tear down the EM out from under the still-running outer transaction. Extracted the shared code-generation logic into a transaction-free regenerateCodesForUser(), so each public method now opens exactly one transaction. * fix: make recovery-code audit logging best-effort Both generateRecoveryCodes() and enableTwoFactorAndGenerateCodes() logged audit events after the codes were already committed and about to be returned to the client. An audit-logging failure there would 500 a response whose side effects already succeeded, and a client retry on that 500 would regenerate and invalidate the codes it was never shown. Wrap both in try/catch + Log::warning, matching the best-effort pattern already used for audit logging in UserController. * fix: use the configured app name in the downloaded recovery-codes file recovery_code_display.js hardcoded "FNTECH" in both the file header and the downloaded filename, which would misbrand any non-FNTECH deployment. Threaded the existing appName prop (already exposed by profile.blade.php as config.appName, sourced from Config::get('app.app_name')) down through ProfilePage -> TwoFactorSection -> RecoveryCodesPanel -> RecoveryCodeModal -> RecoveryCodeDisplay, with an OpenStackID fallback matching the config default. * fix: uppercase uniqid() in recovery-code test fixtures verifyRecoveryCode() uppercases the submitted code (in addition to stripping separators) before Hash::check() - real generated codes are always uppercase alphanumeric. Three fixtures built their "plain" code with a raw uniqid() suffix, which is lowercase hex, so the hash (made from the original mixed-case string) could never match its own normalized submission. Verified standalone with password_hash/ password_verify that the old fixture reproduces the exact CI failure and the fixed one passes. --------- Signed-off-by: romanetar <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com>

github-actions · 2026-08-11T16:40:50Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-133/

This page is automatically updated on each push to this PR.

…y, EmailOTP) (#129) * feat: Implement Multi-Factor Authentication challenge strategies and tests * chore: Add PR's requested changes * Feature | Add Device Trust Service (#133) * feat: Add Device Trust Service * Feature | Two-Factor Audit Service (#134) * feat: Two-Factor Audit Service * Feature | MFAGateService (Two-Factor Gate Decision Service) (#135) * feat: MFAGateService (Two-Factor Gate Decision Service) * Feature | UserController MFA Integration, Device Trust Cookie Management, Audit Wiring, and 2FA Rate Limiting (#136) * feat: UserController MFA Integration, Device Trust Cookie Management, Audit Wiring, and 2FA Rate Limiting * chore: Add PR's requested changed * chore: Add PR's requested changes * Add TWO_FACTOR_ENABLED global kill-switch to MFA gate MFAGateService::requiresChallenge() had no master on/off switch, contradicting the SDS idp-mfa.md §10.1 rollout plan, which requires being able to instantly revert to password-only login without a code rollback if something goes wrong post-deploy. config/two_factor.php gains an 'enabled' key (env TWO_FACTOR_ENABLED, default true) checked first in requiresChallenge(), short-circuiting before any per-user or device-trust evaluation. * Route MFA challenge responses through login_strategy, not hardcoded JSON 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. * Clear pending MFA challenge and UI-restoration state on cancelLogin() 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. * Block passwordless MFA bypass; make challengeRequired self-contained Two related fixes to the MFA login flow: 1. Passwordless (flow=otp) login never checked shouldRequire2FA(), so an enforced-2FA user could bypass MFA entirely via emitOTP() + postLogin with flow=otp instead of flow=password (SDS idp-mfa.md §7.4 / Open Question #3 explicitly treats passwordless as single-factor). Now throws AuthenticationException before loginWithOTP(), reusing the existing errorLogin() redirect+flash path - the OTP form still submits as a native form POST, so this needed no new response contract. 2. challengeRequired()'s redirect-based implementations (DefaultLoginStrategy, DisplayResponseUserAgentStrategy) previously ignored the $params they received, silently depending on the caller having already flashed otp_length/otp_lifetime to session - an implicit contract that would silently break for any other caller. Both now flash their own $params (persistent, not one-shot, so it survives repeated refreshes) and set error_code, mirroring what DisplayResponseJsonStrategy already sends native clients in JSON. clearMFAUISessionState() now clears error_code too. The '2fa' flow value moves from a new ILoginStrategy constant to IAuthService::AuthenticationFlowMFA, alongside its siblings AuthenticationFlowPassword/AuthenticationFlowPasswordless - all three are the same session 'flow' enum (already flashed together in the AuthenticationException catch block), so splitting the third value into a different interface would have been inconsistent. New test: OAuth2NativeMFALoginFlowTest gains a non-native (page/popup/ touch) case proving the 302+session-flash contract, alongside the existing native 412+JSON case. TwoFactorLoginFlowTest covers the passwordless-bypass rejection (including that it still reuses errorLogin(), not a new JSON contract) and the error_code flash/clear. * Rate-limit the initial MFA challenge issuance in postLogin() The '2fa.rate' middleware could never gate postLogin()'s initial OTP issuance: its before-phase reads 2fa_pending_user_id from session to know which user to throttle, but that key is only written by issueChallenge() - inside the very request that would need throttling. A user with valid credentials could repeatedly POST to the plain login route and trigger unlimited email-OTP sends, bypassing the 5-per-15-minute resend cap entirely (SDS idp-mfa.md §4.12 explicitly requires the initial issuance to share the same 2fa_rate:resend:{user_id} window as resend()). Extracted the cache-key/window logic that lived only in TwoFactorRateLimitMiddleware into ITwoFactorRateLimitService / TwoFactorRateLimitService (same pattern as DeviceTrustService / TwoFactorAuditService / MFAGateService, registered in TwoFactorServiceProvider), so both the middleware (verify/recovery/resend routes) and UserController::postLogin() (initial issuance, now knows the user id post-validateCredentials()) share one source of truth instead of duplicating cache-key construction. postLogin() checks isRateLimited() before issuing a challenge and calls increment() after a successful issue. The rejection throws AuthenticationException, reusing the existing catch block's errorLogin() redirect+flash path - consistent with challengeRequired() already being redirect-based, since the password form still submits as a native form POST. resend2FA()/verify2FA()/verifyRecoveryCode() stay JSON+429 via the middleware, unaffected, since those are AJAX-only endpoints. New test proves postLogin() and resend() share the same window: after max_otp_requests postLogin() calls, the next one is rejected. * Fix op_browser_state ordering bug in AuthService::loginUser() Investigated the "session fixation" finding from the PR review (SDS idp-mfa.md §9.3 asks for a test proving 2fa_pending_user_id cannot be injected). Traced actual runtime behavior via debug instrumentation before writing a fix, since pattern-matching "no explicit Session::regenerate() call" as a vulnerability turned out to be wrong. Laravel's SessionGuard::login() (invoked via Auth::login(), already called unconditionally at the end of loginUser()) already calls $session->migrate(true) internally - the session-fixation window was already closed by the framework, with no code change needed for that property specifically. An added test asserting this (comparing session ID before/after login) passed identically with or without any fix, proving it was a false positive caused by this test harness resetting the session ID between $this->action() calls regardless of production behavior - that test was written and then discarded rather than kept for false confidence. What IS real, found via the same investigation: PrincipalService::register() (called by loginUser() before this fix) hashes the CURRENT session ID into op_browser_state, used for OIDC Session Management (check-session iframe). Since register() ran BEFORE Auth::login(), its hash was computed from a session ID that Auth::login()'s own migrate(true) was about to invalidate moments later - any relying party polling the check-session iframe would see a value that no longer matched what the server would recompute, incorrectly signaling a session change. Fix: call Auth::login() first, then principal_service->clear()/register() after, so the hash uses the final, stable post-login session ID. No new Session::regenerate() call needed - Auth::login() already provides one. New tests: - AuthServiceLoginUserTest (unit, Mockery-alias facades, same pattern as AuthServiceLogoutTest): asserts the call order directly. - TwoFactorLoginFlowTest::testCompletedMFALoginKeepsOPBrowserStateConsistentWithSessionId (integration): proves op_browser_state matches a freshly-computed hash of the post-login session ID end-to-end through the real MFA verify flow. Confirmed failing against the pre-fix ordering, passing after. * Add test proving OTP redeem rolls back on mid-transaction failure 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. * Make verify2FARecovery audit logging best-effort EventRecoveryUsed was logged unguarded after loginUser() and clearPendingState(), so an audit-sink failure at that point propagated to the outer catch(Exception) and returned a 500 to a user who was already authenticated with an already-burned recovery code — the account's last-resort login path. Mirrors the same best-effort try/catch already applied to verify2FA()'s EventChallengeSucceeded audit call. Adds testRecoveryAuditFailureDoesNotBlockLogin, the recovery-path analogue of testAuditFailureDoesNotBlockLogin, reproducing the 500 before the fix and asserting a 302 + established session after it. * Add real concurrent-connection tests for OTP/recovery-code row locks testOTPCodeRejectsReuseAfterSuccessfulVerification and testRecoveryCodeRejectsReuseAfterTransactionCommit only prove sequential reuse is rejected after a transaction commits. Neither exercises the actual property refreshExclusiveLock() exists for: blocking a second, concurrent request from redeeming the same unredeemed OTP or recovery code while the first request's transaction still holds the row. Adds two tests that open a genuinely independent physical DB connection (verified via differing MySQL CONNECTION_ID()) and prove FOR UPDATE from that connection is blocked (lock wait timeout) while EmailOTPMFAChallengeStrategy/AbstractMFAChallengeStrategy's production refreshExclusiveLock() call holds the row. Verified the assertion is non-vacuous by temporarily disabling the lock call and confirming the test fails as expected, then restoring it. * Guard all MFA audit-log calls against Throwable, not just Exception Best-effort audit logging around the MFA flows only caught Exception, which misses Error subtypes (TypeError, ArgumentCountError, etc.). An Error escaping any of these would still turn a clean response into an uncaught 500 or, worse for the two failure-path calls, drop the error_code the rate-limit middleware keys its failure counter on (TwoFactorRateLimitMiddleware::isFailure() only sees the JSON body of whatever response actually gets returned). Applies the codebase's existing convention for this exact situation (see app/Audit/AuditLoggerFactory.php, TrackRequestMiddleware.php) to all 7 best-effort audit/device-trust sites in this controller: - postLogin(): initial challenge issuance audit log (was unguarded) - verify2FA(): failure-path audit log (was unguarded) - verify2FA(): queueDeviceTrustCookie() call (was catch(Exception)) - verify2FA(): success-path audit log (was catch(Exception)) - verify2FARecovery(): failure-path audit log (was unguarded) - verify2FARecovery(): success-path audit log (was catch(Exception)) - resend2FA(): challenge-reissue audit log (was unguarded) Verified: full Two Factor Authentication Test Suite (83 tests, 241 assertions) passes unchanged. Co-Authored-By: Claude <noreply@anthropic.com> * Honor the global 2FA kill-switch in User::shouldRequire2FA() The passwordless-login guard called shouldRequire2FA() directly, which ignored config('two_factor.enabled'), so an enforced admin stayed blocked from passwordless login even with the kill-switch off (SDS idp-mfa.md rollout, section 10.1). Move the enabled check into shouldRequire2FA() as the single source of truth shared by both the MFA gate and the passwordless guard, and drop the now-redundant check in MFAGateService. * Feature | Add Login UI MFA Flow (#142) * feat: Add Login UI MFA Flow * fix: rename HTMLRender.jsx to .js so webpack can resolve it 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. * fix: revert password submit to native form POST 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. * fix: add missing React import in HTMLRender to prevent ReferenceError 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). * fix: cancel login now invalidates the pending MFA challenge server-side 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). * fix: stop the 2FA verify XHR from following cross-origin OAuth2 redirects 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. * fix: cancel and session-expiry now correctly return to the password screen 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. * fix: clear identity fields from session on MFA cancel/verify-success 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. * fix: invalidate pending MFA challenge when identity chip is cleared 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. * fix: block submitting an expired MFA code 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. * fix: seed the MFA countdown with the remaining OTP lifetime after refresh 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. * feat: add expiry countdown to the passwordless OTP form, dedup shared 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. * fix: surface cancelLogin failures instead of swallowing them in console 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. * feat: add 30s resend cooldown to the passwordless OTP screen 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. * feat: rate-limit the passwordless OTP issuance endpoint server-side 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. * fix: mock EmailOTPMFAChallengeStrategy's new getCreatedAt() call in unit tests CI broke on push: issueChallenge()/resendChallenge() gained a call to $otp->getCreatedAt() in an earlier commit this session (0330c3be, 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. * feat: passwordless OTP screen survives browser refresh 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. * fix: show success snackbar when passwordless OTP code is (re)sent 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. * fix: set Retry-After/X-RateLimit-* headers on 2FA rate-limit 429s 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. * fix: persist identity chip fallback for new passwordless-OTP users 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. * Refactor 2FA rate limiting to use RateLimiter::for() named limiters 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 1167374c (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. * Feat/fe testing infrastructure (#144) * feat: first tests Signed-off-by: romanetar <roman_ag@hotmail.com> * feat: add testing infrastructure for login MFA flow and E2E suite Signed-off-by: romanetar <roman_ag@hotmail.com> * feat: add testing infrastructure for login MFA flow and E2E suite Signed-off-by: romanetar <roman_ag@hotmail.com> * test: isolate login.spec.ts in CI, fix MFA mock route ordering Comment out login-mfa-flow.spec.ts and register.spec.ts so CI runs login.spec.ts alone to verify it now passes without account lockout interference. Also fix the MFA beforeEach mock: fulfill() must run before unroute(), otherwise Playwright auto-resolves the in-flight route on unroute and the later fulfill() throws "Route is already handled" - which was letting the real POST through with a wrong password and locking out test@test.com. * test: re-enable MFA and registration e2e suites login.spec.ts verified green in isolation; re-enable the MFA flow suite (route-ordering fix already applied) and the registration suite now that the account-lockout cascade is gone. Signed-off-by: romanetar <roman_ag@hotmail.com> * fix: align MFA e2e/JS tests with PR #142's native-form-POST mechanism PR #142 reverted the password login step from AJAX back to a native form POST + server redirect/session flow (commit 0eca371c), removing handleAuthenticatePasswordFlow/Ok/Error, authenticateWithPassword, and the MFA_CHALLENGE_REQUIRED constant. The tests added by this branch were written against the old AJAX contract and needed to be realigned. - tests/js/login/login.mfa.test.js: remove the handleAuthenticatePasswordOk describe block - it tested a client-side AJAX handler that no longer exists in login.js. - tests/e2e/tests/auth/login-mfa-flow.spec.ts: - beforeEach no longer mocks the password POST as JSON; it performs a real native login against a real MFA-enforced account, matching how postLogin() actually issues a challenge (redirect + session state). - Each TS-* test now uses its own seeded MFA user (mfa-ts-NNN@test.com) instead of sharing one fixed account - a real challenge issuance counts against two_factor.rate_limit.max_otp_requests, so 8 tests sharing one account exhausted the limit before the suite finished. - Fixed VERIFY_URL/RESEND_URL/RECOVERY_URL/CANCEL_URL glob patterns to end with '**': postRawRequest() appends every param as a query string in addition to the body, so the exact-suffix glob never matched and silently left every route mock inert (requests were hitting the real backend instead). - TS-004/TS-007: resetToPasswordFlow() keeps the verified identity and returns to the password step (authFlow: FLOW.PASSWORD) - it does not clear user_name/user_verified. Both tests asserted the email step was shown instead, contradicting their own titles and the function's name. - TS-002: widened the post-verify assertion timeout - onVerify2FA() always assigns window.location.href on success, so even a same-URL mock response occasionally triggers a real navigation that raced the original 1s timeout. - .github/workflows/{pull_request,push}_frontend_tests.yml: seed the 8 mfa-ts-NNN@test.com accounts alongside the existing test@test.com / e2e@test.com fixtures. - .gitignore: add /test-results/ (Playwright's screenshot/video/trace output directory) - only /tests/e2e/report/ was previously ignored. Verified: 40/40 PHP (TwoFactorLoginFlowTest), 23/23 Jest, 13/13 Playwright e2e, stable across repeated runs via `docker compose --profile e2e run --rm playwright npx playwright test`. * feat: add e2e coverage for the OAuth2 authorization code flow Adds tests/e2e/tests/oauth2/auth-code-flow.spec.ts, exercising the full authorization code grant end to end - including the memento (pending OAuth2 request) surviving a real MFA detour, consent-bypass for a returning user, and MFA-skip for a trusted device: - unauthenticated /oauth2/auth redirects to login (memento serialized). - full flow: real login -> real MFA challenge -> real OTP -> consent screen for the correct client -> Accept -> authorization code -> code exchanged at the token endpoint for a real access_token. - returning user with prior consent: a second /oauth2/auth for the same client+scope skips the consent screen entirely and redirects straight to redirect_uri (InteractiveGrantType::handle()'s has_former_consent + auto_approval branch). - trusted device: checking "Trust this device" during MFA sets the Secure device_trust_token cookie; logging out and logging back in then skips the MFA challenge entirely. Infrastructure needed to drive this for real (no mocks): - app/Console/Commands/GetLatestOtp.php (idp:get-latest-otp {email}): prints the newest not-yet-redeemed OTP for a user, since the mailer queues via Redis and there is no catchable local mailbox to read the code from. Registered in app/Console/Kernel.php. - tests/e2e/utils/otp.ts: reads that OTP from the test runner - directly via `php artisan` when reachable in-process (CI, host dev), or via `docker exec idp-app php artisan ...` when running against the dockerized stack (APP_URL points at nginx). - docker-compose/playwright/Dockerfile + docker-compose.yml: the playwright service now builds this image (adds the Docker CLI on top of the stock Playwright image) and mounts /var/run/docker.sock so the above `docker exec` path works from inside that container. Scoped to the e2e profile only. - The suite works around two config('app.url')-vs-actual-origin mismatches (e.g. app.url=http://localhost but this suite runs against http://nginx in the docker-compose e2e profile - cookies are domain-scoped, so following the server's literal absolute redirect/ form-action URLs client-side would drop the session): verify2FA's redirect_url, the consent form's action, and the password step's postLogin() redirect are all replayed via page.request (shares the page's cookies) instead of trusting the browser/client-side JS to follow them unassisted. - .github/workflows/{pull_request,push}_frontend_tests.yml: seed mfa-oauth2-consent@test.com and mfa-oauth2-trust@test.com alongside the existing mfa-oauth2@test.com fixture. Known environment limitation (not a bug): the trusted-device assertion requires a "potentially trustworthy origin" for the Secure cookie to persist - true for http://localhost (host dev, and CI, which already uses APP_URL=http://localhost:8001) but not for the docker-compose e2e profile's http://nginx, where browsers silently drop the cookie. Verified: 16/17 e2e via `docker compose --profile e2e run --rm playwright npx playwright test` (the trusted-device test is the one expected miss, per the above), 4/4 in tests/e2e/tests/oauth2/ via host (`npx playwright test`), 40/40 PHP (TwoFactorLoginFlowTest), 23/23 Jest. * fix: seed the e2e OAuth2 test client without depending on TestSeeder CI was red: tests/e2e/tests/oauth2/auth-code-flow.spec.ts authorizes against a client_id that only exists as a side effect of database/seeds/TestSeeder.php, which is wired ONLY into PHPUnit's BrowserKitTestCase ($this->seed('TestSeeder')) - never into `php artisan db:seed`, which is all the CI workflow runs. On a genuinely fresh database the client_id never resolves, so InteractiveGrantType::handle() throws InvalidClientException before ever reaching the "redirect to login" branch, and the very first oauth2 test ("unauthenticated request redirects to login") gets a 400 error page instead of a redirect - exactly what the failing CI run showed. Local testing never caught this because the long-lived docker-compose dev database already had TestSeeder's fixtures from past PHPUnit runs. TestSeeder itself is not a safe fix for CI: its run() truncates users/groups/oauth2_client (and otp/consent/session-adjacent tables) before reseeding its own fixed set - correct for PHPUnit's isolated test lifecycle, destructive against the same shared database this workflow also seeds idp:create-super-admin/idp:create-raw-user users into. - app/Console/Commands/CreateOAuth2TestClient.php (idp:create-oauth2-test-client): idempotent, additive-only - creates just the one confidential client (same client_id/secret/redirect_uri the e2e suite already uses) plus a dedicated owner user (the consent screen's getDeveloperEmail() dereferences the owner unconditionally - an ownerless client 500s as soon as a real login reaches /accounts/user/consent) and grants it the 'profile' scope. Registered in app/Console/Kernel.php. - .github/workflows/{pull_request,push}_frontend_tests.yml: run the new command alongside the existing user fixtures. Verified: 16/17 e2e via `docker compose --profile e2e run --rm playwright npx playwright test` (the 17th, trusted-device, is the pre-existing environment-only miss - Secure cookies don't persist over http://nginx), 40/40 PHP, 23/23 Jest. * feat: recovery code management (#146) * feat: recovery code management Signed-off-by: romanetar <roman_ag@hotmail.com> * fix: add missing postRawRequestFull to base_actions.js profile/actions.js imports postRawRequestFull for the new enableTwoFactor and regenerateRecoveryCodes flows, but it was never exported, causing a runtime TypeError on both actions. Falling back to postRawRequest is unsafe here since it copies params into the URL query string, which would leak current_password into access logs. * fix: reject enableTwoFactor when 2FA is already enabled enable2FA() had no already-enrolled guard, so a second POST to /2fa/enable for an enrolled user silently regenerated recovery codes with no password confirmation, bypassing the password-gated rotation flow required by CU-86ba2zp66 and sds/idp-mfa.md §4.10.3. * refactor: move 2FA enrollment orchestration into RecoveryCodeService The transaction plus enable2FA + repository->add + code generation lived in UserApiController, breaking the thin-controllers/fat-services convention and diverging from the regenerateRecoveryCodes path, which already delegates to the service. UserApiController::enableTwoFactor now only validates input and calls RecoveryCodeService::enableTwoFactorAndGenerateCodes. * fix: normalize recovery code server-side before hash check Hash::check() compared the raw submitted code against the dash-less uppercase hash, so the "strip separators + uppercase" contract was only enforced by the login.js client. Any other consumer submitting a code exactly as displayed (XXXX-XXXX) would fail verification on this lockout-critical path. Apply the same normalization in AbstractMFAChallengeStrategy::verifyRecoveryCode() before Hash::check. * feat: warn on low recovery codes after MFA recovery login CU-86ba2zp66 / sds/idp-mfa.md §4.10.3, §4.11 step 5 require a dismissable low-code warning after a successful MFA login, but it was only wired into the profile page - a user who burns codes at login never saw it unless they happened to visit their profile. verify2FARecovery now returns recovery_codes_remaining and the configured low threshold; login.js holds the post-login redirect and shows a dismissable banner when the count is low, before navigating away. The sessionStorage dismissal key is shared with the profile page's RecoveryCodesPanel via a new shared module so dismissing in either place suppresses it everywhere for the rest of the session. * test: cover recovery-code redemption, re-enrollment, and the real request layer Three gaps mapped to the riskiest parts of this PR were unpinned: 1. Nothing proved a code returned as XXXX-XXXX actually redeems through AbstractMFAChallengeStrategy::verifyRecoveryCode() - the hash is of the dash-less string, so the generate->display->redeem contract (including the dash normalization) was untested. 2. enableTwoFactor()'s already-enrolled guard (412) had no regression test. 3. Every JS test mocked profile/actions, so nothing exercised the real request layer - exactly where the missing postRawRequestFull export lived. tests/js/profile/actions.test.js only stubs the transport (superagent) and calls the real enableTwoFactor/regenerateRecoveryCodes; verified it reproduces the original "postRawRequestFull is not a function" TypeError when that export is removed. * fix: fix CI failures from the recovery-code round-trip test and dash normalization 1. testDisplayedRecoveryCodeRedeemsThroughVerifyRecoveryCode called AbstractMFAChallengeStrategy::verifyRecoveryCode() directly, but it takes a PESSIMISTIC_WRITE row lock that requires an open transaction (Doctrine\ORM\TransactionRequiredException in CI). Route it through IAuthService::verifyMFARecoveryCode(), like the real login flow, which wraps the call in a transaction. 2. Several pre-existing test fixtures hashed a "plain" recovery code with a literal "-" baked in (e.g. 'RECOVERY-REUSE-TX-' . uniqid()) and then submitted that same string for verification. The dash normalization added earlier in this PR strips separators from the submitted code before Hash::check(), so a hash made from a dash-containing string can never match its own normalized submission - a real generated code never contains a dash in its raw/hashed form, only in its display formatting. Fixed the 7 affected fixtures across TwoFactorLoginFlowTest and AbstractMFAChallengeStrategyTest to drop the literal dash. * fix: remove nested transaction in enableTwoFactorAndGenerateCodes enableTwoFactorAndGenerateCodes() wrapped enable2FA()/user persist in one transaction() call while also calling generateRecoveryCodes(), which opens its own. DoctrineTransactionService::transaction() closes the entity manager and connection on failure, so an inner failure could tear down the EM out from under the still-running outer transaction. Extracted the shared code-generation logic into a transaction-free regenerateCodesForUser(), so each public method now opens exactly one transaction. * fix: make recovery-code audit logging best-effort Both generateRecoveryCodes() and enableTwoFactorAndGenerateCodes() logged audit events after the codes were already committed and about to be returned to the client. An audit-logging failure there would 500 a response whose side effects already succeeded, and a client retry on that 500 would regenerate and invalidate the codes it was never shown. Wrap both in try/catch + Log::warning, matching the best-effort pattern already used for audit logging in UserController. * fix: use the configured app name in the downloaded recovery-codes file recovery_code_display.js hardcoded "FNTECH" in both the file header and the downloaded filename, which would misbrand any non-FNTECH deployment. Threaded the existing appName prop (already exposed by profile.blade.php as config.appName, sourced from Config::get('app.app_name')) down through ProfilePage -> TwoFactorSection -> RecoveryCodesPanel -> RecoveryCodeModal -> RecoveryCodeDisplay, with an OpenStackID fallback matching the config default. * fix: uppercase uniqid() in recovery-code test fixtures verifyRecoveryCode() uppercases the submitted code (in addition to stripping separators) before Hash::check() - real generated codes are always uppercase alphanumeric. Three fixtures built their "plain" code with a raw uniqid() suffix, which is lowercase hex, so the hash (made from the original mixed-case string) could never match its own normalized submission. Verified standalone with password_hash/ password_verify that the old fixture reproduces the exact CI failure and the fixed one passes. --------- Signed-off-by: romanetar <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com>

* feat: Add AuthService validateCredentials method - test: cover canLogin()=false branch in validateCredentials() unit tests - docs: document known double-query cost in validateCredentials() - fix: use consistent error message in validateCredentials() * chore: lint file app/libs/Auth/AuthService.php * chore: Add PR's requested changes * chore: Add PR's requested changes Add tests changes with suggestion * chore: Fix issues created on rebase * Feature | MFA Challenge Strategy Pattern (Interface, Abstract, Factory, EmailOTP) (#129) * feat: Implement Multi-Factor Authentication challenge strategies and tests * chore: Add PR's requested changes * Feature | Add Device Trust Service (#133) * feat: Add Device Trust Service * Feature | Two-Factor Audit Service (#134) * feat: Two-Factor Audit Service * Feature | MFAGateService (Two-Factor Gate Decision Service) (#135) * feat: MFAGateService (Two-Factor Gate Decision Service) * Feature | UserController MFA Integration, Device Trust Cookie Management, Audit Wiring, and 2FA Rate Limiting (#136) * feat: UserController MFA Integration, Device Trust Cookie Management, Audit Wiring, and 2FA Rate Limiting * chore: Add PR's requested changed * chore: Add PR's requested changes * Add TWO_FACTOR_ENABLED global kill-switch to MFA gate MFAGateService::requiresChallenge() had no master on/off switch, contradicting the SDS idp-mfa.md §10.1 rollout plan, which requires being able to instantly revert to password-only login without a code rollback if something goes wrong post-deploy. config/two_factor.php gains an 'enabled' key (env TWO_FACTOR_ENABLED, default true) checked first in requiresChallenge(), short-circuiting before any per-user or device-trust evaluation. * Route MFA challenge responses through login_strategy, not hardcoded JSON 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. * Clear pending MFA challenge and UI-restoration state on cancelLogin() 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. * Block passwordless MFA bypass; make challengeRequired self-contained Two related fixes to the MFA login flow: 1. Passwordless (flow=otp) login never checked shouldRequire2FA(), so an enforced-2FA user could bypass MFA entirely via emitOTP() + postLogin with flow=otp instead of flow=password (SDS idp-mfa.md §7.4 / Open Question #3 explicitly treats passwordless as single-factor). Now throws AuthenticationException before loginWithOTP(), reusing the existing errorLogin() redirect+flash path - the OTP form still submits as a native form POST, so this needed no new response contract. 2. challengeRequired()'s redirect-based implementations (DefaultLoginStrategy, DisplayResponseUserAgentStrategy) previously ignored the $params they received, silently depending on the caller having already flashed otp_length/otp_lifetime to session - an implicit contract that would silently break for any other caller. Both now flash their own $params (persistent, not one-shot, so it survives repeated refreshes) and set error_code, mirroring what DisplayResponseJsonStrategy already sends native clients in JSON. clearMFAUISessionState() now clears error_code too. The '2fa' flow value moves from a new ILoginStrategy constant to IAuthService::AuthenticationFlowMFA, alongside its siblings AuthenticationFlowPassword/AuthenticationFlowPasswordless - all three are the same session 'flow' enum (already flashed together in the AuthenticationException catch block), so splitting the third value into a different interface would have been inconsistent. New test: OAuth2NativeMFALoginFlowTest gains a non-native (page/popup/ touch) case proving the 302+session-flash contract, alongside the existing native 412+JSON case. TwoFactorLoginFlowTest covers the passwordless-bypass rejection (including that it still reuses errorLogin(), not a new JSON contract) and the error_code flash/clear. * Rate-limit the initial MFA challenge issuance in postLogin() The '2fa.rate' middleware could never gate postLogin()'s initial OTP issuance: its before-phase reads 2fa_pending_user_id from session to know which user to throttle, but that key is only written by issueChallenge() - inside the very request that would need throttling. A user with valid credentials could repeatedly POST to the plain login route and trigger unlimited email-OTP sends, bypassing the 5-per-15-minute resend cap entirely (SDS idp-mfa.md §4.12 explicitly requires the initial issuance to share the same 2fa_rate:resend:{user_id} window as resend()). Extracted the cache-key/window logic that lived only in TwoFactorRateLimitMiddleware into ITwoFactorRateLimitService / TwoFactorRateLimitService (same pattern as DeviceTrustService / TwoFactorAuditService / MFAGateService, registered in TwoFactorServiceProvider), so both the middleware (verify/recovery/resend routes) and UserController::postLogin() (initial issuance, now knows the user id post-validateCredentials()) share one source of truth instead of duplicating cache-key construction. postLogin() checks isRateLimited() before issuing a challenge and calls increment() after a successful issue. The rejection throws AuthenticationException, reusing the existing catch block's errorLogin() redirect+flash path - consistent with challengeRequired() already being redirect-based, since the password form still submits as a native form POST. resend2FA()/verify2FA()/verifyRecoveryCode() stay JSON+429 via the middleware, unaffected, since those are AJAX-only endpoints. New test proves postLogin() and resend() share the same window: after max_otp_requests postLogin() calls, the next one is rejected. * Fix op_browser_state ordering bug in AuthService::loginUser() Investigated the "session fixation" finding from the PR review (SDS idp-mfa.md §9.3 asks for a test proving 2fa_pending_user_id cannot be injected). Traced actual runtime behavior via debug instrumentation before writing a fix, since pattern-matching "no explicit Session::regenerate() call" as a vulnerability turned out to be wrong. Laravel's SessionGuard::login() (invoked via Auth::login(), already called unconditionally at the end of loginUser()) already calls $session->migrate(true) internally - the session-fixation window was already closed by the framework, with no code change needed for that property specifically. An added test asserting this (comparing session ID before/after login) passed identically with or without any fix, proving it was a false positive caused by this test harness resetting the session ID between $this->action() calls regardless of production behavior - that test was written and then discarded rather than kept for false confidence. What IS real, found via the same investigation: PrincipalService::register() (called by loginUser() before this fix) hashes the CURRENT session ID into op_browser_state, used for OIDC Session Management (check-session iframe). Since register() ran BEFORE Auth::login(), its hash was computed from a session ID that Auth::login()'s own migrate(true) was about to invalidate moments later - any relying party polling the check-session iframe would see a value that no longer matched what the server would recompute, incorrectly signaling a session change. Fix: call Auth::login() first, then principal_service->clear()/register() after, so the hash uses the final, stable post-login session ID. No new Session::regenerate() call needed - Auth::login() already provides one. New tests: - AuthServiceLoginUserTest (unit, Mockery-alias facades, same pattern as AuthServiceLogoutTest): asserts the call order directly. - TwoFactorLoginFlowTest::testCompletedMFALoginKeepsOPBrowserStateConsistentWithSessionId (integration): proves op_browser_state matches a freshly-computed hash of the post-login session ID end-to-end through the real MFA verify flow. Confirmed failing against the pre-fix ordering, passing after. * Add test proving OTP redeem rolls back on mid-transaction failure 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. * Make verify2FARecovery audit logging best-effort EventRecoveryUsed was logged unguarded after loginUser() and clearPendingState(), so an audit-sink failure at that point propagated to the outer catch(Exception) and returned a 500 to a user who was already authenticated with an already-burned recovery code — the account's last-resort login path. Mirrors the same best-effort try/catch already applied to verify2FA()'s EventChallengeSucceeded audit call. Adds testRecoveryAuditFailureDoesNotBlockLogin, the recovery-path analogue of testAuditFailureDoesNotBlockLogin, reproducing the 500 before the fix and asserting a 302 + established session after it. * Add real concurrent-connection tests for OTP/recovery-code row locks testOTPCodeRejectsReuseAfterSuccessfulVerification and testRecoveryCodeRejectsReuseAfterTransactionCommit only prove sequential reuse is rejected after a transaction commits. Neither exercises the actual property refreshExclusiveLock() exists for: blocking a second, concurrent request from redeeming the same unredeemed OTP or recovery code while the first request's transaction still holds the row. Adds two tests that open a genuinely independent physical DB connection (verified via differing MySQL CONNECTION_ID()) and prove FOR UPDATE from that connection is blocked (lock wait timeout) while EmailOTPMFAChallengeStrategy/AbstractMFAChallengeStrategy's production refreshExclusiveLock() call holds the row. Verified the assertion is non-vacuous by temporarily disabling the lock call and confirming the test fails as expected, then restoring it. * Guard all MFA audit-log calls against Throwable, not just Exception Best-effort audit logging around the MFA flows only caught Exception, which misses Error subtypes (TypeError, ArgumentCountError, etc.). An Error escaping any of these would still turn a clean response into an uncaught 500 or, worse for the two failure-path calls, drop the error_code the rate-limit middleware keys its failure counter on (TwoFactorRateLimitMiddleware::isFailure() only sees the JSON body of whatever response actually gets returned). Applies the codebase's existing convention for this exact situation (see app/Audit/AuditLoggerFactory.php, TrackRequestMiddleware.php) to all 7 best-effort audit/device-trust sites in this controller: - postLogin(): initial challenge issuance audit log (was unguarded) - verify2FA(): failure-path audit log (was unguarded) - verify2FA(): queueDeviceTrustCookie() call (was catch(Exception)) - verify2FA(): success-path audit log (was catch(Exception)) - verify2FARecovery(): failure-path audit log (was unguarded) - verify2FARecovery(): success-path audit log (was catch(Exception)) - resend2FA(): challenge-reissue audit log (was unguarded) Verified: full Two Factor Authentication Test Suite (83 tests, 241 assertions) passes unchanged. Co-Authored-By: Claude <noreply@anthropic.com> * Honor the global 2FA kill-switch in User::shouldRequire2FA() The passwordless-login guard called shouldRequire2FA() directly, which ignored config('two_factor.enabled'), so an enforced admin stayed blocked from passwordless login even with the kill-switch off (SDS idp-mfa.md rollout, section 10.1). Move the enabled check into shouldRequire2FA() as the single source of truth shared by both the MFA gate and the passwordless guard, and drop the now-redundant check in MFAGateService. * Feature | Add Login UI MFA Flow (#142) * feat: Add Login UI MFA Flow * fix: rename HTMLRender.jsx to .js so webpack can resolve it 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. * fix: revert password submit to native form POST 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. * fix: add missing React import in HTMLRender to prevent ReferenceError 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). * fix: cancel login now invalidates the pending MFA challenge server-side 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). * fix: stop the 2FA verify XHR from following cross-origin OAuth2 redirects 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. * fix: cancel and session-expiry now correctly return to the password screen 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. * fix: clear identity fields from session on MFA cancel/verify-success 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. * fix: invalidate pending MFA challenge when identity chip is cleared 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. * fix: block submitting an expired MFA code 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. * fix: seed the MFA countdown with the remaining OTP lifetime after refresh 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. * feat: add expiry countdown to the passwordless OTP form, dedup shared 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. * fix: surface cancelLogin failures instead of swallowing them in console 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. * feat: add 30s resend cooldown to the passwordless OTP screen 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. * feat: rate-limit the passwordless OTP issuance endpoint server-side 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. * fix: mock EmailOTPMFAChallengeStrategy's new getCreatedAt() call in unit tests CI broke on push: issueChallenge()/resendChallenge() gained a call to $otp->getCreatedAt() in an earlier commit this session (0330c3be, 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. * feat: passwordless OTP screen survives browser refresh 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. * fix: show success snackbar when passwordless OTP code is (re)sent 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. * fix: set Retry-After/X-RateLimit-* headers on 2FA rate-limit 429s 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. * fix: persist identity chip fallback for new passwordless-OTP users 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. * Refactor 2FA rate limiting to use RateLimiter::for() named limiters 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 1167374c (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. * Feat/fe testing infrastructure (#144) * feat: first tests Signed-off-by: romanetar <roman_ag@hotmail.com> * feat: add testing infrastructure for login MFA flow and E2E suite Signed-off-by: romanetar <roman_ag@hotmail.com> * feat: add testing infrastructure for login MFA flow and E2E suite Signed-off-by: romanetar <roman_ag@hotmail.com> * test: isolate login.spec.ts in CI, fix MFA mock route ordering Comment out login-mfa-flow.spec.ts and register.spec.ts so CI runs login.spec.ts alone to verify it now passes without account lockout interference. Also fix the MFA beforeEach mock: fulfill() must run before unroute(), otherwise Playwright auto-resolves the in-flight route on unroute and the later fulfill() throws "Route is already handled" - which was letting the real POST through with a wrong password and locking out test@test.com. * test: re-enable MFA and registration e2e suites login.spec.ts verified green in isolation; re-enable the MFA flow suite (route-ordering fix already applied) and the registration suite now that the account-lockout cascade is gone. Signed-off-by: romanetar <roman_ag@hotmail.com> * fix: align MFA e2e/JS tests with PR #142's native-form-POST mechanism PR #142 reverted the password login step from AJAX back to a native form POST + server redirect/session flow (commit 0eca371c), removing handleAuthenticatePasswordFlow/Ok/Error, authenticateWithPassword, and the MFA_CHALLENGE_REQUIRED constant. The tests added by this branch were written against the old AJAX contract and needed to be realigned. - tests/js/login/login.mfa.test.js: remove the handleAuthenticatePasswordOk describe block - it tested a client-side AJAX handler that no longer exists in login.js. - tests/e2e/tests/auth/login-mfa-flow.spec.ts: - beforeEach no longer mocks the password POST as JSON; it performs a real native login against a real MFA-enforced account, matching how postLogin() actually issues a challenge (redirect + session state). - Each TS-* test now uses its own seeded MFA user (mfa-ts-NNN@test.com) instead of sharing one fixed account - a real challenge issuance counts against two_factor.rate_limit.max_otp_requests, so 8 tests sharing one account exhausted the limit before the suite finished. - Fixed VERIFY_URL/RESEND_URL/RECOVERY_URL/CANCEL_URL glob patterns to end with '**': postRawRequest() appends every param as a query string in addition to the body, so the exact-suffix glob never matched and silently left every route mock inert (requests were hitting the real backend instead). - TS-004/TS-007: resetToPasswordFlow() keeps the verified identity and returns to the password step (authFlow: FLOW.PASSWORD) - it does not clear user_name/user_verified. Both tests asserted the email step was shown instead, contradicting their own titles and the function's name. - TS-002: widened the post-verify assertion timeout - onVerify2FA() always assigns window.location.href on success, so even a same-URL mock response occasionally triggers a real navigation that raced the original 1s timeout. - .github/workflows/{pull_request,push}_frontend_tests.yml: seed the 8 mfa-ts-NNN@test.com accounts alongside the existing test@test.com / e2e@test.com fixtures. - .gitignore: add /test-results/ (Playwright's screenshot/video/trace output directory) - only /tests/e2e/report/ was previously ignored. Verified: 40/40 PHP (TwoFactorLoginFlowTest), 23/23 Jest, 13/13 Playwright e2e, stable across repeated runs via `docker compose --profile e2e run --rm playwright npx playwright test`. * feat: add e2e coverage for the OAuth2 authorization code flow Adds tests/e2e/tests/oauth2/auth-code-flow.spec.ts, exercising the full authorization code grant end to end - including the memento (pending OAuth2 request) surviving a real MFA detour, consent-bypass for a returning user, and MFA-skip for a trusted device: - unauthenticated /oauth2/auth redirects to login (memento serialized). - full flow: real login -> real MFA challenge -> real OTP -> consent screen for the correct client -> Accept -> authorization code -> code exchanged at the token endpoint for a real access_token. - returning user with prior consent: a second /oauth2/auth for the same client+scope skips the consent screen entirely and redirects straight to redirect_uri (InteractiveGrantType::handle()'s has_former_consent + auto_approval branch). - trusted device: checking "Trust this device" during MFA sets the Secure device_trust_token cookie; logging out and logging back in then skips the MFA challenge entirely. Infrastructure needed to drive this for real (no mocks): - app/Console/Commands/GetLatestOtp.php (idp:get-latest-otp {email}): prints the newest not-yet-redeemed OTP for a user, since the mailer queues via Redis and there is no catchable local mailbox to read the code from. Registered in app/Console/Kernel.php. - tests/e2e/utils/otp.ts: reads that OTP from the test runner - directly via `php artisan` when reachable in-process (CI, host dev), or via `docker exec idp-app php artisan ...` when running against the dockerized stack (APP_URL points at nginx). - docker-compose/playwright/Dockerfile + docker-compose.yml: the playwright service now builds this image (adds the Docker CLI on top of the stock Playwright image) and mounts /var/run/docker.sock so the above `docker exec` path works from inside that container. Scoped to the e2e profile only. - The suite works around two config('app.url')-vs-actual-origin mismatches (e.g. app.url=http://localhost but this suite runs against http://nginx in the docker-compose e2e profile - cookies are domain-scoped, so following the server's literal absolute redirect/ form-action URLs client-side would drop the session): verify2FA's redirect_url, the consent form's action, and the password step's postLogin() redirect are all replayed via page.request (shares the page's cookies) instead of trusting the browser/client-side JS to follow them unassisted. - .github/workflows/{pull_request,push}_frontend_tests.yml: seed mfa-oauth2-consent@test.com and mfa-oauth2-trust@test.com alongside the existing mfa-oauth2@test.com fixture. Known environment limitation (not a bug): the trusted-device assertion requires a "potentially trustworthy origin" for the Secure cookie to persist - true for http://localhost (host dev, and CI, which already uses APP_URL=http://localhost:8001) but not for the docker-compose e2e profile's http://nginx, where browsers silently drop the cookie. Verified: 16/17 e2e via `docker compose --profile e2e run --rm playwright npx playwright test` (the trusted-device test is the one expected miss, per the above), 4/4 in tests/e2e/tests/oauth2/ via host (`npx playwright test`), 40/40 PHP (TwoFactorLoginFlowTest), 23/23 Jest. * fix: seed the e2e OAuth2 test client without depending on TestSeeder CI was red: tests/e2e/tests/oauth2/auth-code-flow.spec.ts authorizes against a client_id that only exists as a side effect of database/seeds/TestSeeder.php, which is wired ONLY into PHPUnit's BrowserKitTestCase ($this->seed('TestSeeder')) - never into `php artisan db:seed`, which is all the CI workflow runs. On a genuinely fresh database the client_id never resolves, so InteractiveGrantType::handle() throws InvalidClientException before ever reaching the "redirect to login" branch, and the very first oauth2 test ("unauthenticated request redirects to login") gets a 400 error page instead of a redirect - exactly what the failing CI run showed. Local testing never caught this because the long-lived docker-compose dev database already had TestSeeder's fixtures from past PHPUnit runs. TestSeeder itself is not a safe fix for CI: its run() truncates users/groups/oauth2_client (and otp/consent/session-adjacent tables) before reseeding its own fixed set - correct for PHPUnit's isolated test lifecycle, destructive against the same shared database this workflow also seeds idp:create-super-admin/idp:create-raw-user users into. - app/Console/Commands/CreateOAuth2TestClient.php (idp:create-oauth2-test-client): idempotent, additive-only - creates just the one confidential client (same client_id/secret/redirect_uri the e2e suite already uses) plus a dedicated owner user (the consent screen's getDeveloperEmail() dereferences the owner unconditionally - an ownerless client 500s as soon as a real login reaches /accounts/user/consent) and grants it the 'profile' scope. Registered in app/Console/Kernel.php. - .github/workflows/{pull_request,push}_frontend_tests.yml: run the new command alongside the existing user fixtures. Verified: 16/17 e2e via `docker compose --profile e2e run --rm playwright npx playwright test` (the 17th, trusted-device, is the pre-existing environment-only miss - Secure cookies don't persist over http://nginx), 40/40 PHP, 23/23 Jest. * feat: recovery code management (#146) * feat: recovery code management Signed-off-by: romanetar <roman_ag@hotmail.com> * fix: add missing postRawRequestFull to base_actions.js profile/actions.js imports postRawRequestFull for the new enableTwoFactor and regenerateRecoveryCodes flows, but it was never exported, causing a runtime TypeError on both actions. Falling back to postRawRequest is unsafe here since it copies params into the URL query string, which would leak current_password into access logs. * fix: reject enableTwoFactor when 2FA is already enabled enable2FA() had no already-enrolled guard, so a second POST to /2fa/enable for an enrolled user silently regenerated recovery codes with no password confirmation, bypassing the password-gated rotation flow required by CU-86ba2zp66 and sds/idp-mfa.md §4.10.3. * refactor: move 2FA enrollment orchestration into RecoveryCodeService The transaction plus enable2FA + repository->add + code generation lived in UserApiController, breaking the thin-controllers/fat-services convention and diverging from the regenerateRecoveryCodes path, which already delegates to the service. UserApiController::enableTwoFactor now only validates input and calls RecoveryCodeService::enableTwoFactorAndGenerateCodes. * fix: normalize recovery code server-side before hash check Hash::check() compared the raw submitted code against the dash-less uppercase hash, so the "strip separators + uppercase" contract was only enforced by the login.js client. Any other consumer submitting a code exactly as displayed (XXXX-XXXX) would fail verification on this lockout-critical path. Apply the same normalization in AbstractMFAChallengeStrategy::verifyRecoveryCode() before Hash::check. * feat: warn on low recovery codes after MFA recovery login CU-86ba2zp66 / sds/idp-mfa.md §4.10.3, §4.11 step 5 require a dismissable low-code warning after a successful MFA login, but it was only wired into the profile page - a user who burns codes at login never saw it unless they happened to visit their profile. verify2FARecovery now returns recovery_codes_remaining and the configured low threshold; login.js holds the post-login redirect and shows a dismissable banner when the count is low, before navigating away. The sessionStorage dismissal key is shared with the profile page's RecoveryCodesPanel via a new shared module so dismissing in either place suppresses it everywhere for the rest of the session. * test: cover recovery-code redemption, re-enrollment, and the real request layer Three gaps mapped to the riskiest parts of this PR were unpinned: 1. Nothing proved a code returned as XXXX-XXXX actually redeems through AbstractMFAChallengeStrategy::verifyRecoveryCode() - the hash is of the dash-less string, so the generate->display->redeem contract (including the dash normalization) was untested. 2. enableTwoFactor()'s already-enrolled guard (412) had no regression test. 3. Every JS test mocked profile/actions, so nothing exercised the real request layer - exactly where the missing postRawRequestFull export lived. tests/js/profile/actions.test.js only stubs the transport (superagent) and calls the real enableTwoFactor/regenerateRecoveryCodes; verified it reproduces the original "postRawRequestFull is not a function" TypeError when that export is removed. * fix: fix CI failures from the recovery-code round-trip test and dash normalization 1. testDisplayedRecoveryCodeRedeemsThroughVerifyRecoveryCode called AbstractMFAChallengeStrategy::verifyRecoveryCode() directly, but it takes a PESSIMISTIC_WRITE row lock that requires an open transaction (Doctrine\ORM\TransactionRequiredException in CI). Route it through IAuthService::verifyMFARecoveryCode(), like the real login flow, which wraps the call in a transaction. 2. Several pre-existing test fixtures hashed a "plain" recovery code with a literal "-" baked in (e.g. 'RECOVERY-REUSE-TX-' . uniqid()) and then submitted that same string for verification. The dash normalization added earlier in this PR strips separators from the submitted code before Hash::check(), so a hash made from a dash-containing string can never match its own normalized submission - a real generated code never contains a dash in its raw/hashed form, only in its display formatting. Fixed the 7 affected fixtures across TwoFactorLoginFlowTest and AbstractMFAChallengeStrategyTest to drop the literal dash. * fix: remove nested transaction in enableTwoFactorAndGenerateCodes enableTwoFactorAndGenerateCodes() wrapped enable2FA()/user persist in one transaction() call while also calling generateRecoveryCodes(), which opens its own. DoctrineTransactionService::transaction() closes the entity manager and connection on failure, so an inner failure could tear down the EM out from under the still-running outer transaction. Extracted the shared code-generation logic into a transaction-free regenerateCodesForUser(), so each public method now opens exactly one transaction. * fix: make recovery-code audit logging best-effort Both generateRecoveryCodes() and enableTwoFactorAndGenerateCodes() logged audit events after the codes were already committed and about to be returned to the client. An audit-logging failure there would 500 a response whose side effects already succeeded, and a client retry on that 500 would regenerate and invalidate the codes it was never shown. Wrap both in try/catch + Log::warning, matching the best-effort pattern already used for audit logging in UserController. * fix: use the configured app name in the downloaded recovery-codes file recovery_code_display.js hardcoded "FNTECH" in both the file header and the downloaded filename, which would misbrand any non-FNTECH deployment. Threaded the existing appName prop (already exposed by profile.blade.php as config.appName, sourced from Config::get('app.app_name')) down through ProfilePage -> TwoFactorSection -> RecoveryCodesPanel -> RecoveryCodeModal -> RecoveryCodeDisplay, with an OpenStackID fallback matching the config default. * fix: uppercase uniqid() in recovery-code test fixtures verifyRecoveryCode() uppercases the submitted code (in addition to stripping separators) before Hash::check() - real generated codes are always uppercase alphanumeric. Three fixtures built their "plain" code with a raw uniqid() suffix, which is lowercase hex, so the hash (made from the original mixed-case string) could never match its own normalized submission. Verified standalone with password_hash/ password_verify that the old fixture reproduces the exact CI failure and the fixed one passes. --------- Signed-off-by: romanetar <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com>

…#126) * feat: Add MultiFactor Authentication * Feature | Add AuthService validateCredentials method (#127) * feat: Add AuthService validateCredentials method - test: cover canLogin()=false branch in validateCredentials() unit tests - docs: document known double-query cost in validateCredentials() - fix: use consistent error message in validateCredentials() * chore: lint file app/libs/Auth/AuthService.php * chore: Add PR's requested changes * chore: Add PR's requested changes Add tests changes with suggestion * chore: Fix issues created on rebase * Feature | MFA Challenge Strategy Pattern (Interface, Abstract, Factory, EmailOTP) (#129) * feat: Implement Multi-Factor Authentication challenge strategies and tests * chore: Add PR's requested changes * Feature | Add Device Trust Service (#133) * feat: Add Device Trust Service * Feature | Two-Factor Audit Service (#134) * feat: Two-Factor Audit Service * Feature | MFAGateService (Two-Factor Gate Decision Service) (#135) * feat: MFAGateService (Two-Factor Gate Decision Service) * Feature | UserController MFA Integration, Device Trust Cookie Management, Audit Wiring, and 2FA Rate Limiting (#136) * feat: UserController MFA Integration, Device Trust Cookie Management, Audit Wiring, and 2FA Rate Limiting * chore: Add PR's requested changed * chore: Add PR's requested changes * Add TWO_FACTOR_ENABLED global kill-switch to MFA gate MFAGateService::requiresChallenge() had no master on/off switch, contradicting the SDS idp-mfa.md §10.1 rollout plan, which requires being able to instantly revert to password-only login without a code rollback if something goes wrong post-deploy. config/two_factor.php gains an 'enabled' key (env TWO_FACTOR_ENABLED, default true) checked first in requiresChallenge(), short-circuiting before any per-user or device-trust evaluation. * Route MFA challenge responses through login_strategy, not hardcoded JSON 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. * Clear pending MFA challenge and UI-restoration state on cancelLogin() 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. * Block passwordless MFA bypass; make challengeRequired self-contained Two related fixes to the MFA login flow: 1. Passwordless (flow=otp) login never checked shouldRequire2FA(), so an enforced-2FA user could bypass MFA entirely via emitOTP() + postLogin with flow=otp instead of flow=password (SDS idp-mfa.md §7.4 / Open Question #3 explicitly treats passwordless as single-factor). Now throws AuthenticationException before loginWithOTP(), reusing the existing errorLogin() redirect+flash path - the OTP form still submits as a native form POST, so this needed no new response contract. 2. challengeRequired()'s redirect-based implementations (DefaultLoginStrategy, DisplayResponseUserAgentStrategy) previously ignored the $params they received, silently depending on the caller having already flashed otp_length/otp_lifetime to session - an implicit contract that would silently break for any other caller. Both now flash their own $params (persistent, not one-shot, so it survives repeated refreshes) and set error_code, mirroring what DisplayResponseJsonStrategy already sends native clients in JSON. clearMFAUISessionState() now clears error_code too. The '2fa' flow value moves from a new ILoginStrategy constant to IAuthService::AuthenticationFlowMFA, alongside its siblings AuthenticationFlowPassword/AuthenticationFlowPasswordless - all three are the same session 'flow' enum (already flashed together in the AuthenticationException catch block), so splitting the third value into a different interface would have been inconsistent. New test: OAuth2NativeMFALoginFlowTest gains a non-native (page/popup/ touch) case proving the 302+session-flash contract, alongside the existing native 412+JSON case. TwoFactorLoginFlowTest covers the passwordless-bypass rejection (including that it still reuses errorLogin(), not a new JSON contract) and the error_code flash/clear. * Rate-limit the initial MFA challenge issuance in postLogin() The '2fa.rate' middleware could never gate postLogin()'s initial OTP issuance: its before-phase reads 2fa_pending_user_id from session to know which user to throttle, but that key is only written by issueChallenge() - inside the very request that would need throttling. A user with valid credentials could repeatedly POST to the plain login route and trigger unlimited email-OTP sends, bypassing the 5-per-15-minute resend cap entirely (SDS idp-mfa.md §4.12 explicitly requires the initial issuance to share the same 2fa_rate:resend:{user_id} window as resend()). Extracted the cache-key/window logic that lived only in TwoFactorRateLimitMiddleware into ITwoFactorRateLimitService / TwoFactorRateLimitService (same pattern as DeviceTrustService / TwoFactorAuditService / MFAGateService, registered in TwoFactorServiceProvider), so both the middleware (verify/recovery/resend routes) and UserController::postLogin() (initial issuance, now knows the user id post-validateCredentials()) share one source of truth instead of duplicating cache-key construction. postLogin() checks isRateLimited() before issuing a challenge and calls increment() after a successful issue. The rejection throws AuthenticationException, reusing the existing catch block's errorLogin() redirect+flash path - consistent with challengeRequired() already being redirect-based, since the password form still submits as a native form POST. resend2FA()/verify2FA()/verifyRecoveryCode() stay JSON+429 via the middleware, unaffected, since those are AJAX-only endpoints. New test proves postLogin() and resend() share the same window: after max_otp_requests postLogin() calls, the next one is rejected. * Fix op_browser_state ordering bug in AuthService::loginUser() Investigated the "session fixation" finding from the PR review (SDS idp-mfa.md §9.3 asks for a test proving 2fa_pending_user_id cannot be injected). Traced actual runtime behavior via debug instrumentation before writing a fix, since pattern-matching "no explicit Session::regenerate() call" as a vulnerability turned out to be wrong. Laravel's SessionGuard::login() (invoked via Auth::login(), already called unconditionally at the end of loginUser()) already calls $session->migrate(true) internally - the session-fixation window was already closed by the framework, with no code change needed for that property specifically. An added test asserting this (comparing session ID before/after login) passed identically with or without any fix, proving it was a false positive caused by this test harness resetting the session ID between $this->action() calls regardless of production behavior - that test was written and then discarded rather than kept for false confidence. What IS real, found via the same investigation: PrincipalService::register() (called by loginUser() before this fix) hashes the CURRENT session ID into op_browser_state, used for OIDC Session Management (check-session iframe). Since register() ran BEFORE Auth::login(), its hash was computed from a session ID that Auth::login()'s own migrate(true) was about to invalidate moments later - any relying party polling the check-session iframe would see a value that no longer matched what the server would recompute, incorrectly signaling a session change. Fix: call Auth::login() first, then principal_service->clear()/register() after, so the hash uses the final, stable post-login session ID. No new Session::regenerate() call needed - Auth::login() already provides one. New tests: - AuthServiceLoginUserTest (unit, Mockery-alias facades, same pattern as AuthServiceLogoutTest): asserts the call order directly. - TwoFactorLoginFlowTest::testCompletedMFALoginKeepsOPBrowserStateConsistentWithSessionId (integration): proves op_browser_state matches a freshly-computed hash of the post-login session ID end-to-end through the real MFA verify flow. Confirmed failing against the pre-fix ordering, passing after. * Add test proving OTP redeem rolls back on mid-transaction failure 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. * Make verify2FARecovery audit logging best-effort EventRecoveryUsed was logged unguarded after loginUser() and clearPendingState(), so an audit-sink failure at that point propagated to the outer catch(Exception) and returned a 500 to a user who was already authenticated with an already-burned recovery code — the account's last-resort login path. Mirrors the same best-effort try/catch already applied to verify2FA()'s EventChallengeSucceeded audit call. Adds testRecoveryAuditFailureDoesNotBlockLogin, the recovery-path analogue of testAuditFailureDoesNotBlockLogin, reproducing the 500 before the fix and asserting a 302 + established session after it. * Add real concurrent-connection tests for OTP/recovery-code row locks testOTPCodeRejectsReuseAfterSuccessfulVerification and testRecoveryCodeRejectsReuseAfterTransactionCommit only prove sequential reuse is rejected after a transaction commits. Neither exercises the actual property refreshExclusiveLock() exists for: blocking a second, concurrent request from redeeming the same unredeemed OTP or recovery code while the first request's transaction still holds the row. Adds two tests that open a genuinely independent physical DB connection (verified via differing MySQL CONNECTION_ID()) and prove FOR UPDATE from that connection is blocked (lock wait timeout) while EmailOTPMFAChallengeStrategy/AbstractMFAChallengeStrategy's production refreshExclusiveLock() call holds the row. Verified the assertion is non-vacuous by temporarily disabling the lock call and confirming the test fails as expected, then restoring it. * Guard all MFA audit-log calls against Throwable, not just Exception Best-effort audit logging around the MFA flows only caught Exception, which misses Error subtypes (TypeError, ArgumentCountError, etc.). An Error escaping any of these would still turn a clean response into an uncaught 500 or, worse for the two failure-path calls, drop the error_code the rate-limit middleware keys its failure counter on (TwoFactorRateLimitMiddleware::isFailure() only sees the JSON body of whatever response actually gets returned). Applies the codebase's existing convention for this exact situation (see app/Audit/AuditLoggerFactory.php, TrackRequestMiddleware.php) to all 7 best-effort audit/device-trust sites in this controller: - postLogin(): initial challenge issuance audit log (was unguarded) - verify2FA(): failure-path audit log (was unguarded) - verify2FA(): queueDeviceTrustCookie() call (was catch(Exception)) - verify2FA(): success-path audit log (was catch(Exception)) - verify2FARecovery(): failure-path audit log (was unguarded) - verify2FARecovery(): success-path audit log (was catch(Exception)) - resend2FA(): challenge-reissue audit log (was unguarded) Verified: full Two Factor Authentication Test Suite (83 tests, 241 assertions) passes unchanged. Co-Authored-By: Claude <noreply@anthropic.com> * Honor the global 2FA kill-switch in User::shouldRequire2FA() The passwordless-login guard called shouldRequire2FA() directly, which ignored config('two_factor.enabled'), so an enforced admin stayed blocked from passwordless login even with the kill-switch off (SDS idp-mfa.md rollout, section 10.1). Move the enabled check into shouldRequire2FA() as the single source of truth shared by both the MFA gate and the passwordless guard, and drop the now-redundant check in MFAGateService. * Feature | Add Login UI MFA Flow (#142) * feat: Add Login UI MFA Flow * fix: rename HTMLRender.jsx to .js so webpack can resolve it 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. * fix: revert password submit to native form POST 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. * fix: add missing React import in HTMLRender to prevent ReferenceError 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). * fix: cancel login now invalidates the pending MFA challenge server-side 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). * fix: stop the 2FA verify XHR from following cross-origin OAuth2 redirects 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. * fix: cancel and session-expiry now correctly return to the password screen 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. * fix: clear identity fields from session on MFA cancel/verify-success 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. * fix: invalidate pending MFA challenge when identity chip is cleared 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. * fix: block submitting an expired MFA code 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. * fix: seed the MFA countdown with the remaining OTP lifetime after refresh 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. * feat: add expiry countdown to the passwordless OTP form, dedup shared 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. * fix: surface cancelLogin failures instead of swallowing them in console 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. * feat: add 30s resend cooldown to the passwordless OTP screen 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. * feat: rate-limit the passwordless OTP issuance endpoint server-side 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. * fix: mock EmailOTPMFAChallengeStrategy's new getCreatedAt() call in unit tests CI broke on push: issueChallenge()/resendChallenge() gained a call to $otp->getCreatedAt() in an earlier commit this session (0330c3be, 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. * feat: passwordless OTP screen survives browser refresh 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. * fix: show success snackbar when passwordless OTP code is (re)sent 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. * fix: set Retry-After/X-RateLimit-* headers on 2FA rate-limit 429s 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. * fix: persist identity chip fallback for new passwordless-OTP users 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. * Refactor 2FA rate limiting to use RateLimiter::for() named limiters 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 1167374c (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. * Feat/fe testing infrastructure (#144) * feat: first tests Signed-off-by: romanetar <roman_ag@hotmail.com> * feat: add testing infrastructure for login MFA flow and E2E suite Signed-off-by: romanetar <roman_ag@hotmail.com> * feat: add testing infrastructure for login MFA flow and E2E suite Signed-off-by: romanetar <roman_ag@hotmail.com> * test: isolate login.spec.ts in CI, fix MFA mock route ordering Comment out login-mfa-flow.spec.ts and register.spec.ts so CI runs login.spec.ts alone to verify it now passes without account lockout interference. Also fix the MFA beforeEach mock: fulfill() must run before unroute(), otherwise Playwright auto-resolves the in-flight route on unroute and the later fulfill() throws "Route is already handled" - which was letting the real POST through with a wrong password and locking out test@test.com. * test: re-enable MFA and registration e2e suites login.spec.ts verified green in isolation; re-enable the MFA flow suite (route-ordering fix already applied) and the registration suite now that the account-lockout cascade is gone. Signed-off-by: romanetar <roman_ag@hotmail.com> * fix: align MFA e2e/JS tests with PR #142's native-form-POST mechanism PR #142 reverted the password login step from AJAX back to a native form POST + server redirect/session flow (commit 0eca371c), removing handleAuthenticatePasswordFlow/Ok/Error, authenticateWithPassword, and the MFA_CHALLENGE_REQUIRED constant. The tests added by this branch were written against the old AJAX contract and needed to be realigned. - tests/js/login/login.mfa.test.js: remove the handleAuthenticatePasswordOk describe block - it tested a client-side AJAX handler that no longer exists in login.js. - tests/e2e/tests/auth/login-mfa-flow.spec.ts: - beforeEach no longer mocks the password POST as JSON; it performs a real native login against a real MFA-enforced account, matching how postLogin() actually issues a challenge (redirect + session state). - Each TS-* test now uses its own seeded MFA user (mfa-ts-NNN@test.com) instead of sharing one fixed account - a real challenge issuance counts against two_factor.rate_limit.max_otp_requests, so 8 tests sharing one account exhausted the limit before the suite finished. - Fixed VERIFY_URL/RESEND_URL/RECOVERY_URL/CANCEL_URL glob patterns to end with '**': postRawRequest() appends every param as a query string in addition to the body, so the exact-suffix glob never matched and silently left every route mock inert (requests were hitting the real backend instead). - TS-004/TS-007: resetToPasswordFlow() keeps the verified identity and returns to the password step (authFlow: FLOW.PASSWORD) - it does not clear user_name/user_verified. Both tests asserted the email step was shown instead, contradicting their own titles and the function's name. - TS-002: widened the post-verify assertion timeout - onVerify2FA() always assigns window.location.href on success, so even a same-URL mock response occasionally triggers a real navigation that raced the original 1s timeout. - .github/workflows/{pull_request,push}_frontend_tests.yml: seed the 8 mfa-ts-NNN@test.com accounts alongside the existing test@test.com / e2e@test.com fixtures. - .gitignore: add /test-results/ (Playwright's screenshot/video/trace output directory) - only /tests/e2e/report/ was previously ignored. Verified: 40/40 PHP (TwoFactorLoginFlowTest), 23/23 Jest, 13/13 Playwright e2e, stable across repeated runs via `docker compose --profile e2e run --rm playwright npx playwright test`. * feat: add e2e coverage for the OAuth2 authorization code flow Adds tests/e2e/tests/oauth2/auth-code-flow.spec.ts, exercising the full authorization code grant end to end - including the memento (pending OAuth2 request) surviving a real MFA detour, consent-bypass for a returning user, and MFA-skip for a trusted device: - unauthenticated /oauth2/auth redirects to login (memento serialized). - full flow: real login -> real MFA challenge -> real OTP -> consent screen for the correct client -> Accept -> authorization code -> code exchanged at the token endpoint for a real access_token. - returning user with prior consent: a second /oauth2/auth for the same client+scope skips the consent screen entirely and redirects straight to redirect_uri (InteractiveGrantType::handle()'s has_former_consent + auto_approval branch). - trusted device: checking "Trust this device" during MFA sets the Secure device_trust_token cookie; logging out and logging back in then skips the MFA challenge entirely. Infrastructure needed to drive this for real (no mocks): - app/Console/Commands/GetLatestOtp.php (idp:get-latest-otp {email}): prints the newest not-yet-redeemed OTP for a user, since the mailer queues via Redis and there is no catchable local mailbox to read the code from. Registered in app/Console/Kernel.php. - tests/e2e/utils/otp.ts: reads that OTP from the test runner - directly via `php artisan` when reachable in-process (CI, host dev), or via `docker exec idp-app php artisan ...` when running against the dockerized stack (APP_URL points at nginx). - docker-compose/playwright/Dockerfile + docker-compose.yml: the playwright service now builds this image (adds the Docker CLI on top of the stock Playwright image) and mounts /var/run/docker.sock so the above `docker exec` path works from inside that container. Scoped to the e2e profile only. - The suite works around two config('app.url')-vs-actual-origin mismatches (e.g. app.url=http://localhost but this suite runs against http://nginx in the docker-compose e2e profile - cookies are domain-scoped, so following the server's literal absolute redirect/ form-action URLs client-side would drop the session): verify2FA's redirect_url, the consent form's action, and the password step's postLogin() redirect are all replayed via page.request (shares the page's cookies) instead of trusting the browser/client-side JS to follow them unassisted. - .github/workflows/{pull_request,push}_frontend_tests.yml: seed mfa-oauth2-consent@test.com and mfa-oauth2-trust@test.com alongside the existing mfa-oauth2@test.com fixture. Known environment limitation (not a bug): the trusted-device assertion requires a "potentially trustworthy origin" for the Secure cookie to persist - true for http://localhost (host dev, and CI, which already uses APP_URL=http://localhost:8001) but not for the docker-compose e2e profile's http://nginx, where browsers silently drop the cookie. Verified: 16/17 e2e via `docker compose --profile e2e run --rm playwright npx playwright test` (the trusted-device test is the one expected miss, per the above), 4/4 in tests/e2e/tests/oauth2/ via host (`npx playwright test`), 40/40 PHP (TwoFactorLoginFlowTest), 23/23 Jest. * fix: seed the e2e OAuth2 test client without depending on TestSeeder CI was red: tests/e2e/tests/oauth2/auth-code-flow.spec.ts authorizes against a client_id that only exists as a side effect of database/seeds/TestSeeder.php, which is wired ONLY into PHPUnit's BrowserKitTestCase ($this->seed('TestSeeder')) - never into `php artisan db:seed`, which is all the CI workflow runs. On a genuinely fresh database the client_id never resolves, so InteractiveGrantType::handle() throws InvalidClientException before ever reaching the "redirect to login" branch, and the very first oauth2 test ("unauthenticated request redirects to login") gets a 400 error page instead of a redirect - exactly what the failing CI run showed. Local testing never caught this because the long-lived docker-compose dev database already had TestSeeder's fixtures from past PHPUnit runs. TestSeeder itself is not a safe fix for CI: its run() truncates users/groups/oauth2_client (and otp/consent/session-adjacent tables) before reseeding its own fixed set - correct for PHPUnit's isolated test lifecycle, destructive against the same shared database this workflow also seeds idp:create-super-admin/idp:create-raw-user users into. - app/Console/Commands/CreateOAuth2TestClient.php (idp:create-oauth2-test-client): idempotent, additive-only - creates just the one confidential client (same client_id/secret/redirect_uri the e2e suite already uses) plus a dedicated owner user (the consent screen's getDeveloperEmail() dereferences the owner unconditionally - an ownerless client 500s as soon as a real login reaches /accounts/user/consent) and grants it the 'profile' scope. Registered in app/Console/Kernel.php. - .github/workflows/{pull_request,push}_frontend_tests.yml: run the new command alongside the existing user fixtures. Verified: 16/17 e2e via `docker compose --profile e2e run --rm playwright npx playwright test` (the 17th, trusted-device, is the pre-existing environment-only miss - Secure cookies don't persist over http://nginx), 40/40 PHP, 23/23 Jest. * feat: recovery code management (#146) * feat: recovery code management Signed-off-by: romanetar <roman_ag@hotmail.com> * fix: add missing postRawRequestFull to base_actions.js profile/actions.js imports postRawRequestFull for the new enableTwoFactor and regenerateRecoveryCodes flows, but it was never exported, causing a runtime TypeError on both actions. Falling back to postRawRequest is unsafe here since it copies params into the URL query string, which would leak current_password into access logs. * fix: reject enableTwoFactor when 2FA is already enabled enable2FA() had no already-enrolled guard, so a second POST to /2fa/enable for an enrolled user silently regenerated recovery codes with no password confirmation, bypassing the password-gated rotation flow required by CU-86ba2zp66 and sds/idp-mfa.md §4.10.3. * refactor: move 2FA enrollment orchestration into RecoveryCodeService The transaction plus enable2FA + repository->add + code generation lived in UserApiController, breaking the thin-controllers/fat-services convention and diverging from the regenerateRecoveryCodes path, which already delegates to the service. UserApiController::enableTwoFactor now only validates input and calls RecoveryCodeService::enableTwoFactorAndGenerateCodes. * fix: normalize recovery code server-side before hash check Hash::check() compared the raw submitted code against the dash-less uppercase hash, so the "strip separators + uppercase" contract was only enforced by the login.js client. Any other consumer submitting a code exactly as displayed (XXXX-XXXX) would fail verification on this lockout-critical path. Apply the same normalization in AbstractMFAChallengeStrategy::verifyRecoveryCode() before Hash::check. * feat: warn on low recovery codes after MFA recovery login CU-86ba2zp66 / sds/idp-mfa.md §4.10.3, §4.11 step 5 require a dismissable low-code warning after a successful MFA login, but it was only wired into the profile page - a user who burns codes at login never saw it unless they happened to visit their profile. verify2FARecovery now returns recovery_codes_remaining and the configured low threshold; login.js holds the post-login redirect and shows a dismissable banner when the count is low, before navigating away. The sessionStorage dismissal key is shared with the profile page's RecoveryCodesPanel via a new shared module so dismissing in either place suppresses it everywhere for the rest of the session. * test: cover recovery-code redemption, re-enrollment, and the real request layer Three gaps mapped to the riskiest parts of this PR were unpinned: 1. Nothing proved a code returned as XXXX-XXXX actually redeems through AbstractMFAChallengeStrategy::verifyRecoveryCode() - the hash is of the dash-less string, so the generate->display->redeem contract (including the dash normalization) was untested. 2. enableTwoFactor()'s already-enrolled guard (412) had no regression test. 3. Every JS test mocked profile/actions, so nothing exercised the real request layer - exactly where the missing postRawRequestFull export lived. tests/js/profile/actions.test.js only stubs the transport (superagent) and calls the real enableTwoFactor/regenerateRecoveryCodes; verified it reproduces the original "postRawRequestFull is not a function" TypeError when that export is removed. * fix: fix CI failures from the recovery-code round-trip test and dash normalization 1. testDisplayedRecoveryCodeRedeemsThroughVerifyRecoveryCode called AbstractMFAChallengeStrategy::verifyRecoveryCode() directly, but it takes a PESSIMISTIC_WRITE row lock that requires an open transaction (Doctrine\ORM\TransactionRequiredException in CI). Route it through IAuthService::verifyMFARecoveryCode(), like the real login flow, which wraps the call in a transaction. 2. Several pre-existing test fixtures hashed a "plain" recovery code with a literal "-" baked in (e.g. 'RECOVERY-REUSE-TX-' . uniqid()) and then submitted that same string for verification. The dash normalization added earlier in this PR strips separators from the submitted code before Hash::check(), so a hash made from a dash-containing string can never match its own normalized submission - a real generated code never contains a dash in its raw/hashed form, only in its display formatting. Fixed the 7 affected fixtures across TwoFactorLoginFlowTest and AbstractMFAChallengeStrategyTest to drop the literal dash. * fix: remove nested transaction in enableTwoFactorAndGenerateCodes enableTwoFactorAndGenerateCodes() wrapped enable2FA()/user persist in one transaction() call while also calling generateRecoveryCodes(), which opens its own. DoctrineTransactionService::transaction() closes the entity manager and connection on failure, so an inner failure could tear down the EM out from under the still-running outer transaction. Extracted the shared code-generation logic into a transaction-free regenerateCodesForUser(), so each public method now opens exactly one transaction. * fix: make recovery-code audit logging best-effort Both generateRecoveryCodes() and enableTwoFactorAndGenerateCodes() logged audit events after the codes were already committed and about to be returned to the client. An audit-logging failure there would 500 a response whose side effects already succeeded, and a client retry on that 500 would regenerate and invalidate the codes it was never shown. Wrap both in try/catch + Log::warning, matching the best-effort pattern already used for audit logging in UserController. * fix: use the configured app name in the downloaded recovery-codes file recovery_code_display.js hardcoded "FNTECH" in both the file header and the downloaded filename, which would misbrand any non-FNTECH deployment. Threaded the existing appName prop (already exposed by profile.blade.php as config.appName, sourced from Config::get('app.app_name')) down through ProfilePage -> TwoFactorSection -> RecoveryCodesPanel -> RecoveryCodeModal -> RecoveryCodeDisplay, with an OpenStackID fallback matching the config default. * fix: uppercase uniqid() in recovery-code test fixtures verifyRecoveryCode() uppercases the submitted code (in addition to stripping separators) before Hash::check() - real generated codes are always uppercase alphanumeric. Three fixtures built their "plain" code with a raw uniqid() suffix, which is lowercase hex, so the hash (made from the original mixed-case string) could never match its own normalized submission. Verified standalone with password_hash/ password_verify that the old fixture reproduces the exact CI failure and the fixed one passes. --------- Signed-off-by: romanetar <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com> --------- Signed-off-by: romanetar <roman_ag@hotmail.com> Co-authored-by: smarcet <smarcet@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Román Gutierrez <roman_ag@hotmail.com>

matiasperrone-exo self-assigned this May 21, 2026

matiasperrone-exo marked this pull request as ready for review May 21, 2026 17:32

matiasperrone-exo marked this pull request as draft May 21, 2026 17:33

matiasperrone-exo force-pushed the feat/mfa-device-trust-service branch from 09750ff to 9faa517 Compare May 21, 2026 17:42

matiasperrone-exo force-pushed the feat/mfa-challenge-strategy-pattern branch from d9beb67 to b1ef3fa Compare May 21, 2026 20:04

matiasperrone-exo requested review from caseylocker and smarcet May 21, 2026 20:16

matiasperrone-exo force-pushed the feat/mfa-device-trust-service branch from 9faa517 to 236b644 Compare May 21, 2026 20:19

matiasperrone-exo marked this pull request as ready for review May 21, 2026 20:19

matiasperrone-exo force-pushed the feat/mfa-device-trust-service branch from 236b644 to f1e8af5 Compare May 21, 2026 20:56

matiasperrone-exo force-pushed the feat/mfa-challenge-strategy-pattern branch from b1ef3fa to 2c99a62 Compare May 21, 2026 21:25

matiasperrone-exo force-pushed the feat/mfa-device-trust-service branch from f1e8af5 to e096f94 Compare May 21, 2026 21:25

matiasperrone-exo force-pushed the feat/mfa-device-trust-service branch from e096f94 to 16c4945 Compare May 21, 2026 21:40

matiasperrone-exo force-pushed the feat/mfa-device-trust-service branch from 16c4945 to 4d99419 Compare May 21, 2026 22:05

caseylocker approved these changes May 27, 2026

View reviewed changes

matiasperrone-exo force-pushed the feat/mfa-device-trust-service branch from 4d99419 to 53e7ac9 Compare May 29, 2026 16:33

matiasperrone-exo changed the title ~~Feat | Add Device Trust Service~~ Feature | Add Device Trust Service Jun 1, 2026

matiasperrone-exo force-pushed the feat/mfa-challenge-strategy-pattern branch from 5cb18c2 to 2c99a62 Compare June 1, 2026 15:21

matiasperrone-exo force-pushed the feat/mfa-device-trust-service branch from 53e7ac9 to 4fd7655 Compare June 1, 2026 15:23

matiasperrone-exo force-pushed the feat/mfa-challenge-strategy-pattern branch from 2c99a62 to 43c31bb Compare June 8, 2026 20:01

feat: Add Device Trust Service

39a7e4a

matiasperrone-exo force-pushed the feat/mfa-device-trust-service branch from 4fd7655 to 39a7e4a Compare June 8, 2026 20:48

matiasperrone-exo assigned caseylocker and unassigned matiasperrone-exo Jun 24, 2026

romanetar merged commit c3d4e70 into feat/mfa-challenge-strategy-pattern Aug 11, 2026
6 checks passed

Uh oh!

Conversation

matiasperrone-exo commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Task:

Blocked by:

Changes:

New service layer (app/Services/Auth/)

Repository changes (IUserTrustedDeviceRepository / DoctrineUserTrustedDeviceRepository)

Entity updates (UserTrustedDevice)

Config

Tests (tests/DeviceTrustServiceTest.php)

Requested GOAL

Current state

Target state

TASKS

ACCEPTANCE CRITERIA

Uh oh!

coderabbitai Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

caseylocker left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 29, 2026

Uh oh!

github-actions Bot commented Jun 1, 2026

Uh oh!

github-actions Bot commented Jun 8, 2026

Uh oh!

Uh oh!

github-actions Bot commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matiasperrone-exo commented May 21, 2026 •

edited

Loading

New service layer (`app/Services/Auth/`)

Repository changes (`IUserTrustedDeviceRepository` / `DoctrineUserTrustedDeviceRepository`)

Entity updates (`UserTrustedDevice`)

Tests (`tests/DeviceTrustServiceTest.php`)

coderabbitai Bot commented May 21, 2026 •

edited

Loading