Say whether the client still works when forget_device fails

crates/connetto-client/src/auth.rs

@@ -713,27 +713,46 @@ impl NativeAuthenticator {
     ///
     /// # Errors
     ///
+    /// [`ClientError::Auth`] if the store cannot be read or cleared, and else
     /// [`ClientError::Transport`] or [`ClientError::Auth`] if the revoke fails,
-    /// after the local clear. [`ClientError::Auth`] if the store cannot be read
-    /// or cleared.
+    /// after the local clear.
     pub async fn logout(&self) -> Result<(), ClientError> {
+        let signed_out = self.sign_out().await;
+        signed_out.credential.and(signed_out.revoke)
+    }
+
+    /// [`Self::logout`] with the clear and the revoke reported apart.
+    pub(crate) async fn sign_out(&self) -> SignedOut {
         // Signing out is per account: any other stored credential survives, and
         // the marker is left naming an account with none, which the next start
         // answers with an interactive login rather than by picking somebody else.
         let Some(account) = self.account() else {
-            return Ok(());
+            return SignedOut::default();
         };
-        let Some(refresh) = self.store.load(&account).await? else {
-            return Ok(());
+        let refresh = match self.store.load(&account).await {
+            Ok(Some(refresh)) => refresh,
+            Ok(None) => return SignedOut::default(),
+            Err(err) => {
+                return SignedOut {
+                    credential: Err(err),
+                    revoke: Err(ClientError::Auth(
+                        "the stored credential could not be read, so the session was not revoked"
+                            .to_owned(),
+                    )),
+                };
+            }
         };
-        let revoked = self
+        let revoke = self
             .post(
                 &format!("{}/auth/logout", self.server_base),
                 &serde_json::json!({ "refresh_token": refresh }),
             )
-            .await;
-        self.store.clear(&account).await?;
-        revoked.map(drop)
+            .await
+            .map(drop);
+        SignedOut {
+            credential: self.store.clear(&account).await,
+            revoke,
+        }
     }
 
     async fn post_json<R: DeserializeOwned>(
@@ -779,6 +798,24 @@ impl NativeAuthenticator {
     }
 }
 
+/// How each half of a sign-out went.
+#[derive(Debug)]
+pub(crate) struct SignedOut {
+    /// Reading and clearing the stored credential.
+    pub(crate) credential: Result<(), ClientError>,
+    /// Revoking the session on the server.
+    pub(crate) revoke: Result<(), ClientError>,
+}
+
+impl Default for SignedOut {
+    fn default() -> Self {
+        Self {
+            credential: Ok(()),
+            revoke: Ok(()),
+        }
+    }
+}
+
 /// A 256-bit random token as URL-safe base64, for the PKCE verifier and the
 /// CSRF state.
 fn random_token() -> String {
@@ -1103,4 +1140,92 @@ mod tests {
             Some(key_from_byte(0x22))
         );
     }
+
+    /// A refresh store holding one token whose reads or clears fail as told.
+    struct BrokenRefresh {
+        load_fails: bool,
+    }
+
+    impl connetto_core::traits::RefreshTokenStore for BrokenRefresh {
+        type Error = crate::ClientError;
+
+        fn load<'a>(
+            &'a self,
+            _account: &'a str,
+        ) -> connetto_core::traits::RefreshFuture<'a, Option<String>, crate::ClientError> {
+            let token = if self.load_fails {
+                Err(crate::ClientError::Auth(
+                    "the store cannot be read".to_owned(),
+                ))
+            } else {
+                Ok(Some("session-id.secret".to_owned()))
+            };
+            Box::pin(std::future::ready(token))
+        }
+
+        fn store<'a>(
+            &'a self,
+            _account: &'a str,
+            _token: &'a str,
+        ) -> connetto_core::traits::RefreshFuture<'a, (), crate::ClientError> {
+            Box::pin(std::future::ready(Ok(())))
+        }
+
+        fn clear<'a>(
+            &'a self,
+            _account: &'a str,
+        ) -> connetto_core::traits::RefreshFuture<'a, (), crate::ClientError> {
+            Box::pin(std::future::ready(Err(crate::ClientError::Auth(
+                "the store cannot be cleared".to_owned(),
+            ))))
+        }
+
+        fn accounts(
+            &self,
+        ) -> connetto_core::traits::RefreshFuture<'_, Vec<String>, crate::ClientError> {
+            Box::pin(std::future::ready(Ok(vec!["account".to_owned()])))
+        }
+
+        fn protection(&self) -> crate::Custody {
+            crate::Custody::Unverified(crate::NoGate::Unsupported)
+        }
+    }
+
+    /// An authenticator over `store` whose server is unreachable, since
+    /// nothing listens on port 1.
+    fn offline_authenticator(store: BrokenRefresh) -> super::NativeAuthenticator {
+        super::NativeAuthenticator::new(
+            "http://127.0.0.1:1",
+            "dev-idp",
+            std::sync::Arc::new(store),
+            Some("account".to_owned()),
+        )
+    }
+
+    /// A credential that cannot be cleared is reported as kept, apart from
+    /// the revoke, which fails on its own.
+    #[tokio::test]
+    async fn a_failed_clear_reports_the_credential_apart_from_the_revoke() {
+        let signed_out = offline_authenticator(BrokenRefresh { load_fails: false })
+            .sign_out()
+            .await;
+        assert!(
+            matches!(&signed_out.credential, Err(crate::ClientError::Auth(message)) if message.contains("cleared")),
+            "the clear failed, got {signed_out:?}"
+        );
+        assert!(
+            matches!(signed_out.revoke, Err(crate::ClientError::Transport(_))),
+            "and the revoke failed on the unreachable server"
+        );
+    }
+
+    /// A credential that cannot be read cannot be revoked either.
+    #[tokio::test]
+    async fn an_unreadable_credential_is_neither_cleared_nor_revoked() {
+        let signed_out = offline_authenticator(BrokenRefresh { load_fails: true })
+            .sign_out()
+            .await;
+        assert!(signed_out.credential.is_err(), "the read failed");
+        assert!(signed_out.revoke.is_err(), "so nothing could be revoked");
+    }
 }

