colin/NetworkManager
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

device: implement "auth-request" as async operation nm_manager_device_auth_request()

Browse Source 
GObject signals only complicate the code and are less efficient.

Also, NM_DEVICE_AUTH_REQUEST signal really invoked an asynchronous
request. Of course, fundamentally emitting a signal *is* the same as
calling a method. However, implementing this as signal is really not
nice nor best practice. For one, there is a (negligible) overhead emitting
a GObject signal. But what is worse, GObject signals are not as strongly
typed and make it harder to understand what happens.

The signal had the appearance of providing some special decoupling of
NMDevice and NMManager. Of course, in practice, they were not more
decoupled (both forms are the same in nature), but it was harder to
understand how they work together.

Add and call a method nm_manager_device_auth_request() instead. This
has the notion of invoking an asynchronous method. Also, never invoke
the callback synchronously and provide a cancellable. Like every asynchronous
operation, it *must* be cancellable, and callers should make sure to
provide a mechanism to abort.
This commit is contained in:
Thomas Haller
2020-04-26 13:59:13 +02:00
parent d935692bc7
commit b50702775f
8 changed files with 183 additions and 105 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
src/devices/nm-device-private.h
View File

@@ -205,4 +205,15 @@ gboolean nm_device_match_parent_hwaddr (NMDevice *device,
NMConnection *connection,
gboolean fail_if_no_hwaddr);
/*****************************************************************************/
void nm_device_auth_request (NMDevice *self,
GDBusMethodInvocation *context,
NMConnection *connection,
const char *permission,
gboolean allow_interaction,
GCancellable *cancellable,
NMManagerDeviceAuthRequestFunc callback,
gpointer user_data);
#endif /* NM_DEVICE_PRIVATE_H */

105
src/devices/nm-device.c
View File

