mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
denkfabrik-li
7ebc9b0905
consumeRecoveryCode() reads the whole list, filters the used code out, and writes the whole list back. Two requests that both read before either writes each store their own copy, and the second write puts back the code the first removed. So a spent code comes back, and the same code offered twice is accepted twice -- while the method's first line says "each code works exactly once". Nobody gets in through this who was not already holding a valid code, so it is a promise not being kept rather than a door standing open. The promise is worth keeping anyway: it is the whole reason a printed sheet of recovery codes can be crossed off, and it is what makes a code that somebody watched being typed in stop working. The decision now comes from the row as it stands, re-read under a lock inside the transaction that writes it -- the shape SendNotificationDigest already uses to claim the rows it is about to delete. A conditional update, as in PublicShareController's downloads_count and the delivered_at claim in #1692, is the other precedent in the tree, but the column is `encrypted:array`: there is nothing in it a database can compare, so the comparison has to happen after decryption, under something that holds the row while it does. 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 a test can show: SQLite ignores lockForUpdate, so the accompanying tests pin the re-read and say so rather than claiming to prove the locking. config/database.php runs MySQL or Postgres in production, and both honour it. Saving through the caller's own instance keeps that instance in step with the row, so a caller cannot go on to decide from a list the database no longer has.
97 lines
3.7 KiB
PHP
97 lines
3.7 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
use App\Models\User;
|
|
use App\Modules\Identity\SignIn;
|
|
use App\Modules\Identity\TwoFactor\TwoFactorService;
|
|
|
|
/**
|
|
* "Each code works exactly once" -- the promise in the docblock, and the
|
|
* reason a printed sheet of recovery codes can be crossed off.
|
|
*
|
|
* Two independently loaded instances of the same row stand in for two
|
|
* requests that both read the list before either writes it back. That is
|
|
* what the old read-filter-write lost, and it is a thing SQLite can
|
|
* demonstrate; lockForUpdate is a no-op there, so what these pin is the
|
|
* re-read, not the lock.
|
|
*/
|
|
beforeEach(function () {
|
|
$this->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<string> $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']);
|
|
});
|