diff --git a/rust/agama-dbus-server/src/network.rs b/rust/agama-dbus-server/src/network.rs index 35158a10a8..536fdfdbf8 100644 --- a/rust/agama-dbus-server/src/network.rs +++ b/rust/agama-dbus-server/src/network.rs @@ -47,7 +47,7 @@ mod nm; pub mod system; pub use action::Action; -pub use adapter::Adapter; +pub use adapter::{Adapter, NetworkAdapterError}; pub use dbus::NetworkService; pub use model::NetworkState; pub use nm::NetworkManagerAdapter; diff --git a/rust/agama-dbus-server/src/network/action.rs b/rust/agama-dbus-server/src/network/action.rs index 0b3d00b5c2..3b98ee912f 100644 --- a/rust/agama-dbus-server/src/network/action.rs +++ b/rust/agama-dbus-server/src/network/action.rs @@ -4,7 +4,7 @@ use tokio::sync::oneshot; use uuid::Uuid; use zbus::zvariant::OwnedObjectPath; -use super::error::NetworkStateError; +use super::{error::NetworkStateError, NetworkAdapterError}; pub type Responder = oneshot::Sender; pub type ControllerConnection = (Connection, Vec); @@ -24,7 +24,9 @@ pub enum Action { /// Gets a connection GetConnection(Uuid, Responder>), /// Gets a connection - GetConnectionPath(String, Responder>), + GetConnectionPath(Uuid, Responder>), + /// Gets a connection + GetConnectionPathById(String, Responder>), /// Get connections paths GetConnectionsPaths(Responder>), /// Gets a controller connection @@ -44,7 +46,7 @@ pub enum Action { /// Update a connection (replacing the old one). UpdateConnection(Box), /// Remove the connection with the given Uuid. - RemoveConnection(String), + RemoveConnection(Uuid), /// Apply the current configuration. - Apply, + Apply(Responder>), } diff --git a/rust/agama-dbus-server/src/network/adapter.rs b/rust/agama-dbus-server/src/network/adapter.rs index c96ce8e649..8aba3251db 100644 --- a/rust/agama-dbus-server/src/network/adapter.rs +++ b/rust/agama-dbus-server/src/network/adapter.rs @@ -1,10 +1,27 @@ use crate::network::NetworkState; +use agama_lib::error::ServiceError; use async_trait::async_trait; -use std::error::Error; +use thiserror::Error; + +#[derive(Error, Debug)] +pub enum NetworkAdapterError { + #[error("Could not read the network configuration: {0}")] + Read(ServiceError), + #[error("Could not update the network configuration: {0}")] + Write(ServiceError), + #[error("Checkpoint handling error: {0}")] + Checkpoint(ServiceError), // only relevant for adapters that implement a checkpoint mechanism +} /// A trait for the ability to read/write from/to a network service #[async_trait] pub trait Adapter { - async fn read(&self) -> Result>; - async fn write(&self, network: &NetworkState) -> Result<(), Box>; + async fn read(&self) -> Result; + async fn write(&self, network: &NetworkState) -> Result<(), NetworkAdapterError>; +} + +impl From for zbus::fdo::Error { + fn from(value: NetworkAdapterError) -> zbus::fdo::Error { + zbus::fdo::Error::Failed(value.to_string()) + } } diff --git a/rust/agama-dbus-server/src/network/dbus/interfaces/connection_configs.rs b/rust/agama-dbus-server/src/network/dbus/interfaces/connection_configs.rs index c0967bb93e..f35d82f489 100644 --- a/rust/agama-dbus-server/src/network/dbus/interfaces/connection_configs.rs +++ b/rust/agama-dbus-server/src/network/dbus/interfaces/connection_configs.rs @@ -198,6 +198,20 @@ impl Wireless { .await?; Ok(()) } + + /// Whether the network is hidden or not. + #[dbus_interface(property)] + pub async fn hidden(&self) -> zbus::fdo::Result { + let config = self.get_config::().await?; + Ok(config.hidden) + } + + #[dbus_interface(property)] + pub async fn set_hidden(&mut self, hidden: bool) -> zbus::fdo::Result<()> { + self.update_config::(|c| c.hidden = hidden) + .await?; + Ok(()) + } } #[async_trait] diff --git a/rust/agama-dbus-server/src/network/dbus/interfaces/connections.rs b/rust/agama-dbus-server/src/network/dbus/interfaces/connections.rs index 9fa57d7e6f..885aefb042 100644 --- a/rust/agama-dbus-server/src/network/dbus/interfaces/connections.rs +++ b/rust/agama-dbus-server/src/network/dbus/interfaces/connections.rs @@ -60,14 +60,31 @@ impl Connections { Ok(path) } - /// Returns the D-Bus path of the network connection. + /// Returns the D-Bus path of the network connection by its UUID. + /// + /// * `uuid`: connection UUID. + pub async fn get_connection(&self, uuid: &str) -> zbus::fdo::Result { + let uuid: Uuid = uuid + .parse() + .map_err(|_| NetworkStateError::InvalidUuid(uuid.to_string()))?; + let actions = self.actions.lock().await; + let (tx, rx) = oneshot::channel(); + actions.send(Action::GetConnectionPath(uuid, tx)).unwrap(); + let path = rx + .await + .unwrap() + .ok_or(NetworkStateError::UnknownConnection(uuid.to_string()))?; + Ok(path) + } + + /// Returns the D-Bus path of the network connection by its ID. /// /// * `id`: connection ID. - pub async fn get_connection(&self, id: &str) -> zbus::fdo::Result { + pub async fn get_connection_by_id(&self, id: &str) -> zbus::fdo::Result { let actions = self.actions.lock().await; let (tx, rx) = oneshot::channel(); actions - .send(Action::GetConnectionPath(id.to_string(), tx)) + .send(Action::GetConnectionPathById(id.to_string(), tx)) .unwrap(); let path = rx .await @@ -79,11 +96,12 @@ impl Connections { /// Removes a network connection. /// /// * `uuid`: connection UUID.. - pub async fn remove_connection(&mut self, id: &str) -> zbus::fdo::Result<()> { + pub async fn remove_connection(&mut self, uuid: &str) -> zbus::fdo::Result<()> { + let uuid = uuid + .parse() + .map_err(|_| NetworkStateError::InvalidUuid(uuid.to_string()))?; let actions = self.actions.lock().await; - actions - .send(Action::RemoveConnection(id.to_string())) - .unwrap(); + actions.send(Action::RemoveConnection(uuid)).unwrap(); Ok(()) } @@ -92,7 +110,9 @@ impl Connections { /// It includes adding, updating and removing connections as needed. pub async fn apply(&self) -> zbus::fdo::Result<()> { let actions = self.actions.lock().await; - actions.send(Action::Apply).unwrap(); + let (tx, rx) = oneshot::channel(); + actions.send(Action::Apply(tx)).unwrap(); + rx.await.unwrap()?; Ok(()) } diff --git a/rust/agama-dbus-server/src/network/dbus/tree.rs b/rust/agama-dbus-server/src/network/dbus/tree.rs index 149d9ab1ef..a1872c2da9 100644 --- a/rust/agama-dbus-server/src/network/dbus/tree.rs +++ b/rust/agama-dbus-server/src/network/dbus/tree.rs @@ -1,4 +1,5 @@ use agama_lib::error::ServiceError; +use uuid::Uuid; use zbus::zvariant::{ObjectPath, OwnedObjectPath}; use crate::network::{action::Action, dbus::interfaces, model::*}; @@ -77,15 +78,15 @@ impl Tree { /// * `notify`: whether to notify the added connection pub async fn add_connection( &mut self, - conn: &mut Connection, + conn: &Connection, ) -> Result { let uuid = conn.uuid; - let (id, path) = self.objects.register_connection(conn); - if id != conn.id { - conn.id = id.clone(); - } - let path: OwnedObjectPath = path.into(); - log::info!("Publishing network connection '{}' on '{}'", id, &path); + let path: OwnedObjectPath = self.objects.register_connection(conn.uuid).into(); + log::info!( + "Publishing network connection '{}' on '{}'", + &conn.id, + &path + ); self.add_interface( &path, @@ -114,13 +115,13 @@ impl Tree { /// Removes a connection from the tree /// - /// * `id`: connection ID. - pub async fn remove_connection(&mut self, id: &str) -> Result<(), ServiceError> { - let Some(path) = self.objects.connection_path(id) else { + /// * `uuid`: connection UUID. + pub async fn remove_connection(&mut self, uuid: Uuid) -> Result<(), ServiceError> { + let Some(path) = self.objects.connection_path(uuid) else { return Ok(()); }; self.remove_connection_on(path.as_str()).await?; - self.objects.deregister_connection(id).unwrap(); + self.objects.deregister_connection(uuid).unwrap(); Ok(()) } @@ -134,8 +135,8 @@ impl Tree { self.objects.connections_paths() } - pub fn connection_path(&self, id: &str) -> Option { - self.objects.connection_path(id).map(|o| o.into()) + pub fn connection_path(&self, uuid: Uuid) -> Option { + self.objects.connection_path(uuid).map(|o| o.into()) } /// Adds connections to the D-Bus tree. @@ -187,6 +188,7 @@ impl Tree { _ = object_server.remove::(path).await; _ = object_server.remove::(path).await; object_server.remove::(path).await?; + object_server.remove::(path).await?; object_server .remove::(path) .await?; @@ -210,7 +212,7 @@ struct ObjectsRegistry { /// device_name (eth0) -> object_path devices: HashMap, /// id -> object_path - connections: HashMap, + connections: HashMap, } impl ObjectsRegistry { @@ -227,30 +229,26 @@ impl ObjectsRegistry { /// It returns the connection Id and the D-Bus path. Bear in mind that the Id can be different /// in case the original one already existed. /// - /// * `conn`: network connection. - pub fn register_connection(&mut self, conn: &Connection) -> (String, ObjectPath) { + /// * `uuid`: network connection's UUID. + pub fn register_connection(&mut self, uuid: Uuid) -> ObjectPath { let path = format!("{}/{}", CONNECTIONS_PATH, self.connections.len()); let path = ObjectPath::try_from(path).unwrap(); - let mut id = conn.id.clone(); - if self.connection_path(&id).is_some() { - id = self.propose_id(&id); - }; - self.connections.insert(id.clone(), path.clone().into()); - (id, path) + self.connections.insert(uuid, path.clone().into()); + path } /// Returns the path for a connection. /// - /// * `id`: connection ID. - pub fn connection_path(&self, id: &str) -> Option { - self.connections.get(id).map(|p| p.as_ref()) + /// * `uuid`: connection ID. + pub fn connection_path(&self, uuid: Uuid) -> Option { + self.connections.get(&uuid).map(|p| p.as_ref()) } /// Deregisters a network connection. /// /// * `id`: connection ID. - pub fn deregister_connection(&mut self, id: &str) -> Option { - self.connections.remove(id) + pub fn deregister_connection(&mut self, uuid: Uuid) -> Option { + self.connections.remove(&uuid) } /// Returns all devices paths. @@ -262,18 +260,4 @@ impl ObjectsRegistry { pub fn connections_paths(&self) -> Vec { self.connections.values().cloned().collect() } - - /// Proposes a connection ID. - /// - /// * `id`: original connection ID. - fn propose_id(&self, id: &str) -> String { - let prefix = format!("{}-", id); - let filtered: Vec<_> = self - .connections - .keys() - .filter_map(|i| i.strip_prefix(&prefix).and_then(|n| n.parse::().ok())) - .collect(); - let index = filtered.into_iter().max().unwrap_or(0); - format!("{}{}", prefix, index + 1) - } } diff --git a/rust/agama-dbus-server/src/network/error.rs b/rust/agama-dbus-server/src/network/error.rs index 04819a971b..f8df1e916b 100644 --- a/rust/agama-dbus-server/src/network/error.rs +++ b/rust/agama-dbus-server/src/network/error.rs @@ -1,6 +1,5 @@ //! Error types. use thiserror::Error; -use uuid::Uuid; /// Errors that are related to the network configuration. #[derive(Error, Debug)] @@ -16,7 +15,7 @@ pub enum NetworkStateError { #[error("Invalid wireless mode: '{0}'")] InvalidWirelessMode(String), #[error("Connection '{0}' already exists")] - ConnectionExists(Uuid), + ConnectionExists(String), #[error("Invalid security wireless protocol: '{0}'")] InvalidSecurityProtocol(String), #[error("Adapter error: '{0}'")] diff --git a/rust/agama-dbus-server/src/network/model.rs b/rust/agama-dbus-server/src/network/model.rs index 98de6914fc..4a963b07d0 100644 --- a/rust/agama-dbus-server/src/network/model.rs +++ b/rust/agama-dbus-server/src/network/model.rs @@ -43,11 +43,18 @@ impl NetworkState { /// Get connection by UUID /// - /// * `id`: connection UUID + /// * `uuid`: connection UUID pub fn get_connection_by_uuid(&self, uuid: Uuid) -> Option<&Connection> { self.connections.iter().find(|c| c.uuid == uuid) } + /// Get connection by UUID as mutable + /// + /// * `uuid`: connection UUID + pub fn get_connection_by_uuid_mut(&mut self, uuid: Uuid) -> Option<&mut Connection> { + self.connections.iter_mut().find(|c| c.uuid == uuid) + } + /// Get connection by interface /// /// * `name`: connection interface name @@ -85,7 +92,7 @@ impl NetworkState { /// It uses the `id` to decide whether the connection already exists. pub fn add_connection(&mut self, conn: Connection) -> Result<(), NetworkStateError> { if self.get_connection(&conn.id).is_some() { - return Err(NetworkStateError::ConnectionExists(conn.uuid)); + return Err(NetworkStateError::ConnectionExists(conn.id)); } self.connections.push(conn); @@ -109,9 +116,9 @@ impl NetworkState { /// Removes a connection from the state. /// /// Additionally, it registers the connection to be removed when the changes are applied. - pub fn remove_connection(&mut self, id: &str) -> Result<(), NetworkStateError> { - let Some(conn) = self.get_connection_mut(id) else { - return Err(NetworkStateError::UnknownConnection(id.to_string())); + pub fn remove_connection(&mut self, uuid: Uuid) -> Result<(), NetworkStateError> { + let Some(conn) = self.get_connection_by_uuid_mut(uuid) else { + return Err(NetworkStateError::UnknownConnection(uuid.to_string())); }; conn.remove(); @@ -268,10 +275,10 @@ mod tests { #[test] fn test_remove_connection() { let mut state = NetworkState::default(); - let id = "eth0".to_string(); - let conn0 = Connection::new(id, DeviceType::Ethernet); + let conn0 = Connection::new("eth0".to_string(), DeviceType::Ethernet); + let uuid = conn0.uuid; state.add_connection(conn0).unwrap(); - state.remove_connection("eth0").unwrap(); + state.remove_connection(uuid).unwrap(); let found = state.get_connection("eth0").unwrap(); assert!(found.is_removed()); } @@ -279,7 +286,7 @@ mod tests { #[test] fn test_remove_unknown_connection() { let mut state = NetworkState::default(); - let error = state.remove_connection("eth0").unwrap_err(); + let error = state.remove_connection(Uuid::new_v4()).unwrap_err(); assert!(matches!(error, NetworkStateError::UnknownConnection(_))); } @@ -369,7 +376,7 @@ pub struct Device { } /// Represents an availble network connection. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq)] pub struct Connection { pub id: String, pub uuid: Uuid, @@ -383,12 +390,6 @@ pub struct Connection { pub config: ConnectionConfig, } -impl PartialEq for Connection { - fn eq(&self, other: &Self) -> bool { - self.id == other.id && self.uuid == other.uuid && self.ip_config == other.ip_config - } -} - impl Connection { pub fn new(id: String, device_type: DeviceType) -> Self { let config = match device_type { @@ -494,7 +495,7 @@ impl From for ConnectionConfig { #[error("Invalid MAC address: {0}")] pub struct InvalidMacAddress(String); -#[derive(Debug, Default, Clone)] +#[derive(Debug, Default, Clone, PartialEq)] pub enum MacAddress { MacAddress(macaddr::MacAddr6), Preserve, diff --git a/rust/agama-dbus-server/src/network/nm/adapter.rs b/rust/agama-dbus-server/src/network/nm/adapter.rs index eb42b36afb..0df720e5ba 100644 --- a/rust/agama-dbus-server/src/network/nm/adapter.rs +++ b/rust/agama-dbus-server/src/network/nm/adapter.rs @@ -1,7 +1,7 @@ use crate::network::{ model::{Connection, NetworkState}, nm::NetworkManagerClient, - Adapter, + Adapter, NetworkAdapterError, }; use agama_lib::error::ServiceError; use async_trait::async_trait; @@ -29,9 +29,17 @@ impl<'a> NetworkManagerAdapter<'a> { #[async_trait] impl<'a> Adapter for NetworkManagerAdapter<'a> { - async fn read(&self) -> Result> { - let devices = self.client.devices().await?; - let connections = self.client.connections().await?; + async fn read(&self) -> Result { + let devices = self + .client + .devices() + .await + .map_err(NetworkAdapterError::Read)?; + let connections = self + .client + .connections() + .await + .map_err(NetworkAdapterError::Read)?; Ok(NetworkState::new(devices, connections)) } @@ -42,14 +50,26 @@ impl<'a> Adapter for NetworkManagerAdapter<'a> { /// simpler approach. /// /// * `network`: network model. - async fn write(&self, network: &NetworkState) -> Result<(), Box> { - // By now, traits do not support async functions. Using `task::block_on` allows - // to use 'await'. + async fn write(&self, network: &NetworkState) -> Result<(), NetworkAdapterError> { + let old_state = self.read().await?; + let checkpoint = self + .client + .create_checkpoint() + .await + .map_err(NetworkAdapterError::Checkpoint)?; + for conn in ordered_connections(network) { if !Self::is_writable(conn) { continue; } + if let Some(old_conn) = old_state.get_connection_by_uuid(conn.uuid) { + if old_conn == conn { + continue; + } + } + + log::info!("Updating connection {} ({})", conn.id, conn.uuid); let result = if conn.is_removed() { self.client.remove_connection(conn.uuid).await } else { @@ -60,10 +80,18 @@ impl<'a> Adapter for NetworkManagerAdapter<'a> { }; if let Err(e) = result { - log::error!("Could not process the connection {}: {}", conn.id, e); + self.client + .rollback_checkpoint(&checkpoint.as_ref()) + .await + .map_err(NetworkAdapterError::Checkpoint)?; + log::error!("Could not process the connection {}: {}", conn.id, &e); + return Err(NetworkAdapterError::Write(e)); } } - // FIXME: indicate which connections could not be written. + self.client + .destroy_checkpoint(&checkpoint.as_ref()) + .await + .map_err(NetworkAdapterError::Checkpoint)?; Ok(()) } } diff --git a/rust/agama-dbus-server/src/network/nm/client.rs b/rust/agama-dbus-server/src/network/nm/client.rs index 9f3a5990ed..8396d9d649 100644 --- a/rust/agama-dbus-server/src/network/nm/client.rs +++ b/rust/agama-dbus-server/src/network/nm/client.rs @@ -152,6 +152,34 @@ impl<'a> NetworkManagerClient<'a> { Ok(()) } + /// Creates a checkpoint. + pub async fn create_checkpoint(&self) -> Result { + let path = self.nm_proxy.checkpoint_create(&[], 0, 0).await?; + Ok(path) + } + + /// Destroys a checkpoint. + /// + /// * `checkpoint`: checkpoint's D-Bus path. + pub async fn destroy_checkpoint( + &self, + checkpoint: &ObjectPath<'_>, + ) -> Result<(), ServiceError> { + self.nm_proxy.checkpoint_destroy(checkpoint).await?; + Ok(()) + } + + /// Rolls the configuration back to the given checkpoint. + /// + /// * `checkpoint`: checkpoint's D-Bus path. + pub async fn rollback_checkpoint( + &self, + checkpoint: &ObjectPath<'_>, + ) -> Result<(), ServiceError> { + self.nm_proxy.checkpoint_rollback(checkpoint).await?; + Ok(()) + } + /// Activates a NetworkManager connection. /// /// * `path`: D-Bus patch of the connection. diff --git a/rust/agama-dbus-server/src/network/nm/dbus.rs b/rust/agama-dbus-server/src/network/nm/dbus.rs index e0d68be160..75876ad163 100644 --- a/rust/agama-dbus-server/src/network/nm/dbus.rs +++ b/rust/agama-dbus-server/src/network/nm/dbus.rs @@ -210,11 +210,17 @@ pub fn cleanup_dbus_connection(conn: &mut NestedHash) { if let Some(ipv4) = conn.get_mut("ipv4") { ipv4.remove("addresses"); ipv4.remove("dns"); + if ipv4.get("address-data").is_some_and(is_empty_value) { + ipv4.remove("gateway"); + } } if let Some(ipv6) = conn.get_mut("ipv6") { ipv6.remove("addresses"); ipv6.remove("dns"); + if ipv6.get("address-data").is_some_and(is_empty_value) { + ipv6.remove("gateway"); + } } } @@ -727,6 +733,7 @@ fn wireless_config_from_dbus(conn: &OwnedNestedHash) -> Option { *bssid.get(5)?, )); } + if let Some(hidden) = wireless.get("hidden") { wireless_config.hidden = *hidden.downcast_ref::()?; } @@ -830,6 +837,10 @@ fn is_empty_value(value: &zvariant::Value) -> bool { return value.is_empty(); } + if let Some(value) = value.downcast_ref::() { + return value.is_empty(); + } + false } @@ -1183,6 +1194,7 @@ mod test { Value::new(ETHERNET_KEY.to_string()).to_owned(), ), ]); + let ipv4 = HashMap::from([ ( "method".to_string(), @@ -1241,10 +1253,8 @@ mod test { *ipv4.get("method").unwrap(), Value::new("disabled".to_string()) ); - assert_eq!( - *ipv4.get("gateway").unwrap(), - Value::new("192.168.1.1".to_string()) - ); + // there are not addresses ("address-data"), so no gateway is allowed + assert!(ipv4.get("gateway").is_none()); assert!(ipv4.get("addresses").is_none()); let ipv6 = merged.get("ipv6").unwrap(); @@ -1252,10 +1262,8 @@ mod test { *ipv6.get("method").unwrap(), Value::new("disabled".to_string()) ); - assert_eq!( - *ipv6.get("gateway").unwrap(), - Value::new("::ffff:c0a8:101".to_string()) - ); + // there are not addresses ("address-data"), so no gateway is allowed + assert!(ipv6.get("gateway").is_none()); } #[test] diff --git a/rust/agama-dbus-server/src/network/system.rs b/rust/agama-dbus-server/src/network/system.rs index 5272d58596..a347caca6a 100644 --- a/rust/agama-dbus-server/src/network/system.rs +++ b/rust/agama-dbus-server/src/network/system.rs @@ -1,4 +1,4 @@ -use super::error::NetworkStateError; +use super::{error::NetworkStateError, NetworkAdapterError}; use crate::network::{dbus::Tree, model::Connection, Action, Adapter, NetworkState}; use agama_lib::network::types::DeviceType; use std::{error::Error, sync::Arc}; @@ -35,7 +35,7 @@ impl NetworkSystem { } /// Writes the network configuration. - pub async fn write(&mut self) -> Result<(), Box> { + pub async fn write(&mut self) -> Result<(), NetworkAdapterError> { self.adapter.write(&self.state).await?; self.state = self.adapter.read().await?; Ok(()) @@ -79,9 +79,13 @@ impl NetworkSystem { let conn = self.state.get_connection_by_uuid(uuid); tx.send(conn.cloned()).unwrap(); } - Action::GetConnectionPath(id, tx) => { + Action::GetConnectionPath(uuid, tx) => { let tree = self.tree.lock().await; - let path = tree.connection_path(&id); + let path = tree.connection_path(uuid); + tx.send(path).unwrap(); + } + Action::GetConnectionPathById(id, tx) => { + let path = self.get_connection_path_by_id_action(&id).await; tx.send(path).unwrap(); } Action::GetController(uuid, tx) => { @@ -103,13 +107,19 @@ impl NetworkSystem { Action::UpdateConnection(conn) => { self.state.update_connection(*conn)?; } - Action::RemoveConnection(id) => { + Action::RemoveConnection(uuid) => { let mut tree = self.tree.lock().await; - tree.remove_connection(&id).await?; - self.state.remove_connection(&id)?; + tree.remove_connection(uuid).await?; + self.state.remove_connection(uuid)?; } - Action::Apply => { - self.write().await?; + Action::Apply(tx) => { + let result = self.write().await; + let failed = result.is_err(); + tx.send(result).unwrap(); + if failed { + return Ok(()); + } + // TODO: re-creating the tree is kind of brute-force and it sends signals about // adding/removing interfaces. We should add/update/delete objects as needed. // NOTE updating the tree at the same time than dispatching actions can cause a @@ -134,14 +144,14 @@ impl NetworkSystem { name: String, ty: DeviceType, ) -> Result { - let mut conn = Connection::new(name, ty); + let conn = Connection::new(name, ty); // TODO: handle tree handling problems + self.state.add_connection(conn.clone())?; let mut tree = self.tree.lock().await; let path = tree - .add_connection(&mut conn) + .add_connection(&conn) .await .expect("Could not update the D-Bus tree"); - self.state.add_connection(conn)?; Ok(path) } @@ -176,4 +186,10 @@ impl NetworkSystem { Ok((conn, controlled)) } + + async fn get_connection_path_by_id_action(&mut self, id: &str) -> Option { + let conn = self.state.get_connection(id)?; + let tree = self.tree.lock().await; + tree.connection_path(conn.uuid) + } } diff --git a/rust/agama-dbus-server/tests/network.rs b/rust/agama-dbus-server/tests/network.rs index 616067d651..2261746325 100644 --- a/rust/agama-dbus-server/tests/network.rs +++ b/rust/agama-dbus-server/tests/network.rs @@ -4,7 +4,7 @@ use self::common::{async_retry, DBusServer}; use agama_dbus_server::network::{ self, model::{self, Ipv4Method, Ipv6Method}, - Adapter, NetworkService, NetworkState, + Adapter, NetworkAdapterError, NetworkService, NetworkState, }; use agama_lib::network::{ settings::{self}, @@ -21,14 +21,11 @@ pub struct NetworkTestAdapter(network::NetworkState); #[async_trait] impl Adapter for NetworkTestAdapter { - async fn read(&self) -> Result> { + async fn read(&self) -> Result { Ok(self.0.clone()) } - async fn write( - &self, - _network: &network::NetworkState, - ) -> Result<(), Box> { + async fn write(&self, _network: &network::NetworkState) -> Result<(), NetworkAdapterError> { unimplemented!("Not used in tests"); } } diff --git a/rust/agama-lib/src/network/client.rs b/rust/agama-lib/src/network/client.rs index 79d9e4466b..b8a6b427a4 100644 --- a/rust/agama-lib/src/network/client.rs +++ b/rust/agama-lib/src/network/client.rs @@ -26,7 +26,7 @@ impl<'a> NetworkClient<'a> { } pub async fn get_connection(&self, id: &str) -> Result { - let path = self.connections_proxy.get_connection(id).await?; + let path = self.connections_proxy.get_connection_by_id(id).await?; self.connection_from(path.as_str()).await } @@ -200,7 +200,7 @@ impl<'a> NetworkClient<'a> { &self, conn: &NetworkConnection, ) -> Result<(), ServiceError> { - let path = match self.connections_proxy.get_connection(&conn.id).await { + let path = match self.connections_proxy.get_connection_by_id(&conn.id).await { Ok(path) => path, Err(_) => self.add_connection(conn).await?, }; @@ -230,7 +230,10 @@ impl<'a> NetworkClient<'a> { }; } - Ok(self.connections_proxy.get_connection(&conn.id).await?) + Ok(self + .connections_proxy + .get_connection_by_id(&conn.id) + .await?) } /// Updates a network connection. diff --git a/rust/agama-lib/src/network/proxies.rs b/rust/agama-lib/src/network/proxies.rs index 77aed57feb..9710980885 100644 --- a/rust/agama-lib/src/network/proxies.rs +++ b/rust/agama-lib/src/network/proxies.rs @@ -33,19 +33,17 @@ trait Device { default_path = "/org/opensuse/Agama1/Network/connections" )] trait Connections { - /// Add a new network connection. - /// - /// `name`: connection name. - /// `ty`: connection type. - fn add_connection(&self, name: &str, ty: u8) -> zbus::Result; + /// AddConnection method + fn add_connection(&self, id: &str, ty: u8) -> zbus::Result; /// Apply method fn apply(&self) -> zbus::Result<()>; - /// Gets a connection D-Bus path by its ID - /// - /// * `id`: connection ID. - fn get_connection(&self, id: &str) -> zbus::Result; + /// GetConnection method + fn get_connection(&self, uuid: &str) -> zbus::Result; + + /// GetConnectionById method + fn get_connection_by_id(&self, id: &str) -> zbus::Result; /// GetConnections method fn get_connections(&self) -> zbus::Result>; @@ -55,7 +53,7 @@ trait Connections { /// ConnectionAdded signal #[dbus_proxy(signal)] - fn connection_added(&self, id: &str, path: &str) -> zbus::Result<()>; + fn connection_added(&self, id: &str, path: zbus::zvariant::ObjectPath<'_>) -> zbus::Result<()>; } #[dbus_proxy( diff --git a/rust/package/agama-cli.changes b/rust/package/agama-cli.changes index d4e1f812b8..4ef0bfa60e 100644 --- a/rust/package/agama-cli.changes +++ b/rust/package/agama-cli.changes @@ -1,3 +1,15 @@ +------------------------------------------------------------------- +Mon Jan 29 15:53:56 UTC 2024 - Imobach Gonzalez Sosa + +- Better network configuration handling (gh#openSUSE/agama#1006): + * Write only changed connections. + * Roll back when updating the NetworkManager configuration + failed. + * Improved error handling when reading or writing the changes. + * Properly remove deleted connections from the D-Bus tree. + * Use the UUID to identify connections. + * Do not support multiple connections with the same ID. + ------------------------------------------------------------------- Mon Jan 29 15:37:56 UTC 2024 - Jorik Cronenberg diff --git a/web/package/cockpit-agama.changes b/web/package/cockpit-agama.changes index 17696a219b..11aa800032 100644 --- a/web/package/cockpit-agama.changes +++ b/web/package/cockpit-agama.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Mon Jan 29 14:34:37 UTC 2024 - Knut Anderssen + +- Partly replacing the NetworkManager client by the agama one + (gh#openSUSE/agama#1006). + ------------------------------------------------------------------- Fri Jan 19 09:34:26 UTC 2024 - Nagu diff --git a/web/src/client/index.js b/web/src/client/index.js index 34942691c8..0f64ac1bcb 100644 --- a/web/src/client/index.js +++ b/web/src/client/index.js @@ -74,7 +74,7 @@ const createClient = (address = "unix:path=/run/agama/bus") => { const l10n = new L10nClient(address); const manager = new ManagerClient(address); const monitor = new Monitor(address, MANAGER_SERVICE); - const network = new NetworkClient(); + const network = new NetworkClient(address); const software = new SoftwareClient(address); const storage = new StorageClient(address); const users = new UsersClient(address); diff --git a/web/src/client/network.test.js b/web/src/client/network.test.js index d4a98f6e56..1841530439 100644 --- a/web/src/client/network.test.js +++ b/web/src/client/network.test.js @@ -22,8 +22,9 @@ // @ts-check import { NetworkClient, ConnectionTypes, ConnectionState } from "./network"; +const ADDRESS = "unix:path=/run/agama/bus"; -const active_conn = { +const mockActiveConnection = { id: "uuid-wired", name: "Wired connection 1", type: ConnectionTypes.ETHERNET, @@ -31,7 +32,7 @@ const active_conn = { addresses: [{ address: "192.168.122.1", prefix: 24 }] }; -const conn = { +const mockConnection = { id: "uuid-wired", name: "Wired connection 1", type: ConnectionTypes.ETHERNET, @@ -43,40 +44,43 @@ const settings = { hostname: "localhost.localdomain" }; -const adapter = { - setUp: jest.fn(), - activeConnections: jest.fn().mockReturnValue([active_conn]), - connections: jest.fn().mockReturnValue([conn]), - subscribe: jest.fn(), - getConnection: jest.fn(), - addConnection: jest.fn(), - updateConnection: jest.fn(), - deleteConnection: jest.fn(), - accessPoints: jest.fn(), - connectTo: jest.fn(), - addAndConnectTo: jest.fn(), - settings: jest.fn().mockReturnValue(settings), -}; +jest.mock("./network/network_manager", () => { + return { + NetworkManagerAdapter: jest.fn().mockImplementation(() => { + return { + setUp: jest.fn(), + activeConnections: jest.fn().mockReturnValue([mockActiveConnection]), + connections: jest.fn().mockReturnValue([mockConnection]), + subscribe: jest.fn(), + getConnection: jest.fn(), + accessPoints: jest.fn(), + connectTo: jest.fn(), + addAndConnectTo: jest.fn(), + settings: jest.fn().mockReturnValue(settings), + }; + }), + }; +}); describe("NetworkClient", () => { describe("#activeConnections", () => { it("returns the list of active connections from the adapter", () => { - const client = new NetworkClient(adapter); + const client = new NetworkClient(ADDRESS); const connections = client.activeConnections(); - expect(connections).toEqual([active_conn]); + expect(connections).toEqual([mockActiveConnection]); }); }); describe("#addresses", () => { it("returns the list of addresses", () => { - const client = new NetworkClient(adapter); + const client = new NetworkClient(ADDRESS); expect(client.addresses()).toEqual([{ address: "192.168.122.1", prefix: 24 }]); }); }); describe("#settings", () => { it("returns network general settings", () => { - const client = new NetworkClient(adapter); + const client = new NetworkClient(ADDRESS); expect(client.settings().hostname).toEqual("localhost.localdomain"); expect(client.settings().wifiScanSupported).toEqual(true); }); diff --git a/web/src/client/network/index.js b/web/src/client/network/index.js index 99641f2f9a..0e02c0bced 100644 --- a/web/src/client/network/index.js +++ b/web/src/client/network/index.js @@ -1,5 +1,5 @@ /* - * Copyright (c) [2022] SUSE LLC + * Copyright (c) [2023] SUSE LLC * * All Rights Reserved. * @@ -21,8 +21,26 @@ // @ts-check +import DBusClient from "../dbus"; import { NetworkManagerAdapter } from "./network_manager"; -import { ConnectionTypes, ConnectionState } from "./model"; +import cockpit from "../../lib/cockpit"; +import { createConnection, ConnectionTypes, ConnectionState } from "./model"; + +const SERVICE_NAME = "org.opensuse.Agama1"; +const CONNECTIONS_IFACE = "org.opensuse.Agama1.Network.Connections"; +const CONNECTIONS_PATH = "/org/opensuse/Agama1/Network/connections"; +const CONNECTION_IFACE = "org.opensuse.Agama1.Network.Connection"; +const CONNECTIONS_NAMESPACE = "/org/opensuse/Agama1/Network/connections"; +const IP_IFACE = "org.opensuse.Agama1.Network.Connection.IP"; +const WIRELESS_IFACE = "org.opensuse.Agama1.Network.Connection.Wireless"; + +const DeviceType = Object.freeze({ + LOOPBACK: 0, + ETHERNET: 1, + WIRELESS: 2, + DUMMY: 3, + BOND: 4 +}); /** * @typedef {import("./model").NetworkSettings} NetworkSettings @@ -77,15 +95,20 @@ const NetworkEventTypes = Object.freeze({ */ class NetworkClient { /** - * @param {NetworkAdapter} [adapter] - Network adapter. By default, it is set to -o * NetworkManagerAdapter. + * @param {string} address - D-Bus address */ - constructor(adapter) { - this.adapter = adapter || new NetworkManagerAdapter(); - /** @type {!boolean} */ + constructor(address) { this.subscribed = false; + this.nm = new NetworkManagerAdapter(); + this.client = new DBusClient(SERVICE_NAME, address); + this.proxies = { + connectionsRoot: null, + connections: {}, + ipConfigs: {}, + wireless: {} + }; + this.eventsHandler = null; this.setUpDone = false; - /** @type {NetworkEventFn[]} */ this.handlers = []; } @@ -110,7 +133,15 @@ o * NetworkManagerAdapter. async setUp() { if (this.setUpDone) return; - return this.adapter.setUp(e => this.handlers.forEach(f => f(e))); + this.proxies = { + connectionsRoot: await this.client.proxy(CONNECTIONS_IFACE, CONNECTIONS_PATH), + connections: await this.client.proxies(CONNECTION_IFACE, CONNECTIONS_NAMESPACE), + ipConfigs: await this.client.proxies(IP_IFACE, CONNECTIONS_NAMESPACE), + wireless: await this.client.proxies(WIRELESS_IFACE, CONNECTIONS_NAMESPACE) + }; + + this.setUpDone = true; + return this.nm.setUp(e => this.handlers.forEach(f => f(e))); } /** @@ -119,7 +150,7 @@ o * NetworkManagerAdapter. * @return {ActiveConnection[]} */ activeConnections() { - return this.adapter.activeConnections(); + return this.nm.activeConnections(); } /** @@ -128,7 +159,7 @@ o * NetworkManagerAdapter. * @return {Promise} */ connections() { - return this.adapter.connections(); + return this.nm.connections(); } /** @@ -137,7 +168,7 @@ o * NetworkManagerAdapter. * @return {AccessPoint[]} */ accessPoints() { - return this.adapter.accessPoints(); + return this.nm.accessPoints(); } /** @@ -146,7 +177,7 @@ o * NetworkManagerAdapter. * @param {Connection} connection - connection to be activated */ async connectTo(connection) { - return this.adapter.connectTo(connection); + return this.nm.connectTo(connection); } /** @@ -156,37 +187,191 @@ o * NetworkManagerAdapter. * @param {object} options - connection options */ async addAndConnectTo(ssid, options) { - return this.adapter.addAndConnectTo(ssid, options); + // duplicated code (see network manager adapter) + const wireless = { ssid }; + if (options.security) wireless.security = options.security; + if (options.password) wireless.password = options.password; + if (options.hidden) wireless.hidden = options.hidden; + + const connection = createConnection({ + id: ssid, + wireless + }); + + // the connection is automatically activated when written + return this.addConnection(connection); } /** * Adds a new connection * + * If a connection with the given ID already exists, it updates such a + * connection. + * * @param {Connection} connection - Connection to add + * @return {Promise} the added connection */ async addConnection(connection) { - return this.adapter.addConnection(connection); + const { id } = connection; + const proxy = await this.client.proxy(CONNECTIONS_IFACE, CONNECTIONS_PATH); + const deviceType = (connection.wireless) ? DeviceType.WIRELESS : DeviceType.ETHERNET; + let path; + try { + path = await proxy.GetConnectionById(id); + } catch { + path = await proxy.AddConnection(id, deviceType); + } + await this.updateConnectionAt(path, connection); + return this.connectionFromPath(path); } /** * Returns the connection with the given ID * - * @param {string} id - Connection ID + * @param {string} uuid - Connection ID + * @return {Promise} + */ + async getConnection(uuid) { + const path = await this.getConnectionPath(uuid); + if (path) { + return this.connectionFromPath(path); + } + } + + /** + * Returns a connection from the given D-Bus path + * + * @param {string} path - Path of the D-Bus object representing the connection * @return {Promise} */ - async getConnection(id) { - return this.adapter.getConnection(id); + async connectionFromPath(path) { + const connection = await this.proxies.connections[path]; + const ip = await this.proxies.ipConfigs[path]; + + const conn = { + id: connection.Id, + uuid: connection.Uuid, + iface: connection.Interface, + ipv4: { + method: ip.Method4, + nameServers: ip.Nameservers, + addresses: ip.Addresses.map(addr => { + const [address, prefix] = addr.split("/"); + return { address, prefix }; + }), + gateway: ip.Gateway4 + }, + }; + + const wireless = await this.proxies.wireless[path]; + if (wireless) { + conn.wireless = { + ssid: window.atob(wireless.SSID), + hidden: wireless.Hidden, + mode: wireless.Mode, + security: wireless.Security // see AgamaSecurityProtocols + }; + } + + return createConnection(conn); + } + + /** + * Sets a property for a given path + * + * @param {string} path - Object path. + * @param {string} iface - Interface name. + * @param {object} values - Properties values (indexed by names). The value + * should be created by using the cockpit.variant() function. + */ + async setProperties(path, iface, values) { + for (const [prop, value] of Object.entries(values)) { + await this.setProperty(path, iface, prop, value); + } + } + + /** + * Sets a property for a given path + * + * @param {string} path - Object path. + * @param {string} iface - Interface name. + * @param {string} property - Property name. + * @param {object} value - Property value. The value should be created by + * using the cockpit.variant() function. + */ + async setProperty(path, iface, property, value) { + return this.client.call(path, "org.freedesktop.DBus.Properties", "Set", [iface, property, value]); + } + + /** + * Returns the D-Bus path of the connection. + * + * @param {string} uuid - Connection UUID + * @return {Promise} - Connection D-Bus path + */ + async getConnectionPath(uuid) { + for (const path in this.proxies.connections) { + const proxy = await this.proxies.connections[path]; + if (proxy.Uuid === uuid) { + return path; + } + } } /** * Updates the connection * - * It uses the 'path' to match the connection in the backend. + * It uses the 'uuid' to match the connection in the backend. * * @param {Connection} connection - Connection to update + * @return {Promise} - the promise resolves to true if the connection + * was successfully updated and to false it it does not exist. */ async updateConnection(connection) { - return this.adapter.updateConnection(connection); + const path = await this.getConnectionPath(connection.uuid); + if (path === undefined) { + return false; + } + + await this.updateConnectionAt(path, connection); + return true; + } + + /** + * Updates the connection in the given path + * + * + * @param {string} path - D-Bus path of the connection to update. + * @param {Connection} connection - Connection to update. + */ + async updateConnectionAt(path, connection) { + const { ipv4, wireless } = connection; + const addresses = ipv4.addresses.map(a => `${a.address}/${a.prefix}`); + const ipv4_props = { + Method4: cockpit.variant("s", ipv4.method), + Gateway4: cockpit.variant("s", ipv4.gateway), + Addresses: cockpit.variant("as", addresses), + Nameservers: cockpit.variant("as", ipv4.nameServers) + }; + await this.setProperties(path, IP_IFACE, ipv4_props); + + if (wireless) { + const wireless_props = { + Mode: cockpit.variant("s", "infrastructure"), + Security: cockpit.variant("s", wireless.security), + SSID: cockpit.variant("ay", cockpit.byte_array(wireless.ssid)), + Hidden: cockpit.variant("b", !!wireless.hidden) + }; + + if (wireless.password) { + wireless_props.Password = cockpit.variant("s", wireless.password); + } + + await this.setProperties(path, WIRELESS_IFACE, wireless_props); + } + + // TODO: apply the changes only in this connection + return this.proxies.connectionsRoot.Apply(); } /** @@ -194,10 +379,12 @@ o * NetworkManagerAdapter. * * It uses the 'path' to match the connection in the backend. * - * @param {Connection} connection - Connection to delete + * @param {String} uuid - Connection uuid */ - async deleteConnection(connection) { - return this.adapter.deleteConnection(connection); + async deleteConnection(uuid) { + const proxy = await this.client.proxy(CONNECTIONS_IFACE, CONNECTIONS_PATH); + await proxy.RemoveConnection(uuid); + return this.proxies.connectionsRoot.Apply(); } /* @@ -208,7 +395,7 @@ o * NetworkManagerAdapter. * @return {Promise} */ addresses() { - const conns = this.adapter.activeConnections(); + const conns = this.activeConnections(); return conns.flatMap(c => c.addresses); } @@ -216,10 +403,13 @@ o * NetworkManagerAdapter. * Returns network general settings */ settings() { - return this.adapter.settings(); + return this.nm.settings(); } } export { - ConnectionState, ConnectionTypes, NetworkClient, NetworkManagerAdapter, NetworkEventTypes + ConnectionState, + ConnectionTypes, + NetworkClient, + NetworkEventTypes }; diff --git a/web/src/client/network/model.js b/web/src/client/network/model.js index f4dae72ecf..1c224a05be 100644 --- a/web/src/client/network/model.js +++ b/web/src/client/network/model.js @@ -60,6 +60,17 @@ const SecurityProtocols = Object.freeze({ _8021X: "802.1X" }); +// security protocols +// const AgamaSecurityProtocols = Object.freeze({ +// WEP: "none", +// OWE: "owe", +// DynamicWEP: "ieee8021x", +// WPA2: "wpa-psk", +// WPA3Personal: "sae", +// WPA2Enterprise: "wpa-eap", +// WPA3Only: "wpa-eap-suite-b-192" +// }); + /** * @typedef {object} IPAddress * @property {string} address - like "129.168.1.2" @@ -69,7 +80,7 @@ const SecurityProtocols = Object.freeze({ /** * @typedef {object} ActiveConnection * @property {string} id - * @property {string} name + * @property {string} uuid * @property {string} type * @property {number} state * @property {IPAddress[]} addresses @@ -78,7 +89,8 @@ const SecurityProtocols = Object.freeze({ /** * @typedef {object} Connection * @property {string} id - * @property {string} name + * @property {string} uuid + * @property {string} iface * @property {IPv4} [ipv4] * @property {Wireless} [wireless] */ @@ -141,15 +153,17 @@ const createIPv4 = ({ method, addresses, nameServers, gateway }) => { * * @param {object} options * @param {string} [options.id] - Connection ID - * @param {string} [options.name] - Connection name + * @param {string} [options.uuid] - Connection UUID + * @param {string} [options.iface] - Connection interface * @param {object} [options.ipv4] IPv4 Settings * @param {object} [options.wireless] Wireless Settings * @return {Connection} */ -const createConnection = ({ id, name, ipv4, wireless }) => { +const createConnection = ({ id, uuid, iface, ipv4, wireless }) => { const connection = { id, - name, + uuid, + iface, ipv4: createIPv4(ipv4 || {}), }; diff --git a/web/src/client/network/model.test.js b/web/src/client/network/model.test.js index 1eef2f8cb5..babfb5c72c 100644 --- a/web/src/client/network/model.test.js +++ b/web/src/client/network/model.test.js @@ -25,10 +25,13 @@ import { createConnection, createAccessPoint, connectionHumanState } from "./mod describe("createConnection", () => { it("creates a connection with the default values", () => { - const connection = createConnection({ name: "Wired connection" }); + const connection = createConnection({ + id: "Wired connection 1", + }); expect(connection).toEqual({ - id: undefined, - name: "Wired connection", + id: "Wired connection 1", + iface: undefined, + uuid: undefined, ipv4: { method: "auto", addresses: [], @@ -52,7 +55,7 @@ describe("createConnection", () => { it("adds a wireless key when given", () => { const wireless = { ssid: "MY_WIRELESS" }; - const connection = createConnection({ name: "Wireless 1", wireless }); + const connection = createConnection({ id: "Wireless connection 1", wireless }); expect(connection.wireless).toEqual(wireless); }); }); diff --git a/web/src/client/network/network_manager.js b/web/src/client/network/network_manager.js index 562ccf9739..5b480126d3 100644 --- a/web/src/client/network/network_manager.js +++ b/web/src/client/network/network_manager.js @@ -24,7 +24,7 @@ import DBusClient from "../dbus"; import cockpit from "../../lib/cockpit"; import { NetworkEventTypes } from "./index"; -import { createAccessPoint, createConnection, SecurityProtocols } from "./model"; +import { createAccessPoint, SecurityProtocols } from "./model"; import { ipPrefixFor } from "./utils"; /** @@ -102,7 +102,7 @@ const connectionToCockpit = (connection) => { const { ipv4, wireless } = connection; const settings = { connection: { - id: cockpit.variant("s", connection.name) + id: cockpit.variant("s", connection.id) }, ipv4: { "address-data": cockpit.variant("aa{sv}", ipv4.addresses.map(addr => ( @@ -173,8 +173,6 @@ const mergeConnectionSettings = (settings, connection) => { class NetworkManagerAdapter { constructor() { this.client = new DBusClient(SERVICE_NAME); - /** @type {{[k: string]: string}} */ - this.connectionIds = {}; this.proxies = { accessPoints: {}, activeConnections: {}, @@ -205,6 +203,7 @@ class NetworkManagerAdapter { settings: await this.client.proxy(SETTINGS_IFACE), connections: await this.client.proxies(CONNECTION_IFACE, SETTINGS_NAMESPACE) }; + this.subscribeToEvents(); } @@ -258,8 +257,9 @@ class NetworkManagerAdapter { connectionFromSettings(settings) { const { connection, ipv4, "802-11-wireless": wireless, path } = settings; const conn = { - id: connection.uuid.v, - name: connection.id.v, + id: connection.id.v, + uuid: connection.uuid.v, + iface: connection.interface?.v, type: connection.type.v }; @@ -286,86 +286,16 @@ class NetworkManagerAdapter { return conn; } - /** - * Returns the connection with the given ID - * - * @param {string} id - Connection ID - * @return {Promise} - */ - async getConnection(id) { - const settingsProxy = await this.connectionSettingsObject(id); - const settings = await settingsProxy.GetSettings(); - - return this.connectionFromSettings(settings); - } - /** * Connects to given Wireless network * * @param {object} settings - connection options */ async connectTo(settings) { - const settingsProxy = await this.connectionSettingsObject(settings.id); + const settingsProxy = await this.connectionSettingsObject(settings.uuid); await this.activateConnection(settingsProxy.path); } - /** - * Connects to given Wireless network - * - * @param {string} ssid - Network id - * @param {object} options - connection options - */ - async addAndConnectTo(ssid, options = {}) { - const wireless = { ssid }; - if (options.security) wireless.security = options.security; - if (options.password) wireless.password = options.password; - if (options.hidden) wireless.hidden = options.hidden; - - const connection = createConnection({ - name: ssid, - wireless - }); - - await this.addConnection(connection); - } - - /** - * Adds a new connection - * - * @param {Connection} connection - Connection to add - */ - async addConnection(connection) { - const proxy = await this.client.proxy(SETTINGS_IFACE); - const connCockpit = connectionToCockpit(connection); - const path = await proxy.AddConnection(connCockpit); - await this.activateConnection(path); - } - - /** - * Updates the connection - * - * @fixme improve it. - * - * @param {import("./index").Connection} connection - Connection to update - */ - async updateConnection(connection) { - const settingsProxy = await this.connectionSettingsObject(connection.id); - const settings = await settingsProxy.GetSettings(); - const newSettings = mergeConnectionSettings(settings, connection); - await settingsProxy.Update(newSettings); - await this.activateConnection(settingsProxy.path); - } - - /** - * Deletes the given connection - * - * @param {import("./index").Connection} connection - Connection to delete - */ - async deleteConnection(connection) { - const settingsProxy = await this.connectionSettingsObject(connection.id); - await settingsProxy.Delete(); - } - /** * Subscribes to network events * @@ -462,8 +392,8 @@ class NetworkManagerAdapter { } return { - id: proxy.Uuid, - name: proxy.Id, + id: proxy.Id, + uuid: proxy.Uuid, addresses, type: proxy.Type, state: proxy.State @@ -475,12 +405,12 @@ class NetworkManagerAdapter { * Returns connection settings for the given connection * * @private - * @param {string} id - Connection ID + * @param {string} uuid - Connection ID * @return {Promise} */ - async connectionSettingsObject(id) { + async connectionSettingsObject(uuid) { const proxy = await this.client.proxy(SETTINGS_IFACE); - const path = await proxy.GetConnectionByUuid(id); + const path = await proxy.GetConnectionByUuid(uuid); return await this.client.proxy(CONNECTION_IFACE, path); } diff --git a/web/src/client/network/network_manager.test.js b/web/src/client/network/network_manager.test.js index 6663cffda0..fbc1248cd8 100644 --- a/web/src/client/network/network_manager.test.js +++ b/web/src/client/network/network_manager.test.js @@ -81,7 +81,7 @@ const defaultDevices = { }; const accessPoints = { - "/org/freedesktop/NetworkManager/AccessPoint/11" : { + "/org/freedesktop/NetworkManager/AccessPoint/11": { Flags: 3, WpaFlags: 0, RsnFlags: 392, @@ -276,16 +276,16 @@ describe("NetworkManagerAdapter", () => { expect(availableConnections.length).toEqual(2); const [wireless, ethernet] = availableConnections; expect(wireless).toEqual({ - name: "active-wifi-connection", - id: "uuid-wifi-1", + id: "active-wifi-connection", + uuid: "uuid-wifi-1", state: ConnectionState.ACTIVATED, type: ConnectionTypes.WIFI, addresses: [{ address: "10.0.0.2", prefix: 22 }] }); expect(ethernet).toEqual({ - name: "active-wired-connection", - id: "uuid-wired-1", + id: "active-wired-connection", + uuid: "uuid-wired-1", state: ConnectionState.ACTIVATED, type: ConnectionTypes.ETHERNET, addresses: [{ address: "10.0.0.1", prefix: 22 }] @@ -302,8 +302,8 @@ describe("NetworkManagerAdapter", () => { const [wifi] = connections; expect(wifi).toEqual({ - name: "Testing", - id: "1f40ddb0-e6e8-4af8-8b7a-0b3898f0f57a", + id: "Testing", + uuid: "1f40ddb0-e6e8-4af8-8b7a-0b3898f0f57a", path: "/org/freedesktop/NetworkManager/Settings/1", type: ConnectionTypes.WIFI, ipv4: { method: 'auto', addresses: [], nameServers: [] }, @@ -312,66 +312,6 @@ describe("NetworkManagerAdapter", () => { }); }); - describe("#getConnection", () => { - it("returns the connection with the given ID", async () => { - const client = new NetworkManagerAdapter(); - const connection = await client.getConnection("uuid-wifi-1"); - expect(connection).toEqual({ - id: "uuid-wifi-1", - name: "active-wifi-connection", - type: "802-11-wireless", - ipv4: { - addresses: [{ address: "192.168.122.200", prefix: 24 }], - gateway: "192.168.122.1", - method: "auto", - nameServers: ["192.168.122.1", "1.1.1.1"] - } - }); - }); - }); - - describe("#addConnection", () => { - it("adds a connection and activates it", async () => { - const client = new NetworkManagerAdapter(); - const connection = createConnection({ name: "Wired connection 1" }); - await client.addConnection(connection); - expect(AddConnectionFn).toHaveBeenCalledWith( - expect.objectContaining({ - connection: expect.objectContaining({ id: cockpit.variant("s", connection.name) }) - }) - ); - }); - }); - - describe("#updateConnection", () => { - it("updates the connection", async () => { - const client = new NetworkManagerAdapter(); - const connection = await client.getConnection("uuid-wifi-1"); - connection.ipv4 = { - ...connection.ipv4, - addresses: [{ address: "192.168.1.2", prefix: "255.255.255.0" }], - gateway: "192.168.1.1", - nameServers: ["1.2.3.4"] - }; - - await client.updateConnection(connection); - expect(connectionSettingsMock.Update).toHaveBeenCalledWith(expect.objectContaining( - { - connection: expect.objectContaining({ - id: cockpit.variant("s", "active-wifi-connection") - }), - ipv4: expect.objectContaining({ - "address-data": cockpit.variant("aa{sv}", [ - { address: cockpit.variant("s", "192.168.1.2"), prefix: cockpit.variant("u", 24) } - ]), - gateway: cockpit.variant("s", "192.168.1.1") - }) - } - )); - expect(ActivateConnectionFn).toHaveBeenCalled(); - }); - }); - describe("#connectTo", () => { it("activates the given connection", async () => { const client = new NetworkManagerAdapter(); @@ -382,33 +322,6 @@ describe("NetworkManagerAdapter", () => { }); }); - describe("#addAndConnectTo", () => { - it("activates the given connection", async () => { - const client = new NetworkManagerAdapter(); - await client.setUp(); - client.addConnection = jest.fn(); - await client.addAndConnectTo("Testing", { security: "wpa-psk", password: "testing.1234" }); - - expect(client.addConnection).toHaveBeenCalledWith( - createConnection({ - name: "Testing", - wireless: { ssid: "Testing", security: "wpa-psk", password: "testing.1234" } - }) - ); - }); - }); - - describe("#deleteConnection", () => { - it("deletes the given connection", async () => { - const client = new NetworkManagerAdapter(); - await client.setUp(); - const [wifi] = await client.connections(); - await client.deleteConnection(wifi); - - expect(connectionSettingsMock.Delete).toHaveBeenCalled(); - }); - }); - describe("#availableWifiDevices", () => { it("returns the list of WiFi devices", async () => { const client = new NetworkManagerAdapter(); @@ -459,7 +372,7 @@ describe("NetworkManagerAdapter", () => { }); describe("#settings", () => { - it("returns the Network Manager settings", async() => { + it("returns the Network Manager settings", async () => { const client = new NetworkManagerAdapter(); await client.setUp(); expect(client.settings().hostname).toEqual("testing-machine"); diff --git a/web/src/components/network/AddressesDataList.jsx b/web/src/components/network/AddressesDataList.jsx index d8d4b9d888..4c1af00b21 100644 --- a/web/src/components/network/AddressesDataList.jsx +++ b/web/src/components/network/AddressesDataList.jsx @@ -88,7 +88,7 @@ export default function AddressesDataList({ updateAddress(id, "address", value)} + onChange={(_, value) => updateAddress(id, "address", value)} // TRANSLATORS: input field name placeholder={_("IP Address")} aria-label={_("IP Address")} @@ -97,7 +97,7 @@ export default function AddressesDataList({ updateAddress(id, "prefix", value)} + onChange={(_, value) => updateAddress(id, "prefix", value)} // TRANSLATORS: input field name placeholder={_("Prefix length or netmask")} aria-label={_("Prefix length or netmask")} diff --git a/web/src/components/network/ConnectionsTable.jsx b/web/src/components/network/ConnectionsTable.jsx index d2b03bbcfe..cbae048365 100644 --- a/web/src/components/network/ConnectionsTable.jsx +++ b/web/src/components/network/ConnectionsTable.jsx @@ -42,7 +42,7 @@ import { _ } from "~/i18n"; * @param {function} props.onEdit - function to be called for editing a connection * @param {function} props.onForget - function to be called for forgetting a connection */ -export default function ConnectionsTable ({ +export default function ConnectionsTable({ connections, onEdit, onForget @@ -61,20 +61,20 @@ export default function ConnectionsTable ({ - { connections.map(connection => { + {connections.map(connection => { const actions = [ { title: _("Edit"), "aria-label": // TRANSLATORS: %s is replaced by a network connection name - sprintf(_("Edit connection %s"), connection.name), + sprintf(_("Edit connection %s"), connection.id), onClick: () => onEdit(connection) }, typeof onForget === 'function' && { title: _("Forget"), "aria-label": // TRANSLATORS: %s is replaced by a network connection name - sprintf(_("Forget connection %s"), connection.name), + sprintf(_("Forget connection %s"), connection.id), icon: , onClick: () => onForget(connection), isDanger: true @@ -82,14 +82,14 @@ export default function ConnectionsTable ({ ].filter(Boolean); return ( - - {connection.name} + + {connection.id} {connection.addresses.map(formatIp).join(", ")} diff --git a/web/src/components/network/ConnectionsTable.test.jsx b/web/src/components/network/ConnectionsTable.test.jsx index e0812175d5..51bb90e54d 100644 --- a/web/src/components/network/ConnectionsTable.test.jsx +++ b/web/src/components/network/ConnectionsTable.test.jsx @@ -29,15 +29,15 @@ import ConnectionsTable from "~/components/network/ConnectionsTable"; jest.mock("~/client"); const firstConnection = { - id: "wifi-1", - name: "WiFi 1", + id: "WiFi 1", + uuid: "89e7d438-8143-4318-9e35-cdedfdf6c98a", type: ConnectionTypes.WIFI, addresses: [{ address: "192.168.69.200", prefix: 24 }] }; const secondConnection = { - id: "wifi-2", - name: "WiFi 2", + id: "WiFi 2", + uuid: "ddc46599-db7f-4306-aee4-2ab0938fd7d6", type: ConnectionTypes.WIFI, addresses: [{ address: "192.168.69.201", prefix: 24 }] }; @@ -71,7 +71,7 @@ describe("ConnectionsTable", () => { const { user } = plainRender(); const connectionActions = screen.getByRole("button", { name: "Actions for connection WiFi 1" }); const actionsColumn = connectionActions.parentNode; - const menu = await within(actionsColumn).queryByRole("menu"); + const menu = within(actionsColumn).queryByRole("menu"); expect(menu).toBeNull(); await user.click(connectionActions); await screen.findByRole("menu"); diff --git a/web/src/components/network/DnsDataList.jsx b/web/src/components/network/DnsDataList.jsx index ed5011518b..bffbd88eb2 100644 --- a/web/src/components/network/DnsDataList.jsx +++ b/web/src/components/network/DnsDataList.jsx @@ -73,7 +73,7 @@ export default function DnsDataList({ servers: originalServers, updateDnsServers updateServer(id, "address", value)} + onChange={(_, value) => updateServer(id, "address", value)} // TRANSLATORS: input field name placeholder={_("Server IP")} aria-label={_("Server IP")} diff --git a/web/src/components/network/IpSettingsForm.jsx b/web/src/components/network/IpSettingsForm.jsx index 0984e3143b..8244590143 100644 --- a/web/src/components/network/IpSettingsForm.jsx +++ b/web/src/components/network/IpSettingsForm.jsx @@ -111,8 +111,10 @@ export default function IpSettingsForm({ connection, onClose }) { } }; - client.network.updateConnection(updatedConnection); - onClose(); + client.network.updateConnection(updatedConnection) + .then(onClose) + // TODO: better error reporting. By now, it sets an error for the whole connection. + .catch(({ message }) => setErrors({ object: message })); }; const renderError = (field) => { @@ -128,7 +130,8 @@ export default function IpSettingsForm({ connection, onClose }) { // TRANSLATORS: manual network configuration mode with a static IP address // %s is replaced by the connection name return ( - + + {renderError("object")}
{ - client.getConnection(id).then(setSelectedConnection); + const selectConnection = ({ uuid }) => { + client.getConnection(uuid).then(setSelectedConnection); }; - const forgetConnection = async ({ id }) => { - const connection = await client.getConnection(id); - client.deleteConnection(connection); + const forgetConnection = async ({ uuid }) => { + await client.deleteConnection(uuid); }; const activeWiredConnections = connections.filter(c => c.type === ConnectionTypes.ETHERNET); @@ -168,14 +167,14 @@ export default function NetworkPage() { return ( // TRANSLATORS: page title - { /* TRANSLATORS: page section */ } + { /* TRANSLATORS: page section */}
- { ready ? : } + {ready ? : }
- { /* TRANSLATORS: page section */ } + { /* TRANSLATORS: page section */}
- { ready ? : } + {ready ? : }
@@ -185,7 +184,7 @@ export default function NetworkPage() { then={} /> - { /* TODO: improve the connections edition */ } + { /* TODO: improve the connections edition */} setSelectedConnection(null)} />} diff --git a/web/src/components/network/NetworkPage.test.jsx b/web/src/components/network/NetworkPage.test.jsx index 75a4bf813a..82405398da 100644 --- a/web/src/components/network/NetworkPage.test.jsx +++ b/web/src/components/network/NetworkPage.test.jsx @@ -30,14 +30,14 @@ jest.mock("~/client"); jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); const wiredConnection = { - id: "wired-1", - name: "Wired 1", + id: "Wired 1", + uuid: "e1ce3e83-f57c-4649-97d5-ecc468b74f97", type: ConnectionTypes.ETHERNET, addresses: [{ address: "192.168.122.20", prefix: 24 }] }; const wiFiConnection = { - id: "wifi-1", - name: "WiFi 1", + id: "WiFi 1", + uuid: "a4cf03c5-cb87-469d-824e-26b855a6bcfc", type: ConnectionTypes.WIFI, addresses: [{ address: "192.168.69.200", prefix: 24 }] }; diff --git a/web/src/components/network/WifiNetworkListItem.jsx b/web/src/components/network/WifiNetworkListItem.jsx index 550b087526..63520a8194 100644 --- a/web/src/components/network/WifiNetworkListItem.jsx +++ b/web/src/components/network/WifiNetworkListItem.jsx @@ -68,7 +68,7 @@ const isStateChanging = (network) => { * @param {function} props.onSelect - function to execute when the network is selected * @param {function} props.onCancel - function to execute when the selection is cancelled */ -function WifiNetworkListItem ({ network, isSelected, isActive, onSelect, onCancel }) { +function WifiNetworkListItem({ network, isSelected, isActive, onSelect, onCancel }) { // Do not wait until receive the next D-Bus network event to have the connection object available // and display the spinner as soon as possible. I.e., renders it immediately when the user clicks // on an already configured network. @@ -94,16 +94,16 @@ function WifiNetworkListItem ({ network, isSelected, isActive, onSelect, onCance />
{/* TRANSLATORS: %s is replaced by a WiFi network name */} - {showSpinner && } + {showSpinner && } - { showSpinner && !network.connection && _("Connecting") } - { networkState(network.connection?.state)} + {showSpinner && !network.connection && _("Connecting")} + {networkState(network.connection?.state)} - { network.settings && - } + {network.settings && + }
- { isSelected && (!network.settings || network.settings.error) && + {isSelected && (!network.settings || network.settings.error) &&
} diff --git a/web/src/components/network/WifiNetworkMenu.jsx b/web/src/components/network/WifiNetworkMenu.jsx index 5b85a7057b..354ae84b69 100644 --- a/web/src/components/network/WifiNetworkMenu.jsx +++ b/web/src/components/network/WifiNetworkMenu.jsx @@ -48,7 +48,7 @@ export default function WifiNetworkMenu({ settings, position = "right" }) { client.network.deleteConnection(settings)} + onClick={() => client.network.deleteConnection(settings.uuid)} icon={} > {/* TRANSLATORS: menu label, remove the selected WiFi network settings */} diff --git a/web/src/components/network/WifiNetworksList.jsx b/web/src/components/network/WifiNetworksList.jsx index ae7c29ce42..aa21c80dcc 100644 --- a/web/src/components/network/WifiNetworksList.jsx +++ b/web/src/components/network/WifiNetworksList.jsx @@ -63,7 +63,7 @@ function WifiNetworksList({ }; return ( -
    +
      {renderElements()}
    • diff --git a/web/src/components/network/WifiSelector.jsx b/web/src/components/network/WifiSelector.jsx index a839504f79..9b68da5af6 100644 --- a/web/src/components/network/WifiSelector.jsx +++ b/web/src/components/network/WifiSelector.jsx @@ -63,7 +63,7 @@ function WifiSelector({ isOpen = false, onClose }) { const network = { ...ap, settings: connections.find(c => c.wireless?.ssid === ap.ssid), - connection: activeConnections.find(c => c.name === ap.ssid) + connection: activeConnections.find(c => c.id === ap.ssid) }; // Group networks @@ -145,7 +145,7 @@ function WifiSelector({ isOpen = false, onClose }) { client.network.connectTo(network.settings); } }} - onCancelSelectionCallback={() => switchSelectedNetwork(activeNetwork) } + onCancelSelectionCallback={() => switchSelectedNetwork(activeNetwork)} /> {_("Close")} diff --git a/web/src/components/overview/NetworkSection.jsx b/web/src/components/overview/NetworkSection.jsx index 2eca68e522..5eecc682c3 100644 --- a/web/src/components/overview/NetworkSection.jsx +++ b/web/src/components/overview/NetworkSection.jsx @@ -23,7 +23,7 @@ import React, { useEffect, useState } from "react"; import { sprintf } from "sprintf-js"; import { Em, Section, SectionSkeleton } from "~/components/core"; -import { ConnectionTypes, NetworkEventTypes } from "~/client/network"; +import { ConnectionTypes } from "~/client/network"; import { useInstallerClient } from "~/context/installer"; import { formatIp } from "~/client/network/utils"; import { _, n_ } from "~/i18n"; @@ -44,31 +44,12 @@ export default function NetworkSection() { }, [client, initialized]); useEffect(() => { - return client.onNetworkEvent(({ type, payload }) => { - switch (type) { - case NetworkEventTypes.ACTIVE_CONNECTION_ADDED: { - setConnections(conns => { - const newConnections = conns.filter(c => c.id !== payload.id); - return [...newConnections, payload]; - }); - break; - } + if (!initialized) return; - case NetworkEventTypes.ACTIVE_CONNECTION_UPDATED: { - setConnections(conns => { - const newConnections = conns.filter(c => c.id !== payload.id); - return [...newConnections, payload]; - }); - break; - } - - case NetworkEventTypes.ACTIVE_CONNECTION_REMOVED: { - setConnections(conns => conns.filter(c => c.id !== payload.id)); - break; - } - } + return client.onNetworkEvent(() => { + setConnections(client.activeConnections()); }); - }); + }, [client, initialized]); const Content = () => { if (!initialized) return ; @@ -78,7 +59,7 @@ export default function NetworkSection() { if (activeConnections.length === 0) return _("No network connections detected"); const summary = activeConnections.map(connection => ( - {connection.name} - {connection.addresses.map(formatIp)} + {connection.id} - {connection.addresses.map(formatIp)} )); const msg = sprintf( diff --git a/web/src/components/overview/NetworkSection.test.jsx b/web/src/components/overview/NetworkSection.test.jsx index 264e4ac606..c1eaad1366 100644 --- a/web/src/components/overview/NetworkSection.test.jsx +++ b/web/src/components/overview/NetworkSection.test.jsx @@ -31,14 +31,14 @@ jest.mock("~/client"); jest.mock('~/components/core/SectionSkeleton', () => () =>
      Section Skeleton
      ); const wiredConnection = { - id: "wired-1", - name: "Wired 1", + id: "Wired 1", + uuid: "d59200d4-838d-4051-99a0-fde8121a242c", type: ConnectionTypes.ETHERNET, addresses: [{ address: "192.168.122.20", prefix: 24 }] }; const wifiConnection = { - id: "wifi-1", - name: "WiFi 1", + id: "WiFi 1", + uuid: "0c00dccb-a6ae-40b2-8495-0de721006bc0", type: ConnectionTypes.WIFI, addresses: [{ address: "192.168.69.200", prefix: 24 }] }; @@ -110,16 +110,20 @@ describe("when connection is added", () => { await screen.findByText(/Wired 1/); await screen.findByText(/WiFi 1/); + // add a new connection + const addedConnection = { + id: "New Wired Network", + uuid: "b143c0a6-ee61-49dc-94aa-e62230afc199", + type: ConnectionTypes.ETHERNET, + addresses: [{ address: "192.168.168.192", prefix: 24 }] + }; + activeConnectionsFn.mockReturnValue([wiredConnection, wifiConnection, addedConnection]); + const [cb] = callbacks; act(() => { cb({ type: NetworkEventTypes.ACTIVE_CONNECTION_ADDED, - payload: { - id: "wired-2", - name: "New Wired Network", - type: ConnectionTypes.ETHERNET, - addresses: [{ address: "192.168.168.192", prefix: 24 }] - } + payload: addedConnection }); }); @@ -138,6 +142,8 @@ describe("when connection is removed", () => { await screen.findByText(/Wired 1/); await screen.findByText(/WiFi 1/); + // remove a connection + activeConnectionsFn.mockReturnValue([wifiConnection]); const [cb] = callbacks; act(() => { cb({ @@ -147,7 +153,7 @@ describe("when connection is removed", () => { }); await screen.findByText(/WiFi 1/); - const removedNetwork = await screen.queryByText(/Wired 1/); + const removedNetwork = screen.queryByText(/Wired 1/); expect(removedNetwork).toBeNull(); }); }); @@ -160,18 +166,21 @@ describe("when connection is updated", () => { await screen.findByText(/Wired 1/); await screen.findByText(/WiFi 1/); + // update a connection + const updatedConnection = { ...wiredConnection, id: "My Wired Connection" }; + activeConnectionsFn.mockReturnValue([updatedConnection, wifiConnection]); const [cb] = callbacks; act(() => { cb({ type: NetworkEventTypes.ACTIVE_CONNECTION_UPDATED, - payload: { ...wiredConnection, name: "My Wired Connection" } + payload: { ...wiredConnection, id: "My Wired Connection" } }); }); await screen.findByText(/My Wired Connection/); await screen.findByText(/WiFi 1/); - const formerWiredName = await screen.queryByText(/Wired 1/); + const formerWiredName = screen.queryByText(/Wired 1/); expect(formerWiredName).toBeNull(); }); });