wifictl dbus_nm: serve AccessPoint interfaces at returned paths; fix active index

Two defects from the 5-lane review of commit b6ac916a2c: 1. AP paths unserved (MAJOR) get_access_points() returned Vec<OwnedObjectPath> but no AP interface objects were exported at those paths. Qt and other NM clients would see the device but no APs at all. Fix: add AccessPointInterface (#[interface(name = "org.freedesktop.NetworkManager.AccessPoint")]) with the standard AP properties (Flags, WpaFlags, RsnFlags, Ssid, Frequency, Mode, MaxBitrate, Strength, HwAddress, LastSeen). serve_on_thread now builds a (path, interface) pair for each AP and calls serve_at() for each, so every returned path refers to a real D-Bus object. 2. ActiveAccessPoint wrong-index bug (MAJOR) When the matching active SSID was at AP index 1 or later, active_access_point_inner() returned path index 0. Now iterates over access_points to find the index where ap.ssid == active_ssid and returns that path. No-match returns '/' sentinel. Verified: 49/49 tests pass (47 unit + 2 cli_transport; was 41 + 2 in the round-2 commit, +6 new AP interface + active-index tests).
2026-07-28 07:22:37 +09:00
parent 25cb25c373
commit cd429e8c74
1 changed files with 269 additions and 16 deletions
@@ -230,7 +230,7 @@ impl NmAccessPoint {
 mod nm_iface {
    use std::sync::{Arc, Mutex};

-    use super::{NmState, NmWifiDevice};
+    use super::{NmAccessPoint, NmState, NmWifiDevice};
    use zbus::{
        blocking::connection::Builder as ConnectionBuilder, fdo, interface,
        zvariant::{ObjectPath, OwnedObjectPath},
@@ -374,19 +374,39 @@ mod nm_iface {
                .collect()
        }

+        /// Build the `(path, interface)` pair for every access point in the
+        /// current snapshot, so `serve_on_thread` can export each path as a
+        /// real `org.freedesktop.NetworkManager.AccessPoint` object.
+        pub(crate) fn access_point_interfaces(
+            &self,
+        ) -> Vec<(String, AccessPointInterface)> {
+            self.snap()
+                .access_points
+                .iter()
+                .enumerate()
+                .map(|(index, ap)| {
+                    (
+                        access_point_path(index),
+                        AccessPointInterface::new(ap.clone()),
+                    )
+                })
+                .collect()
+        }
+
        pub(crate) fn active_access_point_inner(&self) -> fdo::Result<OwnedObjectPath> {
            let snap = self.snap();
-            let has_active = !snap.active_ssid.is_empty()
-                && snap
-                    .access_points
-                    .iter()
-                    .any(|ap| ap.ssid == snap.active_ssid);
-            if has_active {
-                try_path(&access_point_path(0))
-            } else {
-                // Per the NetworkManager spec, `ActiveAccessPoint` is the root
-                // path "/" when no access point is associated.
-                try_path("/")
+            // Per the NetworkManager spec, `ActiveAccessPoint` is the root
+            // path "/" when no access point is associated.
+            if snap.active_ssid.is_empty() {
+                return try_path("/");
+            }
+            match snap
+                .access_points
+                .iter()
+                .position(|ap| ap.ssid == snap.active_ssid)
+            {
+                Some(index) => try_path(&access_point_path(index)),
+                None => try_path("/"),
            }
        }

@@ -488,6 +508,158 @@ mod nm_iface {
        }
    }

+    // -----------------------------------------------------------------------
+    // Access point interface: org.freedesktop.NetworkManager.AccessPoint
+    // -----------------------------------------------------------------------
+
+    /// Subset of NetworkManager's `NM80211ApSecurityFlags` bitmask that the
+    /// daemon's coarse `security` string can represent.
+    mod nm_ap_sec {
+        /// No security.
+        pub const NONE: u32 = 0;
+        /// Pairwise CCMP (AES) cipher.
+        pub const PAIR_CCMP: u32 = 0x8;
+        /// PSK key management (WPA-/WPA2-Personal).
+        pub const KEY_MGMT_PSK: u32 = 0x100;
+        /// SAE key management (WPA3-Personal).
+        pub const KEY_MGMT_SAE: u32 = 0x400;
+    }
+
+    /// Map the access point's `security` label to `(wpa_flags, rsn_flags)`:
+    ///
+    /// | label               | wpa_flags  | rsn_flags  |
+    /// |---------------------|------------|------------|
+    /// | `open`/`wep`/other  | none       | none       |
+    /// | `wpa`               | CCMP + PSK | none       |
+    /// | `wpa2`              | CCMP + PSK | CCMP + PSK |
+    /// | `wpa3`/`sae`        | none       | CCMP + SAE |
+    fn derive_security_flags(security: &str) -> (u32, u32) {
+        let wpa2 = (nm_ap_sec::PAIR_CCMP | nm_ap_sec::KEY_MGMT_PSK,
+                    nm_ap_sec::PAIR_CCMP | nm_ap_sec::KEY_MGMT_PSK);
+        match security.to_ascii_lowercase().as_str() {
+            "wpa2" | "wpa3-personal" => wpa2,
+            "wpa" => (nm_ap_sec::PAIR_CCMP | nm_ap_sec::KEY_MGMT_PSK, nm_ap_sec::NONE),
+            "wpa3" | "sae" | "wpa3-sae" => {
+                (nm_ap_sec::NONE, nm_ap_sec::PAIR_CCMP | nm_ap_sec::KEY_MGMT_SAE)
+            }
+            _ => (nm_ap_sec::NONE, nm_ap_sec::NONE),
+        }
+    }
+
+    /// D-Bus interface for a single access point, served at
+    /// `{DEVICE_PATH}/AccessPoints/{index}`. Wraps an [`NmAccessPoint`]
+    /// snapshot and exposes the standard
+    /// `org.freedesktop.NetworkManager.AccessPoint` properties.
+    #[derive(Clone)]
+    pub(crate) struct AccessPointInterface {
+        ap: NmAccessPoint,
+    }
+
+    impl AccessPointInterface {
+        pub(crate) fn new(ap: NmAccessPoint) -> Self {
+            Self { ap }
+        }
+
+        pub(crate) fn ssid_bytes(&self) -> Vec<u8> {
+            self.ap.ssid.as_bytes().to_vec()
+        }
+
+        pub(crate) fn frequency_value(&self) -> u32 {
+            self.ap.frequency
+        }
+
+        pub(crate) fn strength_value(&self) -> u8 {
+            self.ap.strength
+        }
+
+        pub(crate) fn wpa_flags_value(&self) -> u32 {
+            derive_security_flags(&self.ap.security).0
+        }
+
+        pub(crate) fn rsn_flags_value(&self) -> u32 {
+            derive_security_flags(&self.ap.security).1
+        }
+
+        /// NM 80211Mode: `1` = infrastructure. The daemon does not yet
+        /// distinguish adhoc/ap.
+        pub(crate) fn mode_value(&self) -> u32 {
+            1
+        }
+
+        /// NM 80211ApFlags: `0` = none advertised.
+        pub(crate) fn flags_value(&self) -> u32 {
+            0
+        }
+
+        /// Max bitrate in kbit/s; `0` when not yet determined.
+        pub(crate) fn max_bitrate_value(&self) -> u32 {
+            0
+        }
+
+        /// MAC address; empty until the daemon records one.
+        pub(crate) fn hw_address_value(&self) -> String {
+            String::new()
+        }
+
+        /// Wall-clock timestamp of the last beacon seen; `0` when unknown.
+        pub(crate) fn last_seen_value(&self) -> i32 {
+            0
+        }
+    }
+
+    #[interface(name = "org.freedesktop.NetworkManager.AccessPoint")]
+    impl AccessPointInterface {
+        #[zbus(property)]
+        fn flags(&self) -> u32 {
+            self.flags_value()
+        }
+
+        #[zbus(property)]
+        fn wpa_flags(&self) -> u32 {
+            self.wpa_flags_value()
+        }
+
+        #[zbus(property)]
+        fn rsn_flags(&self) -> u32 {
+            self.rsn_flags_value()
+        }
+
+        #[zbus(property)]
+        fn ssid(&self) -> Vec<u8> {
+            self.ssid_bytes()
+        }
+
+        #[zbus(property)]
+        fn frequency(&self) -> u32 {
+            self.frequency_value()
+        }
+
+        #[zbus(property)]
+        fn mode(&self) -> u32 {
+            self.mode_value()
+        }
+
+        #[zbus(property)]
+        fn max_bitrate(&self) -> u32 {
+            self.max_bitrate_value()
+        }
+
+        #[zbus(property)]
+        fn strength(&self) -> u8 {
+            self.strength_value()
+        }
+
+        #[zbus(property)]
+        fn hw_address(&self) -> String {
+            self.hw_address_value()
+        }
+
+        #[zbus(property)]
+        fn last_seen(&self) -> i32 {
+            self.last_seen_value()
+        }
+    }
+
    // -----------------------------------------------------------------------
    // Registration
    // -----------------------------------------------------------------------
@@ -519,13 +691,25 @@ mod nm_iface {
            let device_path: ObjectPath<'_> = DEVICE_PATH.try_into()?;

            let root = NmRoot::new(Arc::clone(&shared));
-            let wireless = NmWirelessDevice::new(shared);
+            let wireless = NmWirelessDevice::new(Arc::clone(&shared));
+            // Collect AP interfaces before `wireless` is moved into serve_at.
+            let ap_interfaces = wireless.access_point_interfaces();

-            let _connection = ConnectionBuilder::session()?
+            let mut builder = ConnectionBuilder::session()?
                .name(BUS_NAME)?
                .serve_at(root_path, root)?
-                .serve_at(device_path, wireless)?
-                .build()?;
+                .serve_at(device_path, wireless)?;
+
+            // Export each access point at its object path so the paths
+            // returned by GetAccessPoints refer to real D-Bus objects
+            // implementing org.freedesktop.NetworkManager.AccessPoint.
+            for (path_str, iface) in ap_interfaces {
+                let ap_path: ObjectPath<'static> =
+                    path_str.as_str().to_owned().try_into()?;
+                builder = builder.serve_at(ap_path, iface)?;
+            }
+
+            let _connection = builder.build()?;

            log::info!(
                "wifictl: D-Bus NetworkManager interface registered on the \
@@ -756,6 +940,7 @@ mod tests {

        use super::super::nm_iface::{
            DEVICE_PATH, NmRoot, NmWirelessDevice, access_point_path, shared_device,
+            AccessPointInterface,
        };
        use super::{NmAccessPoint, NmDeviceState, NmState, NmWifiDevice};

@@ -825,6 +1010,74 @@ mod tests {
            );
        }

+        #[test]
+        fn wireless_device_active_access_point_resolves_to_matching_index() {
+            // "Guest" is the second access point (index 1). The old code
+            // returned index 0 unconditionally; this locks in the real index.
+            let mut device = device_with(NmDeviceState::Activated);
+            device.active_ssid = "Guest".to_string();
+            device.active_strength = 40;
+            let dev = NmWirelessDevice::new(shared_device(&device));
+            assert_eq!(
+                dev.active_access_point_inner().unwrap().as_str(),
+                access_point_path(1),
+            );
+        }
+
+        #[test]
+        fn wireless_device_active_access_point_empty_ssid_is_root() {
+            let mut device = device_with(NmDeviceState::Disconnected);
+            device.active_ssid.clear();
+            let dev = NmWirelessDevice::new(shared_device(&device));
+            assert_eq!(dev.active_access_point_inner().unwrap().as_str(), "/");
+        }
+
+        #[test]
+        fn access_point_interfaces_match_paths_and_data() {
+            let dev = NmWirelessDevice::new(shared_device(&device_with(
+                NmDeviceState::Disconnected,
+            )));
+            let ifaces = dev.access_point_interfaces();
+            assert_eq!(ifaces.len(), 2);
+            assert_eq!(ifaces[0].0, access_point_path(0));
+            assert_eq!(ifaces[0].1.ssid_bytes(), b"RedBear");
+            assert_eq!(ifaces[0].1.frequency_value(), 2412);
+            assert_eq!(ifaces[0].1.strength_value(), 87);
+            assert_eq!(ifaces[1].0, access_point_path(1));
+            assert_eq!(ifaces[1].1.ssid_bytes(), b"Guest");
+            assert_eq!(ifaces[1].1.frequency_value(), 5180);
+            assert_eq!(ifaces[1].1.strength_value(), 40);
+        }
+
+        #[test]
+        fn access_point_interfaces_is_empty_when_no_aps() {
+            let dev =
+                NmWirelessDevice::new(shared_device(&NmWifiDevice::default()));
+            assert!(dev.access_point_interfaces().is_empty());
+        }
+
+        #[test]
+        fn access_point_interface_exposes_wpa2_security_flags() {
+            let iface = AccessPointInterface::new(NmAccessPoint::new(
+                "RedBear", 87, "wpa2", 2412,
+            ));
+            assert!(iface.wpa_flags_value() != 0);
+            assert_eq!(iface.wpa_flags_value(), iface.rsn_flags_value());
+            assert_eq!(iface.mode_value(), 1);
+            assert_eq!(iface.flags_value(), 0);
+            assert_eq!(iface.max_bitrate_value(), 0);
+            assert!(iface.hw_address_value().is_empty());
+            assert_eq!(iface.last_seen_value(), 0);
+        }
+
+        #[test]
+        fn access_point_interface_open_security_has_no_flags() {
+            let iface =
+                AccessPointInterface::new(NmAccessPoint::new("Guest", 40, "open", 5180));
+            assert_eq!(iface.wpa_flags_value(), 0);
+            assert_eq!(iface.rsn_flags_value(), 0);
+        }
+
        #[test]
        fn wireless_device_active_access_point_is_root_when_no_match() {
            let mut device = device_with(NmDeviceState::Disconnected);