Created attachment 198420 [details] [review]
Add Microsoft Live Messenger OAuth2

I get the same crash than bug #660993 when creating messenger accounts.

Created attachment 200104 [details] [review]
Add Windows Live Messenger OAuth2

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.

Note the code my patch set into #if 0, uncommenting that part will lead to deadlock too.

Created attachment 200497 [details]
backtrace when goa is in deadlock

This happens when removing the #if0 from the patch, and create a messenger account.

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.

Created attachment 200506 [details] [review]
Add Windows Live Messenger OAuth2

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 :)

ok, manually changed the min time before refreshing, and it worked. So I think this is a all fine and ready to review :)

And also added Windows Live icon. Branch: http://cgit.collabora.com/git/user/xclaesse/gnome-online-accounts.git/log/?h=windows-live

+  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 ...

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/

I have an alternative patch that reduce code duplication:
http://cgit.collabora.com/git/user/xclaesse/gnome-online-accounts.git/log/?h=windows-live2

Created attachment 203648 [details] [review]
Add Windows Live oauth2

Created attachment 203649 [details] [review]
Provide icon for windows-live service

PNGs copied from Empathy

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.

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.

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.

(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...

(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!

OK, this is all in git now. Closing. Thanks!