After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 704564 - Add Pocket support
Add Pocket support
Status: RESOLVED FIXED
Product: gnome-online-accounts
Classification: Core
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: GNOME Online Accounts maintainer(s)
GNOME Online Accounts maintainer(s)
Depends on:
Blocks: 720444
 
 
Reported: 2013-07-19 15:34 UTC by Bastien Nocera
Modified: 2014-01-13 16:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add Pocket support (19.94 KB, patch)
2013-07-19 15:34 UTC, Bastien Nocera
none Details | Review
Add Pocket support (19.86 KB, patch)
2013-08-23 15:59 UTC, Bastien Nocera
none Details | Review
oauth2: Backends can parse the redirect URL themselves (5.51 KB, patch)
2013-10-03 12:54 UTC, Bastien Nocera
none Details | Review
Add Pocket support (21.10 KB, patch)
2013-10-03 12:54 UTC, Bastien Nocera
none Details | Review
online-accounts: Mention Pocket in the keywords (1.15 KB, patch)
2013-10-04 07:08 UTC, Bastien Nocera
committed Details | Review
oauth2: Make scope optional (3.32 KB, patch)
2013-10-05 00:27 UTC, Bastien Nocera
reviewed Details | Review
Add Pocket support (21.49 KB, patch)
2013-10-05 00:29 UTC, Bastien Nocera
none Details | Review
webview: Add some debug code to show an inspector (3.35 KB, patch)
2013-10-05 00:30 UTC, Bastien Nocera
needs-work Details | Review
oauth2: Backends can parse the redirect URL themselves (5.45 KB, patch)
2013-10-05 12:12 UTC, Bastien Nocera
none Details | Review
Add Pocket support (21.78 KB, patch)
2013-10-07 11:35 UTC, Bastien Nocera
needs-work Details | Review
webview: Add some debug code to show an inspector (4.43 KB, patch)
2013-10-16 13:40 UTC, Debarshi Ray
committed Details | Review
oauth2: Make scope optional (6.15 KB, patch)
2013-10-30 10:49 UTC, Debarshi Ray
none Details | Review
oauth2: Backends can parse the redirect URL themselves (5.21 KB, patch)
2013-10-30 10:54 UTC, Debarshi Ray
none Details | Review
oauth2: Make scope optional (6.38 KB, patch)
2013-11-25 12:01 UTC, Bastien Nocera
committed Details | Review
oauth2: Backends can parse the redirect URL themselves (5.66 KB, patch)
2013-11-25 12:01 UTC, Bastien Nocera
reviewed Details | Review
Add Pocket support (22.56 KB, patch)
2013-11-25 12:01 UTC, Bastien Nocera
none Details | Review
examples: Add a Pocket example (4.61 KB, patch)
2013-12-14 02:36 UTC, Bastien Nocera
none Details | Review
Add Pocket support (33.29 KB, patch)
2013-12-16 16:30 UTC, Bastien Nocera
reviewed Details | Review
examples: Add a Pocket example (4.62 KB, patch)
2013-12-16 16:30 UTC, Bastien Nocera
committed Details | Review
oauth2: Clean up (1.25 KB, patch)
2013-12-20 16:11 UTC, Debarshi Ray
committed Details | Review
doc: Clarify that is_password_node saves the user's password (1.85 KB, patch)
2013-12-20 16:13 UTC, Debarshi Ray
committed Details | Review
oauth2: Let backends extract the code or token themselves (5.79 KB, patch)
2013-12-20 18:33 UTC, Debarshi Ray
committed Details | Review
Add Pocket support (33.98 KB, patch)
2013-12-20 18:35 UTC, Debarshi Ray
none Details | Review
Add Pocket support (33.67 KB, patch)
2014-01-13 14:14 UTC, Debarshi Ray
committed Details | Review
oauth2: Make the errors consistent (1.63 KB, patch)
2014-01-13 15:03 UTC, Debarshi Ray
committed Details | Review
doc: Add goa_oauth2_provider_process_redirect_url (2.55 KB, patch)
2014-01-13 15:04 UTC, Debarshi Ray
committed Details | Review
online-accounts: Mention Pocket in the keywords (1.15 KB, patch)
2014-01-13 15:23 UTC, Debarshi Ray
needs-work Details | Review

Description Bastien Nocera 2013-07-19 15:34:36 UTC
Not tested, and there's probably a bunch of bits that will need proper
implementation. This is a first pass.
Comment 1 Bastien Nocera 2013-07-19 15:34:38 UTC
Created attachment 249641 [details] [review]
Add Pocket support

Incomplete first pass.

Based on the documentation at:
http://getpocket.com/developer/docs/authentication
Comment 2 Bastien Nocera 2013-08-23 15:59:40 UTC
Created attachment 252898 [details] [review]
Add Pocket support

Incomplete first pass.

