diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a3453ca..3edd0ee8 100644 --- a/CHANGELOG.md +++ b/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::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` diff --git a/src/collections.rs b/src/collections.rs index c97647b7..114207aa 100644 --- a/src/collections.rs +++ b/src/collections.rs @@ -559,12 +559,8 @@ pub trait Indexed { }; let new_entry = new.apply(entry.into()); - txn.put_cf( - self.cf(), - new_key, - new_entry.context("invalid update")?.value(), - ) - .context("failed to write updated entry")?; + txn.put_cf(self.cf(), new_key, new_entry?.value()) + .context("failed to write updated entry")?; txn.put_cf( self.cf(), [], @@ -653,12 +649,8 @@ pub trait Indexed { }; let new_entry = new.apply(entry.into()); - txn.put_cf( - self.cf(), - new_key, - new_entry.context("invalid update")?.value(), - ) - .context("failed to write updated entry")?; + txn.put_cf(self.cf(), new_key, new_entry?.value()) + .context("failed to write updated entry")?; txn.put_cf( self.cf(), [], diff --git a/src/tables/customer.rs b/src/tables/customer.rs index 671fb54d..2f04d453 100644 --- a/src/tables/customer.rs +++ b/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>, } +/// Returns an error if a duplicate `network_group` exists in the provided list. +fn check_duplicate_network_group(networks: &[Network]) -> Result<()> { + for (i, network) in networks.iter().enumerate() { + for other in networks.iter().skip(i + 1) { + if network.network_group == other.network_group { + bail!( + "network range already exists for this customer: {}", + other.name + ); + } + } + } + Ok(()) +} + impl IndexedMapUpdate for Update { type Entry = Customer; @@ -101,6 +116,7 @@ impl IndexedMapUpdate for Update { value.description.push_str(description); } if let Some(networks) = self.networks.as_deref() { + check_duplicate_network_group(networks)?; value.networks.clear(); value.networks.extend(networks.iter().cloned()); } @@ -145,6 +161,20 @@ 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 { + check_duplicate_network_group(&entry.networks)?; + self.indexed_map.insert(entry) + } + /// Updates the `Cutomer` from `old` to `new`, given `id`. /// /// # Errors @@ -157,10 +187,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 +219,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::().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::().unwrap()], + vec![], + vec![], + ); + let network_group2 = HostNetworkGroup::new( + vec!["192.168.1.2".parse::().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::().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::().unwrap()], vec![], vec![]); + let network_group2 = + HostNetworkGroup::new(vec!["10.0.0.2".parse::().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::().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) { let permit = acquire_db_permit(); let db_dir = tempfile::tempdir().unwrap();