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 661067 - Add live messenger OAUTH2 provider
Add live messenger OAUTH2 provider
Status: RESOLVED FIXED
Product: gnome-online-accounts
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: GNOME Online Accounts maintainer(s)
GNOME Online Accounts maintainer(s)
Depends on: 660993
Blocks: 661068
 
 
Reported: 2011-10-06 09:57 UTC by Xavier Claessens
Modified: 2011-12-19 15:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add Microsoft Live Messenger OAuth2 (16.22 KB, patch)
2011-10-06 09:58 UTC, Xavier Claessens
none Details | Review
Add Windows Live Messenger OAuth2 (29.99 KB, patch)
2011-10-27 13:45 UTC, Xavier Claessens
none Details | Review
backtrace when goa is in deadlock (6.72 KB, text/plain)
2011-11-02 12:58 UTC, Xavier Claessens
  Details
Add Windows Live Messenger OAuth2 (29.85 KB, patch)
2011-11-02 13:49 UTC, Xavier Claessens
none Details | Review
Add Windows Live oauth2 (18.52 KB, patch)
2011-12-16 11:18 UTC, Xavier Claessens
reviewed Details | Review
Provide icon for windows-live service (15.40 KB, patch)
2011-12-16 11:28 UTC, Xavier Claessens
none Details | Review
Enable windows-live by default (1.35 KB, patch)
2011-12-16 11:46 UTC, Xavier Claessens
none Details | Review

Description Xavier Claessens 2011-10-06 09:57:14 UTC
We should be able to chat with XMPP using oauth2 mechanism.
Comment 1 Xavier Claessens 2011-10-06 09:58:06 UTC
Created attachment 198420 [details] [review]
Add Microsoft Live Messenger OAuth2
Comment 2 Xavier Claessens 2011-10-06 09:58:40 UTC
I get the same crash than bug #660993 when creating messenger accounts.
Comment 3 Xavier Claessens 2011-10-27 13:45:35 UTC
Created attachment 200104 [details] [review]
Add Windows Live Messenger OAuth2
Comment 4 Xavier Claessens 2011-10-27 13:48:01 UTC
Now this is working for Windows Live, when creating an account. But when I need to renew the credential, it makes a deadlock in soup... dunno what's happening.
Comment 5 Xavier Claessens 2011-11-02 12:40:37 UTC
Note the code my patch set into #if 0, uncommenting that part will lead to deadlock too.
Comment 6 Xavier Claessens 2011-11-02 12:58:32 UTC
Created attachment 200497 [details]
backtrace when goa is in deadlock

This happens when removing the #if0 from the patch, and create a messenger account.
Comment 7 David Zeuthen (not reading bugmail) 2011-11-02 13:07:13 UTC
What makes you think this is a deadlock?

AFAICT libsoup is just waiting to receive a reply in thread 2.... my guess is that you are using libsoup incorrectly by reusing the SoupSession from webkit_get_default_session... e.g. with this SoupSession libsoup implements synchronous behavior by sitting waiting in a mainloop (that never runs) instead of calling the blocking thread.
Comment 8 Xavier Claessens 2011-11-02 13:49:01 UTC
Created attachment 200506 [details] [review]
Add Windows Live Messenger OAuth2
Comment 9 Xavier Claessens 2011-11-02 13:50:05 UTC
Ok, rewrote using librest, and it seems to work now. Just waiting 2100s for the access token to expire here, and test the refresh path.

A review would be welcome :)
Comment 10 Xavier Claessens 2011-11-02 14:14:16 UTC
ok, manually changed the min time before refreshing, and it worked. So I think this is a all fine and ready to review :)
Comment 11 Xavier Claessens 2011-11-03 13:53:13 UTC
And also added Windows Live icon. Branch: http://cgit.collabora.com/git/user/xclaesse/gnome-online-accounts.git/log/?h=windows-live
Comment 12 Elan Ruusamäe 2011-11-21 07:29:38 UTC
+  gchar *email = NULL;
+  gchar *ret = NULL;
...
+  ret = email;
+  email = NULL;
+  if (out_presentation_identity != NULL)
+    *out_presentation_identity = g_strdup (ret);
+
+ out:
+  if (call != NULL)
+    g_object_unref (call);
+  if (proxy != NULL)
+    g_object_unref (proxy);
+  g_free (email);
+
+  return ret;

