diff --git a/app/Modules/Identity/TwoFactor/TwoFactorService.php b/app/Modules/Identity/TwoFactor/TwoFactorService.php index 073869ac..b97f89a4 100644 --- a/app/Modules/Identity/TwoFactor/TwoFactorService.php +++ b/app/Modules/Identity/TwoFactor/TwoFactorService.php @@ -14,6 +14,7 @@ use BaconQrCode\Renderer\RendererStyle\Fill; use BaconQrCode\Renderer\RendererStyle\RendererStyle; use BaconQrCode\Writer; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Str; use PragmaRX\Google2FA\Google2FA; @@ -112,20 +113,45 @@ class TwoFactorService /** * Consume a recovery code; each code works exactly once. + * + * Read the list, filter it, write the whole list back is not once. + * Two requests that both read before either writes each store their + * own filtered copy, and the second write puts back the code the + * first removed -- so a spent code is available again, and the same + * code offered twice is accepted twice. Neither lets in anybody who + * was not already holding a code, which is why this is a promise not + * being kept rather than a door standing open. The promise is the + * sentence above, and it is the reason recovery codes are printed + * out and crossed off. + * + * Decide from the row as it stands, read back under a lock inside + * the transaction that writes it -- the same shape + * SendNotificationDigest uses to claim the rows it is about to + * delete. The lock is what makes it atomic against a request + * arriving at the same moment; the re-read is what makes the + * decision right, and it is the half that can be demonstrated in a + * test, since SQLite ignores lockForUpdate. + * + * The caller's own instance is what gets saved, so it does not walk + * away holding a list the database no longer has. */ public function consumeRecoveryCode(User $user, string $code): bool { - /** @var list|null $codes */ - $codes = $user->two_factor_recovery_codes; + return DB::transaction(function () use ($user, $code): bool { + $locked = User::query()->whereKey($user->getKey())->lockForUpdate()->first(); - if ($codes === null || ! in_array($code, $codes, true)) { - return false; - } + /** @var list|null $codes */ + $codes = $locked?->two_factor_recovery_codes; - $user->forceFill([ - 'two_factor_recovery_codes' => array_values(array_diff($codes, [$code])), - ])->save(); + if ($codes === null || ! in_array($code, $codes, true)) { + return false; + } - return true; + $user->forceFill([ + 'two_factor_recovery_codes' => array_values(array_diff($codes, [$code])), + ])->save(); + + return true; + }); } } diff --git a/tests/Feature/Identity/RecoveryCodeSingleUseTest.php b/tests/Feature/Identity/RecoveryCodeSingleUseTest.php new file mode 100644 index 00000000..cff9d62b --- /dev/null +++ b/tests/Feature/Identity/RecoveryCodeSingleUseTest.php @@ -0,0 +1,96 @@ +user = User::factory()->create(); + $this->user->forceFill([ + 'two_factor_secret' => 'SECRET', + 'two_factor_recovery_codes' => ['aaa-111', 'bbb-222', 'ccc-333'], + 'two_factor_confirmed_at' => now(), + ])->save(); + + $this->service = app(TwoFactorService::class); +}); + +function storedCodes(User $user): array +{ + /** @var list $codes */ + $codes = User::query()->findOrFail($user->id)->two_factor_recovery_codes ?? []; + + return $codes; +} + +test('spending two different codes at once does not put either back', function () { + $first = User::query()->findOrFail($this->user->id); + $second = User::query()->findOrFail($this->user->id); + + expect($this->service->consumeRecoveryCode($first, 'aaa-111'))->toBeTrue() + ->and($this->service->consumeRecoveryCode($second, 'bbb-222'))->toBeTrue() + ->and(storedCodes($this->user))->toBe(['ccc-333']); +}); + +test('the same code is not accepted twice', function () { + $first = User::query()->findOrFail($this->user->id); + $second = User::query()->findOrFail($this->user->id); + + expect($this->service->consumeRecoveryCode($first, 'aaa-111'))->toBeTrue() + ->and($this->service->consumeRecoveryCode($second, 'aaa-111'))->toBeFalse() + ->and(storedCodes($this->user))->toBe(['bbb-222', 'ccc-333']); +}); + +test('the caller instance is left holding what the database holds', function () { + $instance = User::query()->findOrFail($this->user->id); + + $this->service->consumeRecoveryCode($instance, 'aaa-111'); + + expect($instance->two_factor_recovery_codes)->toBe(['bbb-222', 'ccc-333']) + ->and($instance->isDirty())->toBeFalse(); +}); + +test('a code still works, once, in the ordinary case', function () { + expect($this->service->consumeRecoveryCode($this->user, 'bbb-222'))->toBeTrue() + ->and(storedCodes($this->user))->toBe(['aaa-111', 'ccc-333']) + ->and($this->service->consumeRecoveryCode($this->user, 'bbb-222'))->toBeFalse(); +}); + +test('a code nobody issued is refused, and an account with none at all', function () { + expect($this->service->consumeRecoveryCode($this->user, 'zzz-999'))->toBeFalse() + ->and(storedCodes($this->user))->toBe(['aaa-111', 'bbb-222', 'ccc-333']); + + $plain = User::factory()->create(); + + expect($this->service->consumeRecoveryCode($plain, 'aaa-111'))->toBeFalse(); +}); + +test('spending the last code empties the list', function () { + $this->user->forceFill(['two_factor_recovery_codes' => ['only-one']])->save(); + + expect($this->service->consumeRecoveryCode($this->user, 'only-one'))->toBeTrue() + ->and(storedCodes($this->user))->toBe([]); +}); + +test('the challenge screen still signs somebody in with one, and spends it', function () { + $this->withSession([ + SignIn::TWO_FACTOR_ID => $this->user->id, + SignIn::TWO_FACTOR_REMEMBER => false, + ])->post('/two-factor-challenge', ['recovery_code' => ' aaa-111 ']) + ->assertRedirect(route('dashboard', absolute: false)); + + $this->assertAuthenticatedAs($this->user); + expect(storedCodes($this->user))->toBe(['bbb-222', 'ccc-333']); +});