Skip to content

Commit 2b00f80

Browse files
smarcetsmarcet
committed
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.
1 parent d2bffb8 commit 2b00f80

9 files changed

Lines changed: 209 additions & 5 deletions

‎app/Http/Controllers/UserController.php‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -490,10 +490,20 @@ public function postLogin()
490490
IPHelper::getUserIp()
491491
);
492492

493-
return Response::json(
494-
array_merge(['error_code' => 'mfa_required'], $payload),
495-
HttpResponse::HTTP_OK
496-
);
493+
// Restore-on-refresh: a subsequent GET /login can rehydrate
494+
// the 2FA screen from session instead of dropping back to the
495+
// password form (mirrors what a redirect-based flow gets for
496+
// free via session-flashed params).
497+
Session::put('flow', '2fa');
498+
Session::put('mfa_method', $method);
499+
if (isset($payload['otp_length'])) {
500+
Session::put('otp_length', $payload['otp_length']);
501+
}
502+
if (isset($payload['otp_lifetime'])) {
503+
Session::put('otp_lifetime', $payload['otp_lifetime']);
504+
}
505+
506+
return $this->login_strategy->challengeRequired($payload);
497507
}
498508

499509
// No challenge required: establish the session and continue.
@@ -710,6 +720,7 @@ public function verify2FA()
710720
}
711721

712722
$strategy->clearPendingState();
723+
$this->clearMFAUISessionState();
713724

714725
try {
715726
$this->two_factor_audit_service->log(
@@ -784,6 +795,7 @@ public function verify2FARecovery()
784795

785796
$this->auth_service->loginUser($user, (bool) $pending['remember']);
786797
$strategy->clearPendingState();
798+
$this->clearMFAUISessionState();
787799

788800
$this->two_factor_audit_service->log(
789801
$user,
@@ -835,6 +847,17 @@ public function resend2FA()
835847

836848
$payload = $this->auth_service->resendMFAChallenge($user, $strategy, $this->resolveClientFromMemento(), (bool) $pending['remember']);
837849

850+
// Keep the refresh-restorable session state in sync with the
851+
// fresh challenge (e.g. otp_lifetime countdown resets on resend,
852+
// mfa_method changes if this resend is actually a method switch).
853+
Session::put('mfa_method', $method);
854+
if (isset($payload['otp_length'])) {
855+
Session::put('otp_length', $payload['otp_length']);
856+
}
857+
if (isset($payload['otp_lifetime'])) {
858+
Session::put('otp_lifetime', $payload['otp_lifetime']);
859+
}
860+
838861
$this->two_factor_audit_service->log(
839862
$user,
840863
TwoFactorAuditLog::EventChallengeIssued,
@@ -857,9 +880,26 @@ public function resend2FA()
857880
*/
858881
private function mfaSessionExpired()
859882
{
883+
$this->clearMFAUISessionState();
860884
return Response::json(['error_code' => 'mfa_session_expired'], HttpResponse::HTTP_UNAUTHORIZED);
861885
}
862886

887+
/**
888+
* Clears the UI-restoration session keys written when a challenge is
889+
* issued (see postLogin()'s mfa_required branch). Companion to
890+
* IMFAChallengeStrategy::clearPendingState(), which only owns the
891+
* 2fa_* pending-state keys.
892+
*
893+
* @return void
894+
*/
895+
private function clearMFAUISessionState(): void
896+
{
897+
Session::forget('flow');
898+
Session::forget('mfa_method');
899+
Session::forget('otp_length');
900+
Session::forget('otp_lifetime');
901+
}
902+
863903
/**
864904
* @return \Illuminate\Http\Response|mixed
865905
*/

‎app/Strategies/DefaultLoginStrategy.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@
2121
use Utils\Services\IAuthService;
2222
use Illuminate\Support\Facades\Auth;
2323
use Illuminate\Support\Facades\Redirect;
24+
use Illuminate\Support\Facades\Response;
2425
use Illuminate\Support\Facades\View;
2526
use Illuminate\Support\Facades\URL;
27+
use Symfony\Component\HttpFoundation\Response as HttpResponse;
2628
/**
2729
* Class DefaultLoginStrategy
2830
* @package Strategies
@@ -113,4 +115,16 @@ public function errorLogin(array $params)
113115
$response = $response->with($key, $val);
114116
return $response;
115117
}
118+
119+
/**
120+
* @param array $params
121+
* @return mixed
122+
*/
123+
public function challengeRequired(array $params)
124+
{
125+
return Response::json(
126+
array_merge(['error_code' => ILoginStrategy::MFA_REQUIRED], $params),
127+
HttpResponse::HTTP_OK
128+
);
129+
}
116130
}

‎app/Strategies/DisplayResponseJsonStrategy.php‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,4 +96,13 @@ public function getLoginErrorResponse(array $data = [])
9696
}
9797
return Response::json($data, 412);
9898
}
99+
100+
/**
101+
* @param array $data
102+
* @return SymfonyResponse
103+
*/
104+
public function getChallengeRequiredResponse(array $data = [])
105+
{
106+
return Response::json(array_merge(['error_code' => ILoginStrategy::MFA_REQUIRED], $data), 412);
107+
}
99108
}

