GNOME Bugzilla – Bug 703276
[review] dcbw/vpn-need-secrets: allow VPN plugins to request secrets during the connect process
Last modified: 2013-07-31 13:16:13 UTC
To allow plugins to request new secrets when the existing ones aren't valid, add some new API to let plugins signal to NetworkManager that specific new secrets are requested, and to pass those hints through to the secret agents. NetworkManager then returns those secrets back to the plugin through a new D-Bus method call.
> vpn: consolidate connect handling code >+ g_set_error (error, NM_VPN_PLUGIN_ERROR, NM_VPN_PLUGIN_ERROR_BAD_ARGUMENTS, >+ "Invalid connection: '%s' / '%s' invalid: %d", >+ g_type_name (nm_connection_lookup_setting_type_by_quark (local->domain)), >+ local->message, local->code); The message here is clearly assuming that error->message contains just a property name, but that's not true any more. You probably want just ("Invalid connection: %s", local->message). (Bug in the existing code, but if you're changing it anyway...) >+ /* Stop the plugin from and idle handler so that the Connect typo "and" > libnm-glib-vpn: add asynchronous NeedSecrets signal and NewSecrets reply This means NMVpnPlugin now has both a daemon->client NeedSecrets method, and a client->daemon NeedSecrets signal. A little confusing? impl_vpn_plugin_new_secrets() has the same error message issue and typo as in the previous commit >+ if (!NM_VPN_PLUGIN_GET_CLASS (plugin)->new_secrets) { >+ g_set_error_literal (error, NM_VPN_PLUGIN_ERROR, NM_VPN_PLUGIN_ERROR_WRONG_STATE, should be ERROR_GENERAL, not ERROR_WRONG_STATE? >+ * @message: an information message about why secrets are requried, if any "required" > vpn: handle asynchronous secrets requests + dbus_g_proxy_begin_call (priv->proxy, "NewSecrets", + plugin_new_secrets_cb, self, NULL, indentation (likewise with the NeedSecrets call)
> libnm-glib-vpn: add asynchronous NeedSecrets signal and NewSecrets reply impl_vpn_plugin_new_secrets(): + if (!NM_VPN_PLUGIN_GET_CLASS (plugin)->new_secrets) { + g_set_error_literal (error, NM_VPN_PLUGIN_ERROR, NM_VPN_PLUGIN_ERROR_WRONG_STATE, + "Could not accept new secrets: plugin cannot process asynchronous secrets"); + return FALSE; + } should unref 'connection' before 'return FALSE' * add Since tag to nm_vpn_plugin_need_secrets() * also, gtk-doc comments are missing for older functions > vpn: handle asynchronous secrets requests ^Idbus_g_proxy_connect_signal (priv->proxy, "Config",$ ^I^I G_CALLBACK (nm_vpn_connection_config_get),$ ^I^I connection, NULL);$ indentation - two <tab>s instead of one
In my opinion a drop off is not strictly a programmer error but an external condition. Are the g_return_if_fail() just last resort checks or do you expect them to actually occur? In my opinion an attempt to talk to a disconnected agent is either a programming error that should be fixed (if the caller knows that in time) or it's internally a valid condition that deserves at least one real error message but no [soft] assertions afterwards. No other comments.
(In reply to comment #3) > In my opinion a drop off is not strictly a programmer error but an external > condition. Are the g_return_if_fail() just last resort checks or do you expect > them to actually occur? In my opinion an attempt to talk to a disconnected Last resort checks to ensure we don't try to use a NULL proxy, and to indicate to us as the developers that there's a bug in the code instead of getting glib errors that don't point to the actual problem.
Renamed the NeedSecrets signal to SecretsRequested. jklimes: "also, gtk-doc comments are missing for older functions"; would you like me to add them for some of the functions? Or can that be done later? Branch fixed up and re-pushed for review comments.
> libnm-glib-vpn: add asynchronous SecretsRequired signal and NewSecrets reply the NewSecrets introspection docs still refer to "NeedSecrets" rather than "SecretsRequired". > vpn: handle asynchronous secrets requests > dbus_g_proxy_connect_signal (priv->proxy, "Config", >- G_CALLBACK (nm_vpn_connection_config_get), >- connection, NULL); >+ G_CALLBACK (nm_vpn_connection_config_get), >+ connection, NULL); this got reindented but is still wrong...
Fixed and re-pushed.
Additional fixes and repushed; I added the ConnectAsyncAllowed method which determines whether or not the plugin can call SEcretsRequired. The core problem is that if a secret agent cannot pass hints to the VPN plugin (like anything except nm-applet for now) then the auth dialog won't be able to respond correctly to the SecretsRequired signal, becasue that's how the plugin tells its auth dialog what's required. So if any agent doesn't support VPN hints, we can't use the new SecretsRequired signal. This re-push also adds agent capabilities, which we could use later for stuff like hotspot detection and device secrets.
(In reply to comment #5) > Renamed the NeedSecrets signal to SecretsRequested. > > jklimes: "also, gtk-doc comments are missing for older functions"; would you > like me to add them for some of the functions? Or can that be done later? I think this can be done later. > Branch fixed up and re-pushed for review comments. I'm fine with the branch.
> libnm-glib: add support for agent capabilities during registration >+ g_param_spec_uint (NM_SECRET_AGENT_CAPABILITIES, enums/flags types should be registered as g_param_spec_enum()/flags(). We weren't doing that in the past because we weren't registering all of our enum types, and we can't change those old properties now for backward-compatibility reasons, but we ought to at least get new properties right. (Likewise g_value_get/set_flags(), not uint(), int get/set_property.) In reg_with_caps_cb(), you don't need the GError since you never look at it. > libnm-glib-vpn: add support for asynchronous secrets requests "ConnectAsyncAllowed" sounds like it returns a boolean telling you whether or not you can ConnectAsync. "ConnectWithAsyncSecrets"? Or maybe "ConnectInteractive"? + /* Cancel the connect timer since secrets might take a while. It'll + * get restarted when the secrets come back via Connect(). + */ "via NewSecrets()" ?
(In reply to comment #10) > > libnm-glib: add support for agent capabilities during registration > > >+ g_param_spec_uint (NM_SECRET_AGENT_CAPABILITIES, > > enums/flags types should be registered as g_param_spec_enum()/flags(). We > weren't doing that in the past because we weren't registering all of our enum > types, and we can't change those old properties now for backward-compatibility > reasons, but we ought to at least get new properties right. > > (Likewise g_value_get/set_flags(), not uint(), int get/set_property.) Fixed. Can you double-check my glib fu? > In reg_with_caps_cb(), you don't need the GError since you never look at it. Fixed. > > > > libnm-glib-vpn: add support for asynchronous secrets requests > > "ConnectAsyncAllowed" sounds like it returns a boolean telling you whether or > not you can ConnectAsync. "ConnectWithAsyncSecrets"? Or maybe > "ConnectInteractive"? Renamed to ConnectInteractive() everywhere in core + libnm-glib. > + /* Cancel the connect timer since secrets might take a while. It'll > + * get restarted when the secrets come back via Connect(). > + */ > > "via NewSecrets()" ? Fixed.
looks good
Pushed to master.