Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions app/Strategies/MFA/AbstractMFAChallengeStrategy.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<?php namespace Strategies\MFA;

use Auth\Exceptions\AuthenticationException;
use Auth\Repositories\IUserRecoveryCodeRepository;
use Auth\User;
use Illuminate\Support\Facades\Hash;
use Illuminate\Support\Facades\Session;
use Models\OAuth2\Client;

abstract class AbstractMFAChallengeStrategy implements IMFAChallengeStrategy
{
private const SESSION_TTL = 300;
private const KEY_USER_ID = '2fa_pending_user_id';
private const KEY_PENDING_AT = '2fa_pending_at';
private const KEY_REMEMBER = '2fa_remember';
private const KEY_RECOVERY_ATTEMPTS = '2fa_recovery_attempts';

public function __construct(protected IUserRecoveryCodeRepository $recovery_code_repository) {}

public function getPendingState(): ?array
{
$user_id = Session::get(self::KEY_USER_ID);
$pending_at = Session::get(self::KEY_PENDING_AT);

if (is_null($user_id) || is_null($pending_at)) {
return null;
}

if ((time() - $pending_at) > self::SESSION_TTL) {
$this->clearPendingState();
return null;
}

return [
'user_id' => $user_id,
'pending_at' => $pending_at,
'remember' => Session::get(self::KEY_REMEMBER, false),
];
}

public function clearPendingState(): void
{
Session::remove(self::KEY_USER_ID);
Session::remove(self::KEY_PENDING_AT);
Session::remove(self::KEY_REMEMBER);
Session::remove(self::KEY_RECOVERY_ATTEMPTS);
}

public function verifyRecoveryCode(User $user, string $code): void
{
foreach ($this->recovery_code_repository->getUnusedByUser($user) as $recoveryCode) {
if (Hash::check($code, $recoveryCode->getCodeHash())) {
$recoveryCode->markUsed();
$this->recovery_code_repository->add($recoveryCode, true);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add($recoveryCode, true) forces an immediate persist() + flush() (see DoctrineRepository::add). This strategy has no transaction boundary — its constructor injects only IUserRecoveryCodeRepository, no ITransactionService. Everywhere else in the auth layer, mutations run inside tx_service->transaction() and are flushed once at commit; no repository self-flushes with sync = true (cf. AuthService::finalizeRedemption in app/libs/Auth/AuthService.php).

Per the SDS this method is invoked from the controller (UserController::verify2FARecovery, §4.11 + route §4.13), and controllers here do not own transactions — services do. So a controller-invoked strategy self-flushing a write bypasses the service/transaction layer every other auth write goes through. It is also unsafe to compose inside an outer transaction (premature mid-tx flush) and is excluded from the row-locking / atomicity the OTP redemption path relies on.

Fix: move the mark-used write into a transaction-managed service method and use add(..., false). The SDS snippet here uses raw EntityManager::flush() and should be corrected too.

return;
}
}
throw new AuthenticationException("Invalid recovery code.");
}

protected function storePendingState(int $userId, bool $remember): void
{
Session::put(self::KEY_USER_ID, $userId);
Session::put(self::KEY_PENDING_AT, time());
Session::put(self::KEY_REMEMBER, $remember);
}

public function verifyChallenge(User $user, string $code, ?Client $client = null): void
{
}

public function issueChallenge(User $user, ?Client $client, bool $remember): array
{
return [];
}
}
69 changes: 69 additions & 0 deletions app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?php namespace Strategies\MFA;

use App\libs\OAuth2\Repositories\IOAuth2OTPRepository;
use Auth\Exceptions\AuthenticationException;
use Auth\Repositories\IUserRecoveryCodeRepository;
use Auth\User;
use Models\OAuth2\Client;
use Models\OAuth2\OAuth2OTP;
use OAuth2\OAuth2Protocol;
use OAuth2\Services\ITokenService;