@@ -192,7 +192,6 @@ typedef struct {
enum {
STATE_CHANGED,
AUTOCONNECT_ALLOWED,
AUTH_REQUEST,
IP4_CONFIG_CHANGED,
IP6_CONFIG_CHANGED,
IP6_PREFIX_DELEGATED,
@@ -6265,6 +6264,27 @@ dnsmasq_state_changed_cb (NMDnsMasqManager *manager, guint32 status, gpointer us
}
}
void
nm_device_auth_request (NMDevice *self,
GDBusMethodInvocation *context,
NMConnection *connection,
const char *permission,
gboolean allow_interaction,
GCancellable *cancellable,
NMManagerDeviceAuthRequestFunc callback,
gpointer user_data)
{
nm_manager_device_auth_request (nm_device_get_manager (self),
self,
context,
connection,
permission,
allow_interaction,
cancellable,
callback,
user_data);
}
/*****************************************************************************/
static void
@@ -12303,13 +12323,14 @@ impl_device_reapply (NMDBusObject *obj,
} else
reapply_data = NULL;
g_signal_emit (self, signals[AUTH_REQUEST], 0,
invocation,
nm_device_get_applied_connection (self),
NM_AUTH_PERMISSION_NETWORK_CONTROL,
TRUE,
reapply_cb,
reapply_data);
nm_device_auth_request (self,
invocation,
nm_device_get_applied_connection (self),
NM_AUTH_PERMISSION_NETWORK_CONTROL,
TRUE,
NULL,
reapply_cb,
reapply_data);
}
/*****************************************************************************/
@@ -12346,13 +12367,14 @@ get_applied_connection_cb (NMDevice *self,
if (applied_connection != user_data) {
/* The applied connection changed due to a race. Reauthenticate. */
g_signal_emit (self, signals[AUTH_REQUEST], 0,
context,
applied_connection,
NM_AUTH_PERMISSION_NETWORK_CONTROL,
TRUE,
get_applied_connection_cb,
applied_connection /* no need take a ref. We will not dereference this pointer. */);
nm_device_auth_request (self,
context,
applied_connection,
NM_AUTH_PERMISSION_NETWORK_CONTROL,
TRUE,
NULL,
get_applied_connection_cb,
applied_connection /* no need take a ref. We will not dereference this pointer. */);
return;
}
@@ -12399,13 +12421,14 @@ impl_device_get_applied_connection (NMDBusObject *obj,
return;
}
g_signal_emit (self, signals[AUTH_REQUEST], 0,
invocation,
applied_connection,
NM_AUTH_PERMISSION_NETWORK_CONTROL,
TRUE,
get_applied_connection_cb,
applied_connection /* no need take a ref. We will not dereference this pointer. */);
nm_device_auth_request (self,
invocation,
applied_connection,
NM_AUTH_PERMISSION_NETWORK_CONTROL,
TRUE,
NULL,
get_applied_connection_cb,
applied_connection /* no need take a ref. We will not dereference this pointer. */);
}
/*****************************************************************************/
@@ -12573,13 +12596,14 @@ impl_device_disconnect (NMDBusObject *obj,
connection = nm_device_get_applied_connection (self);
nm_assert (connection);
g_signal_emit (self, signals[AUTH_REQUEST], 0,
invocation,
connection,
NM_AUTH_PERMISSION_NETWORK_CONTROL,
TRUE,
disconnect_cb,
NULL);
nm_device_auth_request (self,
invocation,
connection,
NM_AUTH_PERMISSION_NETWORK_CONTROL,
TRUE,
NULL,
disconnect_cb,
NULL);
}
static void
@@ -12625,13 +12649,14 @@ impl_device_delete (NMDBusObject *obj,
return;
}
g_signal_emit (self, signals[AUTH_REQUEST], 0,
invocation,
NULL,
NM_AUTH_PERMISSION_NETWORK_CONTROL,
TRUE,
delete_cb,
NULL);
nm_device_auth_request (self,
invocation,
NULL,
NM_AUTH_PERMISSION_NETWORK_CONTROL,
TRUE,
NULL,
delete_cb,
NULL);
}
static void
@@ -18014,14 +18039,6 @@ nm_device_class_init (NMDeviceClass *klass)
autoconnect_allowed_accumulator, NULL, NULL,
G_TYPE_BOOLEAN, 0);
signals[AUTH_REQUEST] =
g_signal_new (NM_DEVICE_AUTH_REQUEST,
G_OBJECT_CLASS_TYPE (object_class),
G_SIGNAL_RUN_FIRST,
0, NULL, NULL, NULL,
/* context, connection, permission, allow_interaction, callback, user_data */
G_TYPE_NONE, 6, G_TYPE_DBUS_METHOD_INVOCATION, NM_TYPE_CONNECTION, G_TYPE_STRING, G_TYPE_BOOLEAN, G_TYPE_POINTER, G_TYPE_POINTER);
signals[IP4_CONFIG_CHANGED] =
g_signal_new (NM_DEVICE_IP4_CONFIG_CHANGED,
G_OBJECT_CLASS_TYPE (object_class),

7
src/devices/nm-device.h
View File

@@ -115,7 +115,6 @@ nm_device_state_reason_check (NMDeviceStateReason reason)
#define NM_DEVICE_HAS_PENDING_ACTION "has-pending-action" /* Internal only */
/* Internal signals */
#define NM_DEVICE_AUTH_REQUEST "auth-request"
#define NM_DEVICE_IP4_CONFIG_CHANGED "ip4-config-changed"
#define NM_DEVICE_IP6_CONFIG_CHANGED "ip6-config-changed"
#define NM_DEVICE_IP6_PREFIX_DELEGATED "ip6-prefix-delegated"
@@ -455,12 +454,6 @@ typedef struct _NMDeviceClass {
} NMDeviceClass;
typedef void (*NMDeviceAuthRequestFunc) (NMDevice *device,
GDBusMethodInvocation *context,
NMAuthSubject *subject,
GError *error,
gpointer user_data);
GType nm_device_get_type (void);
struct _NMDedupMultiIndex *nm_device_get_multi_index (NMDevice *self);

16
src/devices/wifi/nm-device-iwd.c
View File

@@ -1061,14 +1061,14 @@ _nm_device_iwd_request_scan (NMDeviceIwd *self,
return;
}
g_signal_emit_by_name (device,
NM_DEVICE_AUTH_REQUEST,
invocation,
NULL,
NM_AUTH_PERMISSION_WIFI_SCAN,
TRUE,
dbus_request_scan_cb,
options ? g_variant_ref (options) : NULL);
nm_device_auth_request (device,
invocation,
NULL,
NM_AUTH_PERMISSION_WIFI_SCAN,
TRUE,
NULL,
dbus_request_scan_cb,
nm_g_variant_ref (options));
}
static gboolean

