Keep the sign-in attempt when the phone kills the app mid sign-in

app/Devnull/SignIn.php

@@ -2,28 +2,48 @@
 
 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;
 
     /**
      * @var array<string, string>|null
      */
     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<string, string>, 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) {
+            //
+        }
+    }
 }

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();

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]);