final class EmailOTPMFAChallengeStrategy extends AbstractMFAChallengeStrategy
{
public function __construct(
IUserRecoveryCodeRepository $recovery_code_repository,
private readonly ITokenService $token_service,
private readonly IOAuth2OTPRepository $otp_repository,
) {
parent::__construct($recovery_code_repository);
}

public function issueChallenge(User $user, ?Client $client, bool $remember): array
{
$this->storePendingState($user->getId(), $remember);

$otp = $this->token_service->createOTPFromPayload([
OAuth2Protocol::OAuth2PasswordlessConnection => OAuth2Protocol::OAuth2PasswordlessConnectionEmail,
OAuth2Protocol::OAuth2PasswordlessSend => OAuth2Protocol::OAuth2PasswordlessSendCode,
OAuth2Protocol::OAuth2PasswordlessEmail => $user->getEmail(),
], $client);

return [
'otp_length' => $otp->getLength(),
'otp_lifetime' => $otp->getLifetime(),
];
}

public function verifyChallenge(User $user, string $code, ?Client $client = null): void
{
$otp = OAuth2OTP::fromParams($user->getEmail(), OAuth2Protocol::OAuth2PasswordlessConnectionEmail, $code);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method does not actually verify the OTP. OAuth2OTP::fromParams() is a pure in-memory factory (new self(...), app/Models/OAuth2/OAuth2OTP.php:448) — it never reads the DB. The code never calls otp_repository->getByValueConnectionAndUserName(...) and never compares the submitted value against the stored OTP. Consequences:

  • is_null($otp) (L42) is dead code — fromParams always returns an object.
  • isAlive() / isValid() (L48–53) run against a freshly-built object with default state, so they effectively always pass.
  • $otp->redeem() (L56) and the sibling revoke mutate transient entities that are never persisted (no tx_service, no add, no flush).

Net effect if wired as-is: the submitted code is not validated against what was issued, and nothing is persisted. SDS §4.5 fetches the persisted OTP via the repository and does if ($otp->getValue() != $value) throw — both were dropped here. Recommend delegating to the existing AuthService::verifyOTPChallenge() (transaction-managed, value-checked, row-locked, session-user bound) instead of re-implementing redemption.


if (is_null($otp)) {
throw new AuthenticationException("Non existent single-use code.");
}

$otp->logRedeemAttempt();

if (!$otp->isAlive()) {
throw new AuthenticationException("Verification code is expired.");
}

if (!$otp->isValid()) {
throw new AuthenticationException("Verification code is not valid.");
}

$otp->redeem();

foreach ($this->otp_repository->getByUserNameNotRedeemed($user->getEmail()) as $otpToRevoke) {
if ($otpToRevoke->getValue() !== $otp->getValue()) {
$otpToRevoke->redeem();
}
}
}

public function resendChallenge(User $user, ?Client $client, bool $remember): array
{
return $this->issueChallenge($user, $client, $remember);
}
}
14 changes: 14 additions & 0 deletions app/Strategies/MFA/IMFAChallengeStrategy.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?php namespace Strategies\MFA;

use Auth\User;
use Models\OAuth2\Client;

interface IMFAChallengeStrategy
{
public function issueChallenge(User $user, ?Client $client, bool $remember): array;
public function verifyChallenge(User $user, string $code, ?Client $client = null): void;
public function resendChallenge(User $user, ?Client $client, bool $remember): array;
public function getPendingState(): ?array;
public function clearPendingState(): void;
public function verifyRecoveryCode(User $user, string $code): void;
}
12 changes: 12 additions & 0 deletions app/Strategies/MFA/MFAChallengeStrategyFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php namespace Strategies\MFA;

final class MFAChallengeStrategyFactory
{
public static function create(string $method): IMFAChallengeStrategy
{
return match($method) {
'email_otp' => app()->make(EmailOTPMFAChallengeStrategy::class),
default => throw new \InvalidArgumentException("Unknown MFA method: {$method}"),
};
}
}
3 changes: 3 additions & 0 deletions phpunit.xml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
<testsuite name="Two Factor Authentication Test Suite">
<file>./tests/TwoFactorRepositoriesTest.php</file>
<file>./tests/unit/UserTwoFactorTest.php</file>
<file>./tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php</file>
<file>./tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php</file>
<file>./tests/Unit/MFA/MFAChallengeStrategyFactoryTest.php</file>
</testsuite>
</testsuites>
<php>
Expand Down
144 changes: 144 additions & 0 deletions tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
<?php namespace Tests\Unit\MFA;

use Auth\Exceptions\AuthenticationException;
use Auth\Repositories\IUserRecoveryCodeRepository;
use Auth\User;
use Illuminate\Support\Facades\Hash;
use Illuminate\Support\Facades\Session;
use Models\OAuth2\Client;
use Strategies\MFA\AbstractMFAChallengeStrategy;
use Tests\TestCase;

