fix(device-client): keep the keys and session when the save fails

crates/orbyn-device-client/src/signup.rs

@@ -15,10 +15,18 @@ const MAX_PROOF_ROUNDS: usize = 2;
 
 /// A sign-up that waits for the emailed code. It holds the new device's keys in memory only: an
 /// abandoned sign-up leaves nothing behind. Drop it to cancel.
+///
+/// Once the code is spent the account exists and the session the server answered with is held
+/// here too (in memory, wiped on drop) until it is saved: if the key store fails at that moment,
+/// the keys and the session stay here and [`DeviceClient::retry_save`] saves them again with no
+/// new code. Dropping it before that leaves an account no device holds; see "A sign-up that
+/// could not be saved" in `docs/device-client.md`.
 pub struct PendingSignUp {
     signup_id: String,
     email: String,
     keys: Option<DeviceKeys>,
+    /// The session of a spent code that is not saved yet.
+    created: Option<Created>,
 }
 
 impl PendingSignUp {
@@ -97,6 +105,7 @@ impl<A: DeviceApi, S: KeyStore, C: Clock> DeviceClient<A, S, C> {
                     signup_id: pending.signup_id,
                     email,
                     keys: Some(keys),
+                    created: None,
                 });
             }
             if response.status != 428 || round == MAX_PROOF_ROUNDS {
@@ -111,15 +120,23 @@ impl<A: DeviceApi, S: KeyStore, C: Clock> DeviceClient<A, S, C> {
     /// Finishes a sign-up with the emailed code (any case, hyphen or not). A wrong code is
     /// [`DeviceError::InvalidCode`] and leaves the sign-up open: try again or ask for a new code.
     /// On success the keys, ids and session are saved and the account exists.
+    ///
+    /// If the code was accepted but the key store failed ([`DeviceError::Store`]), the code is
+    /// spent and the account exists: the keys and the session stay in `pending`, and
+    /// [`DeviceClient::retry_save`] (or this call again, whatever the code) saves them without
+    /// asking the server for anything.
     pub async fn verify_sign_up(
         &self,
         pending: &mut PendingSignUp,
         code: &str,
     ) -> Result<SessionInfo, DeviceError> {
-        let keys = pending
-            .keys
-            .as_ref()
-            .ok_or(DeviceError::InvalidInput("this sign-up is finished"))?;
+        if pending.keys.is_none() {
+            return Err(DeviceError::InvalidInput("this sign-up is finished"));
+        }
+        // A spent code cannot be sent again: finish saving what it made.
+        if pending.created.is_some() {
+            return self.retry_save(pending);
+        }
         // A sign-in finished in between would be overwritten: refuse before the code is spent.
         if self.store.load()?.is_some() {
             return Err(DeviceError::StoreNotEmpty);
@@ -133,8 +150,24 @@ impl<A: DeviceApi, S: KeyStore, C: Clock> DeviceClient<A, S, C> {
                 },
             )
             .await?;
-        let info = self.save_session(&created, keys, &pending.email, None)?;
+        pending.created = Some(created);
+        self.retry_save(pending)
+    }
+
+    /// Saves the keys and the session of a sign-up whose code was spent but whose save failed.
+    /// Nothing is sent to the server. A failure keeps everything in `pending` for another try;
+    /// a key store that already holds a device is [`DeviceError::StoreNotEmpty`]. With nothing
+    /// to save it is [`DeviceError::InvalidInput`].
+    pub fn retry_save(&self, pending: &mut PendingSignUp) -> Result<SessionInfo, DeviceError> {
+        let (Some(keys), Some(created)) = (pending.keys.as_ref(), pending.created.as_ref()) else {
+            return Err(DeviceError::InvalidInput("there is nothing to save"));
+        };
+        if self.store.load()?.is_some() {
+            return Err(DeviceError::StoreNotEmpty);
+        }
+        let info = self.save_session(created, keys, &pending.email, None)?;
         pending.keys = None;
+        pending.created = None;
         Ok(info)
     }
 

crates/orbyn-device-client/tests/client.rs

@@ -6,7 +6,8 @@ mod support;
 use orbyn_contracts::{DeviceApp, DevicePlatform, DeviceState};
 use orbyn_device_client::{
     ClientConfig, Clock, DeviceApi, DeviceClient, DeviceError, HttpApi, KeyStore, MemoryStore,
-    Method, Request, Response, SecondFactor, SignInOutcome, SignUpRequest, StoredIdentity,
+    Method, Request, Response, SecondFactor, SignInOutcome, SignUpRequest, StoreError,
+    StoredIdentity,
 };
 use std::sync::Arc;
 use std::sync::atomic::{AtomicI64, Ordering};
@@ -580,3 +581,155 @@ async fn the_app_label_is_signed_at_sign_up_and_sign_in_and_a_client_with_none_s
         ]
     );
 }
