Merge branch 'fix/daemon-crash-no-access-methods'

4 files changed, 88 insertions, 37 deletions

diff --git a/mullvad-daemon/src/access_method.rs b/mullvad-daemon/src/access_method.rs
index 013cf9d7ce..4584aa374a 100644
--- a/mullvad-daemon/src/access_method.rs
+++ b/mullvad-daemon/src/access_method.rs
@@ -135,6 +135,11 @@ where
         &mut self,
         access_method_update: AccessMethodSetting,
     ) -> Result<(), Error> {
+        // We have to be a bit careful. If we are about to disable the last
+        // remaining enabled access method, we would cause an inconsistent state
+        // in the daemon's settings. Therefore, we have to safeguard against
+        // this by explicitly checking for & disallow any update which would
+        // cause the last enabled access method to become disabled.
         let current = self.get_current_access_method()?;
         let mut command = Command::Nothing;
         let settings_update = |settings: &mut Settings| {
@@ -184,16 +189,29 @@ where
             self.event_listener
                 .notify_settings(self.settings.to_settings());
 
+            let access_methods: Vec<_> = self
+                .settings
+                .api_access_methods
+                .access_method_settings
+                .iter()
+                .filter(|api_access_method| api_access_method.enabled())
+                .cloned()
+                .collect();
+
             let mut connection_modes = self.connection_modes.lock().unwrap();
-            connection_modes.update_access_methods(
-                self.settings
-                    .api_access_methods
-                    .access_method_settings
-                    .iter()
-                    .filter(|api_access_method| api_access_method.enabled())
-                    .cloned()
-                    .collect(),
-            )
+            match connection_modes.update_access_methods(access_methods) {
+                Ok(_) => (),
+                Err(crate::api::Error::NoAccessMethods) => {
+                    // `access_methods` was empty! This implies that the user
+                    // disabled all access methods. If we ever get into this
+                    // state, we should default to using the direct access
+                    // method.
+                    let default = access_method::Settings::direct();
+                    connection_modes
+                        .update_access_methods(vec![default])
+                        .expect("Failed to create the data structure responsible for managing access methods");
+                }
+            }
         };
         self
     }
diff --git a/mullvad-daemon/src/api.rs b/mullvad-daemon/src/api.rs
index 3ac526e688..d5099ae74a 100644
--- a/mullvad-daemon/src/api.rs
+++ b/mullvad-daemon/src/api.rs
@@ -10,7 +10,7 @@ use mullvad_api::{
     ApiEndpointUpdateCallback,
 };
 use mullvad_relay_selector::RelaySelector;
