-
Notifications
You must be signed in to change notification settings - Fork 0
Feature | MFA Challenge Strategy Pattern (Interface, Abstract, Factory, EmailOTP) #129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/mfa-phase1-authservice-validatecredentials-method
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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); | ||
| 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 []; | ||
| } | ||
| } | ||
| 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); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method does not actually verify the OTP.
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 (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); | ||
| } | ||
| } | ||
| 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; | ||
| } |
| 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}"), | ||
| }; | ||
| } | ||
| } |
| 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'); | ||
| } | ||
| } |
There was a problem hiding this comment.
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 immediatepersist() + flush()(seeDoctrineRepository::add). This strategy has no transaction boundary — its constructor injects onlyIUserRecoveryCodeRepository, noITransactionService. Everywhere else in the auth layer, mutations run insidetx_service->transaction()and are flushed once at commit; no repository self-flushes withsync = true(cf.AuthService::finalizeRedemptioninapp/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 rawEntityManager::flush()and should be corrected too.