16
src/devices/wifi/nm-device-wifi.c
View File

@@ -1323,14 +1323,14 @@ _nm_device_wifi_request_scan (NMDeviceWifi *self,
return;
}
g_signal_emit_by_name (device,
NM_DEVICE_AUTH_REQUEST,
invocation,
NULL,
NM_AUTH_PERMISSION_WIFI_SCAN,
TRUE,
dbus_request_scan_cb,
g_steal_pointer (&ssids));
nm_device_auth_request (device,
invocation,
NULL,
NM_AUTH_PERMISSION_WIFI_SCAN,
TRUE,
NULL,
dbus_request_scan_cb,
g_steal_pointer (&ssids));
}
static gboolean

115
src/nm-manager.c
View File

@@ -2375,8 +2375,9 @@ device_auth_done_cb (NMAuthChain *chain,
gs_free_error GError *error = NULL;
NMAuthCallResult result;
NMDevice *device;
GCancellable *cancellable;
const char *permission;
NMDeviceAuthRequestFunc callback;
NMManagerDeviceAuthRequestFunc callback;
NMAuthSubject *subject;
nm_assert (G_IS_DBUS_METHOD_INVOCATION (context));
@@ -2390,18 +2391,26 @@ device_auth_done_cb (NMAuthChain *chain,
device = nm_auth_chain_get_data (chain, "device");
nm_assert (NM_IS_DEVICE (device));
cancellable = nm_auth_chain_get_cancellable (chain);
nm_assert (!cancellable || G_IS_CANCELLABLE (cancellable));
result = nm_auth_chain_get_result (chain, permission);
subject = nm_auth_chain_get_subject (chain);
if (result != NM_AUTH_CALL_RESULT_YES) {
_LOGD (LOGD_CORE, "%s request failed: not authorized", permission);
error = g_error_new (NM_MANAGER_ERROR,
NM_MANAGER_ERROR_PERMISSION_DENIED,
"%s request failed: not authorized",
permission);
}
if ( cancellable
&& g_cancellable_set_error_if_cancelled (cancellable, &error)) {
/* pass. */
} else {
if (result != NM_AUTH_CALL_RESULT_YES) {
_LOGD (LOGD_CORE, "%s request failed: not authorized", permission);
error = g_error_new (NM_MANAGER_ERROR,
NM_MANAGER_ERROR_PERMISSION_DENIED,
"%s request failed: not authorized",
permission);
}
nm_assert (error || (result == NM_AUTH_CALL_RESULT_YES));
nm_assert (error || (result == NM_AUTH_CALL_RESULT_YES));
}
callback (device,
context,
@@ -2411,28 +2420,53 @@ device_auth_done_cb (NMAuthChain *chain,
}
static void
device_auth_request_cb (NMDevice *device,
GDBusMethodInvocation *context,
NMConnection *connection,
const char *permission,
gboolean allow_interaction,
NMDeviceAuthRequestFunc callback,
gpointer user_data,
NMManager *self)
_device_auth_done_fail_on_idle (gpointer user_data, GCancellable *cancellable)
{
gs_unref_object NMManager *self = NULL;
gs_unref_object NMDevice *device = NULL;
gs_unref_object GDBusMethodInvocation *context = NULL;
gs_unref_object NMAuthSubject *subject = NULL;
gs_free_error GError *error_original = NULL;
gs_free_error GError *error_cancelled = NULL;
NMManagerDeviceAuthRequestFunc callback;
gpointer callback_user_data;
nm_utils_user_data_unpack (&self, &device, &context, &subject, &error_original, &callback, &callback_user_data);
g_cancellable_set_error_if_cancelled (cancellable, &error_cancelled);
callback (device,
context,
subject,
error_cancelled ?: error_original,
callback_user_data);
}
void
nm_manager_device_auth_request (NMManager *self,
NMDevice *device,
GDBusMethodInvocation *context,
NMConnection *connection,
const char *permission,
gboolean allow_interaction,
GCancellable *cancellable,
NMManagerDeviceAuthRequestFunc callback,
gpointer user_data)
{
NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self);
GError *error = NULL;
NMAuthSubject *subject = NULL;
gs_free_error GError *error = NULL;
gs_unref_object NMAuthSubject *subject = NULL;
NMAuthChain *chain;
char *permission_dup;
/* Validate the caller */
subject = nm_dbus_manager_new_auth_subject_from_context (context);
if (!subject) {
error = g_error_new_literal (NM_MANAGER_ERROR,
NM_MANAGER_ERROR_PERMISSION_DENIED,
NM_UTILS_ERROR_MSG_REQ_UID_UKNOWN);
goto done;
g_set_error_literal (&error,
NM_MANAGER_ERROR,
NM_MANAGER_ERROR_PERMISSION_DENIED,
NM_UTILS_ERROR_MSG_REQ_UID_UKNOWN);
goto fail_on_idle;
}
/* Ensure the subject has permissions for this connection */
@@ -2442,17 +2476,21 @@ device_auth_request_cb (NMDevice *device,
NM_MANAGER_ERROR,
NM_MANAGER_ERROR_PERMISSION_DENIED,
&error))
goto done;
goto fail_on_idle;
/* Validate the request */
chain = nm_auth_chain_new_subject (subject, context, device_auth_done_cb, self);
if (!chain) {
error = g_error_new_literal (NM_MANAGER_ERROR,
NM_MANAGER_ERROR_PERMISSION_DENIED,
NM_UTILS_ERROR_MSG_REQ_AUTH_FAILED);
goto done;
g_set_error (&error,
NM_MANAGER_ERROR,
NM_MANAGER_ERROR_PERMISSION_DENIED,
NM_UTILS_ERROR_MSG_REQ_AUTH_FAILED);
goto fail_on_idle;
}
if (cancellable)
nm_auth_chain_set_cancellable (chain, cancellable);
permission_dup = g_strdup (permission);
c_list_link_tail (&priv->auth_lst_head, nm_auth_chain_parent_lst_list (chain));
@@ -2461,13 +2499,18 @@ device_auth_request_cb (NMDevice *device,
nm_auth_chain_set_data (chain, "user-data", user_data, NULL);
nm_auth_chain_set_data (chain, "perm", permission_dup /* transfer ownership */, g_free);
nm_auth_chain_add_call_unsafe (chain, permission_dup, allow_interaction);
return;
done:
if (error)
callback (device, context, subject, error, user_data);
g_clear_object (&subject);
g_clear_error (&error);
fail_on_idle:
nm_utils_invoke_on_idle (cancellable,
_device_auth_done_fail_on_idle,
nm_utils_user_data_pack (g_object_ref (self),
g_object_ref (device),
g_object_ref (context),
g_steal_pointer (&subject),
g_steal_pointer (&error),
callback,
user_data));
}
static gboolean
@@ -3130,10 +3173,6 @@ add_device (NMManager *self, NMDevice *device, GError **error)
G_CALLBACK (manager_device_state_changed),
self);
g_signal_connect (device, NM_DEVICE_AUTH_REQUEST,
G_CALLBACK (device_auth_request_cb),
self);
g_signal_connect (device, NM_DEVICE_REMOVED,
G_CALLBACK (device_removed_cb),
self);

12
src/nm-manager.h
View File

@@ -193,4 +193,16 @@ NMMetered nm_manager_get_metered (NMManager *self);
void nm_manager_notify_device_availibility_maybe_changed (NMManager *self);
/*****************************************************************************/
void nm_manager_device_auth_request (NMManager *self,
NMDevice *device,
GDBusMethodInvocation *context,
NMConnection *connection,
const char *permission,
gboolean allow_interaction,
GCancellable *cancellable,
NMManagerDeviceAuthRequestFunc callback,
gpointer user_data);
#endif /* __NETWORKMANAGER_MANAGER_H__ */

6
src/nm-types.h
View File

@@ -40,6 +40,12 @@ typedef struct _NMSleepMonitor NMSleepMonitor;
typedef struct _NMLldpListener NMLldpListener;
typedef struct _NMConfigDeviceStateData NMConfigDeviceStateData;
typedef void (*NMManagerDeviceAuthRequestFunc) (NMDevice *device,
GDBusMethodInvocation *context,
NMAuthSubject *subject,
GError *error,
gpointer user_data);
struct _NMDedupMultiIndex;
typedef struct _NMRefString NMRefString;