Skip to content

Commit 1e688cb

Browse files
committed
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.
1 parent 89e058c commit 1e688cb

3 files changed

Lines changed: 21 additions & 8 deletions

File tree

tests/RecoveryCodeRegenerationTest.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use Illuminate\Support\Facades\Session;
2020
use LaravelDoctrine\ORM\Facades\EntityManager;
2121
use Strategies\MFA\MFAChallengeStrategyFactory;
22+
use Utils\Services\IAuthService;
2223

2324
/**
2425
* Integration tests for regenerating recovery codes from the user profile
@@ -141,8 +142,11 @@ public function testDisplayedRecoveryCodeRedeemsThroughVerifyRecoveryCode(): voi
141142

142143
// The hash was generated over the dash-less string; redeeming the code
143144
// exactly as it was displayed (with its "-" separator) must still work.
145+
// Goes through IAuthService, like the real login flow, because
146+
// verifyRecoveryCode() takes a PESSIMISTIC_WRITE row lock that requires
147+
// an open transaction.
144148
$strategy = MFAChallengeStrategyFactory::create(User::MFAMethod_OTP);
145-
$strategy->verifyRecoveryCode($admin, $displayedCode);
149+
app(IAuthService::class)->verifyMFARecoveryCode($admin, $strategy, $displayedCode);
146150

147151
EntityManager::clear();
148152
$unusedAfter = EntityManager::getRepository(UserRecoveryCode::class)

tests/TwoFactorLoginFlowTest.php

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,10 @@ public function testOTPCodeRejectsReuseAfterSuccessfulVerification(): void
482482
public function testRecoveryCodeRejectsReuseAfterTransactionCommit(): void
483483
{
484484
$admin = $this->user(self::ADMIN_EMAIL);
485-
$plain = 'RECOVERY-REUSE-TX-' . uniqid();
485+
// No "-" in the fixture: verifyRecoveryCode() now strips separators before
486+
// Hash::check() (real codes are hashed dash-less; the dash is display-only),
487+
// so a fixture hashed with one baked in would never match its own submission.
488+
$plain = 'RECOVERYREUSETX' . uniqid();
486489
$this->createRecoveryCode($admin, $plain, false);
487490

488491
// First use — must succeed.
@@ -603,7 +606,7 @@ public function testOTPRedeemRowLockBlocksConcurrentConnection(): void
603606
public function testRecoveryCodeRowLockBlocksConcurrentConnection(): void
604607
{
605608
$admin = $this->user(self::ADMIN_EMAIL);
606-
$plain = 'RECOVERY-LOCK-' . uniqid();
609+
$plain = 'RECOVERYLOCK' . uniqid();
607610
$codeId = $this->createRecoveryCode($admin, $plain, false);
608611

609612
/** @var IUserRecoveryCodeRepository $recoveryRepo */
@@ -783,7 +786,7 @@ public function testRecoveryAuditFailureDoesNotBlockLogin(): void
783786
// Audit is best-effort: a failure emitting recovery_used must NOT 500 a
784787
// user whose recovery code is already burned and session established.
785788
$admin = $this->user(self::ADMIN_EMAIL);
786-
$plain = 'RECOVERY-AUDIT-FAIL-' . uniqid();
789+
$plain = 'RECOVERYAUDITFAIL' . uniqid();
787790
$this->createRecoveryCode($admin, $plain, false);
788791

789792
$auditMock = \Mockery::mock(ITwoFactorAuditService::class);
@@ -836,7 +839,7 @@ public function testDeviceTrustFailureDoesNotBlockLogin(): void
836839
public function testRecoveryCodeLoginSucceeds(): void
837840
{
838841
$admin = $this->user(self::ADMIN_EMAIL);
839-
$plain = 'RECOVERY-PLAIN-123';
842+
$plain = 'RECOVERYPLAIN123';
840843
$codeId = $this->createRecoveryCode($admin, $plain, false);
841844

842845
$this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD);
@@ -856,7 +859,7 @@ public function testRecoveryCodeLoginSucceeds(): void
856859
public function testUsedRecoveryCodeFails(): void
857860
{
858861
$admin = $this->user(self::ADMIN_EMAIL);
859-
$plain = 'RECOVERY-USED-456';
862+
$plain = 'RECOVERYUSED456';
860863
$this->createRecoveryCode($admin, $plain, true); // already used
861864

862865
$this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD);

tests/unit/MFA/AbstractMFAChallengeStrategyTest.php

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,10 @@ public function testClearPendingState_removesAllSessionKeys(): void
9595
public function testVerifyRecoveryCode_withMatchingCode_marksAsUsed(): void
9696
{
9797
$user = new User();
98-
$code = 'VALID-CODE';
98+
// No "-" in the fixture: verifyRecoveryCode() strips separators before
99+
// Hash::check() (real codes are hashed dash-less), so a fixture hashed
100+
// with one baked in would never match its own submission.
101+
$code = 'VALIDCODE';
99102

100103
$recoveryCode = \Mockery::mock(\App\libs\Auth\Models\UserRecoveryCode::class);
101104
$recoveryCode->shouldReceive('getCodeHash')->andReturn(Hash::make($code));
@@ -120,7 +123,10 @@ public function resendChallenge(User $user, ?Client $client, bool $remember): ar
120123
public function testVerifyRecoveryCode_locksAndRejects_whenUsedAfterLock(): void
121124
{
122125
$user = new User();
123-
$code = 'VALID-CODE';
126+
// No "-" in the fixture: verifyRecoveryCode() strips separators before
127+
// Hash::check() (real codes are hashed dash-less), so a fixture hashed
128+
// with one baked in would never match its own submission.
129+
$code = 'VALIDCODE';
124130

125131
// Hash matches, but a concurrent winner marked it used; the lock+recheck
126132
// must reject without double-spending (regression guard for 3357348455).

0 commit comments

Comments
 (0)