diff --git a/app/Devnull/SignIn.php b/app/Devnull/SignIn.php index c037cab..40974ff 100644 --- a/app/Devnull/SignIn.php +++ b/app/Devnull/SignIn.php @@ -2,16 +2,32 @@ namespace App\Devnull; +use App\Devnull\Exceptions\KeychainUnavailable; +use Carbon\CarbonImmutable; + /** - * The sign-in under way, held in memory only. + * The sign-in under way. * * It keeps the attempt the browser is showing, and the browser's answer once * devnull://auth/callback brings it back, until onboarding takes both. A new * attempt replaces the old one, and an answer is taken once, so each attempt - * is good for one answer whatever it says. Nothing here is ever written down. + * is good for one answer whatever it says. + * + * The attempt is also kept in the Keychain until it's answered. Phones short + * on memory, Xiaomi's above all, kill the app while the browser is in front, + * and the callback then lands in a fresh process that would know of no + * attempt. Only the Keychain holds it, it's gone once taken, and one older + * than KeptFor is never used. The browser's answer stays in memory. */ final class SignIn { + public const string StorageKey = 'devnull.sign-in'; + + /** + * How long a kept attempt can still be answered, in seconds. + */ + public const int KeptFor = 600; + private ?SignInAttempt $attempt = null; /** @@ -19,11 +35,15 @@ final class SignIn */ private ?array $callback = null; + public function __construct(private KeyStore $store) {} + public function begin(): SignInAttempt { $this->callback = null; + $this->attempt = SignInAttempt::start(); + $this->keep($this->attempt); - return $this->attempt = SignInAttempt::start(); + return $this->attempt; } /** @@ -41,6 +61,7 @@ public function hasCallback(): bool /** * The browser's answer and the attempt it should answer, forgetting both. + * The attempt comes from the Keychain when the app was started afresh. * * @return array{0: array, 1: SignInAttempt|null}|null */ @@ -50,10 +71,58 @@ public function take(): ?array return null; } - $taken = [$this->callback, $this->attempt]; + $taken = [$this->callback, $this->attempt ?? $this->kept()]; $this->callback = null; $this->attempt = null; + $this->forgetKept(); return $taken; } + + /** + * Keeping it is a fallback: when the Keychain won't take it, the attempt + * still works for as long as the app stays alive. + */ + private function keep(SignInAttempt $attempt): void + { + try { + $this->store->set(self::StorageKey, (string) json_encode([ + 'verifier' => $attempt->verifier(), + 'state' => $attempt->state, + 'started_at' => CarbonImmutable::now()->getTimestamp(), + ])); + } catch (KeychainUnavailable) { + // + } + } + + private function kept(): ?SignInAttempt + { + try { + $stored = $this->store->read(self::StorageKey); + } catch (KeychainUnavailable) { + return null; + } + + $kept = $stored->found() ? json_decode((string) $stored->value, true) : null; + + if (! is_array($kept) || ! is_string($kept['verifier'] ?? null) || ! is_string($kept['state'] ?? null) || ! is_int($kept['started_at'] ?? null)) { + return null; + } + + if (CarbonImmutable::now()->getTimestamp() - $kept['started_at'] > self::KeptFor) { + return null; + } + + return SignInAttempt::resume($kept['verifier'], $kept['state']); + } + + private function forgetKept(): void + { + try { + $this->store->delete(self::StorageKey); + } catch (KeychainUnavailable) { + // + } + } } diff --git a/app/Devnull/SignInAttempt.php b/app/Devnull/SignInAttempt.php index 1b01587..ccde5fb 100644 --- a/app/Devnull/SignInAttempt.php +++ b/app/Devnull/SignInAttempt.php @@ -2,15 +2,16 @@ namespace App\Devnull; +use SensitiveParameter; use SensitiveParameterValue; /** * One go at signing in through dev-null.sh: OAuth's authorization code flow * with PKCE, devnull being the provider. * - * The verifier and state are made fresh for each attempt and live only in - * memory. The verifier is wrapped so it never shows up in a dump or a stack - * trace, and can't be serialized to disk. + * The verifier and state are made fresh for each attempt, and kept outside + * memory only in the Keychain (see SignIn). The verifier is wrapped so it + * never shows up in a dump or a stack trace, and can't be serialized to disk. */ final readonly class SignInAttempt { @@ -23,6 +24,14 @@ public static function start(): self return new self(new SensitiveParameterValue(self::base64Url(random_bytes(32))), self::base64Url(random_bytes(16))); } + /** + * The attempt SignIn kept in the Keychain, after the app was started afresh. + */ + public static function resume(#[SensitiveParameter] string $verifier, string $state): self + { + return new self(new SensitiveParameterValue($verifier), $state); + } + public function verifier(): string { return $this->verifier->getValue(); diff --git a/tests/Feature/Screens/OnboardingTest.php b/tests/Feature/Screens/OnboardingTest.php index 05b279c..9027e23 100644 --- a/tests/Feature/Screens/OnboardingTest.php +++ b/tests/Feature/Screens/OnboardingTest.php @@ -375,7 +375,7 @@ function sendBack(TestableComponent $screen, array $query): TestableComponent Http::assertSentCount(1); }); -it('never writes the verifier or the state down', function () { +it('keeps the verifier and the state only in the Keychain, and forgets them once answered', function () { Storage::fake('local'); $bridge = signInBridge(testSeed()); fakeApi(['POST /api/v1/devices' => [['data' => userWithCountsJson()]]]); @@ -388,17 +388,56 @@ function sendBack(TestableComponent $screen, array $query): TestableComponent ->and(json_encode($screen->tree()))->not->toContain($verifier)->not->toContain($state) ->and(Storage::disk('local')->allFiles())->toBe([]) ->and(fn () => serialize($attempt))->toThrow(Exception::class) - ->and(print_r($attempt, true))->not->toContain($verifier); - - $bridge->assertNotCalled('SecureStorage.Set'); + ->and(print_r($attempt, true))->not->toContain($verifier) + ->and(array_column(array_column($bridge->callsTo('SecureStorage.Set'), 'params'), 'key'))->toBe([SignIn::StorageKey]); sendBack($screen, ['code' => 'c', 'state' => $state])->assertReplacedWith('/log'); expect(app(SignIn::class)->take())->toBeNull() ->and(pendingAttempt())->toBeNull() ->and(json_encode(app(Account::class)->user()?->toArray()))->not->toContain($verifier)->not->toContain($state); + + $bridge->assertCalled('SecureStorage.Delete', fn (array $params): bool => $params['key'] === SignIn::StorageKey); +}); + +it('finishes a sign-in when the phone killed the app while the browser was open', function () { + $bridge = signInBridge(testSeed()); + fakeApi(['POST /api/v1/devices' => [['data' => userWithCountsJson()]]]); + $state = tapSignIn(Native::visit('/onboarding'), $bridge); + $verifier = pendingAttempt()->verifier(); + + // The callback starts the app afresh, with nothing of the attempt left in memory. + app()->forgetInstance(SignIn::class); + + Native::visit('/auth/callback?'.http_build_query(['code' => 'c', 'state' => $state])) + ->assertReplacedWith('/onboarding') + ->follow() + ->assertReplacedWith('/log'); + + Http::assertSent(fn (Request $request): bool => $request->method() === 'POST' + && $request->body() === json_encode(['code' => 'c', 'code_verifier' => $verifier, 'terms_accepted' => true])); }); +it('rejects a callback after a restart when the state is not the kept one, or the attempt is too old', function (int $minutesLater, string $state) { + $bridge = signInBridge(testSeed()); + fakeApi([]); + $sent = tapSignIn(Native::visit('/onboarding'), $bridge); + + app()->forgetInstance(SignIn::class); + $this->travel($minutesLater)->minutes(); + + Native::visit('/auth/callback?'.http_build_query(['code' => 'c', 'state' => $state === 'sent' ? $sent : $state])) + ->assertReplacedWith('/onboarding') + ->follow() + ->assertSee(Onboarding::NotStartedHere); + + Http::assertNothingSent(); + $bridge->assertCalled('SecureStorage.Delete', fn (array $params): bool => $params['key'] === SignIn::StorageKey); +})->with([ + 'another state' => [1, 'not-the-state'], + 'kept too long' => [11, 'sent'], +]); + it('says so when the auth browser does not open', function () { fakeKeychain(testSeed())->respondTo('Browser.OpenAuth', ['success' => false]);