Based on the documentation at:
http://getpocket.com/developer/docs/authentication
Comment 3 Bastien Nocera 2013-10-03 12:54:25 UTC
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?
Comment 4 Bastien Nocera 2013-10-03 12:54:36 UTC
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
Comment 5 Bastien Nocera 2013-10-04 07:08:50 UTC
Created attachment 256447 [details] [review]
online-accounts: Mention Pocket in the keywords
Comment 6 Bastien Nocera 2013-10-05 00:27:23 UTC
Created attachment 256514 [details] [review]
oauth2: Make scope optional

Some provider don't use the scope, so no need for it.
Comment 7 Bastien Nocera 2013-10-05 00:29:41 UTC
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...
Comment 8 Bastien Nocera 2013-10-05 00:30:14 UTC
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
Comment 9 Bastien Nocera 2013-10-05 12:12:55 UTC
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?
Comment 10 Bastien Nocera 2013-10-07 11:35:30 UTC
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...
Comment 11 Debarshi Ray 2013-10-16 11:43:43 UTC
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.
Comment 12 Debarshi Ray 2013-10-16 11:47:17 UTC
For your information, I filed bug 710263 about the search issue.
Comment 13 Debarshi Ray 2013-10-16 12:05:15 UTC
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.
Comment 14 Debarshi Ray 2013-10-16 13:38:50 UTC
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.
Comment 15 Debarshi Ray 2013-10-16 13:40:17 UTC
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
Comment 16 Debarshi Ray 2013-10-16 14:08:59 UTC
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
Comment 17 Bastien Nocera 2013-10-17 22:48:57 UTC
(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...
Comment 18 Debarshi Ray 2013-10-30 08:59:14 UTC
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!
Comment 19 Debarshi Ray 2013-10-30 09:35:47 UTC
(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
  • #0 ??
  • #1 check_cookie
    at goaoauth2provider.c line 781
  • #2 g_slist_foreach
    at gslist.c line 896
  • #3 on_web_view_document_load_finished
    at goaoauth2provider.c line 833
  • #4 g_cclosure_marshal_VOID__OBJECTv
    at gmarshal.c line 1312
  • #5 _g_closure_invoke_va
    at gclosure.c line 840
  • #6 g_signal_emit_valist
    at gsignal.c line 3238
  • #7 g_signal_emit_by_name
    at gsignal.c line 3426
  • #8 WebCore::FrameLoader::finishedParsing()
    from /lib64/libwebkitgtk-3.0.so.0
  • #9 WebCore::Document::finishedParsing()
    from /lib64/libwebkitgtk-3.0.so.0
  • #10 WebCore::HTMLDocumentParser::prepareToStopParsing()
    from /lib64/libwebkitgtk-3.0.so.0
  • #11 WebCore::HTMLDocumentParser::executeScriptsWaitingForStylesheets()
    from /lib64/libwebkitgtk-3.0.so.0
  • #12 WebCore::Document::didRemoveAllPendingStylesheet()
    from /lib64/libwebkitgtk-3.0.so.0
  • #13 WebCore::HTMLLinkElement::sheetLoaded()
    from /lib64/libwebkitgtk-3.0.so.0
  • #14 WebCore::StyleSheetContents::checkLoaded()
    from /lib64/libwebkitgtk-3.0.so.0
  • #15 WebCore::HTMLLinkElement::setCSSStyleSheet(WTF::String const&, WebCore::KURL const&, WTF::String const&, WebCore::CachedCSSStyleSheet const*)
    from /lib64/libwebkitgtk-3.0.so.0
  • #16 WebCore::CachedCSSStyleSheet::checkNotify()
    from /lib64/libwebkitgtk-3.0.so.0
  • #17 WebCore::CachedCSSStyleSheet::finishLoading(WebCore::ResourceBuffer*)
    from /lib64/libwebkitgtk-3.0.so.0
  • #18 WebCore::SubresourceLoader::didFinishLoading(double)
    from /lib64/libwebkitgtk-3.0.so.0
  • #19 WebCore::readCallback(_GObject*, _GAsyncResult*, void*)
    from /lib64/libwebkitgtk-3.0.so.0
  • #20 async_ready_callback_wrapper
    at ginputstream.c line 519
  • #21 g_task_return_now
    at gtask.c line 1108
  • #22 complete_in_idle_cb
    at gtask.c line 1117
  • #23 g_main_dispatch
    at gmain.c line 3065
  • #24 g_main_context_dispatch
    at gmain.c line 3641
  • #25 g_main_context_iterate
    at gmain.c line 3712
  • #26 g_main_loop_run
    at gmain.c line 3906
  • #27 gtk_dialog_run
    from /lib64/libgtk-3.so.0
  • #28 get_tokens_and_identity
    at goaoauth2provider.c line 1156
  • #29 goa_oauth2_provider_add_account
    at goaoauth2provider.c line 1304
  • #30 goa_provider_add_account
    at goaprovider.c line 381
  • #31 add_account_dialog_add_account
  • #32 g_cclosure_marshal_VOID__OBJECTv
    at gmarshal.c line 1312
  • #33 _g_closure_invoke_va
    at gclosure.c line 840
  • #34 g_signal_emit_valist
    at gsignal.c line 3238
  • #35 g_signal_emit
    at gsignal.c line 3386
  • #36 gtk_list_box_real_button_release_event
    from /lib64/libgtk-3.so.0
  • #37 _gtk_marshal_BOOLEAN__BOXEDv
    from /lib64/libgtk-3.so.0
  • #38 _g_closure_invoke_va
    at gclosure.c line 840
  • #39 g_signal_emit_valist
    at gsignal.c line 3238
  • #40 g_signal_emit
    at gsignal.c line 3386
  • #41 gtk_widget_event_internal
    from /lib64/libgtk-3.so.0
  • #42 propagate_event
    from /lib64/libgtk-3.so.0
  • #43 gtk_main_do_event
    from /lib64/libgtk-3.so.0
  • #44 gdk_event_source_dispatch
    from /lib64/libgdk-3.so.0
  • #45 g_main_dispatch
    at gmain.c line 3065
  • #46 g_main_context_dispatch
    at gmain.c line 3641
  • #47 g_main_context_iterate
    at gmain.c line 3712
  • #48 g_main_loop_run
    at gmain.c line 3906
  • #49 gtk_dialog_run
    from /lib64/libgtk-3.so.0
  • #50 get_all_providers_cb
  • #51 g_simple_async_result_complete
    at gsimpleasyncresult.c line 777
  • #52 complete_in_idle_cb
    at gsimpleasyncresult.c line 789
  • #53 g_main_dispatch
    at gmain.c line 3065
  • #54 g_main_context_dispatch
    at gmain.c line 3641
  • #55 g_main_context_iterate
    at gmain.c line 3712
  • #56 g_main_context_iteration
    at gmain.c line 3773
  • #57 g_application_run
    at gapplication.c line 1635
  • #58 main
  • #1 check_cookie
    at goaoauth2provider.c line 781
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)
Comment 20 Debarshi Ray 2013-10-30 10:47:40 UTC
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.
Comment 21 Debarshi Ray 2013-10-30 10:49:07 UTC
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.
Comment 22 Debarshi Ray 2013-10-30 10:54:44 UTC
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
Comment 23 Debarshi Ray 2013-10-30 10:57:45 UTC
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.
Comment 24 Bastien Nocera 2013-11-25 10:02:32 UTC
(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 :(
Comment 25 Bastien Nocera 2013-11-25 12:01:38 UTC
Created attachment 261425 [details] [review]
oauth2: Make scope optional

Some provider don't use the scope, so no need for it.
Comment 26 Bastien Nocera 2013-11-25 12:01:44 UTC
Created attachment 261426 [details] [review]
oauth2: Backends can parse the redirect URL themselves

And pass back the access_token to the application.
Comment 27 Bastien Nocera 2013-11-25 12:01:51 UTC
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...
Comment 28 Bastien Nocera 2013-12-14 02:36:37 UTC
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.
Comment 29 Bastien Nocera 2013-12-16 16:30:48 UTC
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...
Comment 30 Bastien Nocera 2013-12-16 16:30:57 UTC
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.
Comment 31 Debarshi Ray 2013-12-20 14:46:10 UTC
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?
Comment 32 Debarshi Ray 2013-12-20 15:25:41 UTC
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.
Comment 33 Debarshi Ray 2013-12-20 16:11:32 UTC
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.
Comment 34 Debarshi Ray 2013-12-20 16:13:49 UTC
Created attachment 264628 [details] [review]
doc: Clarify that is_password_node saves the user's password
Comment 35 Debarshi Ray 2013-12-20 17:26:05 UTC
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.
Comment 36 Debarshi Ray 2013-12-20 18:33:57 UTC
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.
Comment 37 Debarshi Ray 2013-12-20 18:35:01 UTC
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...
Comment 38 Debarshi Ray 2013-12-20 18:38:02 UTC
Review of attachment 264303 [details] [review]:

Looks good to me. Thanks.
Comment 39 Debarshi Ray 2013-12-20 18:40:12 UTC
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
Comment 40 Bastien Nocera 2014-01-09 15:16:15 UTC
(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?
Comment 41 Debarshi Ray 2014-01-13 14:14:08 UTC
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
Comment 42 Debarshi Ray 2014-01-13 15:03:42 UTC
Created attachment 266167 [details] [review]
oauth2: Make the errors consistent
Comment 43 Debarshi Ray 2014-01-13 15:04:16 UTC
Created attachment 266168 [details] [review]
doc: Add goa_oauth2_provider_process_redirect_url
Comment 44 Debarshi Ray 2014-01-13 15:19:33 UTC
Review of attachment 256447 [details] [review]:

Given the resolution in bug 710263 , I guess it should now be "Read;It;Later;"?
Comment 45 Debarshi Ray 2014-01-13 15:23:13 UTC
Created attachment 266170 [details] [review]
online-accounts: Mention Pocket in the keywords

Maybe something like this?
Comment 46 Bastien Nocera 2014-01-13 16:10:21 UTC
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
Comment 47 Bastien Nocera 2014-01-13 16:35:32 UTC
Attachment 256447 [details] pushed as a3c2f75 - online-accounts: Mention Pocket in the keywords