crates/connetto-client/src/builder/native.rs

@@ -44,7 +44,7 @@ use crate::reconnect::ReconnectPolicy;
 use crate::replica::encode_identity;
 use crate::teardown::content_dir;
 #[cfg(feature = "native-auth")]
-use crate::teardown::{ForgetError, PurgeError, forget_device, wipe_replica};
+use crate::teardown::{ForgetError, ForgetIncomplete, ForgetRefusal, forget_device};
 use crate::{ClientError, ConnettoClient, Custody};
 #[cfg(feature = "peer")]
 use crate::{ClientEvent, HotspotError, HotspotOffer, JoinError};
@@ -1473,35 +1473,47 @@ where
     /// client is closed and its replica swapped for an empty in-memory one
     /// before the files go, since Windows refuses to delete an open file, so
     /// clones still alive read an empty replica. The application drops it
-    /// afterwards.
+    /// afterwards. Once the client is closed every part of the teardown runs
+    /// even when an earlier one failed, so as much as possible is removed.
     ///
     /// # Errors
     ///
-    /// [`ForgetError::NoReplica`] for a build that kept nothing on the
-    /// device, [`ForgetError::Client`] when the unsynced writes cannot be
-    /// read or, once the wipe stands, the device key cannot be deleted,
-    /// [`ForgetError::Purge`] when the guard refuses or the wipe fails,
-    /// and [`ForgetError::NotRevoked`] when the wipe succeeded but the server
-    /// was not reached.
+    /// [`ForgetError::Refused`] when nothing was destroyed and the client
+    /// still works: the build kept nothing on the device, the unsynced writes
+    /// cannot be read, or they block the wipe. [`ForgetError::Incomplete`]
+    /// when the client is closed and part of the teardown failed, naming each
+    /// part.
     pub async fn forget_device(&self, force: bool) -> Result<(), ForgetError> {
-        let teardown = self.teardown.as_ref().ok_or(ForgetError::NoReplica)?;
-        let unsynced = self.core.client().unsynced().await?;
+        let teardown = self.teardown.as_ref().ok_or(ForgetRefusal::NoReplica)?;
+        let unsynced = self
+            .core
+            .client()
+            .unsynced()
+            .await
+            .map_err(ForgetRefusal::Unreadable)?;
         if !unsynced.is_empty() && !force {
-            return Err(ForgetError::Purge(PurgeError::Unsynced(unsynced)));
+            return Err(ForgetRefusal::Unsynced(unsynced).into());
         }
         self.core.close().await;
-        self.core.client().release_replica().await?;
-        let forgotten = teardown.forget(&unsynced, force).await;
+        let mut report = ForgetIncomplete {
+            release: self.core.client().release_replica().await.err(),
+            ..ForgetIncomplete::default()
+        };
+        teardown.forget(&unsynced, &mut report).await;
         // The certificate went with the replica, and the key goes once the
         // wipe stands, whether or not the server heard (lifecycle row
         // "`forget_device`").
         #[cfg(feature = "device-identity")]
-        if matches!(forgotten, Ok(()) | Err(ForgetError::NotRevoked(_)))
+        if report.wipe.is_none()
             && let Some(device) = &self.device
         {
-            device.delete_key().await?;
+            report.device_key = device.delete_key().await.err();
+        }
+        if report.is_complete() {
+            Ok(())
+        } else {
+            Err(ForgetError::Incomplete(Box::new(report)))
         }
-        forgotten
     }
 }
 
@@ -1550,8 +1562,8 @@ trait Forget: Send + Sync {
     fn forget<'a>(
         &'a self,
         unsynced: &'a [u64],
-        force: bool,
-    ) -> core::pin::Pin<Box<dyn Future<Output = Result<(), ForgetError>> + Send + 'a>>;
+        report: &'a mut ForgetIncomplete,
+    ) -> core::pin::Pin<Box<dyn Future<Output = ()> + Send + 'a>>;
 }
 
 /// The replica a durable build wrote, and what revokes its session.
@@ -1571,32 +1583,16 @@ where
     fn forget<'a>(
         &'a self,
         unsynced: &'a [u64],
-        force: bool,
-    ) -> core::pin::Pin<Box<dyn Future<Output = Result<(), ForgetError>> + Send + 'a>> {
-        Box::pin(async move {
-            match &self.authenticator {
-                Some(authenticator) => {
-                    forget_device(
-                        authenticator,
-                        &self.path,
-                        self.key_store.as_ref(),
-                        &self.name,
-                        unsynced,
-                        force,
-                    )
-                    .await
-                }
-                None => wipe_replica(
-                    &self.path,
-                    self.key_store.as_ref(),
-                    &self.name,
-                    unsynced,
-                    force,
-                )
-                .await
-                .map_err(ForgetError::from),
-            }
-        })
+        report: &'a mut ForgetIncomplete,
+    ) -> core::pin::Pin<Box<dyn Future<Output = ()> + Send + 'a>> {
+        Box::pin(forget_device(
+            self.authenticator.as_deref(),
+            &self.path,
+            self.key_store.as_ref(),
+            &self.name,
+            unsynced,
+            report,
+        ))
     }
 }