class AbstractMFAChallengeStrategyTest extends TestCase
{
private AbstractMFAChallengeStrategy $strategy;

protected function setUp(): void
{
parent::setUp();
$repo = \Mockery::mock(IUserRecoveryCodeRepository::class);
$this->strategy = new class($repo) extends AbstractMFAChallengeStrategy {
public function issueChallenge(User $user, ?Client $client, bool $remember): array { return []; }
public function verifyChallenge(User $user, string $code, ?Client $client = null): void {}
public function resendChallenge(User $user, ?Client $client, bool $remember): array { return []; }
public function exposeStorePendingState(int $userId, bool $remember): void {
$this->storePendingState($userId, $remember);
}
};
}

protected function tearDown(): void
{
\Mockery::close();
parent::tearDown();
}

public function testGetPendingState_withValidSession_returnsState(): void
{
$this->strategy->exposeStorePendingState(42, true);

$state = $this->strategy->getPendingState();

$this->assertNotNull($state);
$this->assertSame(42, $state['user_id']);
$this->assertTrue($state['remember']);
$this->assertArrayHasKey('pending_at', $state);
}

public function testGetPendingState_withExpiredSession_returnsNull(): void
{
Session::put('2fa_pending_user_id', 99);
Session::put('2fa_pending_at', time() - 301);
Session::put('2fa_remember', false);

$state = $this->strategy->getPendingState();

$this->assertNull($state);
$this->assertNull(Session::get('2fa_pending_user_id'));
}

public function testGetPendingState_withMissingSession_returnsNull(): void
{
$state = $this->strategy->getPendingState();

$this->assertNull($state);
}

public function testClearPendingState_removesAllSessionKeys(): void
{
Session::put('2fa_pending_user_id', 7);
Session::put('2fa_pending_at', time());
Session::put('2fa_remember', true);
Session::put('2fa_recovery_attempts', 1);

$this->strategy->clearPendingState();

$this->assertNull(Session::get('2fa_pending_user_id'));
$this->assertNull(Session::get('2fa_pending_at'));
$this->assertNull(Session::get('2fa_remember'));
$this->assertNull(Session::get('2fa_recovery_attempts'));
}

public function testVerifyRecoveryCode_withMatchingCode_marksAsUsed(): void
{
$user = new User();
$code = 'VALID-CODE';

$recoveryCode = \Mockery::mock(\App\libs\Auth\Models\UserRecoveryCode::class);
$recoveryCode->shouldReceive('getCodeHash')->andReturn(Hash::make($code));
$recoveryCode->shouldReceive('markUsed')->once();

$repo = \Mockery::mock(IUserRecoveryCodeRepository::class);
$repo->shouldReceive('getUnusedByUser')->with($user)->andReturn([$recoveryCode]);
$repo->shouldReceive('add')->with($recoveryCode, true)->once();

$strategy = new class($repo) extends AbstractMFAChallengeStrategy {
public function issueChallenge(User $user, ?Client $client, bool $remember): array { return []; }
public function verifyChallenge(User $user, string $code, ?Client $client = null): void {}
public function resendChallenge(User $user, ?Client $client, bool $remember): array { return []; }
};

$strategy->verifyRecoveryCode($user, $code);
$this->addToAssertionCount(1); // markUsed()->once() verified by Mockery in tearDown
}

public function testVerifyRecoveryCode_withNonMatchingCode_throwsException(): void
{
$user = new User();

$recoveryCode = \Mockery::mock(\App\libs\Auth\Models\UserRecoveryCode::class);
$recoveryCode->shouldReceive('getCodeHash')->andReturn(Hash::make('CORRECT-CODE'));
$recoveryCode->shouldNotReceive('markUsed');

$repo = \Mockery::mock(IUserRecoveryCodeRepository::class);
$repo->shouldReceive('getUnusedByUser')->andReturn([$recoveryCode]);

$strategy = new class($repo) extends AbstractMFAChallengeStrategy {
public function issueChallenge(User $user, ?Client $client, bool $remember): array { return []; }
public function verifyChallenge(User $user, string $code, ?Client $client = null): void {}
public function resendChallenge(User $user, ?Client $client, bool $remember): array { return []; }
};

$this->expectException(AuthenticationException::class);
$this->expectExceptionMessage("Invalid recovery code.");
$strategy->verifyRecoveryCode($user, 'WRONG-CODE');
}

public function testVerifyRecoveryCode_withAllCodesUsed_throwsException(): void
{
$user = new User();

$repo = \Mockery::mock(IUserRecoveryCodeRepository::class);
$repo->shouldReceive('getUnusedByUser')->andReturn([]);

$strategy = new class($repo) extends AbstractMFAChallengeStrategy {
public function issueChallenge(User $user, ?Client $client, bool $remember): array { return []; }
public function verifyChallenge(User $user, string $code, ?Client $client = null): void {}
public function resendChallenge(User $user, ?Client $client, bool $remember): array { return []; }
};

$this->expectException(AuthenticationException::class);
$this->expectExceptionMessage("Invalid recovery code.");
$strategy->verifyRecoveryCode($user, 'ANY-CODE');
}
}
Loading
Loading