GNOME Bugzilla – Bug 694315
Add a way to request the creation of a new account
Last modified: 2013-04-23 16:33:38 UTC
I'm writing a browser extension[1] (targetting chrome/chromium) to request the creation of a GOA account as soon as a login to a recognized provider is detected. To have a smooth experience I'd like to gather as much info as possible form the browser session and hand it to the GOA provider, in particular the identity and the auth cookies to be able to skip the request of username and password. I have a (very rough) working branch[2] and I'd like to get some feedback on the general approach. I also had to patch GOA itself[3] to make it use the credentials gathered from the browser session. The patch adds a "online-accounts new" sub-sub-command, which receives on stdin the additional info from the user session, encoded in a JSON object with a format defined by the GOA provider (other than a few common fields, it will likely be provider-specific). The inspiration for this comes from the Ubuntu's webaccounts extension[4], which requires Unity and UOA. See bug #694313 for the GOA patch. [1] GOA Browser Extension http://cgit.collabora.com/git/user/em/goa-browser-extension/ [2] GNOME Control Center with "online-account new" sub-sub-command http://cgit.collabora.com/git/user/em/gnome-control-center/log/?h=wip/goa-new-account [3] GOA with goa_provider_add_account_from_json() http://cgit.collabora.com/git/user/em/gnome-online-accounts/log/?h=wip/add-account-json [4] Ubuntu Online Accounts Browser Extensions https://launchpad.net/webaccounts-browser-extension
Created attachment 236987 [details] [review] online-accounts: Add the 'new' sub-sub-command to create accounts By running `gnome-control-center online-accounts new' it is now possible request the creation of a new account by passing the needed info on stdin in JSON format. For instance, the following command would create a new google account: echo { "provider":"google", \ "identity":"example@gmail.com", \ "password":"S3cR3t" } \ | gnome-control-center online-accounts new "provider" and "identity" are the only mandatory keys while all the other keys are provider-dependent and documented in GOA (see the documentation for the goa_provider_add_account_from_json() function).
Review of attachment 236987 [details] [review]: Passing the JSON data from stdin just will not work if gnome-control-center is already running. Another option would be to setup a D-Bus object with a single method in the browser plugin (filter so that only g-c-c can call it) and pass the object path as an argv. Or stuff the JSON data in a well-known format in the keyring on the browser side, and pass the identifier to g-c-c. If the request is cancelled, remove the data you've just added.
I've not found any way to set an expiration for keyring items and I fear that the first solution may leave cruft around in case the plugin/browser crashes, etc., so I'd try to go the dbus route. I'm thinking about exporting objects under something like /com/collabora/goa-browser-extension/42, each implementing the org.gnome.ControlCenter.OnlineAccountRequestNew interface with the GetProviderData() method returning a (ss{sv}) tuple (provider,identity,parameters_dict). Does it sound sensible? Also, given that I'll have to deal with gvariant, does it make sense to get rid of JSON altogether and instead of goa_provider_add_account_from_json() add a goa_provider_add_account_from_data(provider, identity, gvariant) method to GOA?
(In reply to comment #3) > I've not found any way to set an expiration for keyring items and I fear that > the first solution may leave cruft around in case the plugin/browser crashes, > etc., so I'd try to go the dbus route. Fair enough. > I'm thinking about exporting objects under something like > /com/collabora/goa-browser-extension/42, each implementing the > org.gnome.ControlCenter.OnlineAccountRequestNew interface with the > GetProviderData() method returning a (ss{sv}) tuple > (provider,identity,parameters_dict). Does it sound sensible? That sounds good. Might want to check with Simon or one of your co-workers that's more knowledgeable than me about D-Bus to see if this is sensible, security-wise. > Also, given that I'll have to deal with gvariant, does it make sense to get rid > of JSON altogether and instead of goa_provider_add_account_from_json() add a > goa_provider_add_account_from_data(provider, identity, gvariant) method to GOA? Unless the JSON API could be used for some other purpose, that's what I would do indeed.
(In reply to comment #4) > > I'm thinking about exporting objects under something like > > /com/collabora/goa-browser-extension/42, each implementing the > > org.gnome.ControlCenter.OnlineAccountRequestNew interface with the > > GetProviderData() method returning a (ss{sv}) tuple > > (provider,identity,parameters_dict). Does it sound sensible? > > That sounds good. Might want to check with Simon or one of your co-workers > that's more knowledgeable than me about D-Bus to see if this is sensible, > security-wise. What's in this tuple? How much of it is "secret" (approximately: something you'd like to put in gnome-keyring)? With my security hat on, the session bus is not considered a privilege boundary. Telepathy has historically passed passwords across it in clear-text, and that's not considered a vulnerability: anything that can eavesdrop on messages could equally well ptrace the dbus-daemon (or Chromium or GOA for that matter). OS vendors that want the session bus to be a privilege boundary (e.g. MeeGo Harmattan) have to lock it down a *lot*, making it look more like the system bus, with a default-deny policy and certainly no eavesdropping. With my D-Bus API design hat on, I'm surprised this isn't the other way round. If the Control Center was an activatable session service, it could have a CreateOnlineAccount method, and then it wouldn't need any messing about with command-line parameters or calling back into its client for more information? If the CC exported a method, there would also be an easy way to prevent it from being read by other processes on a more-locked-down bus: prevent tracing the dbus-daemon, and prevent eavesdropping. No need for elaborate filters.
(In reply to comment #5) > If the Control Center was an activatable session service, it could have a > CreateOnlineAccount method It doesn't necessarily *need* to be activatable - the client (the Chromium plugin in this case) could run gnome-control-center, wait for it to take the org.gnome.ControlCenter name, then do its stuff - but that's basically reinventing D-Bus service activation, so there seems little point.
> What's in this tuple? How much of it is "secret" (approximately: something > you'd like to put in gnome-keyring)? Something like: ( provider_name (eg. "google"), identity (eg. "example@gmail.com"), { "cookies": [ { "domain": "google.com", "name": "LSID", "value": "asdfasdfasdf" }, { "domain": "accounts.google.com", "name": "SSID", "value": "asdfasdfasdf" }, ... ] ) Other providers may also have additional info (eg. the "url" for owncloud). I'm not sure if passing cookies this way is too much JSON-like and there are smarter ways to do it. > With my security hat on, the session bus is not considered a privilege > boundary. Telepathy has historically passed passwords across it in clear-text, > and that's not considered a vulnerability: anything that can eavesdrop on > messages could equally well ptrace the dbus-daemon (or Chromium or GOA for that > matter). OS vendors that want the session bus to be a privilege boundary (e.g. > MeeGo Harmattan) have to lock it down a *lot*, making it look more like the > system bus, with a default-deny policy and certainly no eavesdropping. Good. > With my D-Bus API design hat on, I'm surprised this isn't the other way round. > If the Control Center was an activatable session service, it could have a > CreateOnlineAccount method, and then it wouldn't need any messing about with > command-line parameters or calling back into its client for more information? > > If the CC exported a method, there would also be an easy way to prevent it from > being read by other processes on a more-locked-down bus: prevent tracing the > dbus-daemon, and prevent eavesdropping. No need for elaborate filters. CC maintainers, what about exporting an activatable session service? ^ Since in both cases we end up using dbus, this seems a bit cleaner since it will require no new command line parameters and also straightforward.
I asked Ryan about using the GApplication API to pass the arguments and here's the gist of what he mentioned: - Add a GAction to g-c-c that would handle adding a new account - Register g-c-c as a service: - Use G_APPLICATION_IS_SERVICE when registering g-c-c - Create a D-Bus activation file for g-c-c - Probably fix bugs in g-c-c ;) - Write a small launcher program to call that action, for example: g_application_new("your.app.id", G_APPLICATION_IS_LAUNCHER); g_application_register(app); g_action_group_activate_action(app, "action-name", args); - Call the small app from the browser plugin (and you can pass the data through the command-line for that one)
If I add to control-center.c the "online-account-new" action, which takes a GVariant with the aforementioned signature, and a DBus activation file I suppose I should be able to invoke CC without the launcher program by calling the org.gtk.Actions.Activate() DBus method, right? I'd rather avoid stuffing all this data on the command line (the auth cookies can be quite large).
(In reply to comment #9) > I'd rather avoid stuffing all this data on the command line (the auth cookies > can be quite large). I think Bastien's intention was that the launcher program would receive the data on stdin. Note that on most platforms (including Linux), command-line arguments are visible to all users, so they are specifically not suitable for secrets.
(In reply to comment #9) > If I add to control-center.c the "online-account-new" action, which takes a > GVariant with the aforementioned signature, and a DBus activation file I > suppose I should be able to invoke CC without the launcher program by calling > the org.gtk.Actions.Activate() DBus method, right? The D-Bus API for GApplication is private.
Created attachment 237332 [details] [review] online-accounts: Add the 'online-account-new' app action It is now possible to use the GApplication/DBus-based activation and the 'online-account-new' action to request the creation of a new online account. The action accepts an array of variants to store the details for the account to be created: provider, identity and provider specific options (eg. cookies). See the documentation for the goa_provider_add_account_with_data() function for further details about the accepted data.
Created attachment 237334 [details] Control Center launcher for the online-accounts-new action This is the sample program that reads the JSON-encoded data and send it to the "online-accounts-new" action of GNOME Control Center through DBus.
Wih the last patch I'm now able to invoke the Control Center with the sample program attached, passing arbitrary parameters through the array-of-GVariant parameter. I've started to replace the "argv" property in CcPanel with the new GVariant-based "parameters" to convey both command-line parameters and the parameter given to DBus-activation. Is this acceptable or it's better to leave "argv" where it was? Please note that in the patch I've migrated only the online-accounts panel: if deemed useful the plan would be to port the other panels and split the patch from the "online-account-new" stuff.
Review of attachment 237332 [details] [review]: This should be split in more patches. First: - argv -> parameters change (including fixing all the existing panels, right? :) - turn g-c-c into a d-bus service - goa panel specific changes ::: configure.ac @@ +148,3 @@ + goa-1.0 + goa-backend-1.0 >= $GOA_REQUIRED_VERSION + json-glib-1.0) I couldn't find something that required json-glib. Should that be in goa-backend's dependencies instead? ::: shell/cc-application.c @@ +296,3 @@ + /* Request a new online account. The parameter is an array of values containing + * (provider_name, identity, dict_of_provider_specific_parameters) */ + action = g_simple_action_new ("online-accounts-new", G_VARIANT_TYPE ("av")); The creation and handling of those new actions should be separated in its own functions.
(In reply to comment #14) > Wih the last patch I'm now able to invoke the Control Center with the sample > program attached, passing arbitrary parameters through the array-of-GVariant > parameter. > > I've started to replace the "argv" property in CcPanel with the new > GVariant-based "parameters" to convey both command-line parameters and the > parameter given to DBus-activation. Is this acceptable or it's better to leave > "argv" where it was? Please note that in the patch I've migrated only the > online-accounts panel: if deemed useful the plan would be to port the other > panels and split the patch from the "online-account-new" stuff. Replacing argv is probably best. otherwise we'll likely have problems with conflicting features (available half through argv, half through parameters).
(In reply to comment #15) > This should be split in more patches. First: > - argv -> parameters change (including fixing all the existing panels, right? > :) > - turn g-c-c into a d-bus service > - goa panel specific changes Yup, sure, the patch is *very* rough, it was just to make sure I'm on the right path. :) > ::: configure.ac > @@ +148,3 @@ > + goa-1.0 > + goa-backend-1.0 >= $GOA_REQUIRED_VERSION > + json-glib-1.0) > > I couldn't find something that required json-glib. Should that be in > goa-backend's dependencies instead? Oops, it' a leftover from the previous patch. The only place that uses JSON is the cmdline util, goa and cc use only GVariant. > ::: shell/cc-application.c > @@ +296,3 @@ > + /* Request a new online account. The parameter is an array of values > containing > + * (provider_name, identity, dict_of_provider_specific_parameters) */ > + action = g_simple_action_new ("online-accounts-new", G_VARIANT_TYPE ("av")); > > The creation and handling of those new actions should be separated in its own > functions. I'm not sure I understand what you mean: just moving the four lines about "online-accounts-new" in a ad-hoc function would be enough? Now that I think of it, a more general "launch-panel" action with a (sav) parameter (panel_id, parameters) would be preferable or I'm overengineering a bit too much?
(In reply to comment #17) > > ::: shell/cc-application.c > > @@ +296,3 @@ > > + /* Request a new online account. The parameter is an array of values > > containing > > + * (provider_name, identity, dict_of_provider_specific_parameters) */ > > + action = g_simple_action_new ("online-accounts-new", G_VARIANT_TYPE ("av")); > > > > The creation and handling of those new actions should be separated in its own > > functions. > > I'm not sure I understand what you mean: just moving the four lines about > "online-accounts-new" in a ad-hoc function would be enough? I just don't want to see panel specific code in the shell itself. > Now that I think of it, a more general "launch-panel" action with a (sav) > parameter (panel_id, parameters) would be preferable or I'm overengineering a > bit too much? That would solve the problem nicely.
Created attachment 237700 [details] [review] shell: Use GVariant to convey panel arguments instead of a string array By using a GVariant of type "av" we can potentially pass more structured data to panels, which will become relevant with the ability to invoke them by GAction-based DBus-activation introduced in the following patch.
Created attachment 237701 [details] [review] shell: Expose panel launching with DBus-activation Turn Control Center in a DBus-activable service and export a 'launch-panel' GAction which accepts a tuple containing the id of the desired panel and its parameters as a GVariant array. The snippet below show how the custom shortcuts section of the keyboard panel can be invoked by a external programs through DBus: GVariantBuilder *b = g_variant_builder_new (G_VARIANT_TYPE ("av")); g_variant_builder_add (b, "v", g_variant_new_string ("shortcuts")); g_variant_builder_add (b, "v", g_variant_new_string ("custom")); GVariant *params = g_variant_new ("(s@av)", "keyboard, g_variant_builder_end (b)); GApplication *gnomecc = g_application_new ("org.gnome.ControlCenter", G_APPLICATION_IS_LAUNCHER); if (!g_application_register (gnomecc, NULL, &error)) g_error ("Failed to register launcher: %s", error->message); g_action_group_activate_action (G_ACTION_GROUP (gnomecc), "launch-panel", params);
Created attachment 237702 [details] [review] online-accounts: Accept requests for account creation on DBus/cmdline Also accepts additional data to pre-seed the GOA dialog, in particular the provider name (eg. 'google') and identity (eg. 'example@gmail.com'). Extra provider-specific data can be added as a GVariant dict ('a{sv}') to eg. pass cookies from an existing web session (this is only available from DBus activation). It can be run from the command line as: $ gnome-control-center online-account new google example@gmail.com
Review of attachment 237700 [details] [review]: ::: panels/keyboard/cc-keyboard-panel.c @@ +86,3 @@ + + parameters = g_value_get_variant (value); + if (parameters && g_variant_n_children (parameters) > 0) { switch (g_variant_n_children (parameters)) { case 1: etc. ::: panels/network/cc-network-panel.c @@ +164,3 @@ } +static const gchar** I doubt you're returning const gchar ** Rather gchar * const * (or whatever the C is for that, the container isn't const, the members are). @@ +170,3 @@ + GVariant *v; + const gchar **strv; + gsize i = 0, n; assign "i" later on. ::: panels/online-accounts/cc-online-accounts-panel.c @@ +103,3 @@ switch (property_id) { + case PROP_PARAMETERS: You can't leave the online-accounts panel half-broken before adding the support for creating new accounts in your other patch. ::: panels/sound/cc-sound-panel.c @@ +59,3 @@ + if (parameters && g_variant_n_children (parameters) > 0) { + GVariant *v; + g_variant_get_child (parameters, 0, "v", &v); Wouldn't something like g_variant_get (parameters, "(s)", &string); work? ::: shell/cc-application.c @@ +193,3 @@ + builder = g_variant_builder_new (G_VARIANT_TYPE ("av")); + for (i=1; start_panels[i] != NULL; i++) for (i = 1...) ::: shell/cc-panel.c @@ +212,3 @@ + pspec = g_param_spec_variant ("parameters", + "Structured parameters", + "Additional parameters passed from outside (ie. command line, dbus activation)", externally
Review of attachment 237701 [details] [review]: The snippet is missing at least a closing quote after "keyboard". Please make sure it works. I also wouldn't include error checking in the snippet. ::: shell/Makefile.am @@ +92,3 @@ +servicefile_DATA = $(servicefile_in_files:.service.in=.service) +$(servicefile_DATA): $(servicefile_in_files) Makefile + $(AM_V_GEN) sed -e 's|[@]bindir[@]|$(bindir)|' $< > $@ I don't think you need to escape the '@' as you've done, they're already inside a single-quote. ::: shell/cc-application.c @@ +378,3 @@ { + GApplicationFlags flags; + flags = G_APPLICATION_HANDLES_COMMAND_LINE|G_APPLICATION_IS_SERVICE; No need to use a separate variable for that.
Review of attachment 237702 [details] [review]: ::: panels/online-accounts/cc-online-accounts-add-account-dialog.c @@ +90,3 @@ g_list_free (children); + // This spins gtk_dialog_run() ? And don't use C++ style comments please. @@ +270,3 @@ + GVariant *extra) +{ + GoaPanelAddAccountDialogPrivate *priv = add_account->priv; line feed here. @@ +321,3 @@ + priv->collected_data.identity, priv->collected_data.extra); + else + gtk_dialog_run (GTK_DIALOG (add_account)); We really shouldn't use gtk_dialog_run(), it steals the mainloop and creates all sorts of problems. ::: panels/online-accounts/cc-online-accounts-panel.c @@ +133,3 @@ + if (g_strcmp0 (first_arg, "new") == 0) + { + /* The whole allocation/idle dance is to run the dialog after You have ways to detect that you're still in the init phase of your panel, so save the parameters and apply them when appropriate. Or just punt the problem to Rishi, you shouldn't need to care about that in this code, as it's irrelevant to this patch working. @@ +530,3 @@ + + n_children = g_variant_n_children (panel->parameters); + if (n_children > 1) As I've mentioned, I prefer switch statements.
(In reply to comment #22) > ::: panels/keyboard/cc-keyboard-panel.c > @@ +86,3 @@ > + > + parameters = g_value_get_variant (value); > + if (parameters && g_variant_n_children (parameters) > 0) { > > switch (g_variant_n_children (parameters)) { > case 1: > etc. Something along these line could work? I'm not sure if relying on switch/case fall-through is acceptable: parameters = g_value_get_variant (value); if (!parameters) break; page = section = NULL; switch (g_variant_n_children (parameters)) { case 2: g_variant_get_child (parameters, 1, "v", &v); section = g_variant_get_string (v, NULL); g_variant_unref (v); case 1: g_variant_get_child (parameters, 0, "v", &v); page = g_variant_get_string (v, NULL); g_variant_unref (v); break; cc_keyboard_panel_set_page (panel, page, section); case 0: break; default: g_warning ("Unexpected parameters found, ignore all of them"); } > > ::: panels/network/cc-network-panel.c > @@ +164,3 @@ > } > > +static const gchar** > > I doubt you're returning const gchar ** > Rather gchar * const * (or whatever the C is for that, the container isn't > const, the members are). Sigh, I'll never grasp it. My reading of the the C++ rule of "read left-to-right" an array of mutable pointers to immutable characters should produce 'gchar const **', which should be equivalent to the above. > @@ +170,3 @@ > + GVariant *v; > + const gchar **strv; > + gsize i = 0, n; > > assign "i" later on. > > ::: panels/online-accounts/cc-online-accounts-panel.c > @@ +103,3 @@ > switch (property_id) > { > + case PROP_PARAMETERS: > > You can't leave the online-accounts panel half-broken before adding the support > for creating new accounts in your other patch. Ouch, I added the final change to the index and then somehow failed to call 'amend'. :/ > ::: panels/sound/cc-sound-panel.c > @@ +59,3 @@ > + if (parameters && g_variant_n_children (parameters) > 0) { > + GVariant *v; > + g_variant_get_child (parameters, 0, "v", &v); > > Wouldn't something like > g_variant_get (parameters, "(s)", &string); > work? No, unfortunately. We have a "av" and you can't cast it to a tuple or an array of string, and neither you can access the "v" variant contents directly.
(In reply to comment #23) > ::: shell/Makefile.am > @@ +92,3 @@ > +servicefile_DATA = $(servicefile_in_files:.service.in=.service) > +$(servicefile_DATA): $(servicefile_in_files) Makefile > + $(AM_V_GEN) sed -e 's|[@]bindir[@]|$(bindir)|' $< > $@ > > I don't think you need to escape the '@' as you've done, they're already inside > a single-quote. Leaving them in place would cause autoconf to replace them when generating Makefile.in. > ::: shell/cc-application.c > @@ +378,3 @@ > { > + GApplicationFlags flags; > + flags = G_APPLICATION_HANDLES_COMMAND_LINE|G_APPLICATION_IS_SERVICE; > > No need to use a separate variable for that. I'm not sure how to format the resulting code: return g_object_new (CC_TYPE_APPLICATION, "application-id", "org.gnome.ControlCenter", "flags", G_APPLICATION_HANDLES_COMMAND_LINE | G_APPLICATION_IS_SERVICE, NULL); Or leave it on a single overly long line?
(In reply to comment #26) > (In reply to comment #23) > > > ::: shell/Makefile.am > > @@ +92,3 @@ > > +servicefile_DATA = $(servicefile_in_files:.service.in=.service) > > +$(servicefile_DATA): $(servicefile_in_files) Makefile > > + $(AM_V_GEN) sed -e 's|[@]bindir[@]|$(bindir)|' $< > $@ > > > > I don't think you need to escape the '@' as you've done, they're already inside > > a single-quote. > > Leaving them in place would cause autoconf to replace them when generating > Makefile.in. Just put the file in configure.ac to generate it then. > > ::: shell/cc-application.c > > @@ +378,3 @@ > > { > > + GApplicationFlags flags; > > + flags = G_APPLICATION_HANDLES_COMMAND_LINE|G_APPLICATION_IS_SERVICE; > > > > No need to use a separate variable for that. > > I'm not sure how to format the resulting code: > > return g_object_new (CC_TYPE_APPLICATION, > "application-id", "org.gnome.ControlCenter", > "flags", G_APPLICATION_HANDLES_COMMAND_LINE | > G_APPLICATION_IS_SERVICE, > NULL); > > Or leave it on a single overly long line? Overly long? My screen is wider than 80 characters...
(In reply to comment #25) > (In reply to comment #22) > > ::: panels/keyboard/cc-keyboard-panel.c > > @@ +86,3 @@ > > + > > + parameters = g_value_get_variant (value); > > + if (parameters && g_variant_n_children (parameters) > 0) { > > > > switch (g_variant_n_children (parameters)) { > > case 1: > > etc. > > Something along these line could work? I'm not sure if relying on switch/case > fall-through is acceptable: Sure. <snip> > case 1: > g_variant_get_child (parameters, 0, "v", &v); > page = g_variant_get_string (v, NULL); > g_variant_unref (v); > break; > cc_keyboard_panel_set_page (panel, page, section); > case 0: > break; As long as you actually call cc_keyboard_panel_set_page() :)
Created attachment 237722 [details] [review] shell: Use GVariant to convey panel arguments instead of a string array By using a GVariant of type "av" we can potentially pass more structured data to panels, which will become relevant with the ability to invoke them by GAction-based DBus-activation introduced in the following patch.
Created attachment 237723 [details] [review] shell: Expose panel launching with DBus-activation Turn Control Center in a DBus-activable service and export a 'launch-panel' GAction which accepts a tuple containing the id of the desired panel and its parameters as a GVariant array. The snippet below show how the custom shortcuts section of the keyboard panel can be invoked by a external programs through DBus: GVariantBuilder *b = g_variant_builder_new (G_VARIANT_TYPE ("av")); g_variant_builder_add (b, "v", g_variant_new_string ("shortcuts")); g_variant_builder_add (b, "v", g_variant_new_string ("custom")); GVariant *params = g_variant_new ("(s@av)", "keyboard", g_variant_builder_end (b)); GApplication *gnomecc = g_application_new ("org.gnome.ControlCenter", G_APPLICATION_IS_LAUNCHER); GError *error = NULL; if (!g_application_register (gnomecc, NULL, &error)) g_error ("Failed to register launcher: %s", error->message); g_action_group_activate_action (G_ACTION_GROUP (gnomecc), "launch-panel", params);
(In reply to comment #24) > @@ +321,3 @@ > + priv->collected_data.identity, > priv->collected_data.extra); > + else > + gtk_dialog_run (GTK_DIALOG (add_account)); > > We really shouldn't use gtk_dialog_run(), it steals the mainloop and creates > all sorts of problems. Yep, but that's what the original code did and goa_provider_add_account() calls it too. I tried to avoid having too many balls in the air at the same time. :) > ::: panels/online-accounts/cc-online-accounts-panel.c > @@ +133,3 @@ > + if (g_strcmp0 (first_arg, "new") == 0) > + { > + /* The whole allocation/idle dance is to run the dialog after > > You have ways to detect that you're still in the init phase of your panel, so > save the parameters and apply them when appropriate. > > Or just punt the problem to Rishi, you shouldn't need to care about that in > this code, as it's irrelevant to this patch working. I can completely get rid of the whole idle/realized stuff if having the dialog pop-up while the greyed-out main window behind it is still in overview mode is acceptable. Avoiding gtk_dialog_run() is probably enough to solve the issue, I'll check with Rishi what to do but I'd rather not block on it if possible.
Created attachment 237724 [details] [review] online-accounts: Accept requests for account creation on DBus/cmdline Also accepts additional data to pre-seed the GOA dialog, in particular the provider name (eg. 'google') and identity (eg. 'example@gmail.com'). Extra provider-specific data can be added as a GVariant dict ('a{sv}') to eg. pass cookies from an existing web session (this is only available from DBus activation). It can be run from the command line as: $ gnome-control-center online-accounts new google example@gmail.com
(In reply to comment #27) > > > +servicefile_DATA = $(servicefile_in_files:.service.in=.service) > > > +$(servicefile_DATA): $(servicefile_in_files) Makefile > > > + $(AM_V_GEN) sed -e 's|[@]bindir[@]|$(bindir)|' $< > $@ > > > > > > I don't think you need to escape the '@' as you've done, they're already inside > > > a single-quote. > > > > Leaving them in place would cause autoconf to replace them when generating > > Makefile.in. > > Just put the file in configure.ac to generate it then. Some lines below that, the 'completions/gnome-control-center' rule does something similar, but with a variable which does not exists at configure time. Doing it in the Makefile seems a better solution as it allows the usage of both configure- and build-time variables. Relatedly, the 'completions/gnome-control-center' rule would probably break in case someone defined an AC_SUBST()'ituded PANELS variable in configure.ac as it is not quoted. (As a side note, I started by copying from Raphael Slinckx's DBus activation tutorial and when noticed the bug I copied the solution from Empathy). > > > ::: shell/cc-application.c > > > @@ +378,3 @@ > > > { > > > + GApplicationFlags flags; > > > + flags = G_APPLICATION_HANDLES_COMMAND_LINE|G_APPLICATION_IS_SERVICE; > > > > > > No need to use a separate variable for that. > > > > I'm not sure how to format the resulting code: > > > > return g_object_new (CC_TYPE_APPLICATION, > > "application-id", "org.gnome.ControlCenter", > > "flags", G_APPLICATION_HANDLES_COMMAND_LINE | > > G_APPLICATION_IS_SERVICE, > > NULL); > > > > Or leave it on a single overly long line? > > Overly long? My screen is wider than 80 characters... Inlined. The previous patch has it line-wrapped, but the edited commit is here (to avoid re-sending for a trivial change): http://cgit.collabora.com/git/user/em/gnome-control-center/commit/?h=wip/bug-694315-goa-new-account&id=3cd550b6ec8619c
Created attachment 237889 [details] [review] shell: Expose panel launching with DBus-activation Fix `make distcheck' failure.
Created attachment 237890 [details] [review] online-accounts: Accept requests for account creation on DBus/cmdline Updated after last gnome-online-account patch update (https://bugzilla.gnome.org/attachment.cgi?id=237883)
Review of attachment 237889 [details] [review]: This breaks launching "gnome-control-center network connect-hidden-wifi" with an already opened settings window.
Created attachment 237974 [details] [review] shell: Expose panel launching with DBus-activation Don't set G_APPLICATION_IS_SERVICE otherwise other non-launcher instances will exit when failing to register the bus name.
Created attachment 238283 [details] [review] online-accounts: Accept requests for account creation on DBus/cmdline Rebased on top of latest master and added some more useful warnings if the GVariant arguments have unexpected types.
Created attachment 238772 [details] [review] shell: Use GVariant to convey panel arguments instead of a string array Reworked a bit how parameters are handled in online-accounts to better accomodate for hypotetic multiple sub-sub-commands.
Created attachment 238773 [details] [review] online-accounts: Accept requests for account creation on DBus/cmdline Split parameters handling for the 'new' sub-sub-command in a separate function.
Created attachment 239121 [details] [review] shell: Expose panel launching with DBus-activation Turn Control Center in a DBus-activable service and export a 'launch-panel' GAction which accepts a tuple containing the id of the desired panel and its parameters as a GVariant array. The snippet below show how the custom shortcuts section of the keyboard panel can be invoked by a external programs through DBus: GVariantBuilder *b = g_variant_builder_new (G_VARIANT_TYPE ("av")); g_variant_builder_add (b, "v", g_variant_new_string ("shortcuts")); g_variant_builder_add (b, "v", g_variant_new_string ("custom")); GVariant *params = g_variant_new ("(s@av)", "keyboard", g_variant_builder_end (b)); GApplication *gnomecc = g_application_new ("org.gnome.ControlCenter", G_APPLICATION_IS_LAUNCHER); GError *error = NULL; if (!g_application_register (gnomecc, NULL, &error)) g_error ("Failed to register launcher: %s", error->message); g_action_group_activate_action (G_ACTION_GROUP (gnomecc), "launch-panel", params);
Created attachment 239122 [details] [review] online-accounts: Accept requests for account creation on DBus/cmdline Also accepts additional data to pre-seed the GOA dialog, in particular the provider name (eg. 'google') and provider-specific data encoded in a a GVariant dict ('a{sv}') to eg. pass cookies from an existing web session (this is only available from DBus activation). It can be run from the command line as: $ gnome-control-center online-accounts add google Rebase on top of latest master + bug #696054, and change the 'new' sub-sub-command to 'create'.
Comment on attachment 238772 [details] [review] shell: Use GVariant to convey panel arguments instead of a string array Obsoleted by bug #695885
Comment on attachment 239121 [details] [review] shell: Expose panel launching with DBus-activation Obsoleted by bug #695885
Created attachment 240761 [details] [review] online-accounts: Accept requests for account creation on DBus/cmdline Rebased on current master (f79c23a)
Review of attachment 240761 [details] [review]: Thanks for the patch! ::: panels/online-accounts/cc-online-accounts-add-account-dialog.c @@ +256,3 @@ + g_clear_object (&priv->provider); + Pedantically speaking this should be in dispose. @@ +396,3 @@ + goa_provider_set_preseed_data (provider, preseed); + } +} What about using g_return_if_fail for the "provider != NULL" check? This isn't public API for a library, so maybe what you have here is better. ::: panels/online-accounts/cc-online-accounts-panel.c @@ +141,3 @@ + } + + add_account (panel, provider, preseed); If you do "gnome-control-center online-accounts add" without having an instance of the panel already running, you will have the following issues: (1) The height calculation of the AddAccountDialog will be wrong if we do this without waiting for the toplevel GtkWindow to finish resizing (see stack_page_notify_cb in cc-window.c for the call to gtk_window_resize). You will end up with a really tall dialog. (2) Since the dialog is modal we will still be seeing the "overview" instead of the online accounts panel in the backdrop. We can fix (2) by using an idle handler, however I couldn't figure out a way to address (1) without using a small timeout to let the resizing finish. Better ideas welcome. @@ +693,3 @@ GList *l; GoaObject *object; + GError *error = NULL; This is an unrelated change. @@ +702,3 @@ for (l = providers; l != NULL; l = l->next) + goa_panel_add_account_dialog_add_provider (GOA_PANEL_ADD_ACCOUNT_DIALOG (dialog), + GOA_PROVIDER (l->data)); This is an unrelated change. @@ -661,3 @@ - gtk_widget_destroy (dialog); - goto out; - } Since the label "out" is not being used anymore, it should be removed. Otherwise it leads to a warning.
Created attachment 240801 [details] [review] online-accounts: Accept requests for account creation on DBus/cmdline I was trying out different things to fix the height issues that I mentioned earlier, and thought I might just as well update your patch with the few lines I added. I don't like using an arbitrary timeout, so if you have a better idea feel free to try it out.
(In reply to comment #46) > + g_clear_object (&priv->provider); > + > > Pedantically speaking this should be in dispose. Right, moved. > @@ +396,3 @@ > + goa_provider_set_preseed_data (provider, preseed); > > What about using g_return_if_fail for the "provider != NULL" check? This isn't > public API for a library, so maybe what you have here is better. Currently goa_panel_add_account_dialog_set_preseed_data(dialog, NULL, NULL) is called to reset the dialog (eg. when you go back to the overview and back again to the online accounts panel), maybe having preseed != NULL and provider == NULL doesn't have much sense, but I'm not sure the check would add much value. Your call. :) > ::: panels/online-accounts/cc-online-accounts-panel.c > @@ +141,3 @@ > + } > + > + add_account (panel, provider, preseed); > > If you do "gnome-control-center online-accounts add" without having an instance > of the panel already running, you will have the following issues: > > (1) The height calculation of the AddAccountDialog will be wrong if we do this > without waiting for the toplevel GtkWindow to finish resizing (see > stack_page_notify_cb in cc-window.c for the call to gtk_window_resize). You > will end up with a really tall dialog. > > (2) Since the dialog is modal we will still be seeing the "overview" instead of > the online accounts panel in the backdrop. > > We can fix (2) by using an idle handler, however I couldn't figure out a way to > address (1) without using a small timeout to let the resizing finish. Better > ideas welcome. I started with those hack and I actualy got it working by relying on idle handlers and realize callbacks, but Bastien rightly rejected the idea. The real fix is to avoid gtk_dialog_run() in goa, I have posted some rough patches to do that in bug 695632. Do you think it's a blocker? > @@ +693,3 @@ > GList *l; > GoaObject *object; > + GError *error = NULL; > > This is an unrelated change. > > @@ +702,3 @@ > for (l = providers; l != NULL; l = l->next) > + goa_panel_add_account_dialog_add_provider (GOA_PANEL_ADD_ACCOUNT_DIALOG > (dialog), > + GOA_PROVIDER (l->data)); > > This is an unrelated change. > > @@ -661,3 @@ > - gtk_widget_destroy (dialog); > - goto out; > - } > > Since the label "out" is not being used anymore, it should be removed. > Otherwise it leads to a warning. Removed. Patch will follow.
Created attachment 242123 [details] [review] online-accounts: Accept requests for account creation on DBus/cmdline Fix issues per comment #48.
(In reply to comment #48) Thanks for the patch. > The real fix is to avoid gtk_dialog_run() in goa, I have posted some rough > patches to do that in bug 695632. Do you think it's a blocker? Ok, so lets do it the right way. I think you meant bug 695118 :-)
(In reply to comment #50) > > The real fix is to avoid gtk_dialog_run() in goa, I have posted some rough > > patches to do that in bug 695632. Do you think it's a blocker? > > Ok, so lets do it the right way. I think you meant bug 695118 :-) Oops, definitely. %-)
(In reply to comment #48) > > @@ +396,3 @@ > > + goa_provider_set_preseed_data (provider, preseed); > > > > What about using g_return_if_fail for the "provider != NULL" check? This isn't > > public API for a library, so maybe what you have here is better. > > Currently goa_panel_add_account_dialog_set_preseed_data(dialog, NULL, NULL) is > called to reset the dialog (eg. when you go back to the overview and back again > to the online accounts panel), maybe having preseed != NULL and provider == > NULL doesn't have much sense, but I'm not sure the check would add much value. > Your call. :) You are right.
Review of attachment 242123 [details] [review]: Looks good.