GNOME Bugzilla – Bug 694313
Add a way to pass custom info to the provider when creating the account
Last modified: 2013-03-07 12:32:03 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 gnome-control-center[3] to have a way to request new accounts and to funnel the gathered data from the browser to the provider. The patch adds a GoaProvider:add_account_from_json() virtual method which should replace GoaProvider:add_account() and uses the JSON data to add cookies to the GoaOauth2Provider cookie jar to skip the credentials prompt. The inspiration for this comes from the Ubuntu's webaccounts extension[4], which requires Unity and UOA. [1] GOA Browser Extension http://cgit.collabora.com/git/user/em/goa-browser-extension/ [2] GOA with goa_provider_add_account_from_json() http://cgit.collabora.com/git/user/em/gnome-online-accounts/log/?h=wip/add-account-json [3] 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 [4] Ubuntu Online Accounts Browser Extensions https://launchpad.net/webaccounts-browser-extension
Created attachment 236985 [details] [review] Add goa_provider_add_account_from_json() and wire it for OAuth2 Add a way to request the creation of a new account by providing additional info, such as session cookies from an already logged in web browser.
See bug #694315 for the GNOME Control Center patch.
Created attachment 237333 [details] [review] Add goa_provider_add_account_with_data() and wire it for OAuth2 Add a way to request the creation of a new account by providing additional info, such as session cookies from an already logged in web browser.
Review of attachment 237333 [details] [review]: Thanks for the patch. This looks interesting! ::: src/goabackend/goaoauth2provider.c @@ +1301,3 @@ + return ret; +} + Could you please put a /* --- */ separator here so that add_account_with_data and all the static methods used by it are in the same group? ::: src/goabackend/goaprovider.c @@ +266,3 @@ + * @dialog: A #GtkDialog. + * @vbox: A vertically oriented #GtkBox to put content in. + * @identity: The name of the new account Do we really need the @identity parameter while creating the account? If not, then it is one less thing that we have to scrape off the browsers. ::: src/goabackend/goaprovider.h @@ +65,3 @@ * @get_provider_group: Virtual function for goa_provider_get_provider_group(). * @add_account: Virtual function for goa_provider_add_account(). + * @add_account_with_data: Virtual function for goa_provider_add_account_with_data\(). Is that a stray \ ? @@ +95,3 @@ + gchar *identity, + GVariant *extra, + GError **error); Maybe add_account_from_data is a better name? Just saying. I don't have any strong feelings. I just want to be sure that this is not breaking the public ABI for libgoabackend, which is only meant to be used from the control-center. Looks fine to me since all the providers are in-tree and only they should be using the function pointers directly. Which makes me realize that this struct should be moved to goaprovider-priv.h.
Created attachment 237882 [details] [review] provider: Add the :preseed-data property for account creation The :pre-seed property is a a{sv} GVariant used to store a provider-type specific set of collected data that can be useful during account creation (eg. http cookies from an existing browser session or the entrypoint url for self-hosted services).
Created attachment 237883 [details] [review] oauth2: Push cookies stored in :preseed-data to the web view Push cookies collected from existing browser sessions to the web view to skip the username/password prompt if already logged in.
After your comments I took a different approach which does not requires new virtual methods on each GoaProvider subclass and instead add a :preseed-data property to GoaProvider itself. I also removed the 'identity' parameter: if needed it can always be stuffed into the preseed GVariant dict.
Created attachment 238024 [details] [review] oauth2: Push cookies stored in :preseed-data to the web view Ignore expiration times, in our use case they just bring troubles for no benefit.
Review of attachment 237882 [details] [review]: ::: src/goabackend/goaprovider.c @@ +64,3 @@ +}; + +static GParamSpec *properties[NUM_PROPERTIES] = { NULL, }; Most of the code directly calls g_object_class_install_property and names the first enum as PROP_0. Would be nice if you could do it that way for consistency. @@ +858,3 @@ +{ + return provider->priv->preseed_data; +} We will have to eventually document these because they are public. Not a blocker for this patch, though.
Review of attachment 238024 [details] [review]: ::: src/goabackend/goaoauth2provider.c @@ +548,3 @@ + return NULL; + + g_return_val_if_fail (g_variant_is_of_type (cookies_v, G_VARIANT_TYPE ("av")), NULL); We would be leaking cookies_v in this case. @@ +556,3 @@ + if (cookie != NULL) + cookies = g_slist_prepend (cookies, cookie); + g_variant_unref(cookie_v); When g_variant_unref is reached cookie_v could be NULL.
Created attachment 238279 [details] [review] provider: Add the :preseed-data property for account creation Added docs, unref on dispose() instead of finalize(), use PROP_0.
Created attachment 238280 [details] [review] oauth2: Push cookies stored in :preseed-data to the web view Do not leak the cookies GVariant if it has the wrong type and warn the user with a more explict message.
(In reply to comment #10) > We would be leaking cookies_v in this case. Fixed, thanks. > @@ +556,3 @@ > + if (cookie != NULL) > + cookies = g_slist_prepend (cookies, cookie); > + g_variant_unref(cookie_v); > > When g_variant_unref is reached cookie_v could be NULL. Mh, I'm not sure how: while ((cookie_v = g_variant_iter_next_value (&iter)) != NULL) { SoupCookie *cookie = create_cookie_from_variant (g_variant_get_variant (cookie_v)); if (cookie != NULL) cookies = g_slist_prepend (cookies, cookie); g_variant_unref(cookie_v); } cookie_v cannot be NULL inside the loop, or you meant something else? Maybe I should use different names for those variables, there are too many cookie{,_v,s,s_v} variations. :)
(In reply to comment #13) > (In reply to comment #10) > > > We would be leaking cookies_v in this case. > > Fixed, thanks. > > > @@ +556,3 @@ > > + if (cookie != NULL) > > + cookies = g_slist_prepend (cookies, cookie); > > + g_variant_unref(cookie_v); > > > > When g_variant_unref is reached cookie_v could be NULL. > > Mh, I'm not sure how: You are right. I mixed up cookie with cookie_v. Sorry about that it.
The following fixes have been pushed: 01447f6 oauth2: Push cookies stored in :preseed-data to the web view 1f714a8 provider: Add the :preseed-data property for account creation
Created attachment 238284 [details] [review] oauth2: Push cookies stored in :preseed-data to the web view Push cookies collected from existing browser sessions to the web view to skip the username/password prompt if already logged in.
Created attachment 238285 [details] [review] provider: Add the :preseed-data property for account creation The :pre-seed property is a a{sv} GVariant used to store a provider-type specific set of collected data that can be useful during account creation (eg. http cookies from an existing browser session or the entrypoint url for self-hosted services).