-use mullvad_types::access_method::{AccessMethod, AccessMethodSetting, BuiltInAccessMethod};
+use mullvad_types::access_method::{self, AccessMethod, AccessMethodSetting, BuiltInAccessMethod};
 use std::{
     path::PathBuf,
     pin::Pin,
@@ -82,14 +82,14 @@ impl ApiConnectionModeProvider {
         cache_dir: PathBuf,
         relay_selector: RelaySelector,
         connection_modes: Vec<AccessMethodSetting>,
-    ) -> Self {
-        let connection_modes_iterator = ConnectionModesIterator::new(connection_modes);
-        Self {
+    ) -> Result<Self, Error> {
+        let connection_modes_iterator = ConnectionModesIterator::new(connection_modes)?;
+        Ok(Self {
             cache_dir,
             relay_selector,
             current_task: None,
             connection_modes: Arc::new(Mutex::new(connection_modes_iterator)),
-        }
+        })
     }
 
     /// Return a pointer to the underlying iterator over [`AccessMethod`].
@@ -121,7 +121,6 @@ impl ApiConnectionModeProvider {
     /// [`ApiConnectionModeProvider`] the standard [`std::convert::From`] trait
     /// can not be implemented.
     fn from(&mut self, access_method: AccessMethod) -> ApiConnectionMode {
-        use mullvad_types::access_method;
         match access_method {
             AccessMethod::BuiltIn(access_method) => match access_method {
                 BuiltInAccessMethod::Direct => ApiConnectionMode::Direct,
@@ -173,14 +172,20 @@ pub struct ConnectionModesIterator {
     current: AccessMethodSetting,
 }
 
+#[derive(err_derive::Error, Debug)]
+pub enum Error {
+    #[error(display = "No access methods were provided.")]
+    NoAccessMethods,
+}
+
 impl ConnectionModesIterator {
-    pub fn new(access_methods: Vec<AccessMethodSetting>) -> ConnectionModesIterator {
-        let mut iterator = Self::cycle(access_methods);
-        Self {
+    pub fn new(access_methods: Vec<AccessMethodSetting>) -> Result<ConnectionModesIterator, Error> {
+        let mut iterator = Self::new_iterator(access_methods)?;
+        Ok(Self {
             next: None,
-            current: iterator.next().unwrap(),
+            current: iterator.next().ok_or(Error::NoAccessMethods)?,
             available_modes: iterator,
-        }
+        })
     }
 
     /// Set the next [`AccessMethod`] to be returned from this iterator.
@@ -189,14 +194,26 @@ impl ConnectionModesIterator {
     }
     /// Update the collection of [`AccessMethod`] which this iterator will
     /// return.
-    pub fn update_access_methods(&mut self, access_methods: Vec<AccessMethodSetting>) {
-        self.available_modes = Self::cycle(access_methods)
+    pub fn update_access_methods(
+        &mut self,
+        access_methods: Vec<AccessMethodSetting>,
+    ) -> Result<(), Error> {
+        self.available_modes = Self::new_iterator(access_methods)?;
+        Ok(())
     }
 
-    fn cycle(
+    /// Create a cyclic iterator of [`AccessMethodSetting`]s.
+    ///
+    /// If the `access_methods` argument is an empty vector, an [`Error`] is
+    /// returned.
+    fn new_iterator(
         access_methods: Vec<AccessMethodSetting>,
-    ) -> Box<dyn Iterator<Item = AccessMethodSetting> + Send> {
-        Box::new(access_methods.into_iter().cycle())
+    ) -> Result<Box<dyn Iterator<Item = AccessMethodSetting> + Send>, Error> {
+        if access_methods.is_empty() {
+            Err(Error::NoAccessMethods)
+        } else {
+            Ok(Box::new(access_methods.into_iter().cycle()))
+        }
     }
 
     /// Look at the currently active [`AccessMethod`]
diff --git a/mullvad-daemon/src/lib.rs b/mullvad-daemon/src/lib.rs
index 890cdfb13e..b8a21dda07 100644
--- a/mullvad-daemon/src/lib.rs
+++ b/mullvad-daemon/src/lib.rs
@@ -680,7 +680,7 @@ where
                 .set_config(new_selector_config(settings));
         });
 
-        let proxy_provider = api::ApiConnectionModeProvider::new(
+        let proxy_provider = match api::ApiConnectionModeProvider::new(
             cache_dir.clone(),
             relay_selector.clone(),
             settings
@@ -691,7 +691,22 @@ where
                 .filter(|api_access_method| api_access_method.enabled())
                 .cloned()
                 .collect(),
-        );
+        ) {
+            Ok(provider) => provider,
+            Err(api::Error::NoAccessMethods) => {
+                // No settings seem to have been found. Default to using the the
+                // direct access method.
+                let default = mullvad_types::access_method::Settings::direct();
+                api::ApiConnectionModeProvider::new(
+                    cache_dir.clone(),
+                    relay_selector.clone(),
+                    vec![default],
+                )
+                .expect(
+                    "Failed to create the data structure responsible for managing access methods",
+                )
+            }
+        };
 
         let connection_modes = proxy_provider.handle();
 
diff --git a/mullvad-types/src/access_method.rs b/mullvad-types/src/access_method.rs
index b714a79edd..fe4b2507ed 100644
--- a/mullvad-types/src/access_method.rs
+++ b/mullvad-types/src/access_method.rs
@@ -51,21 +51,22 @@ impl Settings {
     pub fn cloned(&self) -> Vec<AccessMethodSetting> {
         self.access_method_settings.clone()
     }
+
+    pub fn direct() -> AccessMethodSetting {
+        let method = BuiltInAccessMethod::Direct;
+        AccessMethodSetting::new(method.canonical_name(), true, AccessMethod::from(method))
+    }
+
+    pub fn mullvad_bridges() -> AccessMethodSetting {
+        let method = BuiltInAccessMethod::Bridge;
+        AccessMethodSetting::new(method.canonical_name(), true, AccessMethod::from(method))
+    }
 }
 
 impl Default for Settings {
     fn default() -> Self {
         Self {
-            access_method_settings: vec![BuiltInAccessMethod::Direct, BuiltInAccessMethod::Bridge]
-                .into_iter()
-                .map(|built_in| {
-                    AccessMethodSetting::new(
-                        built_in.canonical_name(),
-                        true,
-                        AccessMethod::from(built_in),
-                    )
-                })
-                .collect(),
+            access_method_settings: vec![Settings::direct(), Settings::mullvad_bridges()],
         }
     }
 }