sorry, isn't this code returning unreferenced (free()-d) memory?
as ret points to same memory that email pointed to ...
Comment 13 Xavier Claessens 2011-11-21 08:46:54 UTC
email is set to NULL and g_free(NULL) is no-op. This is almost the same code as copied from goafacebookprovider.c::get_identity_sync() with s/email/id/
Comment 14 Xavier Claessens 2011-12-15 13:47:32 UTC
I have an alternative patch that reduce code duplication:
http://cgit.collabora.com/git/user/xclaesse/gnome-online-accounts.git/log/?h=windows-live2
Comment 15 Xavier Claessens 2011-12-16 11:18:58 UTC
Created attachment 203648 [details] [review]
Add Windows Live oauth2
Comment 16 Xavier Claessens 2011-12-16 11:28:43 UTC
Created attachment 203649 [details] [review]
Provide icon for windows-live service

PNGs copied from Empathy
Comment 17 Xavier Claessens 2011-12-16 11:39:13 UTC
So the good news is we actually don't need an app secret key for MSN! A MS developer told me that using https://oauth.live.com/desktop as redirect URI make it work without a secret key, and is the recommended way for any client-side auth.

I think we should still make the client ID a configure option, like that distro can tweak the app name and logo :)

The other good news is that we don't have to special case the GET/POST for MSN because facebook actually accepts with GET too.
Comment 18 Xavier Claessens 2011-12-16 11:46:30 UTC
Created attachment 203654 [details] [review]
Enable windows-live by default

It does not need a secret app key, so we can build by default with my
own client-id.
Comment 19 David Zeuthen (not reading bugmail) 2011-12-16 14:49:35 UTC
Review of attachment 203648 [details] [review]:

Generally looks great, but how about calling it GoaLiveProvider instead? I mean, we're likely to make it do more than chat in the future

::: src/goabackend/goaoauth2provider.c
@@ +409,3 @@
+  if (client_secret != NULL)
+    rest_proxy_call_add_param (call, "client_secret", client_secret);
+  rest_proxy_call_add_param (call, "redirect_uri", goa_oauth2_provider_get_redirect_uri (provider));

This would break other OAuth2 providers, no?

I think we should probably use quirks here. Suggest to add a GoaOAuth2Quirk flag enumeration and a vfunc get_quirks() that returns a set of flags. In this case the quirk would be something like GOA_OAUTH2_QUIRK_USE_GET_METHOD_FOR_GET_ACCESS_TOKEN and only the Live backend would return this one.
Comment 20 Xavier Claessens 2011-12-19 14:17:17 UTC
(In reply to comment #19)
> Review of attachment 203648 [details] [review]:
> 
> Generally looks great, but how about calling it GoaLiveProvider instead? I
> mean, we're likely to make it do more than chat in the future

Good point. "Live" could be a bit too generic and meaningless term though. What about "GoaWindowsLiveProvider" which is the name I've given internally?

> ::: src/goabackend/goaoauth2provider.c
> @@ +409,3 @@
> +  if (client_secret != NULL)
> +    rest_proxy_call_add_param (call, "client_secret", client_secret);
> +  rest_proxy_call_add_param (call, "redirect_uri",
> goa_oauth2_provider_get_redirect_uri (provider));
> 
> This would break other OAuth2 providers, no?

Actually not, facebook if happy for that too. I'm wondering if that POST stuff is old revision of oauth2, or something...
Comment 21 David Zeuthen (not reading bugmail) 2011-12-19 14:20:18 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > Review of attachment 203648 [details] [review] [details]:
> > 
> > Generally looks great, but how about calling it GoaLiveProvider instead? I
> > mean, we're likely to make it do more than chat in the future
> 
> Good point. "Live" could be a bit too generic and meaningless term though. What
> about "GoaWindowsLiveProvider" which is the name I've given internally?

Sounds good to me.

> > ::: src/goabackend/goaoauth2provider.c
> > @@ +409,3 @@
> > +  if (client_secret != NULL)
> > +    rest_proxy_call_add_param (call, "client_secret", client_secret);
> > +  rest_proxy_call_add_param (call, "redirect_uri",
> > goa_oauth2_provider_get_redirect_uri (provider));
> > 
> > This would break other OAuth2 providers, no?
> 
> Actually not, facebook if happy for that too. I'm wondering if that POST stuff
> is old revision of oauth2, or something...

OK, let's just go with that change then since WindowsLive will be the first provider to use OAuth2. We can always fix up stuff when we look into enabling other providers.

Looks OK to just commit all patches with the name change then. Thanks!
Comment 22 David Zeuthen (not reading bugmail) 2011-12-19 15:17:26 UTC
OK, this is all in git now. Closing. Thanks!