‎app/Strategies/DisplayResponseUserAgentStrategy.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,4 +72,17 @@ public function getLoginErrorResponse(array $data = [])
7272

7373
return $response;
7474
}
75+
76+
/**
77+
* Same live-JSON, no-reload contract as DefaultLoginStrategy: OAuth2
78+
* page/popup/touch flows render the same login.js SPA, so the MFA
79+
* challenge transitions in-place rather than doing a full page reload.
80+
*
81+
* @param array $data
82+
* @return SymfonyResponse
83+
*/
84+
public function getChallengeRequiredResponse(array $data = [])
85+
{
86+
return Response::json(array_merge(['error_code' => ILoginStrategy::MFA_REQUIRED], $data), 200);
87+
}
7588
}

‎app/Strategies/IDisplayResponseStrategy.php‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,4 +34,13 @@ public function getLoginResponse(array $data = []);
3434
* @return SymfonyResponse
3535
*/
3636
public function getLoginErrorResponse(array $data = []);
37+
38+
/**
39+
* Factor 1 (password) passed but a 2FA challenge must be completed before
40+
* a session is established.
41+
*
42+
* @param array $data
43+
* @return SymfonyResponse
44+
*/
45+
public function getChallengeRequiredResponse(array $data = []);
3746
}

‎app/Strategies/ILoginStrategy.php‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,12 @@
55
*/
66
interface ILoginStrategy
77
{
8+
/**
9+
* error_code returned by challengeRequired() when factor 1 passed but a
10+
* 2FA challenge is pending.
11+
*/
12+
const MFA_REQUIRED = 'mfa_required';
13+
814
/**
915
* @return mixed
1016
*/
@@ -26,4 +32,14 @@ public function cancelLogin();
2632
* @return mixed
2733
*/
2834
public function errorLogin(array $params);
29-
}
35+
36+
/**
37+
* Factor 1 (password) passed but a 2FA challenge must be completed before
38+
* a session is established. Distinct from errorLogin(): this is a pending
39+
* mid-flow state, not a failed attempt.
40+
*
41+
* @param array $params
42+
* @return mixed
43+
*/
44+
public function challengeRequired(array $params);
45+
}

‎app/Strategies/OAuth2LoginStrategy.php‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,4 +127,21 @@ public function errorLogin(array $params)
127127

128128
return $response_strategy->getLoginErrorResponse($params);
129129
}
130+
131+
/**
132+
* @param array $params
133+
* @return mixed
134+
*/
135+
public function challengeRequired(array $params)
136+
{
137+
$auth_request = OAuth2AuthorizationRequestFactory::getInstance()->build(
138+
OAuth2Message::buildFromMemento(
139+
$this->memento_service->load()
140+
)
141+
);
142+
143+
$response_strategy = DisplayResponseStrategyFactory::build($auth_request->getDisplay());
144+
145+
return $response_strategy->getChallengeRequiredResponse($params);
146+
}
130147
}

