fixup! activation-request: cease using the secrets if the original connection changed

The @reqid returned from nm_settings_connection_get_secrets() is not the
same domain as g_idle_add(). Hence, we cannot mix them but must treat
them separate.

While at it, replace the guint32 return type by an opaque pointer.

Also, invoke now ~always~ the callback (possibly with
G_IO_ERROR/CANCELLED).

7 files changed, 147 insertions, 113 deletions

diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c
index fd7184ca52..0c7f19d397 100644
--- a/src/devices/nm-device-ethernet.c
+++ b/src/devices/nm-device-ethernet.c
@@ -480,7 +480,7 @@ supplicant_interface_release (NMDeviceEthernet *self)
 
 static void
 wired_secrets_cb (NMActRequest *req,
-                  guint32 call_id,
+                  NMActRequestGetSecretsCallId call_id,
                   NMSettingsConnection *connection,
                   GError *error,
                   gpointer user_data)
@@ -488,7 +488,9 @@ wired_secrets_cb (NMActRequest *req,
 	NMDeviceEthernet *self = NM_DEVICE_ETHERNET (user_data);
 	NMDevice *dev = NM_DEVICE (self);
 
-	g_return_if_fail (req == nm_device_get_act_request (dev));
+	if (req != nm_device_get_act_request (dev))
+		return;
+
 	g_return_if_fail (nm_device_get_state (dev) == NM_DEVICE_STATE_NEED_AUTH);
 	g_return_if_fail (nm_act_request_get_settings_connection (req) == connection);
 
diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c
index 232fb358df..8e94aaacc1 100644
--- a/src/devices/wifi/nm-device-wifi.c
+++ b/src/devices/wifi/nm-device-wifi.c
@@ -1645,7 +1645,7 @@ cleanup_association_attempt (NMDeviceWifi *self, gboolean disconnect)
 
 static void
 wifi_secrets_cb (NMActRequest *req,
-                 guint32 call_id,
+                 NMActRequestGetSecretsCallId call_id,
                  NMSettingsConnection *connection,
                  GError *error,
                  gpointer user_data)
@@ -1653,7 +1653,9 @@ wifi_secrets_cb (NMActRequest *req,
 	NMDevice *device = NM_DEVICE (user_data);
 	NMDeviceWifi *self = NM_DEVICE_WIFI (device);
 
-	g_return_if_fail (req == nm_device_get_act_request (device));
+	if (req != nm_device_get_act_request (device))
+		return;
+
 	g_return_if_fail (nm_device_get_state (device) == NM_DEVICE_STATE_NEED_AUTH);
 	g_return_if_fail (nm_act_request_get_settings_connection (req) == connection);
 
diff --git a/src/devices/wwan/nm-modem.c b/src/devices/wwan/nm-modem.c
index bf210fef8c..fd6de8afdf 100644
--- a/src/devices/wwan/nm-modem.c
+++ b/src/devices/wwan/nm-modem.c
@@ -31,6 +31,7 @@
 #include "nm-dbus-glib-types.h"
 #include "nm-modem-enum-types.h"
 #include "nm-route-manager.h"
+#include "gsystem-local-alloc.h"
 
 G_DEFINE_TYPE (NMModem, nm_modem, G_TYPE_OBJECT)
 
@@ -74,7 +75,7 @@ typedef struct {
 
 	NMActRequest *act_request;
 	guint32 secrets_tries;
-	guint32 secrets_id;
+	NMActRequestGetSecretsCallId secrets_id;
 
 	guint32 mm_ip_timeout;
 
@@ -723,15 +724,12 @@ cancel_get_secrets (NMModem *self)
 {
 	NMModemPrivate *priv = NM_MODEM_GET_PRIVATE (self);
 
-	if (priv->secrets_id) {
-		nm_act_request_cancel_secrets (priv->act_request, priv->secrets_id);
-		priv->secrets_id = 0;
-	}
+	nm_act_request_cancel_secrets (priv->act_request, priv->secrets_id);
 }
 
 static void
 modem_secrets_cb (NMActRequest *req,
-                  guint32 call_id,
+                  NMActRequestGetSecretsCallId call_id,
                   NMSettingsConnection *connection,
                   GError *error,
                   gpointer user_data)
@@ -741,7 +739,10 @@ modem_secrets_cb (NMActRequest *req,
 
 	g_return_if_fail (call_id == priv->secrets_id);
 
-	priv->secrets_id = 0;
+	priv->secrets_id = NULL;
+
+	if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+		return;
 
 	if (error)
 		nm_log_warn (LOGD_MB, "(%s): %s", nm_modem_get_uid (self), error->message);
@@ -768,10 +769,8 @@ nm_modem_get_secrets (NMModem *self,
 	                                               hint,
 	                                               modem_secrets_cb,
 	                                               self);
-	if (priv->secrets_id)
-		g_signal_emit (self, signals[AUTH_REQUESTED], 0);
-
-	return !!(priv->secrets_id);
+	g_signal_emit (self, signals[AUTH_REQUESTED], 0);
+	return TRUE;
 }
 
 /*****************************************************************************/
@@ -791,8 +790,7 @@ nm_modem_act_stage1_prepare (NMModem *self,
                              NMDeviceStateReason *reason)
 {
 	NMModemPrivate *priv = NM_MODEM_GET_PRIVATE (self);
-	NMActStageReturn ret;
-	GPtrArray *hints = NULL;
+	gs_unref_ptrarray GPtrArray *hints = NULL;
 	const char *setting_name = NULL;
 	NMSecretAgentGetSecretsFlags flags = NM_SECRET_AGENT_GET_SECRETS_FLAG_ALLOW_INTERACTION;
 	NMConnection *connection;
@@ -821,18 +819,8 @@ nm_modem_act_stage1_prepare (NMModem *self,
 	                                               hints ? g_ptr_array_index (hints, 0) : NULL,
 	                                               modem_secrets_cb,
 	                                               self);
-	if (priv->secrets_id) {
-		g_signal_emit (self, signals[AUTH_REQUESTED], 0);
-		ret = NM_ACT_STAGE_RETURN_POSTPONE;
-	} else {
-		*reason = NM_DEVICE_STATE_REASON_NO_SECRETS;
-		ret = NM_ACT_STAGE_RETURN_FAILURE;
-	}
-
-	if (hints)
-		g_ptr_array_free (hints, TRUE);
-
-	return ret;
+	g_signal_emit (self, signals[AUTH_REQUESTED], 0);
+	return NM_ACT_STAGE_RETURN_POSTPONE;
 }
 
 /*****************************************************************************/
diff --git a/src/nm-activation-request.c b/src/nm-activation-request.c
index 3f31007d23..82326e884c 100644
--- a/src/nm-activation-request.c
+++ b/src/nm-activation-request.c
@@ -42,6 +42,11 @@ G_DEFINE_TYPE (NMActRequest, nm_act_request, NM_TYPE_ACTIVE_CONNECTION)
                                        NM_TYPE_ACT_REQUEST, \
                                        NMActRequestPrivate))
 
+typedef enum {
+	GET_SECRETS_INFO_TYPE_REQ,
+	GET_SECRETS_INFO_TYPE_IDLE,
+} GetSecretsInfoType;
+
 typedef struct {
 	char *table;
 	char *rule;
@@ -99,17 +104,37 @@ check_connection_unmodified (NMActRequest *self, GError **error)
 	return TRUE;
 }
 
-typedef struct {
+/*******************************************************************/
+
+struct _NMActRequestGetSecretsCallId {
 	NMActRequest *self;
-	guint32 call_id;
 	NMActRequestSecretsFunc callback;
 	gpointer callback_data;
-	GError *error;
-} GetSecretsInfo;
+	GetSecretsInfoType type;
+	union {
+		struct {
+			guint32 id;
+		} req;
+		struct {
+			guint32 id;
+			GError *error;
+		} idle;
+	} t;
+};
+
+typedef struct _NMActRequestGetSecretsCallId GetSecretsInfo;
+
+static void
+get_secrets_info_free (GetSecretsInfo *info)
+{
+	if (info->type == GET_SECRETS_INFO_TYPE_IDLE)
+		g_clear_error (&info->t.idle.error);
+	g_slice_free (GetSecretsInfo, info);
+}
 
 static void
 get_secrets_cb (NMSettingsConnection *connection,
-                guint32 call_id,
+                guint32 req_id,
                 const char *agent_username,
                 const char *setting_name,
                 GError *error,
@@ -119,7 +144,7 @@ get_secrets_cb (NMSettingsConnection *connection,
 	NMActRequestPrivate *priv = NM_ACT_REQUEST_GET_PRIVATE (info->self);
 	gs_free_error GError *local = NULL;
 
-	g_return_if_fail (info->call_id == call_id);
+	g_return_if_fail (info->t.req.id == req_id);
 	priv->secrets_calls = g_slist_remove (priv->secrets_calls, info);
 
 	if (!error) {
@@ -127,22 +152,19 @@ get_secrets_cb (NMSettingsConnection *connection,
 		error = local;
 	}
 
-	info->callback (info->self, call_id, connection, error, info->callback_data);
-	g_free (info);
+	info->callback (info->self, info, connection, error, info->callback_data);
+	get_secrets_info_free (info);
 }
 
 static gboolean
-not_getting_secrets_cb (gpointer user_data)
+get_secrets_idle_cb (GetSecretsInfo *info)
 {
-	GetSecretsInfo *info = user_data;
-
-	info->callback (info->self, info->call_id, NULL, info->error, info->callback_data);
-	g_free (info);
-
+	info->callback (info->self, info, NULL, info->t.idle.error, info->callback_data);
+	get_secrets_info_free (info);
 	return FALSE;
 }
 
-guint32
+NMActRequestGetSecretsCallId
 nm_act_request_get_secrets (NMActRequest *self,
                             const char *setting_name,
                             NMSecretAgentGetSecretsFlags flags,
@@ -154,73 +176,89 @@ nm_act_request_get_secrets (NMActRequest *self,
 	GetSecretsInfo *info;
 	NMSettingsConnection *connection;
 	const char *hints[2] = { hint, NULL };
+	GError *local = NULL;
+	guint32 req_id;
 
-	g_return_val_if_fail (self, 0);
 	g_return_val_if_fail (NM_IS_ACT_REQUEST (self), 0);
 
 	priv = NM_ACT_REQUEST_GET_PRIVATE (self);
 
 	connection = nm_act_request_get_settings_connection (self);
 
-	info = g_malloc0 (sizeof (GetSecretsInfo));
+	info = g_slice_new (GetSecretsInfo);
 	info->self = self;
 	info->callback = callback;
 	info->callback_data = callback_data;
 
-	if (!check_connection_unmodified (self, &info->error))
-		return info->call_id = g_idle_add_full (G_PRIORITY_DEFAULT_IDLE,
-		                                        not_getting_secrets_cb,
-		                                        info,
-		                                        (GDestroyNotify) not_getting_secrets_cb);
+	priv->secrets_calls = g_slist_append (priv->secrets_calls, info);
+
+	if (!check_connection_unmodified (self, &local))
+		goto schedule_dummy;
 
 	if (nm_active_connection_get_user_requested (NM_ACTIVE_CONNECTION (self)))
 		flags |= NM_SECRET_AGENT_GET_SECRETS_FLAG_USER_REQUESTED;
 
-	info->call_id = nm_settings_connection_get_secrets (connection,
-	                                                    nm_active_connection_get_subject (NM_ACTIVE_CONNECTION (self)),
-	                                                    setting_name,
-	                                                    flags,
-	                                                    hints,
-	                                                    get_secrets_cb,
-	                                                    info,
-	                                                    &info->error);
-	if (info->call_id > 0)
-		priv->secrets_calls = g_slist_append (priv->secrets_calls, info);
+	req_id = nm_settings_connection_get_secrets (connection,
+	                                             nm_active_connection_get_subject (NM_ACTIVE_CONNECTION (self)),
+	                                             setting_name,
+	                                             flags,
+	                                             hints,
+	                                             get_secrets_cb,
+	                                             info,
+	                                             &local);
+	g_assert ((req_id > 0 && !local) || (req_id == 0 && local));
+	if (req_id > 0) {
+		info->type = GET_SECRETS_INFO_TYPE_REQ;
+		info->t.req.id = req_id;
+	} else {
+schedule_dummy:
+		info->type = GET_SECRETS_INFO_TYPE_IDLE;
+		g_propagate_error (&info->t.idle.error, local);
+		info->t.idle.id = g_idle_add ((GSourceFunc) get_secrets_idle_cb, info);
+	}
+	return info;
+}
+
+static void
+get_secrets_cancel (NMActRequest *self, GetSecretsInfo *info, gboolean shutdown)
+{
+	gs_free_error GError *error = NULL;
+
+	if (info->type == GET_SECRETS_INFO_TYPE_REQ)
+		nm_settings_connection_cancel_secrets (nm_act_request_get_settings_connection (self), info->t.req.id);
 	else
-		info->call_id = g_idle_add_full (G_PRIORITY_DEFAULT_IDLE,
-		                                 not_getting_secrets_cb,
-		                                 info,
-		                                 (GDestroyNotify) not_getting_secrets_cb);
+		g_source_remove (info->t.idle.id);
+
+	if (shutdown) {
+		/* hijack an error code. G_IO_ERROR_CANCELLED is only used synchronously
+		 * when the user calls nm_act_request_cancel_secrets(). */
+		g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_NOT_CONNECTED,
+		                     "Disposing NMActRequest instance");
+	} else {
+		g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_CANCELLED,
+		                     "Request cancelled");
+	}
 
-	return info->call_id;
+	info->callback (info->self, info, NULL, error, info->callback_data);
+	get_secrets_info_free (info);
 }
 
 void
-nm_act_request_cancel_secrets (NMActRequest *self, guint32 call_id)
+nm_act_request_cancel_secrets (NMActRequest *self, NMActRequestGetSecretsCallId call_id)
 {
 	NMActRequestPrivate *priv;
-	NMSettingsConnection *connection;
-	GSList *iter;
 
 	g_return_if_fail (self);
 	g_return_if_fail (NM_IS_ACT_REQUEST (self));
-	g_return_if_fail (call_id > 0);
-
-	priv = NM_ACT_REQUEST_GET_PRIVATE (self);
 
-	connection = nm_act_request_get_settings_connection (self);
-	for (iter = priv->secrets_calls; iter; iter = g_slist_next (iter)) {
-		GetSecretsInfo *info = iter->data;
+	if (!call_id)
+		return;
 
-		/* Remove the matching info */
-		if (info->call_id == call_id) {
-			priv->secrets_calls = g_slist_remove_link (priv->secrets_calls, iter);
-			g_slist_free (iter);
+	priv = NM_ACT_REQUEST_GET_PRIVATE (self);
 
-			nm_settings_connection_cancel_secrets (NM_SETTINGS_CONNECTION (connection), call_id);
-			g_free (info);
-			break;
-		}
+	if (g_slist_find (priv->secrets_calls, call_id)) {
+		priv->secrets_calls = g_slist_remove (priv->secrets_calls, call_id);
+		get_secrets_cancel (self, call_id, FALSE);
 	}
 }
 
@@ -479,9 +517,18 @@ nm_act_request_init (NMActRequest *req)
 static void
 dispose (GObject *object)
 {
-	NMActRequestPrivate *priv = NM_ACT_REQUEST_GET_PRIVATE (object);
+	NMActRequest *self = NM_ACT_REQUEST (object);
+	NMActRequestPrivate *priv = NM_ACT_REQUEST_GET_PRIVATE (self);
 	NMSettingsConnection *connection;
-	GSList *iter;
+
+	/* Kill any in-progress secrets requests */
+	connection = nm_act_request_get_settings_connection (NM_ACT_REQUEST (object));
+	while (priv->secrets_calls) {
+		GetSecretsInfo *info = priv->secrets_calls->data;
+
+		priv->secrets_calls = g_slist_delete_link (priv->secrets_calls, priv->secrets_calls);
+		get_secrets_cancel (self, info, TRUE);
+	}
 
 	/* Clear any share rules */
 	if (priv->share_rules) {
@@ -489,17 +536,6 @@ dispose (GObject *object)
 		clear_share_rules (NM_ACT_REQUEST (object));
 	}
 
-	/* Kill any in-progress secrets requests */
-	connection = nm_act_request_get_settings_connection (NM_ACT_REQUEST (object));
-	for (iter = priv->secrets_calls; connection && iter; iter = g_slist_next (iter)) {
-		GetSecretsInfo *info = iter->data;
-
-		nm_settings_connection_cancel_secrets (NM_SETTINGS_CONNECTION (connection), info->call_id);
-		g_free (info);
-	}
-	g_slist_free (priv->secrets_calls);
-	priv->secrets_calls = NULL;
-
 	G_OBJECT_CLASS (nm_act_request_parent_class)->dispose (object);
 }
 
diff --git a/src/nm-activation-request.h b/src/nm-activation-request.h
index bc9c7a8d0e..bc511bab00 100644
--- a/src/nm-activation-request.h
+++ b/src/nm-activation-request.h
@@ -33,6 +33,8 @@
 #define NM_IS_ACT_REQUEST_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), NM_TYPE_ACT_REQUEST))
 #define NM_ACT_REQUEST_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj), NM_TYPE_ACT_REQUEST, NMActRequestClass))
 
+typedef struct _NMActRequestGetSecretsCallId *NMActRequestGetSecretsCallId;
+
 struct _NMActRequest {
 	NMActiveConnection parent;
 };
@@ -64,19 +66,19 @@ void                  nm_act_request_add_share_rule (NMActRequest *req,
 /* Secrets handling */
 
 typedef void (*NMActRequestSecretsFunc) (NMActRequest *req,
-                                         guint32 call_id,
+                                         NMActRequestGetSecretsCallId call_id,
                                          NMSettingsConnection *connection,
                                          GError *error,
                                          gpointer user_data);
 
-guint32 nm_act_request_get_secrets (NMActRequest *req,
-                                    const char *setting_name,
-                                    NMSecretAgentGetSecretsFlags flags,
-                                    const char *hint,
-                                    NMActRequestSecretsFunc callback,
-                                    gpointer callback_data);
+NMActRequestGetSecretsCallId nm_act_request_get_secrets (NMActRequest *req,
+                                                         const char *setting_name,
+                                                         NMSecretAgentGetSecretsFlags flags,
+                                                         const char *hint,
+                                                         NMActRequestSecretsFunc callback,
+                                                         gpointer callback_data);
 
-void nm_act_request_cancel_secrets (NMActRequest *req, guint32 call_id);
+void nm_act_request_cancel_secrets (NMActRequest *req, NMActRequestGetSecretsCallId call_id);
 
 #endif /* __NETWORKMANAGER_ACTIVATION_REQUEST_H__ */
 
diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c
index 6bc02031ad..cb097c1b59 100644
--- a/src/nm-active-connection.c
+++ b/src/nm-active-connection.c
@@ -236,7 +236,9 @@ nm_active_connection_set_connection (NMActiveConnection *self,
 {
 	NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self);
 
-	/* Can't change connection after the ActiveConnection is exported over D-Bus */
+	/* Can't change connection after the ActiveConnection is exported over D-Bus.
+	 * Also, if we would do that, we would have to cancel all pending secret requests
+	 * from NMActRequest. */
 	g_return_if_fail (!nm_exported_object_is_exported (NM_EXPORTED_OBJECT (self)));
 	g_return_if_fail (priv->connection == NULL || !NM_IS_SETTINGS_CONNECTION (priv->connection));
 
diff --git a/src/ppp-manager/nm-ppp-manager.c b/src/ppp-manager/nm-ppp-manager.c
index dde9e2a163..344f57a71b 100644
--- a/src/ppp-manager/nm-ppp-manager.c
+++ b/src/ppp-manager/nm-ppp-manager.c
@@ -78,7 +78,7 @@ typedef struct {
 
 	NMActRequest *act_req;
 	DBusGMethodInvocation *pending_secrets_context;
-	guint32 secrets_id;
+	NMActRequestGetSecretsCallId secrets_id;
 	const char *secrets_setting_name;
 
 	guint32 ppp_watch_id;
@@ -244,7 +244,7 @@ static void
 remove_timeout_handler (NMPPPManager *manager)
 {
 	NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager);
-	
+
 	if (priv->ppp_timeout_handler) {
 		g_source_remove (priv->ppp_timeout_handler);
 		priv->ppp_timeout_handler = 0;
@@ -256,11 +256,10 @@ cancel_get_secrets (NMPPPManager *self)
 {
 	NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self);
 
-	if (priv->secrets_id) {
+	if (priv->secrets_id)
 		nm_act_request_cancel_secrets (priv->act_req, priv->secrets_id);
-		priv->secrets_id = 0;
-	}
-	priv->secrets_setting_name = NULL;
+
+	g_return_if_fail (!priv->secrets_id && !priv->secrets_setting_name);
 }
 
 static gboolean
@@ -324,7 +323,7 @@ extract_details_from_connection (NMConnection *connection,
 
 static void
 ppp_secrets_cb (NMActRequest *req,
-                guint32 call_id,
+                NMActRequestGetSecretsCallId call_id,
                 NMSettingsConnection *connection,
                 GError *error,
                 gpointer user_data)
@@ -339,6 +338,9 @@ ppp_secrets_cb (NMActRequest *req,
 	g_return_if_fail (req == priv->act_req);
 	g_return_if_fail (call_id == priv->secrets_id);
 
+	if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+		goto out;
+
 	if (error) {
 		nm_log_warn (LOGD_PPP, "%s", error->message);
 		dbus_g_method_return_error (priv->pending_secrets_context, error);
@@ -362,7 +364,7 @@ ppp_secrets_cb (NMActRequest *req,
 
  out:
 	priv->pending_secrets_context = NULL;
-	priv->secrets_id = 0;
+	priv->secrets_id = NULL;
 	priv->secrets_setting_name = NULL;
 }