[pve-devel] [PATCH v3 proxmox 21/66] notify: ensure that filter/group/endpoint names are unique

[Lukas Wagner <l.wagner@proxmox.com>]

Otherwise, a filter with the same name as an already existing
endpoint or group can overwrite it.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/src/api/filter.rs   |  7 +----
 proxmox-notify/src/api/gotify.rs   | 10 +------
 proxmox-notify/src/api/group.rs    | 24 ++-------------
 proxmox-notify/src/api/mod.rs      | 47 ++++++++++++++++++++++++++++--
 proxmox-notify/src/api/sendmail.rs |  7 +----
 5 files changed, 51 insertions(+), 44 deletions(-)

diff --git a/proxmox-notify/src/api/filter.rs b/proxmox-notify/src/api/filter.rs
index 824f802d..b5b62849 100644
--- a/proxmox-notify/src/api/filter.rs
+++ b/proxmox-notify/src/api/filter.rs
@@ -31,12 +31,7 @@ pub fn get_filter(config: &Config, name: &str) -> Result<FilterConfig, ApiError>
 /// Returns an `ApiError` if a filter with the same name already exists or
 /// if the filter could not be saved.
 pub fn add_filter(config: &mut Config, filter_config: &FilterConfig) -> Result<(), ApiError> {
-    if get_filter(config, &filter_config.name).is_ok() {
-        return Err(ApiError::bad_request(
-            format!("filter '{}' already exists", filter_config.name),
-            None,
-        ));
-    }
+    super::ensure_unique(config, &filter_config.name)?;
 
     config
         .config
diff --git a/proxmox-notify/src/api/gotify.rs b/proxmox-notify/src/api/gotify.rs
index 5c4db4be..521dd167 100644
--- a/proxmox-notify/src/api/gotify.rs
+++ b/proxmox-notify/src/api/gotify.rs
@@ -43,15 +43,7 @@ pub fn add_endpoint(
         panic!("name for endpoint config and private config must be identical");
     }
 
-    if super::endpoint_exists(config, &endpoint_config.name) {
-        return Err(ApiError::bad_request(
-            format!(
-                "endpoint with name '{}' already exists!",
-                endpoint_config.name
-            ),
-            None,
-        ));
-    }
+    super::ensure_unique(config, &endpoint_config.name)?;
 
     if let Some(filter) = &endpoint_config.filter {
         // Check if filter exists
diff --git a/proxmox-notify/src/api/group.rs b/proxmox-notify/src/api/group.rs
index fe3de12f..d6b8f38d 100644
--- a/proxmox-notify/src/api/group.rs
+++ b/proxmox-notify/src/api/group.rs
@@ -31,12 +31,7 @@ pub fn get_group(config: &Config, name: &str) -> Result<GroupConfig, ApiError> {
 /// Returns an `ApiError` if a group with the same name already exists, or
 /// if the group could not be saved
 pub fn add_group(config: &mut Config, group_config: &GroupConfig) -> Result<(), ApiError> {
-    if get_group(config, &group_config.name).is_ok() {
-        return Err(ApiError::bad_request(
-            format!("group '{}' already exists", group_config.name),
-            None,
-        ));
-    }
+    super::ensure_unique(config, &group_config.name)?;
 
     if group_config.endpoint.is_empty() {
         return Err(ApiError::bad_request(
@@ -50,7 +45,7 @@ pub fn add_group(config: &mut Config, group_config: &GroupConfig) -> Result<(),
         super::filter::get_filter(config, filter)?;
     }
 
-    check_if_endpoints_exist(config, &group_config.endpoint)?;
+    super::ensure_endpoints_exist(config, &group_config.endpoint)?;
 
     config
         .config
@@ -91,7 +86,7 @@ pub fn update_group(
     }
 
     if let Some(endpoints) = &updater.endpoint {
-        check_if_endpoints_exist(config, endpoints)?;
+        super::ensure_endpoints_exist(config, endpoints)?;
         if endpoints.is_empty() {
             return Err(ApiError::bad_request(
                 "group must contain at least one endpoint",
@@ -138,19 +133,6 @@ pub fn delete_group(config: &mut Config, name: &str) -> Result<(), ApiError> {
     Ok(())
 }
 
-fn check_if_endpoints_exist(config: &Config, endpoints: &[String]) -> Result<(), ApiError> {
-    for endpoint in endpoints {
-        if !super::endpoint_exists(config, endpoint) {
-            return Err(ApiError::not_found(
-                format!("endoint '{endpoint}' does not exist"),
-                None,
-            ));
-        }
-    }
-
-    Ok(())
-}
-
 // groups cannot be empty, so only  build the tests if we have the
 // sendmail endpoint available
 #[cfg(all(test, feature = "sendmail"))]
diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
index 81c182c7..994af4d3 100644
--- a/proxmox-notify/src/api/mod.rs
+++ b/proxmox-notify/src/api/mod.rs
@@ -87,7 +87,7 @@ fn verify_digest(config: &Config, digest: Option<&[u8]>) -> Result<(), ApiError>
     Ok(())
 }
 
-fn endpoint_exists(config: &Config, name: &str) -> bool {
+fn ensure_endpoint_exists(config: &Config, name: &str) -> Result<(), ApiError> {
     let mut exists = false;
 
     #[cfg(feature = "sendmail")]
@@ -99,7 +99,33 @@ fn endpoint_exists(config: &Config, name: &str) -> bool {
         exists = exists || gotify::get_endpoint(config, name).is_ok();
     }
 
-    exists
+    if !exists {
+        Err(ApiError::not_found(
+            format!("endpoint '{name}' does not exist"),
+            None,
+        ))
+    } else {
+        Ok(())
+    }
+}
+
+fn ensure_endpoints_exist<T: AsRef<str>>(config: &Config, endpoints: &[T]) -> Result<(), ApiError> {
+    for endpoint in endpoints {
+        ensure_endpoint_exists(config, endpoint.as_ref())?;
+    }
+
+    Ok(())
+}
+
+fn ensure_unique(config: &Config, entity: &str) -> Result<(), ApiError> {
+    if config.config.sections.contains_key(entity) {
+        return Err(ApiError::bad_request(
+            format!("Cannot create '{entity}', an entity with the same name already exists"),
+            None,
+        ));
+    }
+
+    Ok(())
 }
 
 fn get_referrers(config: &Config, entity: &str) -> Result<HashSet<String>, ApiError> {
@@ -326,4 +352,21 @@ mod tests {
         assert!(ensure_unused(&config, "sendmail").is_err());
         assert!(ensure_unused(&config, "group").is_ok());
     }
+
+    #[test]
+    fn test_ensure_unique() {
+        let config = prepare_config().unwrap();
+
+        assert!(ensure_unique(&config, "sendmail").is_err());
+        assert!(ensure_unique(&config, "group").is_err());
+        assert!(ensure_unique(&config, "new").is_ok());
+    }
+
+    #[test]
+    fn test_ensure_endpoints_exist() {
+        let config = prepare_config().unwrap();
+
+        assert!(ensure_endpoints_exist(&config, &vec!["sendmail", "gotify"]).is_ok());
+        assert!(ensure_endpoints_exist(&config, &vec!["group", "filter"]).is_err());
+    }
 }
diff --git a/proxmox-notify/src/api/sendmail.rs b/proxmox-notify/src/api/sendmail.rs
index bf225f29..59a57b47 100644
--- a/proxmox-notify/src/api/sendmail.rs
+++ b/proxmox-notify/src/api/sendmail.rs
@@ -33,12 +33,7 @@ pub fn get_endpoint(config: &Config, name: &str) -> Result<SendmailConfig, ApiEr
 /// Returns an `ApiError` if an endpoint with the same name already exists,
 /// or if the endpoint could not be saved.
 pub fn add_endpoint(config: &mut Config, endpoint: &SendmailConfig) -> Result<(), ApiError> {
-    if super::endpoint_exists(config, &endpoint.name) {
-        return Err(ApiError::bad_request(
-            format!("endpoint with name '{}' already exists!", &endpoint.name),
-            None,
-        ));
-    }
+    super::ensure_unique(config, &endpoint.name)?;
 
     if let Some(filter) = &endpoint.filter {
         // Check if filter exists
-- 
2.39.2