fix(proxy): persist temp dir on unseal in test mode (#1170)

fix(proxy): persist temp dir on unseal in test mode

In test mode unsealing or sealing the keystore does not recreate the
temporary directory and reset all state anymore. This allows us to
enable all tests that involve sealing.

We also fix the enabled tests and adjust the UI to make them work.

Fixes #1124.

cypress/integration/lock.spec.js

@@ -1,6 +1,4 @@
-// TODO We skip these tests because sealing the proxy in test mode
-// currently resets all state including the identity.
-context.skip("lock screen", () => {
+context("lock screen", () => {
   beforeEach(() => {
     cy.resetProxyState();
     cy.onboardUser();
@@ -16,9 +14,9 @@ context.skip("lock screen", () => {
     cy.pick("unlock-button").should("exist");
     cy.pick("passphrase-input").type("wrong-pw");
     cy.pick("unlock-button").click();
-    cy.pick("notification")
-      .contains(/Could not unlock the session/)
-      .should("exist");
+    cy.contains(/Could not unlock the session: Passphrase incorrect/).should(
+      "exist"
+    );
     cy.pick("passphrase-input").should("have.value", "");
     cy.pick("unlock-button").should("be.disabled");
   });

cypress/integration/onboarding.spec.js

@@ -105,10 +105,7 @@ context("onboarding", () => {
       cy.pick("entity-name").contains("cloudhead");
     });
 
-    // TODO We skip these tests because deleting the session and
-    // unsealing the proxy in test mode currently resets all state
-    // including previously created identities.
-    it.skip("is not possible to create the same identity again", () => {
+    it("is not possible to create the same identity again", () => {
       // Intro screen.
       cy.pick("get-started-button").click();
 
@@ -123,7 +120,9 @@ context("onboarding", () => {
 
       // Success screen.
       cy.pick("urn").contains(radIdRegex).should("exist");
+      cy.pick("go-to-profile-button").click();
 
+      cy.log("reset session");
       // Clear session to restart onboarding.
       cy.pick("sidebar", "settings").click();
       cy.pick("clear-session-button").click();
@@ -139,11 +138,9 @@ context("onboarding", () => {
       cy.pick("passphrase-input").type(validUser.passphrase);
       cy.pick("repeat-passphrase-input").type(validUser.passphrase);
       cy.pick("set-passphrase-button").click();
-      cy.pick("notification")
-        .contains(
-          /Could not create identity: the identity 'rad:git:[\w]{3}…[\w]{3}' already exists/
-        )
-        .should("exist");
+      cy.contains(
+        /Could not create identity: the identity 'rad:git:[\w]{3}…[\w]{3}' already exists/
+      ).should("exist");
       cy.pick("handle-input").should("exist");
     });
 

proxy/api/Cargo.toml

@@ -16,6 +16,7 @@ anyhow = "1.0"
 data-encoding = "2.3"
 directories = "2.0"
 futures = { version = "0.3", features = [ "compat" ] }
+lazy_static = "1.4"
 log = "0.4"
 nonempty = { version = "0.6", features = [ "serialize" ] }
 percent-encoding = "2.1"

proxy/api/src/process.rs

@@ -1,7 +1,6 @@
 //! Provides [`run`] to run the proxy process.
 use futures::prelude::*;
 use std::{convert::TryFrom, sync::Arc, time::Duration};
-use tempfile::TempDir;
 use thiserror::Error;
 use tokio::{
     signal::unix::{signal, SignalKind},
@@ -21,8 +20,6 @@ pub struct Args {
 
 /// Data required to run the peer and the API
 struct Rigging {
-    /// Optional temporary directory to use for storage
-    temp: Option<TempDir>,
     /// The context provided to the API
     ctx: context::Context,
     /// The [`Peer`] to run
@@ -44,7 +41,7 @@ pub async fn run(args: Args) -> Result<(), Box<dyn std::error::Error>> {
     let bin_dir = config::bin_dir()?;
     coco::git_helper::setup(&proxy_path, &bin_dir)?;
 
-    let mut service_manager = service::Manager::new(service::Config { key: None });
+    let mut service_manager = service::Manager::new(args.test)?;
     let mut sighup = signal(SignalKind::hangup())?;
 
     let mut handle = service_manager.handle();
@@ -63,8 +60,8 @@ pub async fn run(args: Args) -> Result<(), Box<dyn std::error::Error>> {
     loop {
         let notified_restart = service_manager.notified_restart();
         let service_handle = service_manager.handle();
-        let config = service_manager.config().await;
-        let rigging = rig(args, service_handle, config, auth_token.clone()).await?;
+        let environment = service_manager.environment()?;
+        let rigging = rig(service_handle, environment, auth_token.clone()).await?;
         let result = run_rigging(rigging, notified_restart).await;
         match result {
             // We've been shut down, ignore
@@ -73,10 +70,9 @@ pub async fn run(args: Args) -> Result<(), Box<dyn std::error::Error>> {
             Err(e) => return Err(e.into()),
         }
 
-        // Give sled some time to clean up if we're in persistent mode
-        if !args.test {
-            tokio::time::delay_for(Duration::from_millis(200)).await
-        }
+        // Give `coco::SpawnAbortable` some time to release all the resources.
+        // See https://github.com/radicle-dev/radicle-upstream/issues/1163
+        tokio::time::delay_for(Duration::from_millis(50)).await
     }
 }
 
@@ -106,7 +102,6 @@ async fn run_rigging(
     // Required for `tokio::select`. We can’t put it on the element directly, though.
     #![allow(clippy::unreachable)]
     let Rigging {
-        temp: _dont_drop_me,
         ctx,
         peer,
         seeds_sender,
@@ -188,48 +183,46 @@ async fn run_rigging(
     }
 }
 
+lazy_static::lazy_static! {
+    /// Fixed key to use in test mode
+    static ref TEST_KEY: coco::keys::SecretKey = coco::keys::SecretKey::new();
+}
+
 /// Create [`Rigging`] to run the peer and API.
 async fn rig(
-    args: Args,
     service_handle: service::Handle,
-    config: service::Config,
+    environment: &service::Environment,
     auth_token: Arc<RwLock<Option<String>>>,
 ) -> Result<Rigging, Box<dyn std::error::Error>> {
-    let (temp, paths, store) = if args.test {
-        let temp_dir = tempfile::tempdir()?;
-        log::debug!(
-            "Temporary path being used for this run is: {:?}",
-            temp_dir.path()
-        );
-
+    let (paths, store) = if let Some(temp_dir) = &environment.temp_dir {
         std::env::set_var("RAD_HOME", temp_dir.path());
         let paths =
             coco::Paths::try_from(coco::config::Paths::FromRoot(temp_dir.path().to_path_buf()))?;
         let store = {
             let path = temp_dir.path().join("store");
             kv::Store::new(kv::Config::new(path).flush_every_ms(100))
         }?;
-        (Some(temp_dir), paths, store)
+        (paths, store)
     } else {
         let paths = coco::Paths::try_from(coco::config::Paths::default())?;
         let store = {
             let path = config::dirs().data_dir().join("store");
             kv::Store::new(kv::Config::new(path).flush_every_ms(100))
         }?;
-        (None, paths, store)
+        (paths, store)
     };
 
-    if let Some(_key) = config.key {
-        // We ignore `config.key` for now and use a hard-coded passphrase
+    if let Some(_key) = environment.key {
+        // We ignore `environment.key` for now and use a hard-coded passphrase
         let pw = coco::keystore::SecUtf8::from("radicle-upstream");
-        let key = if args.test {
-            coco::keystore::Keystorage::memory(pw)?.get()
+        let key = if environment.test_mode {
+            *TEST_KEY
         } else {
             coco::keystore::Keystorage::file(&paths, pw).init()?
         };
         let signer = signer::BoxedSigner::new(signer::SomeSigner { signer: key });
 
-        let (peer, state, seeds_sender) = if args.test {
+        let (peer, state, seeds_sender) = if environment.test_mode {
             let config = coco::config::configure(
                 paths,
                 key,
@@ -264,26 +257,24 @@ async fn rig(
             peer_control,
             state,
             store,
-            test: args.test,
+            test: environment.test_mode,
             service_handle: service_handle.clone(),
             auth_token,
         });
 
         Ok(Rigging {
-            temp,
             ctx,
             peer: Some(peer),
             seeds_sender,
         })
     } else {
         let ctx = context::Context::Sealed(context::Sealed {
             store,
-            test: args.test,
+            test: environment.test_mode,
             service_handle,
             auth_token,
         });
         Ok(Rigging {
-            temp,
             ctx,
             peer: None,
             seeds_sender: None,

proxy/api/src/service.rs

@@ -1,40 +1,78 @@
-//! Utilities for dynamic service configuration in [`crate::process`].
+//! Utilities for changing the service environment used in [`crate::process`].
 
 use futures::prelude::*;
 use std::sync::Arc;
 use tokio::sync::{mpsc, Notify};
 
-#[derive(Clone)]
-/// Persistent configuration for running the API and coco peer services.
-pub struct Config {
+/// Persistent environment with depedencies for running the API and coco peer services.
+pub struct Environment {
     /// Secret key for the coco peer.
     ///
     /// If this is `None` coco is not started.
     pub key: Option<coco::keys::SecretKey>,
+    /// If set, we use a temporary directory for on-disk persistence.
+    pub temp_dir: Option<tempfile::TempDir>,
+    /// If true we are running the service in test mode.
+    pub test_mode: bool,
+}
+
+/// Error returned when creating a new [`Environment`].
+#[derive(Debug, thiserror::Error)]
+pub enum Error {
+    /// Failed to create temporary directory
+    #[error("Failed to create temporary directory")]
+    TempDir(
+        #[source]
+        #[from]
+        std::io::Error,
+    ),
+}
+
+impl Environment {
+    /// Create a new initial environment.
+    ///
+    /// If `test_mode` is `true` then `Environment::temp_dir` is set for temporary on-disk
+    /// persistence.
+    fn new(test_mode: bool) -> Result<Self, Error> {
+        let temp_dir = if test_mode {
+            Some(tempfile::tempdir()?)
+        } else {
+            None
+        };
+        Ok(Self {
+            key: None,
+            temp_dir,
+            test_mode,
+        })
+    }
 }
 
-/// Manages changes to [`Config`].
+/// Manages changes to [`Environment`].
 pub struct Manager {
     /// Notifier to restart the services
     reload_notify: Arc<Notify>,
     /// Sender side of the [`Message`] channel
     message_sender: mpsc::Sender<Message>,
     /// Receiver side of the [`Message`] channel
     message_receiver: mpsc::Receiver<Message>,
-    /// The current configuration of the services
-    config: Config,
+    /// The current environemtn of the services
+    environment: Environment,
 }
 
 impl Manager {
-    /// Create a new manager with the initial configuration
-    pub fn new(config: Config) -> Self {
+    /// Create a new manager.
+    ///
+    /// If `test_mode` is `true` then `Environment::temp_dir` is set for temporary on-disk
+    /// persistence.
+    pub fn new(test_mode: bool) -> Result<Self, Error> {
+        let environment = Environment::new(test_mode)?;
         let (message_sender, message_receiver) = mpsc::channel(10);
-        Self {
+        Ok(Self {
             reload_notify: Arc::new(Notify::new()),
             message_sender,
             message_receiver,
-            config,
-        }
+            environment,
+        })
     }
 
     /// Get a handle to send updates to [`Manager`].
@@ -45,32 +83,35 @@ impl Manager {
         }
     }
 
-    /// Get the current configuration.
-    pub async fn config(&mut self) -> Config {
+    /// Get the current environment
+    pub fn environment(&mut self) -> Result<&Environment, Error> {
         while let Ok(message) = self.message_receiver.try_recv() {
             match message {
-                Message::Reset => self.config = Config { key: None },
-                Message::SetSecretKey(key) => self.config.key = Some(key),
-                Message::Seal => self.config.key = None,
+                Message::Reset => {
+                    let test_mode = self.environment.test_mode;
+                    self.environment = Environment::new(test_mode)?
+                },
+                Message::SetSecretKey(key) => self.environment.key = Some(key),
+                Message::Seal => self.environment.key = None,
             }
         }
 
-        self.config.clone()
+        Ok(&self.environment)
     }
 
     /// Returns a future that becomes ready when the service needs to restart because the
-    /// configuration has changed.
+    /// environment has changed.
     pub fn notified_restart(&mut self) -> impl Future<Output = ()> + Send + 'static {
         let reload_notify = Arc::new(Notify::new());
         self.reload_notify = reload_notify.clone();
         async move { reload_notify.notified().await }
     }
 }
 
-/// Messages that are sent from [`Handle`] to [`Manager`] to change the service configuration.
+/// Messages that are sent from [`Handle`] to [`Manager`] to change the service environment.
 #[allow(clippy::clippy::large_enum_variant)]
 enum Message {
-    /// Reset the service to the initial configuration and delete all persisted state
+    /// Reset the service to the initial environment and delete all persisted state
     Reset,
     /// Unseal the key store with the given secret key
     SetSecretKey(coco::keys::SecretKey),

ui/Screen/Lock.svelte

@@ -5,8 +5,11 @@
 
   let passphrase = "";
 
-  const unlock = () => {
-    session.unseal(passphrase);
+  const unlock = async () => {
+    const unlocked = await session.unseal(passphrase);
+    if (!unlocked) {
+      passphrase = "";
+    }
   };
 
   const onEnter = () => {

ui/Screen/Onboarding.svelte

@@ -41,10 +41,11 @@
       await session.createKeystore();
       // Retry until the API is up
       notification.info("Creating the identity...");
-      withRetry(() => createIdentity({ handle, passphrase }), 200).then(id => {
-        identity = id;
-        state = State.SuccessView;
-      });
+      identity = await withRetry(
+        () => createIdentity({ handle, passphrase }),
+        200
+      );
+      state = State.SuccessView;
     } catch (error) {
       animateBackward();
       state = State.EnterName;