Merge branch 'daemon-doesnt-work-properly-after-disabling-all-access-des-529'

MarkusPettersson98 · MarkusPettersson98 · commit 5ac1f894c698 · 2024-01-03T10:07:11.000+01:00
diff --git a/mullvad-daemon/src/access_method.rs b/mullvad-daemon/src/access_method.rs
@@ -81,7 +81,7 @@ where
     ) -> Result<(), Error> {
         // Make sure that we are not trying to remove a built-in API access
         // method
-        let command = match self.settings.api_access_methods.find(&access_method) {
+        let command = match self.settings.api_access_methods.find_by_id(&access_method) {
             Some(api_access_method) => {
                 if api_access_method.is_builtin() {
                     Err(Error::RemoveBuiltIn)
@@ -131,7 +131,7 @@ where
     ) -> Result<AccessMethodSetting, Error> {
         self.settings
             .api_access_methods
-            .find(&access_method)
+            .find_by_id(&access_method)
             .ok_or(Error::NoSuchMethod(access_method))
             .cloned()
     }
@@ -147,22 +147,37 @@ 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.
+        // If the currently active access method is updated, we need to re-set
+        // it after updating the settings.
         let current = self.get_current_access_method().await?;
         let mut command = Command::Nothing;
         let settings_update = |settings: &mut Settings| {
-            if let Some(access_method) = settings
-                .api_access_methods
-                .find_mut(&access_method_update.get_id())
+            let access_methods = &mut settings.api_access_methods;
+            if let Some(access_method) =
+                access_methods.find_by_id_mut(&access_method_update.get_id())
             {
                 *access_method = access_method_update;
                 if access_method.get_id() == current.get_id() {
                     command = Command::Set(access_method.get_id())
                 }
+                // 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 any update which would cause the last
+                // enabled access method to become disabled. In that case, we should
+                // re-enable the `Direct` access method.
+                if access_methods.collect_enabled().is_empty() {
+                    if let Some(direct) = access_methods.get_direct() {
+                        direct.enabled = true;
+                    } else {
+                        // If the `Direct` access method does not exist within the
+                        // settings for some reason, the settings are in an
+                        // inconsistent state. We don't have much choice but to
+                        // reset these settings to their default value.
+                        log::warn!("The built-in access methods can not be found. This might be due to a corrupt settings file");
+                        *access_methods = access_method::Settings::default();
+                    }
+                }
             }
         };
 
diff --git a/mullvad-daemon/src/api.rs b/mullvad-daemon/src/api.rs
@@ -45,6 +45,31 @@ pub enum Error {
     NotRunning(#[error(source)] oneshot::Canceled),
 }
 
+impl std::fmt::Display for Message {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Message::Get(_) => f.write_str("Get"),
+            Message::Set(_, _) => f.write_str("Set"),
+            Message::Next(_) => f.write_str("Next"),
+            Message::Update(_, _) => f.write_str("Update"),
+        }
+    }
+}
+
+impl Error {
+    /// Check if this error implies that the currenly running
+    /// [`AccessModeSelector`] can not continue to operate properly.
+    ///
+    /// To recover from this kind of error, the daemon will probably have to
+    /// intervene.
+    fn is_critical_error(&self) -> bool {
+        matches!(
+            self,
+            Error::SendFailed(..) | Error::OneshotSendFailed | Error::NotRunning(..)
+        )
+    }
+}
+
 type ResponseTx<T> = oneshot::Sender<Result<T>>;
 type Result<T> = std::result::Result<T, Error>;
 
@@ -66,7 +91,7 @@ impl AccessModeSelectorHandle {
 
     pub async fn get_access_method(&self) -> Result<AccessMethodSetting> {
         self.send_command(Message::Get).await.map_err(|err| {
-            log::error!("Failed to get current access method!");
+            log::debug!("Failed to get current access method!");
             err
         })
     }
@@ -75,7 +100,7 @@ impl AccessModeSelectorHandle {
         self.send_command(|tx| Message::Set(tx, value))
             .await
             .map_err(|err| {
-                log::error!("Failed to set new access method!");
+                log::debug!("Failed to set new access method!");
                 err
             })
     }
@@ -84,14 +109,14 @@ impl AccessModeSelectorHandle {
         self.send_command(|tx| Message::Update(tx, values))
             .await
             .map_err(|err| {
-                log::error!("Failed to update new access methods!");
+                log::debug!("Failed to switch to a new set of access methods");
                 err
             })
     }
 
     pub async fn next(&self) -> Result<ApiConnectionMode> {
         self.send_command(Message::Next).await.map_err(|err| {
-            log::error!("Failed to update new access methods!");
+            log::debug!("Failed while getting the next access method");
             err
         })
     }
@@ -163,6 +188,7 @@ impl AccessModeSelector {
 
     async fn into_future(mut self) {
         while let Some(cmd) = self.cmd_rx.next().await {
+            log::trace!("Processing {cmd} command");
             let execution = match cmd {
                 Message::Get(tx) => self.on_get_access_method(tx),
                 Message::Set(tx, value) => self.on_set_access_method(tx, value),
@@ -171,13 +197,13 @@ impl AccessModeSelector {
             };
             match execution {
                 Ok(_) => (),
-                Err(err) => {
-                    log::trace!(
-                        "AccessModeSelector is going down due to {error}",
-                        error = err
-                    );
+                Err(error) if error.is_critical_error() => {
+                    log::error!("AccessModeSelector failed due to an internal error and won't be able to recover without a restart. {error}");
                     break;
                 }
+                Err(error) => {
+                    log::debug!("AccessModeSelector failed processing command due to {error}");
+                }
             }
         }
     }
diff --git a/mullvad-types/src/access_method.rs b/mullvad-types/src/access_method.rs
@@ -21,22 +21,34 @@ impl Settings {
         self.retain(|method| method.get_id() != *api_access_method)
     }
 
-    /// Search for a particular [`AccessMethod`] in `api_access_methods`.
-    pub fn find(&self, element: &Id) -> Option<&AccessMethodSetting> {
+    /// Search for any [`AccessMethod`] in `api_access_methods` which matches `predicate`.
+    pub fn find<P>(&self, predicate: P) -> Option<&AccessMethodSetting>
+    where
+        P: Fn(&AccessMethodSetting) -> bool,
+    {
         self.access_method_settings
             .iter()
-            .find(|api_access_method| *element == api_access_method.get_id())
+            .find(|api_access_method| predicate(api_access_method))
     }
 
-    /// Search for a particular [`AccessMethod`] in `api_access_methods`.
-    ///
-    /// If the [`AccessMethod`] is found to be part of `api_access_methods`, a
-    /// mutable reference to that inner element is returned. Otherwise, `None`
-    /// is returned.
-    pub fn find_mut(&mut self, element: &Id) -> Option<&mut AccessMethodSetting> {
+    /// Search for any [`AccessMethod`] in `api_access_methods`.
+    pub fn find_mut<P>(&mut self, predicate: P) -> Option<&mut AccessMethodSetting>
+    where
+        P: Fn(&AccessMethodSetting) -> bool,
+    {
         self.access_method_settings
             .iter_mut()
-            .find(|api_access_method| *element == api_access_method.get_id())
+            .find(|api_access_method| predicate(api_access_method))
+    }
+
+    /// Search for a particular [`AccessMethod`] in `api_access_methods`.
+    pub fn find_by_id(&self, element: &Id) -> Option<&AccessMethodSetting> {
+        self.find(|api_access_method| *element == api_access_method.get_id())
+    }
+
+    /// Search for a particular [`AccessMethod`] in `api_access_methods`.
+    pub fn find_by_id_mut(&mut self, element: &Id) -> Option<&mut AccessMethodSetting> {
+        self.find_mut(|api_access_method| *element == api_access_method.get_id())
     }
 
     /// Equivalent to [`Vec::retain`].
@@ -52,6 +64,13 @@ impl Settings {
         self.access_method_settings.clone()
     }
 
+    /// Get a reference to the `Direct` access method instance of this [`Settings`].
+    pub fn get_direct(&mut self) -> Option<&mut AccessMethodSetting> {
+        self.find_mut(|access_method| {
+            access_method.access_method == BuiltInAccessMethod::Direct.into()
+        })
+    }
+
     pub fn direct() -> AccessMethodSetting {
         let method = BuiltInAccessMethod::Direct;
         AccessMethodSetting::new(method.canonical_name(), true, AccessMethod::from(method))
