Prevent duplicate network ranges per customer (#681)

CHANGELOG.md

@@ -7,6 +7,14 @@ Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
 
+### Added
+
+- Added validation to prevent duplicate network ranges within a customer's
+  network list. When adding or updating networks under a Customer, the system
+  now rejects entries if any two networks have the same `network_group` value.
+  The new `IndexedTable<Customer>::insert` method validates on creation, and
+  the `Update::apply` method validates on updates.
+
 ### Changed
 
 - Changed `Store::network_tag_set` signature to require a `customer_id: u32`

src/tables/customer.rs

@@ -2,7 +2,7 @@
 
 use std::{borrow::Cow, net::IpAddr};
 
-use anyhow::Result;
+use anyhow::{Result, bail};
 use chrono::{DateTime, Utc};
 use rocksdb::OptimisticTransactionDB;
 use serde::{Deserialize, Serialize};
@@ -84,6 +84,21 @@ pub struct Update {
     pub networks: Option<Vec<Network>>,
 }
 
+/// Checks for duplicate network groups within a list of networks.
+///
+/// Returns `Some(name)` if a duplicate `network_group` is found (with the name of the
+/// first network that has the duplicate), or `None` if all network groups are unique.
+fn find_duplicate_network_group(networks: &[Network]) -> Option<&str> {
+    for (i, network) in networks.iter().enumerate() {
+        for other in networks.iter().skip(i + 1) {
+            if network.network_group == other.network_group {
+                return Some(&other.name);
+            }
+        }
+    }
+    None
+}
+
 impl IndexedMapUpdate for Update {
     type Entry = Customer;
 
@@ -101,6 +116,11 @@ impl IndexedMapUpdate for Update {
             value.description.push_str(description);
         }
         if let Some(networks) = self.networks.as_deref() {
+            if let Some(dup_name) = find_duplicate_network_group(networks) {
+                bail!(
+                    "network range already exists for this customer (duplicate in network \"{dup_name}\")"
+                );
+            }
             value.networks.clear();
             value.networks.extend(networks.iter().cloned());
         }
@@ -145,6 +165,24 @@ impl<'d> IndexedTable<'d, Customer> {
             .ok()
     }
 
+    /// Inserts a new customer after validating that there are no duplicate network
+    /// ranges within its networks list.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if:
+    /// * The customer has duplicate network ranges (same `network_group` value)
+    /// * A customer with the same name already exists
+    /// * The database operation fails
+    pub fn insert(&self, entry: Customer) -> Result<u32> {
+        if let Some(dup_name) = find_duplicate_network_group(&entry.networks) {
+            bail!(
+                "network range already exists for this customer (duplicate in network \"{dup_name}\")"
+            );
+        }
+        self.indexed_map.insert(entry)
+    }
+
     /// Updates the `Cutomer` from `old` to `new`, given `id`.
     ///
     /// # Errors
@@ -157,10 +195,11 @@ impl<'d> IndexedTable<'d, Customer> {
 
 #[cfg(test)]
 mod test {
-    use std::sync::Arc;
+    use std::{net::IpAddr, sync::Arc};
 
+    use crate::event::NetworkType;
     use crate::test::{DbGuard, acquire_db_permit};
-    use crate::{Customer, CustomerUpdate, Store};
+    use crate::{Customer, CustomerNetwork, CustomerUpdate, HostNetworkGroup, Store};
 
     #[test]
     fn update() {
@@ -188,6 +227,242 @@ mod test {
         assert_eq!(entry.map(|e| e.name), Some("b".to_string()));
     }
 
+    #[test]
+    fn insert_with_duplicate_network_range_fails() {
+        let (_permit, store) = setup_store();
+        let table = store.customer_map();
+
+        let network_group = HostNetworkGroup::new(
+            vec!["192.168.1.1".parse::<IpAddr>().unwrap()],
+            vec![],
+            vec![],
+        );
+
+        let networks = vec![
+            CustomerNetwork {
+                name: "network1".to_string(),
+                description: "First network".to_string(),
+                network_type: NetworkType::Intranet,
+                network_group: network_group.clone(),
+            },
+            CustomerNetwork {
+                name: "network2".to_string(),
+                description: "Second network with same range".to_string(),
+                network_type: NetworkType::Intranet,
+                network_group: network_group.clone(),
+            },
+        ];
+
+        let entry = Customer {
+            id: u32::MAX,
+            name: "customer_with_dup".to_string(),
+            description: "description".to_string(),
+            networks,
+            creation_time: chrono::Utc::now(),
+        };
+
+        let result = table.insert(entry);
+        assert!(result.is_err());
+        let err_msg = result.unwrap_err().to_string();
+        assert!(
+            err_msg.contains("network range already exists"),
+            "Expected error about duplicate network range, got: {err_msg}"
+        );
+    }
+
+    #[test]
+    fn insert_with_different_network_ranges_succeeds() {
+        let (_permit, store) = setup_store();
+        let table = store.customer_map();
+
+        let network_group1 = HostNetworkGroup::new(
+            vec!["192.168.1.1".parse::<IpAddr>().unwrap()],
+            vec![],
+            vec![],
+        );
+        let network_group2 = HostNetworkGroup::new(
+            vec!["192.168.1.2".parse::<IpAddr>().unwrap()],
+            vec![],
+            vec![],
+        );
+
+        let networks = vec![
+            CustomerNetwork {
+                name: "network1".to_string(),
+                description: "First network".to_string(),
+                network_type: NetworkType::Intranet,
+                network_group: network_group1,
+            },
+            CustomerNetwork {
+                name: "network2".to_string(),
+                description: "Second network with different range".to_string(),
+                network_type: NetworkType::Intranet,
+                network_group: network_group2,
+            },
+        ];
+
+        let entry = Customer {
+            id: u32::MAX,
+            name: "customer_with_diff_networks".to_string(),
+            description: "description".to_string(),
+            networks,
+            creation_time: chrono::Utc::now(),
+        };
+
+        let result = table.insert(entry);
+        assert!(result.is_ok());
+    }
+
+    #[test]
+    fn update_with_duplicate_network_range_fails() {
+        let (_permit, store) = setup_store();
+        let mut table = store.customer_map();
+
+        // Create customer with no networks
+        let entry = create_entry("customer_for_update");
+        let id = table.put(entry.clone()).unwrap();
+
+        let network_group =
+            HostNetworkGroup::new(vec!["10.0.0.1".parse::<IpAddr>().unwrap()], vec![], vec![]);
+
+        let duplicate_networks = vec![
+            CustomerNetwork {
+                name: "net_a".to_string(),
+                description: "Network A".to_string(),
+                network_type: NetworkType::Intranet,
+                network_group: network_group.clone(),
+            },
+            CustomerNetwork {
+                name: "net_b".to_string(),
+                description: "Network B with same range".to_string(),
+                network_type: NetworkType::Intranet,
+                network_group: network_group.clone(),
+            },
+        ];
+
+        let old = CustomerUpdate {
+            name: Some("customer_for_update".to_string()),
+            description: Some("description".to_string()),
+            networks: Some(vec![]),
+        };
+
+        let update = CustomerUpdate {
+            name: Some("customer_for_update".to_string()),
+            description: Some("description".to_string()),
+            networks: Some(duplicate_networks),
+        };
+
+        let result = table.update(id, &old, &update);
+        assert!(result.is_err());
+        // The error chain includes the root cause; check the full error chain
+        let err = result.unwrap_err();
+        let full_err = format!("{err:#}");
+        assert!(
+            full_err.contains("network range already exists"),
+            "Expected error about duplicate network range, got: {full_err}"
+        );
+    }
+
+    #[test]
+    fn update_with_different_network_ranges_succeeds() {
+        let (_permit, store) = setup_store();
+        let mut table = store.customer_map();
+
+        // Create customer with no networks
+        let entry = create_entry("customer_update_ok");
+        let id = table.put(entry.clone()).unwrap();
+
+        let network_group1 =
+            HostNetworkGroup::new(vec!["10.0.0.1".parse::<IpAddr>().unwrap()], vec![], vec![]);
+        let network_group2 =
+            HostNetworkGroup::new(vec!["10.0.0.2".parse::<IpAddr>().unwrap()], vec![], vec![]);
+
+        let networks = vec![
+            CustomerNetwork {
+                name: "net_a".to_string(),
+                description: "Network A".to_string(),
+                network_type: NetworkType::Intranet,
+                network_group: network_group1,
+            },
+            CustomerNetwork {
+                name: "net_b".to_string(),
+                description: "Network B".to_string(),
+                network_type: NetworkType::Intranet,
+                network_group: network_group2,
+            },
+        ];
+
+        let old = CustomerUpdate {
+            name: Some("customer_update_ok".to_string()),
+            description: Some("description".to_string()),
+            networks: Some(vec![]),
+        };
+
+        let update = CustomerUpdate {
+            name: Some("customer_update_ok".to_string()),
+            description: Some("description".to_string()),
+            networks: Some(networks),
+        };
+
+        let result = table.update(id, &old, &update);
+        assert!(result.is_ok());
+
+        // Verify the networks were saved
+        let saved = table.get_by_id(id).unwrap().unwrap();
+        assert_eq!(saved.networks.len(), 2);
+    }
+
+    #[test]
+    fn different_customers_can_have_same_network_range() {
+        let (_permit, store) = setup_store();
+        let table = store.customer_map();
+
+        let network_group = HostNetworkGroup::new(
+            vec!["172.16.0.1".parse::<IpAddr>().unwrap()],
+            vec![],
+            vec![],
+        );
+
+        // Create first customer with the network
+        let networks1 = vec![CustomerNetwork {
+            name: "shared_range".to_string(),
+            description: "Shared network range".to_string(),
+            network_type: NetworkType::Intranet,
+            network_group: network_group.clone(),
+        }];
+
+        let customer1 = Customer {
+            id: u32::MAX,
+            name: "customer1".to_string(),
+            description: "First customer".to_string(),
+            networks: networks1,
+            creation_time: chrono::Utc::now(),
+        };
+
+        let result1 = table.insert(customer1);
+        assert!(result1.is_ok());
+
+        // Create second customer with the same network range
+        let networks2 = vec![CustomerNetwork {
+            name: "shared_range".to_string(),
+            description: "Same network range, different customer".to_string(),
+            network_type: NetworkType::Intranet,
+            network_group: network_group.clone(),
+        }];
+
+        let customer2 = Customer {
+            id: u32::MAX,
+            name: "customer2".to_string(),
+            description: "Second customer".to_string(),
+            networks: networks2,
+            creation_time: chrono::Utc::now(),
+        };
+
+        // This should succeed because the duplicate check is per-customer
+        let result2 = table.insert(customer2);
+        assert!(result2.is_ok());
+    }
+
     fn setup_store() -> (DbGuard<'static>, Arc<Store>) {
         let permit = acquire_db_permit();
         let db_dir = tempfile::tempdir().unwrap();