+
+/// A store whose next `failures` saves fail, then behaves as a memory store.
+struct FlakyStore {
+    inner: MemoryStore,
+    failures: std::sync::Mutex<usize>,
+}
+
+impl FlakyStore {
+    fn failing(failures: usize) -> Self {
+        Self {
+            inner: MemoryStore::new(),
+            failures: std::sync::Mutex::new(failures),
+        }
+    }
+}
+
+impl KeyStore for FlakyStore {
+    fn load(&self) -> Result<Option<StoredIdentity>, StoreError> {
+        self.inner.load()
+    }
+
+    fn save(&self, identity: &StoredIdentity) -> Result<(), StoreError> {
+        let mut left = self.failures.lock().expect("lock");
+        if *left > 0 {
+            *left -= 1;
+            return Err(StoreError);
+        }
+        drop(left);
+        self.inner.save(identity)
+    }
+
+    fn clear(&self) -> Result<(), StoreError> {
+        self.inner.clear()
+    }
+}
+
+fn flaky_client(
+    engine: &FakeEngine,
+    failures: usize,
+) -> DeviceClient<FakeEngine, FlakyStore, SharedClock> {
+    DeviceClient::new(
+        engine.clone(),
+        FlakyStore::failing(failures),
+        SharedClock(engine.now.clone()),
+        ClientConfig {
+            device_name: "Laptop".to_owned(),
+            platform: DevicePlatform::Linux,
+            app: None,
+        },
+    )
+}
+
+#[tokio::test(flavor = "multi_thread")]
+async fn a_failed_save_after_the_code_is_spent_keeps_the_keys_and_retry_save_finishes() {
+    let engine = FakeEngine::new();
+    let a = flaky_client(&engine, 2);
+    let mut pending = a.begin_sign_up(request(None)).await.expect("begin");
+    let code = engine.code_for(NORMAL);
+    // The code is accepted, the save fails: the account exists and the code is spent.
+    assert!(matches!(
+        a.verify_sign_up(&mut pending, &code).await,
+        Err(DeviceError::Store(_))
+    ));
+    assert_eq!(engine.device_count(NORMAL), 1);
+    assert_eq!(a.identity().expect("store"), None, "nothing was saved");
+    let verifies = |engine: &FakeEngine| {
+        engine
+            .log()
+            .iter()
+            .filter(|(path, _)| path.ends_with("email/verify"))
+            .count()
+    };
+    assert_eq!(verifies(&engine), 1);
+
+    // The same call again, whatever the code, retries the save without asking the server.
+    assert!(matches!(
+        a.verify_sign_up(&mut pending, "00000000").await,
+        Err(DeviceError::Store(_))
+    ));
+    assert_eq!(verifies(&engine), 1, "the spent code was not sent again");
+
+    // The store recovered: the keys and the session saved are the ones the server answered with.
+    let session = a.retry_save(&mut pending).expect("retry");
+    assert_eq!(session.device_state, DeviceState::Trusted);
+    assert_eq!(verifies(&engine), 1);
+    let (account, device, email) = a.identity().expect("store").expect("saved");
+    assert_eq!(
+        (account, device, email),
+        (
+            session.account_id.clone(),
+            session.device_id.clone(),
+            NORMAL.to_owned()
+        )
+    );
+    // The saved device key is the one the server knows: it signs a trusted start.
+    a.reauthenticate(&password(PASSWORD))
+        .await
+        .expect("a trusted start with the saved key");
+
+    // Nothing is left to save, and a finished sign-up is not verified again.
+    assert_eq!(
+        a.retry_save(&mut pending).err(),
+        Some(DeviceError::InvalidInput("there is nothing to save"))
+    );
+    assert_eq!(
+        a.verify_sign_up(&mut pending, &code).await.err(),
+        Some(DeviceError::InvalidInput("this sign-up is finished"))
+    );
+}
+
+#[tokio::test(flavor = "multi_thread")]
+async fn retry_save_with_no_spent_code_or_a_store_that_holds_a_device_saves_nothing() {
+    let engine = FakeEngine::new();
+    let a = flaky_client(&engine, 0);
+    let mut pending = a.begin_sign_up(request(None)).await.expect("begin");
+    assert_eq!(
+        a.retry_save(&mut pending).err(),
+        Some(DeviceError::InvalidInput("there is nothing to save"))
+    );
+
+    // The save fails, and a device lands in the store before the retry: it is never overwritten.
+    let b = flaky_client(&engine, 1);
+    let mut second = b
+        .begin_sign_up(SignUpRequest {
+            email: "other@example.com".to_owned(),
+            password: password(PASSWORD),
+            invite: None,
+        })
+        .await
+        .expect("begin");
+    let code = engine.code_for("other@example.com");
+    assert!(b.verify_sign_up(&mut second, &code).await.is_err());
+    let stored = StoredIdentity {
+        device_keys: orbyn_crypto::DeviceKeys::generate()
+            .expect("keys")
+            .to_bytes(),
+        account_id: "x".into(),
+        device_id: "y".into(),
+        email: "z@example.com".into(),
+        session: None,
+        vault_key: Zeroizing::new([9u8; 32]),
+    };
+    b.store().save(&stored).expect("save");
+    assert_eq!(
+        b.retry_save(&mut second).err(),
+        Some(DeviceError::StoreNotEmpty)
+    );
+    assert_eq!(
+        b.store().load().expect("load").expect("kept").account_id,
+        "x"
+    );
+}

