GNOME Bugzilla – Bug 704564
Add Pocket support
Last modified: 2014-01-13 16:35:46 UTC
Not tested, and there's probably a bunch of bits that will need proper implementation. This is a first pass.
Created attachment 249641 [details] [review] Add Pocket support Incomplete first pass. Based on the documentation at: http://getpocket.com/developer/docs/authentication
Created attachment 252898 [details] [review] Add Pocket support Incomplete first pass. Based on the documentation at: http://getpocket.com/developer/docs/authentication
Created attachment 256381 [details] [review] oauth2: Backends can parse the redirect URL themselves And pass back the access_token to the application. XXX Is this the right semantics? XXX Should we allow returning the authorization_token too?
Created attachment 256382 [details] [review] Add Pocket support Based on the documentation at: http://getpocket.com/developer/docs/authentication v2: - Make it work TODO: - Sync network calls in build_authorization_uri() and process_redirect_url() - Leaks memory (easy-fix) - is_identity_node() and is_deny_node() are completely useless, what are they supposed to do? - Using the mobile browser makes the authorisation/cancellation buttons not work - Make all the apps running against an older goa backend crash
Created attachment 256447 [details] [review] online-accounts: Mention Pocket in the keywords
Created attachment 256514 [details] [review] oauth2: Make scope optional Some provider don't use the scope, so no need for it.
Created attachment 256515 [details] [review] Add Pocket support Based on the documentation at: http://getpocket.com/developer/docs/authentication v2: - Make it work v3: - Fix the memory leaks - We didn't make apps crash by adding an account type, goa-daemon -r did: https://bugzilla.gnome.org/show_bug.cgi?id=698726 - Implement is_identity_node() - Partially implement is_deny_node(), see below TODO: - Sync network calls in build_authorization_uri() and process_redirect_url() - Should detect the Cancel button correctly Doc problems: - is_password_node() should mention that it saves the password... - is_identity_node() shouldn't say it denies access...
Created attachment 256516 [details] [review] webview: Add some debug code to show an inspector This makes it easier to find the various nodes, and check why the mobile view might not work. Note that you'll need to remove the modality on gnome-control-center's "add accounts" dialogue for this to work. FIXME: Mouse and keyboard events don't seem to get to the inspector
Created attachment 256540 [details] [review] oauth2: Backends can parse the redirect URL themselves And pass back the access_token to the application. XXX Is this the right semantics? XXX Should we allow returning the authorization_token too?
Created attachment 256619 [details] [review] Add Pocket support Based on the documentation at: http://getpocket.com/developer/docs/authentication v2: - Make it work v3: - Fix the memory leaks - We didn't make apps crash by adding an account type, goa-daemon -r did: https://bugzilla.gnome.org/show_bug.cgi?id=698726 - Implement is_identity_node() - Partially implement is_deny_node(), see below v4: - Detect the Cancel button, but that only seems to work once TODO: - Sync network calls in build_authorization_uri() and process_redirect_url() - Cancel button doesn't work when we click "Sign Up" first Doc problems: - is_password_node() should mention that it saves the password... - is_identity_node() shouldn't say it denies access...
Review of attachment 256447 [details] [review]: ::: panels/online-accounts/gnome-online-accounts-panel.desktop.in.in @@ +14,3 @@ X-GNOME-Bugzilla-Version=@VERSION@ # Translators: those are keywords for the online-accounts control-center panel +_Keywords=Google;Facebook;Twitter;Yahoo;Web;Online;Chat;Calendar;Mail;Contact;ownCloud;Kerberos;IMAP;SMTP;Pocket;ReadItLater; Shouldn't it be "Read It Later" instead of "ReadItLater"? I can see that the *.desktop files in LibreOffice have such space separated keyword values and they work when searching in GNOME Shell. However searching for "screen" or "screen reader" or "reader" doesn't show the a11y panel even though gnome-universal-access-panel.desktop has it. Could be a bug in GNOME Shell.
For your information, I filed bug 710263 about the search issue.
Review of attachment 256514 [details] [review]: Looks OK in principle. ::: src/goabackend/goaoauth2provider.c @@ -282,3 @@ g_return_val_if_fail (escaped_redirect_uri != NULL, NULL); g_return_val_if_fail (escaped_client_id != NULL, NULL); - g_return_val_if_fail (escaped_scope != NULL, NULL); Shouldn't we also mark it as (allow-none) in the documentation blurb? @@ +403,1 @@ } It would be more consistent with the rest of the file if there was a goa_oauth2_provider_get_scope_default that returned NULL. @@ +1062,3 @@ escaped_client_id = g_uri_escape_string (goa_oauth2_provider_get_client_id (provider), NULL, TRUE); + scope = goa_oauth2_provider_get_scope (provider); + if (scope) Please use "if (scope != NULL)" for pointers.
Review of attachment 256516 [details] [review]: Thanks for finally submitting a patch for this. Sorry for being too lazy to never turn my ad-hoc debug code into a patch. Maybe we should make it a build-time option and have a way for the control-center to figure out if the inspector has been enabled and turn off the modality? With the new panel design that has been proposed, the modality and the problem of events not reaching the inspector might not be a problem anymore. ::: src/goabackend/goawebview.c @@ +260,3 @@ + gtk_window_present (GTK_WINDOW (window)); + + return TRUE; This signal seems to be following GDK_EVENT_STOP / GDK_EVENT_PROPAGATE semantics. Maybe we should use those macros instead of TRUE? @@ +263,3 @@ +} +#endif /* ENABLE_INSPECTOR */ + The indentation does not match the rest of the file.
Created attachment 257420 [details] [review] webview: Add some debug code to show an inspector - Adds a build-time option for the inspector - Fixes the indentation issues - GDK_EVENT_STOP instead of TRUE
Review of attachment 256619 [details] [review]: Currently it crashes for me as I try to log in via the embedded web browser because there is no implementation of get_authentication_cookie. The whole point of get_authentication_cookie is to figure out if we have managed to log in via the web view. Once we have logged in we want to remove the GtkButton to cancel the dialog, because usually the web page offers a way to cancel the operation once you have logged in. In this case, the web page has a cancel button even before we have logged in. So, we can remove the GtkButton immediately after the user has selected Pocket. There is no need to check for cookies. I think that the GoaOAuth2Provider base class needs to be enhanced to take this into account. ::: src/goabackend/goapocketprovider.c @@ +308,3 @@ + g_free (text); + return ret; +} It might not be needed to parse the DOM for this if Pocket returns an error compliant with the OAuth2 spec. See bug 709570
(In reply to comment #16) > Review of attachment 256619 [details] [review]: > > Currently it crashes for me as I try to log in via the embedded web browser > because there is no implementation of get_authentication_cookie. What's the backtrace of the crash? It worked fine in my testing, otherwise I wouldn't have been posting it here...
Review of attachment 257420 [details] [review]: Lets get this in. It is obviously useful, and works apart from the issue of events not reaching the inspector. We can investigate and fix that later and not let this bit rot in bugzilla. Thanks for the patch!
(In reply to comment #17) > (In reply to comment #16) > > Review of attachment 256619 [details] [review] [details]: > > > > Currently it crashes for me as I try to log in via the embedded web browser > > because there is no implementation of get_authentication_cookie. > > What's the backtrace of the crash? It worked fine in my testing, otherwise I > wouldn't have been posting it here... This is what I am getting: (gdb) bt
+ Trace 232682
776 GtkDialog *dialog = priv->dialog; 777 GtkWidget *action_area; 778 const gchar *auth_cookie; 779 const gchar *name; 780 781 auth_cookie = goa_oauth2_provider_get_authentication_cookie (provider); 782 name = soup_cookie_get_name (cookie); 783 if (g_strcmp0 (auth_cookie, name) != 0) 784 return; 785 (gdb)
Review of attachment 256514 [details] [review]: ::: src/goabackend/goaoauth2provider.c @@ +56,3 @@ * #GoaProviderClass.get_provider_type, * #GoaProviderClass.get_provider_name, + * ##GoaOAuth2ProviderClass.get_scope, This is probably a left over from debugging, and should be removed from this patch. However I do realize that this bit of documentation needs to be updated to accurately state which methods are pure virtual and must be implemented, which ones are optional. We can do it in a separate patch.
Created attachment 258545 [details] [review] oauth2: Make scope optional Tweaked the previous version of the patch according to the review comments. There should not be any functional changes.
Created attachment 258546 [details] [review] oauth2: Backends can parse the redirect URL themselves Rebased on top of current master. Conflict caused by patches from bug 710363
Review of attachment 256619 [details] [review]: ::: configure.ac @@ +329,3 @@ + [Enable Pocket provider])], + [], + [enable_pocket=yes]) This should not be enabled by default unless we have some core GNOME components using this. The value of enable_pocket should be printed at the end of the file.
(In reply to comment #16) > Review of attachment 256619 [details] [review]: > > Currently it crashes for me as I try to log in via the embedded web browser > because there is no implementation of get_authentication_cookie. The whole > point of get_authentication_cookie is to figure out if we have managed to log > in via the web view. How come you get a cookie when I don't? What cookie do you get? > Once we have logged in we want to remove the GtkButton to > cancel the dialog, because usually the web page offers a way to cancel the > operation once you have logged in. > > In this case, the web page has a cancel button even before we have logged in. > So, we can remove the GtkButton immediately after the user has selected Pocket. I don't think we should be hiding the cancel button, because if there are problems loading the webpage, we'd end up not being able to close the dialogue at all (no close button, no cancel button). > It might not be needed to parse the DOM for this if Pocket returns an error > compliant with the OAuth2 spec. See bug 709570 It doesn't :(
Created attachment 261425 [details] [review] oauth2: Make scope optional Some provider don't use the scope, so no need for it.
Created attachment 261426 [details] [review] oauth2: Backends can parse the redirect URL themselves And pass back the access_token to the application.
Created attachment 261427 [details] [review] Add Pocket support Based on the documentation at: http://getpocket.com/developer/docs/authentication v2: - Make it work v3: - Fix the memory leaks - We didn't make apps crash by adding an account type, goa-daemon -r did: See https://bugzilla.gnome.org/show_bug.cgi?id=698726 - Implement is_identity_node() - Partially implement is_deny_node(), see below v4: - Detect the Cancel button, but that only seems to work once v5: - Disable by default - Print enablement status at the end of configure - Show client ID as well - Fix crash when cookies were set TODO: - Sync network calls in build_authorization_uri() and process_redirect_url() - Cancel button doesn't work when we click "Sign Up" first Doc problems: - is_password_node() should mention that it saves the password... - is_identity_node() shouldn't say it denies access...
Created attachment 264183 [details] [review] examples: Add a Pocket example This example adds the passed URL to the first Pocket account listed setup in GOA. Optionally, you can pass a tweet ID which would be used to show where the user got the URL from.
Created attachment 264302 [details] [review] Add Pocket support Based on the documentation at: http://getpocket.com/developer/docs/authentication Icons by Jakub Steiner originally from: https://raw.github.com/gnome-design-team/gnome-icons/master/web-accounts/tango/src/web-accounts.svg v2: - Make it work v3: - Fix the memory leaks - We didn't make apps crash by adding an account type, goa-daemon -r did: See https://bugzilla.gnome.org/show_bug.cgi?id=698726 - Implement is_identity_node() - Partially implement is_deny_node(), see below v4: - Detect the Cancel button, but that only seems to work once v5: - Disable by default - Print enablement status at the end of configure - Show client ID as well - Fix crash when cookies were set v6: - Add icons by jimmac TODO: - Sync network calls in build_authorization_uri() and process_redirect_url() - Cancel button doesn't work when we click "Sign Up" first Doc problems: - is_password_node() should mention that it saves the password... - is_identity_node() shouldn't say it denies access...
Created attachment 264303 [details] [review] examples: Add a Pocket example This example adds the passed URL to the first Pocket account listed setup in GOA. Optionally, you can pass a tweet ID which would be used to show where the user got the URL from.
Review of attachment 261426 [details] [review]: ::: src/goabackend/goaoauth2provider.c @@ +305,3 @@ + error); + } + return FALSE; We could just invoke ->process_redirect_url without checking whether it is implemented or not, like the other pure virtual methods in this file. We are doing that check when we actually use this method anyway. @@ +943,3 @@ + if (!GOA_OAUTH2_PROVIDER_GET_CLASS (provider)->process_redirect_url (provider, url, + &priv->access_token, + &priv->error)) Can't we use goa_oauth2_provider_process_redirect_url?
Review of attachment 261426 [details] [review]: ::: src/goabackend/goaoauth2provider.c @@ +943,3 @@ + if (!GOA_OAUTH2_PROVIDER_GET_CLASS (provider)->process_redirect_url (provider, url, + &priv->access_token, + &priv->error)) I don't think this will work correctly if ->process_redirect_url failed. The implementation for Pocket does not set the error when it fails due to either identity or access_token being NULL.
Created attachment 264627 [details] [review] oauth2: Clean up There is no need to check if get_scope is implemented because there is a default implementation that does the right thing. This was removed in one of the versions of the previous patch, but it snuck back in during the review.
Created attachment 264628 [details] [review] doc: Clarify that is_password_node saves the user's password
Review of attachment 264302 [details] [review]: I updated the documentation to mention that is_password_node can be used to save passwords. However, I could not find anything wrong with is_identity_node. Maybe it got fixed in the meantime. ::: src/goabackend/goapocketprovider.c @@ +189,3 @@ + authorization_uri, + code, + escaped_redirect_uri); The business of getting a request token before showing the web page smells of OAuth 1.0. Here is what Google's deprecated OAuth 1.0 implementation does: https://developers.google.com/accounts/docs/OAuth_ref#RequestToken But, then, Pocket claims that are a variant of OAuth 2.0, and I have no clue if it would make our lives easier to derive from OAuthProvider instead of OAuth2Provider. @@ +234,3 @@ + { + g_clear_pointer (&pocket->identity, g_free); + g_clear_pointer (access_token, g_free); Should set an error. @@ +458,3 @@ + provider = GOA_POCKET_PROVIDER (object); + g_clear_pointer (&provider->code, g_free); + g_clear_pointer (&provider->identity, g_free); Needs to chain up.
Created attachment 264640 [details] [review] oauth2: Let backends extract the code or token themselves Tweak the previous patch according to the comments, and explain a bit in the commit message why we are doing this.
Created attachment 264641 [details] [review] Add Pocket support Add Pocket support Based on the documentation at: http://getpocket.com/developer/docs/authentication Icons by Jakub Steiner originally from: https://raw.github.com/gnome-design-team/gnome-icons/master/web-accounts/tango/src/web-accounts.svg v2: - Make it work v3: - Fix the memory leaks - We didn't make apps crash by adding an account type, goa-daemon -r did: See https://bugzilla.gnome.org/show_bug.cgi?id=698726 - Implement is_identity_node() - Partially implement is_deny_node(), see below v4: - Detect the Cancel button, but that only seems to work once v5: - Disable by default - Print enablement status at the end of configure - Show client ID as well - Fix crash when cookies were set v6: - Add icons by jimmac v7: - Chain up in finalize - Set error when processing the redirect URL - Update POTFILES.in TODO: - Sync network calls in build_authorization_uri() and process_redirect_url() - Cancel button doesn't work when we click "Sign Up" first Doc problems: - is_identity_node() shouldn't say it denies access...
Review of attachment 264303 [details] [review]: Looks good to me. Thanks.
I think we can land this now. Unfortunately, I can not add a Pocket account on my system anymore. I click "Authorize" and nothing happens -- the web page does not react. I only get this message in the console: "** Message: console message: https://getpocket.com/auth/j/native.js @189: finishStart
(In reply to comment #39) > I think we can land this now. > > Unfortunately, I can not add a Pocket account on my system anymore. I click > "Authorize" and nothing happens -- the web page does not react. I only get this > message in the console: > "** Message: console message: https://getpocket.com/auth/j/native.js @189: > finishStart It's due to the javascript code to handle the buttons that expects taps, not clicks. In the meanwhile, using the non-mobile version works fine. Could we merge this with the one-liner to use the desktop version instead?
Created attachment 266163 [details] [review] Add Pocket support Based on the documentation at: http://getpocket.com/developer/docs/authentication Icons by Jakub Steiner originally from: https://raw.github.com/gnome-design-team/gnome-icons/master/web-accounts/tango/src/web-accounts.svg v2: - Make it work v3: - Fix the memory leaks - We didn't make apps crash by adding an account type, goa-daemon -r did: See https://bugzilla.gnome.org/show_bug.cgi?id=698726 - Implement is_identity_node() - Partially implement is_deny_node(), see below v4: - Detect the Cancel button, but that only seems to work once v5: - Disable by default - Print enablement status at the end of configure - Show client ID as well - Fix crash when cookies were set v6: - Add icons by jimmac v7: - Chain up in finalize - Set error when processing the redirect URL - Update POTFILES.in v8: - Do not use the mobile web page TODO: - Sync network calls in build_authorization_uri() and process_redirect_url() - Cancel button doesn't work when we click "Sign Up" first https://bugzilla.gnome.org/show_bug.cgi?id=704564
Created attachment 266167 [details] [review] oauth2: Make the errors consistent
Created attachment 266168 [details] [review] doc: Add goa_oauth2_provider_process_redirect_url
Review of attachment 256447 [details] [review]: Given the resolution in bug 710263 , I guess it should now be "Read;It;Later;"?
Created attachment 266170 [details] [review] online-accounts: Mention Pocket in the keywords Maybe something like this?
Review of attachment 266170 [details] [review]: ::: panels/online-accounts/gnome-online-accounts-panel.desktop.in.in @@ +15,3 @@ X-GNOME-Bugzilla-Version=@VERSION@ # Translators: those are keywords for the online-accounts control-center panel +_Keywords=Google;Facebook;Twitter;Yahoo;Web;Online;Chat;Calendar;Mail;Contact;ownCloud;Kerberos;IMAP;SMTP;Pocket;Read;It;Later; Should be ReadItLater. That's the old name of the service. Having them separate is pretty ridiculous. There should also be a comment to say that "Pocket" is the name of the "Read it later" service at http://getpocket.com
Attachment 256447 [details] pushed as a3c2f75 - online-accounts: Mention Pocket in the keywords