From 0c70a9ef6d74a5df24575bc72da0fac8264dae0f Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Fri, 22 Jun 2018 03:21:25 +0200 Subject: [PATCH 1/3] cli: match both ssid and bssid when connecting to wifi Instead of matching candidate APs by either SSID or BSSID (bssid1_arr) make sure both match if a bssid2_arr was given. --- clients/cli/devices.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/clients/cli/devices.c b/clients/cli/devices.c index d9b84c1b0..ac503e2f9 100644 --- a/clients/cli/devices.c +++ b/clients/cli/devices.c @@ -2678,7 +2678,7 @@ find_wifi_device_by_iface (NMDevice **devices, const char *iface, int *idx) } /* - * Find AP on 'device' according to 'bssid' or 'ssid' parameter. + * Find AP on 'device' according to 'bssid' and 'ssid' parameters. * Returns: found AP or NULL */ static NMAccessPoint * @@ -2695,17 +2695,17 @@ find_ap_on_device (NMDevice *device, const char *bssid, const char *ssid, gboole NMAccessPoint *candidate_ap = g_ptr_array_index (aps, i); if (bssid) { - /* Parameter is BSSID */ const char *candidate_bssid = nm_access_point_get_bssid (candidate_ap); + if (!candidate_bssid) + continue; + /* Compare BSSIDs */ if (complete) { if (g_str_has_prefix (candidate_bssid, bssid)) g_print ("%s\n", candidate_bssid); - } else if (strcmp (bssid, candidate_bssid) == 0) { - ap = candidate_ap; - break; - } + } else if (strcmp (bssid, candidate_bssid) != 0) + continue; } if (ssid) { @@ -2724,13 +2724,18 @@ find_ap_on_device (NMDevice *device, const char *bssid, const char *ssid, gboole if (complete) { if (g_str_has_prefix (ssid_tmp, ssid)) g_print ("%s\n", ssid_tmp); - } else if (strcmp (ssid, ssid_tmp) == 0) { - ap = candidate_ap; + } else if (strcmp (ssid, ssid_tmp) != 0) { g_free (ssid_tmp); - break; + continue; } g_free (ssid_tmp); } + + if (complete) + continue; + + ap = candidate_ap; + break; } return ap; @@ -3354,14 +3359,14 @@ do_device_wifi_connect_network (NmCli *nmc, int argc, char **argv) } /* Find an AP to connect to */ - ap = find_ap_on_device (device, bssid1_arr ? param_user : NULL, + ap = find_ap_on_device (device, bssid1_arr ? param_user : bssid, bssid1_arr ? NULL : param_user, FALSE); if (!ap && !ifname) { NMDevice *dev; /* AP not found, ifname was not specified, so try finding the AP on another device. */ while ((dev = find_wifi_device_by_iface (devices, NULL, &devices_idx)) != NULL) { - ap = find_ap_on_device (dev, bssid1_arr ? param_user : NULL, + ap = find_ap_on_device (dev, bssid1_arr ? param_user : bssid, bssid1_arr ? NULL : param_user, FALSE); if (ap) { device = dev; From 35932375272a7f723b63017a8524d91ff7113edc Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Tue, 12 Jun 2018 06:52:01 +0200 Subject: [PATCH 2/3] cli: reuse connections in nmcli dev wifi con Try to locate an existing connection before creating a new one when handling "nmcli device wifi connect". This allows WPA-Enterprise networks to be activated this way, consistent with the comment that this command is equivalent to clicking on an SSID in a GUI client. --- clients/cli/devices.c | 191 +++++++++++++++++++++++++++++------------- man/nmcli.xml | 13 ++- 2 files changed, 139 insertions(+), 65 deletions(-) diff --git a/clients/cli/devices.c b/clients/cli/devices.c index ac503e2f9..a98a2003f 100644 --- a/clients/cli/devices.c +++ b/clients/cli/devices.c @@ -847,14 +847,13 @@ usage_device_wifi (void) "ARGUMENTS := connect <(B)SSID> [password ] [wep-key-type key|phrase] [ifname ]\n" " [bssid ] [name ] [private yes|no] [hidden yes|no]\n" "\n" - "Connect to a Wi-Fi network specified by SSID or BSSID. The command creates\n" - "a new connection and then activates it on a device. This is a command-line\n" - "counterpart of clicking an SSID in a GUI client. The command always creates\n" - "a new connection and thus it is mainly useful for connecting to new Wi-Fi\n" - "networks. If a connection for the network already exists, it is better to\n" - "bring up the existing profile as follows: nmcli con up id . Note that\n" - "only open, WEP and WPA-PSK networks are supported at the moment. It is also\n" - "assumed that IP configuration is obtained via DHCP.\n" + "Connect to a Wi-Fi network specified by SSID or BSSID. The command finds a\n" + "matching connection or creates one and then activates it on a device. This\n" + "is a command-line counterpart of clicking an SSID in a GUI client. If a\n" + "connection for the network already exists, it is possible to bring up the\n" + "existing profile as follows: nmcli con up id . Note that only open,\n" + "WEP and WPA-PSK networks are supported if no previous connection exists.\n" + "It is also assumed that IP configuration is obtained via DHCP.\n" "\n" "ARGUMENTS := hotspot [ifname ] [con-name ] [ssid ]\n" " [band a|bg] [channel ] [password ]\n" @@ -1839,6 +1838,7 @@ typedef struct { NmCli *nmc; NMDevice *device; gboolean hotspot; + gboolean create; } AddAndActivateInfo; static void @@ -1853,15 +1853,21 @@ add_and_activate_cb (GObject *client, NMActiveConnection *active; GError *error = NULL; - active = nm_client_add_and_activate_connection_finish (NM_CLIENT (client), result, &error); + if (info->create) + active = nm_client_add_and_activate_connection_finish (NM_CLIENT (client), result, &error); + else + active = nm_client_activate_connection_finish (NM_CLIENT (client), result, &error); if (error) { if (info->hotspot) g_string_printf (nmc->return_text, _("Error: Failed to setup a Wi-Fi hotspot: %s"), error->message); - else + else if (info->create) g_string_printf (nmc->return_text, _("Error: Failed to add/activate new connection: %s"), error->message); + else + g_string_printf (nmc->return_text, _("Error: Failed to activate connection: %s"), + error->message); g_error_free (error); nmc->return_value = NMC_RESULT_ERROR_CON_ACTIVATION; quit (); @@ -1871,8 +1877,10 @@ add_and_activate_cb (GObject *client, if (state == NM_ACTIVE_CONNECTION_STATE_UNKNOWN) { if (info->hotspot) g_string_printf (nmc->return_text, _("Error: Failed to setup a Wi-Fi hotspot")); - else + else if (info->create) g_string_printf (nmc->return_text, _("Error: Failed to add/activate new connection: Unknown error")); + else + g_string_printf (nmc->return_text, _("Error: Failed to activate connection: Unknown error")); nmc->return_value = NMC_RESULT_ERROR_CON_ACTIVATION; g_object_unref (active); quit (); @@ -1883,12 +1891,16 @@ add_and_activate_cb (GObject *client, if (state == NM_ACTIVE_CONNECTION_STATE_ACTIVATED) { if (nmc->nmc_config.print_output == NMC_PRINT_PRETTY) nmc_terminal_erase_line (); - if (!info->hotspot) + if (info->hotspot) + g_print (_("Hotspot '%s' activated on device '%s'\n"), + nm_active_connection_get_id (active), nm_device_get_iface (device)); + else if (info->create) g_print (_("Connection with UUID '%s' created and activated on device '%s'\n"), nm_active_connection_get_uuid (active), nm_device_get_iface (device)); else - g_print (_("Hotspot '%s' activated on device '%s'\n"), - nm_active_connection_get_id (active), nm_device_get_iface (device)); + g_print (_("Connection with ID '%s', UUID '%s' activated on device '%s'\n"), + nm_active_connection_get_id (active), nm_active_connection_get_uuid (active), + nm_device_get_iface (device)); } g_object_unref (active); quit (); @@ -1947,6 +1959,7 @@ connect_device_cb (GObject *client, GAsyncResult *result, gpointer user_data) if (error) { /* If no connection existed for the device, create one and activate it */ if (g_error_matches (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_CONNECTION)) { + info->create = TRUE; create_connect_connection_for_device (info); return; } @@ -3139,7 +3152,6 @@ do_device_wifi_connect_network (NmCli *nmc, int argc, char **argv) NMConnection *connection = NULL; NMSettingConnection *s_con; NMSettingWireless *s_wifi; - NMSettingWirelessSecurity *s_wsec; AddAndActivateInfo *info; const char *param_user = NULL; const char *ifname = NULL; @@ -3155,6 +3167,10 @@ do_device_wifi_connect_network (NmCli *nmc, int argc, char **argv) int devices_idx; char *ssid_ask = NULL; char *passwd_ask = NULL; + const GPtrArray *avail_cons; + gboolean name_match = FALSE; + gboolean existing_con = FALSE; + int i; /* Set default timeout waiting for operation completion. */ if (nmc->timeout == -1) @@ -3384,45 +3400,75 @@ do_device_wifi_connect_network (NmCli *nmc, int argc, char **argv) goto finish; } - /* If there are some connection data from user, create a connection and - * fill them into proper settings. */ - if (con_name || private || bssid2_arr || password || hidden) - connection = nm_simple_connection_new (); + avail_cons = nm_device_get_available_connections (device); + for (i = 0; i < avail_cons->len; i++) { + NMRemoteConnection *avail_con = g_ptr_array_index (avail_cons, i); + const char *id = nm_connection_get_id (NM_CONNECTION (avail_con)); - if (con_name || private) { - s_con = (NMSettingConnection *) nm_setting_connection_new (); - nm_connection_add_setting (connection, NM_SETTING (s_con)); + if (con_name) { + if (!id || strcmp (id, con_name)) + continue; - /* Set user provided connection name */ - if (con_name) - g_object_set (s_con, NM_SETTING_CONNECTION_ID, con_name, NULL); + name_match = TRUE; + } - /* Connection will only be visible to this user when 'private' is specified */ - if (private) - nm_setting_connection_add_permission (s_con, "user", g_get_user_name (), NULL); + if (nm_access_point_connection_valid (ap, NM_CONNECTION (avail_con))) { + /* ap has been checked against bssid1, bssid2 and the ssid + * and now avail_con has been checked against ap. + */ + connection = NM_CONNECTION (avail_con); + existing_con = TRUE; + break; + } } - if (bssid2_arr || hidden) { - s_wifi = (NMSettingWireless *) nm_setting_wireless_new (); - nm_connection_add_setting (connection, NM_SETTING (s_wifi)); - /* 'bssid' parameter is used to restrict the connection only to the BSSID */ - if (bssid2_arr) - g_object_set (s_wifi, NM_SETTING_WIRELESS_BSSID, bssid2_arr, NULL); + if (name_match && !existing_con) { + g_string_printf (nmc->return_text, _("Error: Connection '%s' exists but properties don't match."), con_name); + nmc->return_value = NMC_RESULT_ERROR_NOT_FOUND; + goto finish; + } - /* 'hidden' parameter is used to indicate that SSID is not broadcasted */ - if (hidden) { - GBytes *ssid = g_bytes_new (param_user, strlen (param_user)); + if (!existing_con) { + /* If there are some connection data from user, create a connection and + * fill them into proper settings. */ + if (con_name || private || bssid2_arr || password || hidden) + connection = nm_simple_connection_new (); - g_object_set (s_wifi, - NM_SETTING_WIRELESS_SSID, ssid, - NM_SETTING_WIRELESS_HIDDEN, hidden, - NULL); - g_bytes_unref (ssid); + if (con_name || private) { + s_con = (NMSettingConnection *) nm_setting_connection_new (); + nm_connection_add_setting (connection, NM_SETTING (s_con)); - /* Warn when the provided AP identifier looks like BSSID instead of SSID */ - if (bssid1_arr) - g_printerr (_("Warning: '%s' should be SSID for hidden APs; but it looks like a BSSID.\n"), - param_user); + /* Set user provided connection name */ + if (con_name) + g_object_set (s_con, NM_SETTING_CONNECTION_ID, con_name, NULL); + + /* Connection will only be visible to this user when 'private' is specified */ + if (private) + nm_setting_connection_add_permission (s_con, "user", g_get_user_name (), NULL); + } + if (bssid2_arr || hidden) { + s_wifi = (NMSettingWireless *) nm_setting_wireless_new (); + nm_connection_add_setting (connection, NM_SETTING (s_wifi)); + + /* 'bssid' parameter is used to restrict the connection only to the BSSID */ + if (bssid2_arr) + g_object_set (s_wifi, NM_SETTING_WIRELESS_BSSID, bssid2_arr, NULL); + + /* 'hidden' parameter is used to indicate that SSID is not broadcasted */ + if (hidden) { + GBytes *ssid = g_bytes_new (param_user, strlen (param_user)); + + g_object_set (s_wifi, + NM_SETTING_WIRELESS_SSID, ssid, + NM_SETTING_WIRELESS_HIDDEN, hidden, + NULL); + g_bytes_unref (ssid); + + /* Warn when the provided AP identifier looks like BSSID instead of SSID */ + if (bssid1_arr) + g_printerr (_("Warning: '%s' should be SSID for hidden APs; but it looks like a BSSID.\n"), + param_user); + } } } @@ -3435,8 +3481,25 @@ do_device_wifi_connect_network (NmCli *nmc, int argc, char **argv) if ( (ap_flags & NM_802_11_AP_FLAGS_PRIVACY) || ap_wpa_flags != NM_802_11_AP_SEC_NONE || ap_rsn_flags != NM_802_11_AP_SEC_NONE) { + const char *con_password = NULL; + NMSettingWirelessSecurity *s_wsec = NULL; + + if (connection) { + s_wsec = nm_connection_get_setting_wireless_security (connection); + if (s_wsec) { + if (ap_wpa_flags == NM_802_11_AP_SEC_NONE && ap_rsn_flags == NM_802_11_AP_SEC_NONE) { + /* WEP */ + con_password = nm_setting_wireless_security_get_wep_key (s_wsec, 0); + } else if ( (ap_wpa_flags & NM_802_11_AP_SEC_KEY_MGMT_PSK) + || (ap_rsn_flags & NM_802_11_AP_SEC_KEY_MGMT_PSK)) { + /* WPA PSK */ + con_password = nm_setting_wireless_security_get_psk (s_wsec); + } + } + } + /* Ask for missing password when one is expected and '--ask' is used */ - if (!password && nmc->ask) { + if (!password && !con_password && nmc->ask) { password = passwd_ask = nmc_readline_echo (&nmc->nmc_config, nmc->nmc_config.show_secrets, _("Password: ")); @@ -3445,8 +3508,10 @@ do_device_wifi_connect_network (NmCli *nmc, int argc, char **argv) if (password) { if (!connection) connection = nm_simple_connection_new (); - s_wsec = (NMSettingWirelessSecurity *) nm_setting_wireless_security_new (); - nm_connection_add_setting (connection, NM_SETTING (s_wsec)); + if (!s_wsec) { + s_wsec = (NMSettingWirelessSecurity *) nm_setting_wireless_security_new (); + nm_connection_add_setting (connection, NM_SETTING (s_wsec)); + } if (ap_wpa_flags == NM_802_11_AP_SEC_NONE && ap_rsn_flags == NM_802_11_AP_SEC_NONE) { /* WEP */ @@ -3462,7 +3527,7 @@ do_device_wifi_connect_network (NmCli *nmc, int argc, char **argv) } } } - // FIXME: WPA-Enterprise is not supported yet. + // FIXME: Creating WPA-Enterprise connections is not supported yet. // We are not able to determine and fill all the parameters for // 802.1X authentication automatically without user providing // the data. Adding nmcli options for the 8021x setting would @@ -3480,14 +3545,24 @@ do_device_wifi_connect_network (NmCli *nmc, int argc, char **argv) info->nmc = nmc; info->device = device; info->hotspot = FALSE; - - nm_client_add_and_activate_connection_async (nmc->client, - connection, - device, - nm_object_get_path (NM_OBJECT (ap)), - NULL, - add_and_activate_cb, - info); + info->create = !existing_con; + if (existing_con) { + nm_client_activate_connection_async (nmc->client, + connection, + device, + nm_object_get_path (NM_OBJECT (ap)), + NULL, + add_and_activate_cb, + info); + } else { + nm_client_add_and_activate_connection_async (nmc->client, + connection, + device, + nm_object_get_path (NM_OBJECT (ap)), + NULL, + add_and_activate_cb, + info); + } finish: if (bssid1_arr) diff --git a/man/nmcli.xml b/man/nmcli.xml index 3986de7a9..0a9e95c4b 100644 --- a/man/nmcli.xml +++ b/man/nmcli.xml @@ -1445,14 +1445,13 @@ Connect to a Wi-Fi network specified by SSID or BSSID. The command - creates a new connection and then activates it on a device. This is a - command-line counterpart of clicking an SSID in a GUI client. The command - always creates a new connection and thus it is mainly useful for connecting to - new Wi-Fi networks. If a connection for the network already exists, it is - better to bring up (activate) the existing connection as follows: + finds a matching connnection or creates one and then activates it on a device. + This is a command-line counterpart of clicking an SSID in a GUI client. If + a connection for the network already exists, it is possible to bring up + (activate) the existing profile as follows: nmcli con up id name. Note that - only open, WEP and WPA-PSK networks are supported at the moment. It is also - supposed that IP configuration is obtained via DHCP. + only open, WEP and WPA-PSK networks are supported if no previous connection + exists. It is also assumed that IP configuration is obtained via DHCP. If option is not specified, the default timeout will be 90 seconds. From f13da72b3c2c0466aa5c37070ca4158b6d123664 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Thu, 29 Nov 2018 17:31:31 +0100 Subject: [PATCH 3/3] cli/devices: remove redundant Device/AC state evaluation The state handling in add_and_activate_cb() and connect_device_cb() is redundant to connected_state_cb() and in fact executed only if the activation is really really really quick (which it never is). Also, the return_text those implementations provide is different from what connected_state_cb(), potentially confusing the users and adding extra work for translators. Not to mention extra lines of code, reading whose wastes our precious time on the planet we could spend doing heroin instead. --- clients/cli/devices.c | 46 +++++++------------------------------------ 1 file changed, 7 insertions(+), 39 deletions(-) diff --git a/clients/cli/devices.c b/clients/cli/devices.c index a98a2003f..48bcdb593 100644 --- a/clients/cli/devices.c +++ b/clients/cli/devices.c @@ -1849,7 +1849,6 @@ add_and_activate_cb (GObject *client, AddAndActivateInfo *info = (AddAndActivateInfo *) user_data; NmCli *nmc = info->nmc; NMDevice *device = info->device; - NMActiveConnectionState state; NMActiveConnection *active; GError *error = NULL; @@ -1872,36 +1871,7 @@ add_and_activate_cb (GObject *client, nmc->return_value = NMC_RESULT_ERROR_CON_ACTIVATION; quit (); } else { - state = nm_active_connection_get_state (active); - - if (state == NM_ACTIVE_CONNECTION_STATE_UNKNOWN) { - if (info->hotspot) - g_string_printf (nmc->return_text, _("Error: Failed to setup a Wi-Fi hotspot")); - else if (info->create) - g_string_printf (nmc->return_text, _("Error: Failed to add/activate new connection: Unknown error")); - else - g_string_printf (nmc->return_text, _("Error: Failed to activate connection: Unknown error")); - nmc->return_value = NMC_RESULT_ERROR_CON_ACTIVATION; - g_object_unref (active); - quit (); - } - - if (nmc->nowait_flag || state == NM_ACTIVE_CONNECTION_STATE_ACTIVATED) { - /* User doesn't want to wait or already activated */ - if (state == NM_ACTIVE_CONNECTION_STATE_ACTIVATED) { - if (nmc->nmc_config.print_output == NMC_PRINT_PRETTY) - nmc_terminal_erase_line (); - if (info->hotspot) - g_print (_("Hotspot '%s' activated on device '%s'\n"), - nm_active_connection_get_id (active), nm_device_get_iface (device)); - else if (info->create) - g_print (_("Connection with UUID '%s' created and activated on device '%s'\n"), - nm_active_connection_get_uuid (active), nm_device_get_iface (device)); - else - g_print (_("Connection with ID '%s', UUID '%s' activated on device '%s'\n"), - nm_active_connection_get_id (active), nm_active_connection_get_uuid (active), - nm_device_get_iface (device)); - } + if (nmc->nowait_flag) { g_object_unref (active); quit (); } else { @@ -1909,6 +1879,8 @@ add_and_activate_cb (GObject *client, g_signal_connect (device, "notify::state", G_CALLBACK (device_state_cb), active); g_signal_connect (active, "notify::state", G_CALLBACK (active_state_cb), device); + connected_state_cb (device, active); + g_timeout_add_seconds (nmc->timeout, timeout_cb, nmc); /* Exit if timeout expires */ if (nmc->nmc_config.print_output == NMC_PRINT_PRETTY) @@ -1952,7 +1924,6 @@ connect_device_cb (GObject *client, GAsyncResult *result, gpointer user_data) GError *error = NULL; const GPtrArray *devices; NMDevice *device; - NMDeviceState state; active = nm_client_activate_connection_finish (NM_CLIENT (client), result, &error); @@ -1982,14 +1953,8 @@ connect_device_cb (GObject *client, GAsyncResult *result, gpointer user_data) } device = g_ptr_array_index (devices, 0); - state = nm_device_get_state (device); - if (nmc->nowait_flag || state == NM_DEVICE_STATE_ACTIVATED) { - /* Don't want to wait or device already activated */ - if (state == NM_DEVICE_STATE_ACTIVATED && nmc->nmc_config.print_output == NMC_PRINT_PRETTY) { - nmc_terminal_erase_line (); - g_print (_("Device '%s' has been connected.\n"), nm_device_get_iface (device)); - } + if (nmc->nowait_flag) { g_object_unref (active); quit (); } else { @@ -2003,6 +1968,9 @@ connect_device_cb (GObject *client, GAsyncResult *result, gpointer user_data) g_object_ref (device); g_signal_connect (device, "notify::state", G_CALLBACK (device_state_cb), active); g_signal_connect (active, "notify::state", G_CALLBACK (active_state_cb), device); + + connected_state_cb (device, active); + /* Start timer not to loop forever if "notify::state" signal is not issued */ g_timeout_add_seconds (nmc->timeout, timeout_cb, nmc); }