docs/device-client.md

@@ -32,6 +32,12 @@ The **zone-key cache is not here.** The host keeps zone keys in its own sealed s
 
 The result is a `PendingSignUp`. It holds the new device's keys **in memory only**: nothing is stored, and dropping it cancels, so an abandoned sign-up leaves nothing behind. `verify_sign_up(&mut pending, code)` sends the emailed code (any case; spaces and hyphens ignored). A wrong code is `InvalidCode` and leaves the sign-up open; `resend_sign_up` asks for a new one. Only on success are the keys, ids, session and a fresh vault key saved. A store that already holds a device is refused up front (`StoreNotEmpty`), and again before the code is sent, so a sign-in that finished in between is never overwritten and the code is not spent.
 
+### A sign-up that could not be saved
+
+The code is spent when the server accepts it: the account exists from then on. If the key store then fails, `verify_sign_up` returns `DeviceError::Store` and the **keys and the session stay in the `PendingSignUp`**, in memory, wiped on drop. `retry_save(&mut pending)` saves them again and sends nothing to the server, so there is no new code and no second account; calling `verify_sign_up` again does the same whatever code is passed. A store that now holds a device is `StoreNotEmpty`.
+
+If the `PendingSignUp` is dropped first (the app quits, or the store never recovers), the account exists and no device holds its keys. The address cannot sign up again, and signing in would need a device that does not exist. The way out is the delete-unused link: ask for it from the sign-in screen's "my sign-up did not finish" path (`POST /v1/accounts/delete-unused {email}`, always answered 202), open the link in the email that comes within 24 hours, and the server erases the account, which was never used, and releases its invite (see `docs/architecture.md`, "Deleting an unused account"); then sign up again. A host shows this path when `verify_sign_up` ended in `Store` and the person leaves without a successful `retry_save`.
+
 ## Sign-in
 
 `sign_in(email, password)` runs OPAQUE login. A wrong password fails **on the device** (`LoginFailed`) and nothing more is sent.