‎tests/OAuth2NativeMFALoginFlowTest.php‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
<?php namespace Tests;
2+
/**
3+
* Copyright 2026 OpenStack Foundation
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
**/
14+
15+
use Illuminate\Support\Facades\Config;
16+
use Illuminate\Support\Facades\Session;
17+
18+
/**
19+
* Verifies that an OAuth2 native client (display=native) requesting login
20+
* against an enforced-2FA account gets the SAME JSON+412 contract as every
21+
* other login error for that client, rather than the plain-flow's JSON+200.
22+
*
23+
* @package Tests
24+
*/
25+
final class OAuth2NativeMFALoginFlowTest extends OpenStackIDBaseTestCase
26+
{
27+
private const ADMIN_EMAIL = 'sebastian@tipit.net';
28+
private const SEED_PASSWORD = '1Qaz2wsx!';
29+
private const CLIENT_ID = '.-_~87D8/Vcvr6fvQbH4HyNgwTlfSyQ3x.openstack.client';
30+
31+
protected function prepareForTests(): void
32+
{
33+
parent::prepareForTests();
34+
Session::start();
35+
}
36+
37+
public function testNativeClientReceives412OnMFARequired(): void
38+
{
39+
// Step 1: unauthenticated authorize request with display=native - the
40+
// grant serializes the OAuth2 memento and hands off to the login flow.
41+
$this->action('POST', 'OAuth2\OAuth2ProviderController@auth', [
42+
'client_id' => self::CLIENT_ID,
43+
'redirect_uri' => 'https://www.test.com:443/oauth2?param=1&BackUrl=123344',
44+
'response_type' => 'code',
45+
'scope' => sprintf('%s/resource-server/read', Config::get('app.url')),
46+
'display' => 'native',
47+
]);
48+
49+
// Step 2: submit the enforced-2FA admin's password within the same
50+
// session - this is what postLogin() sees as an OAuth2-originated
51+
// login attempt via the persisted memento.
52+
$response = $this->action('POST', 'UserController@postLogin', [
53+
'username' => self::ADMIN_EMAIL,
54+
'password' => self::SEED_PASSWORD,
55+
'flow' => 'password',
56+
'_token' => Session::token(),
57+
]);
58+
59+
$this->assertResponseStatus(412);
60+
$payload = json_decode($response->getContent(), true);
61+
$this->assertSame('mfa_required', $payload['error_code']);
62+
}
63+
}

‎tests/TwoFactorLoginFlowTest.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,29 @@ public function testAdminLoginTriggersMFAChallenge(): void
8080
$this->assertGreaterThan(0, $this->countAudit($admin->getId(), TwoFactorAuditLog::EventChallengeIssued));
8181
}
8282

83+
public function testAdminLoginPersistsUIStateForRefreshResilience(): void
84+
{
85+
$this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD);
86+
87+
$this->assertSame('2fa', Session::get('flow'), 'a refresh mid-challenge must restore the 2FA screen, not the password form');
88+
$this->assertNotNull(Session::get('otp_length'));
89+
$this->assertNotNull(Session::get('otp_lifetime'));
90+
$this->assertSame(User::MFAMethod_OTP, Session::get('mfa_method'), 'a refresh must restore the screen for the method actually challenged, not a hardcoded default');
91+
}
92+
93+
public function testSuccessfulVerificationClearsUIState(): void
94+
{
95+
$this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD);
96+
$code = $this->latestOtpCode(self::ADMIN_EMAIL);
97+
98+
$this->verify($code);
99+
100+
$this->assertNull(Session::get('flow'), 'completed challenge must not leave the 2FA screen re-derivable from a stale refresh');
101+
$this->assertNull(Session::get('otp_length'));
102+
$this->assertNull(Session::get('otp_lifetime'));
103+
$this->assertNull(Session::get('mfa_method'));
104+
}
105+
83106
public function testNonAdminWithoutMFALogsInNormally(): void
84107
{
85108
$email = $this->createPlainUser();

0 commit comments

Comments
 (0)