Merge branch 'limit-custom-list-name-to-30-chars'

6 files changed, 113 insertions, 72 deletions

diff --git a/mullvad-daemon/src/custom_list.rs b/mullvad-daemon/src/custom_list.rs
index df9f30a519..71c5fc77e9 100644
--- a/mullvad-daemon/src/custom_list.rs
+++ b/mullvad-daemon/src/custom_list.rs
@@ -14,22 +14,11 @@ where
     ///
     /// Returns an error if the name is not unique.
     pub async fn create_custom_list(&mut self, name: String) -> Result<Id, crate::Error> {
-        if self
-            .settings
-            .custom_lists
-            .iter()
-            .any(|list| list.name == name)
-        {
-            return Err(Error::CustomListExists);
-        }
-
-        let new_list = CustomList::new(name);
+        let new_list = CustomList::new(name).map_err(crate::Error::CustomListError)?;
         let id = new_list.id;
 
         self.settings
-            .update(|settings| {
-                settings.custom_lists.add(new_list);
-            })
+            .try_update(|settings| settings.custom_lists.add(new_list))
             .await
             .map_err(Error::SettingsError)?;
 
@@ -40,20 +29,12 @@ where
     ///
     /// Returns an error if the list doesn't exist.
     pub async fn delete_custom_list(&mut self, id: Id) -> Result<(), Error> {
-        let Some(list_index) = self
-            .settings
-            .custom_lists
-            .iter()
-            .position(|elem| elem.id == id)
-        else {
-            return Err(Error::CustomListNotFound);
-        };
         let settings_changed = self
             .settings
-            .update(|settings| {
+            .try_update(|settings| {
                 // NOTE: Not using swap remove because it would make user output slightly
                 // more confusing and the cost is so small.
-                settings.custom_lists.remove(list_index);
+                settings.custom_lists.remove(&id)
             })
             .await
             .map_err(Error::SettingsError);
@@ -78,32 +59,10 @@ where
     /// - there is no existing list with the same ID,
     /// - or the existing list has a different name.
     pub async fn update_custom_list(&mut self, new_list: CustomList) -> Result<(), Error> {
-        let Some((list_index, old_list)) = self
-            .settings
-            .custom_lists
-            .iter()
-            .enumerate()
-            .find(|elem| elem.1.id == new_list.id)
-        else {
-            return Err(Error::CustomListNotFound);
-        };
-        let id = old_list.id;
-
-        if old_list.name != new_list.name
-            && self
-                .settings
-                .custom_lists
-                .iter()
-                .any(|list| list.name == new_list.name)
-        {
-            return Err(Error::CustomListExists);
-        }
-
+        let list_id = new_list.id;
         let settings_changed = self
             .settings
-            .update(|settings| {
-                settings.custom_lists[list_index] = new_list;
-            })
+            .try_update(|settings| settings.custom_lists.update(new_list))
             .await
             .map_err(Error::SettingsError);
 
@@ -111,7 +70,7 @@ where
             self.relay_selector
                 .set_config(new_selector_config(&self.settings));
 
-            if self.change_should_cause_reconnect(Some(id)) {
+            if self.change_should_cause_reconnect(Some(list_id)) {
                 log::info!("Initiating tunnel restart because a selected custom list changed");
                 self.reconnect_tunnel();
             }
diff --git a/mullvad-daemon/src/lib.rs b/mullvad-daemon/src/lib.rs
index a6b6794d33..875f797ec3 100644
--- a/mullvad-daemon/src/lib.rs
+++ b/mullvad-daemon/src/lib.rs
@@ -173,12 +173,8 @@ pub enum Error {
     TunnelError(#[source] tunnel_state_machine::Error),
 
     /// Custom list already exists
-    #[error("A list with that name already exists")]
-    CustomListExists,
-
-    /// Custom list does not exist
-    #[error("A list with that name does not exist")]
-    CustomListNotFound,
+    #[error("Custom list error: {0}")]
+    CustomListError(#[source] mullvad_types::custom_list::Error),
 
     #[error("Access method error")]
     AccessMethodError(#[source] access_method::Error),
diff --git a/mullvad-daemon/src/management_interface.rs b/mullvad-daemon/src/management_interface.rs
index ce203e7ba8..0699ab697e 100644
--- a/mullvad-daemon/src/management_interface.rs
+++ b/mullvad-daemon/src/management_interface.rs
@@ -1115,16 +1115,6 @@ fn map_daemon_error(error: crate::Error) -> Status {
         DaemonError::NoAccountToken | DaemonError::NoAccountTokenHistory => {
             Status::unauthenticated(error.to_string())
         }
-        DaemonError::CustomListExists => Status::with_details(
-            Code::AlreadyExists,
-            error.to_string(),
-            mullvad_management_interface::CUSTOM_LIST_LIST_EXISTS_DETAILS.into(),
-        ),
-        DaemonError::CustomListNotFound => Status::with_details(
-            Code::NotFound,
-            error.to_string(),
-            mullvad_management_interface::CUSTOM_LIST_LIST_NOT_FOUND_DETAILS.into(),
-        ),
         error => Status::unknown(error.to_string()),
     }
 }
diff --git a/mullvad-daemon/src/settings/mod.rs b/mullvad-daemon/src/settings/mod.rs
index 243764ec90..1e710eebba 100644
--- a/mullvad-daemon/src/settings/mod.rs
+++ b/mullvad-daemon/src/settings/mod.rs
@@ -1,5 +1,7 @@
 #[cfg(not(target_os = "android"))]
 use futures::TryFutureExt;
+#[cfg(not(target_os = "android"))]
+use mullvad_types::custom_list::Error as CustomListError;
 use mullvad_types::{
     relay_constraints::{RelayConstraints, RelaySettings, WireguardConstraints},
     settings::{DnsState, Settings},
@@ -47,11 +49,18 @@ pub enum Error {
 impl From<Error> for mullvad_management_interface::Status {
     fn from(error: Error) -> mullvad_management_interface::Status {
         use mullvad_management_interface::{Code, Status};
-
         match error {
             Error::DeleteError(..) | Error::WriteError(..) | Error::ReadError(..) => {
                 Status::new(Code::FailedPrecondition, error.to_string())
             }
+            Error::UpdateFailed(err)
+                if err
+                    .downcast_ref::<mullvad_types::custom_list::Error>()
+                    .is_some() =>
+            {
+                let custom_list_err = *err.downcast::<CustomListError>().unwrap();
+                handle_custom_list_error(custom_list_err)
+            }
             Error::SerializeError(..) | Error::ParseError(..) | Error::UpdateFailed(..) => {
                 Status::new(Code::Internal, error.to_string())
             }
@@ -59,6 +68,32 @@ impl From<Error> for mullvad_management_interface::Status {
     }
 }
 
+#[cfg(not(target_os = "android"))]
+fn handle_custom_list_error(
+    custom_list_err: CustomListError,
+) -> mullvad_management_interface::Status {
+    use mullvad_management_interface::{Code, Status};
+    match custom_list_err {
+        error @ CustomListError::ListExists | error @ CustomListError::DuplicateName => {
+            Status::with_details(
+                Code::AlreadyExists,
+                error.to_string(),
+                mullvad_management_interface::CUSTOM_LIST_LIST_EXISTS_DETAILS.into(),
+            )
+        }
+        error @ CustomListError::NameTooLong => Status::with_details(
+            Code::InvalidArgument,
+            error.to_string(),
+            mullvad_management_interface::CUSTOM_LIST_LIST_NAME_TOO_LONG_DETAILS.into(),
+        ),
+        error @ CustomListError::ListNotFound => Status::with_details(
+            Code::NotFound,
+            error.to_string(),
+            mullvad_management_interface::CUSTOM_LIST_LIST_NOT_FOUND_DETAILS.into(),
+        ),
+    }
+}
+
 pub struct SettingsPersister {
     settings: Settings,
     path: PathBuf,
diff --git a/mullvad-management-interface/src/lib.rs b/mullvad-management-interface/src/lib.rs
index 882db86ae1..763d2474ec 100644
--- a/mullvad-management-interface/src/lib.rs
+++ b/mullvad-management-interface/src/lib.rs
@@ -28,6 +28,7 @@ static MULLVAD_MANAGEMENT_SOCKET_GROUP: Lazy<Option<String>> =
 
 pub const CUSTOM_LIST_LIST_NOT_FOUND_DETAILS: &[u8] = b"custom_list_list_not_found";
 pub const CUSTOM_LIST_LIST_EXISTS_DETAILS: &[u8] = b"custom_list_list_exists";
+pub const CUSTOM_LIST_LIST_NAME_TOO_LONG_DETAILS: &[u8] = b"custom_list_list_name_too_long";
 
 #[derive(thiserror::Error, Debug)]
 pub enum Error {
diff --git a/mullvad-types/src/custom_list.rs b/mullvad-types/src/custom_list.rs
index 89febe81ab..0f6ca7d461 100644
--- a/mullvad-types/src/custom_list.rs
+++ b/mullvad-types/src/custom_list.rs
@@ -11,6 +11,20 @@ use std::{
     str::FromStr,
 };
 
+const CUSTOM_LIST_NAME_MAX_SIZE: usize = 30;
+
+#[derive(thiserror::Error, Debug)]
+pub enum Error {
+    #[error("Custom list name too long")]
+    NameTooLong,
+    #[error("Custom list with name already exists")]
+    DuplicateName,
+    #[error("Custom list not found")]
+    ListNotFound,
+    #[error("List with given ID already exists")]
+    ListExists,
+}
+
 #[derive(Default, Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
 pub struct Id(uuid::Uuid);
 
@@ -91,18 +105,60 @@ impl From<Vec<CustomList>> for CustomListsSettings {
 }
 
 impl CustomListsSettings {
-    pub fn add(&mut self, list: CustomList) {
-        self.custom_lists.push(list);
+    pub fn add(&mut self, new_list: CustomList) -> Result<(), Error> {
+        self.check_if_id_is_unique(&new_list)?;
+        self.check_list_name_is_unique(&new_list)?;
+        self.custom_lists.push(new_list);
+        Ok(())
     }
 
-    pub fn remove(&mut self, index: usize) {
-        self.custom_lists.remove(index);
+    pub fn remove(&mut self, list_id: &Id) -> Result<(), Error> {
+        let Some(list_index) = self.find_list_index(list_id) else {
+            return Err(Error::ListNotFound);
+        };
+        self.custom_lists.remove(list_index);
+        Ok(())
     }
 
     /// Remove all custom lists
     pub fn clear(&mut self) {
         self.custom_lists.clear();
     }
+
+    pub fn update(&mut self, new_list: CustomList) -> Result<(), Error> {
+        let list_index = self
+            .find_list_index(&new_list.id)
+            .ok_or(Error::ListNotFound)?;
+        self.check_list_name_is_unique(&new_list)?;
+        self.custom_lists[list_index] = new_list;
+        Ok(())
+    }
+
+    fn check_list_name_is_unique(&self, new_list: &CustomList) -> Result<(), Error> {
+        if self
+            .custom_lists
+            .iter()
+            .any(|list| list.name == new_list.name && list.id != new_list.id)
+        {
+            return Err(Error::DuplicateName);
+        }
+        Ok(())
+    }
+
+    fn check_if_id_is_unique(&self, new_list: &CustomList) -> Result<(), Error> {
+        if self.custom_lists.iter().any(|list| list.id == new_list.id) {
+            return Err(Error::ListExists);
+        }
+        Ok(())
+    }
+
+    fn find_list_index(&self, list_id: &Id) -> Option<usize> {
+        self.custom_lists
+            .iter()
+            .enumerate()
+            .find(|(_idx, list)| list.id == *list_id)
+            .map(|(idx, _list)| idx)
+    }
 }
 
 impl IntoIterator for CustomListsSettings {
@@ -144,12 +200,16 @@ pub struct CustomList {
 }
 
 impl CustomList {
-    pub fn new(name: String) -> Self {
-        CustomList {
+    pub fn new(name: String) -> Result<Self, Error> {
+        if name.chars().count() > CUSTOM_LIST_NAME_MAX_SIZE {
+            return Err(Error::NameTooLong);
+        }
+
+        Ok(CustomList {
             id: Id(uuid::Uuid::new_v4()),
             name,
             locations: BTreeSet::new(),
-        }
+        })
     }
 }