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 652544 - Add X-FACEBOOK-PLATFORM and X-MESSENGER-OAUTH2 support
Add X-FACEBOOK-PLATFORM and X-MESSENGER-OAUTH2 support
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: Auth client
2.33.x
Other Linux
: Normal enhancement
: 3.4
Assigned To: empathy-maint
: 661068 (view as bug list)
Depends on:
Blocks: 661062 661064
 
 
Reported: 2011-06-14 11:33 UTC by Guillaume Desmottes
Modified: 2011-11-04 09:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Import Facebook and windows Live GOA accounts (17.00 KB, patch)
2011-10-27 14:42 UTC, Guillaume Desmottes
reviewed Details | Review
Import Facebook and windows Live GOA accounts (26.63 KB, patch)
2011-11-04 08:28 UTC, Guillaume Desmottes
reviewed Details | Review

Description Guillaume Desmottes 2011-06-14 11:33:15 UTC
The empathy-auth-client should gain support for X-FACEBOOK-PLATFORM to enable single sign one integration with GNOME Online account.
Comment 1 Guillaume Desmottes 2011-06-14 11:37:39 UTC
We could re-use code from http://cgit.collabora.com/git/meego-facebook-plugins.git/tree/auth-observer
Comment 2 Guillaume Desmottes 2011-06-14 11:42:26 UTC
See also https://bugs.freedesktop.org/show_bug.cgi?id=37376
Comment 3 Guillaume Desmottes 2011-09-06 07:35:47 UTC
Not needed for 3.2 as GOA will only support Google accounts.
Comment 4 Xavier Claessens 2011-10-06 09:54:34 UTC
Blocked by bug #661064 and bug #661062.

See WIP branch: http://cgit.collabora.com/git/user/xclaesse/empathy.git/log/?h=auth
Comment 5 Guillaume Desmottes 2011-10-08 20:23:56 UTC
+      PARAM ("Service", "messenger");

Is messenger a proper name of this?  live-messenger maybe?

I didn't review the top commit yet as it contains some FIXME. We should deal with the credentials being expired as well.
Comment 6 Xavier Claessens 2011-10-18 07:46:23 UTC
right; I should probably rename to windows-live.
Comment 7 Xavier Claessens 2011-10-27 13:42:57 UTC
Branch updated:
http://cgit.collabora.com/git/user/xclaesse/empathy.git/log/?h=auth
Comment 8 Guillaume Desmottes 2011-10-27 14:29:08 UTC
*** Bug 661068 has been marked as a duplicate of this bug. ***
Comment 9 Guillaume Desmottes 2011-10-27 14:42:33 UTC
Created attachment 200108 [details] [review]
Import Facebook and windows Live GOA accounts

Implement their auth mechanisms

Fixes bug #661068 and #652544
Comment 10 Guillaume Desmottes 2011-10-27 15:02:42 UTC
Review of attachment 200108 [details] [review]:

I don't like the structur of this code. You added loads of logic in the factory, making it much more than just a factory. Ideally most of this should in empathy-server-sasl-handler. For example you connect to the SASLStatusChanged signal but we already have code in the handler doing that.

Most of this code could also use more DEBUG(). Debugging such code from logs can be a pain without proper debug.

::: configure.ac
@@ +48,3 @@
 TELEPATHY_LOGGER=0.2.10
 WEBKIT_REQUIRED=1.3.13
+GOA_REQUIRED=3.3.0

It's not 3.3.1?

@@ +197,3 @@
    webkitgtk-3.0 >= $WEBKIT_REQUIRED
+   libsoup-2.4
+   goa-1.0 >= $GOA_REQUIRED

configure.ac can be simplified if goa is turned to a hard dep (which is fine I think).

::: goa-mc-plugin/mcp-account-manager-goa.c
@@ +141,3 @@
+      PARAM ("protocol", "jabber");
+      PARAM ("Icon", "im-msn");
+      PARAM ("param-account", "chat.facebook.com");

You should add "windows-live" to http://telepathy.freedesktop.org/spec/Account.html#Property:Service

::: libempathy/empathy-auth-factory.c
@@ +37,3 @@
 #include "extensions/extensions.h"
 
+static const gchar *known_goa_services[] = {

Name is kinda miss leading, it doesn't include gtalk.

@@ +56,3 @@
+  GoaClient *goa_client;
+  gboolean goa_client_preparing;
+  GList *goa_queue;

Describe the content and semantic of the list.

@@ +523,3 @@
+  ObserveChannelsData *data = user_data;
+  const gchar *service;
+    const GArray *challenge,

Isn't 'data' leaked at this point?

@@ +591,3 @@
+      return;
+    }
+  tp_cli_channel_interface_sasl_authentication_call_respond (data->channel, -1,

add DEBUG

@@ +626,3 @@
+      return;
+    }
+

add DEBUG.

@@ +654,3 @@
+      if (!tp_strdiff (goa_account_get_id (goa_account), id))
+        {
+  if (!tp_strdiff (service, "facebook"))

I'd add a DEBUG() here.

@@ +739,3 @@
+  provider = tp_account_get_storage_provider (account);
+  service = tp_account_get_service (account);
+  if (!tp_strdiff (provider, "org.gnome.OnlineAccounts") &&

libgoa doesn't have a #define for this string?

@@ +741,3 @@
+  if (!tp_strdiff (provider, "org.gnome.OnlineAccounts") &&
+      tp_strv_contains (known_goa_services, service))
+    {

I'd add a some DEBUG in this function. Would be useful when debugging from logs.
Comment 11 Xavier Claessens 2011-11-02 16:44:05 UTC
(In reply to comment #10)
> Review of attachment 200108 [details] [review]:
> 
> I don't like the structur of this code. You added loads of logic in the
> factory, making it much more than just a factory. Ideally most of this should
> in empathy-server-sasl-handler. For example you connect to the
> SASLStatusChanged signal but we already have code in the handler doing that.

I think file structure is totally broken, it has nothing to do inside libempathy, we should split all this in a separate directory for each client. Wondering why that files is named "factory" too...

Not sure how to make this proper tbh, all the code before the Claim() fallback to try password path so can't really be moved to empathy-sasl-handler. Or should it claim the channel right away and fail auth if something goes wrong instead of fallback to password?

> Most of this code could also use more DEBUG(). Debugging such code from logs
> can be a pain without proper debug.

Added them.

> ::: configure.ac
> @@ +48,3 @@
>  TELEPATHY_LOGGER=0.2.10
>  WEBKIT_REQUIRED=1.3.13
> +GOA_REQUIRED=3.3.0
> 
> It's not 3.3.1?
> 
> @@ +197,3 @@
>     webkitgtk-3.0 >= $WEBKIT_REQUIRED
> +   libsoup-2.4
> +   goa-1.0 >= $GOA_REQUIRED
> 
> configure.ac can be simplified if goa is turned to a hard dep (which is fine I
> think).

For the MC plugin, there is also mission-control-plugins to be checked, which is not harddep atm. Also having separate pkg-config check make the plugin not link on all LIBEMPATHY flags.

> ::: goa-mc-plugin/mcp-account-manager-goa.c
> @@ +141,3 @@
> +      PARAM ("protocol", "jabber");
> +      PARAM ("Icon", "im-msn");
> +      PARAM ("param-account", "chat.facebook.com");
> 
> You should add "windows-live" to
> http://telepathy.freedesktop.org/spec/Account.html#Property:Service

done.

> ::: libempathy/empathy-auth-factory.c
> @@ +37,3 @@
>  #include "extensions/extensions.h"
> 
> +static const gchar *known_goa_services[] = {
> 
> Name is kinda miss leading, it doesn't include gtalk.

Because we can't connect gtalk with goa access token yet, so it uses traditional password auth. Do you have better name to suggest?

> 
> @@ +56,3 @@
> +  GoaClient *goa_client;
> +  gboolean goa_client_preparing;
> +  GList *goa_queue;
> 
> Describe the content and semantic of the list.

done

> @@ +523,3 @@
> +  ObserveChannelsData *data = user_data;
> +  const gchar *service;
> +    const GArray *challenge,
> 
> Isn't 'data' leaked at this point?

right, fixed.

> @@ +591,3 @@
> +      return;
> +    }
> +  tp_cli_channel_interface_sasl_authentication_call_respond (data->channel,
> -1,
> 
> add DEBUG

done

> @@ +626,3 @@
> +      return;
> +    }
> +
> 
> add DEBUG.

done


> @@ +654,3 @@
> +      if (!tp_strdiff (goa_account_get_id (goa_account), id))
> +        {
> +  if (!tp_strdiff (service, "facebook"))
> 
> I'd add a DEBUG() here.

done

> @@ +739,3 @@
> +  provider = tp_account_get_storage_provider (account);
> +  service = tp_account_get_service (account);
> +  if (!tp_strdiff (provider, "org.gnome.OnlineAccounts") &&
> 
> libgoa doesn't have a #define for this string?

It is defined in empathy's MC plugin actually, it is not GOA who create TpAccounts. I guess to share a #define, it must be done in configure.ac via config.h, no?

> @@ +741,3 @@
> +  if (!tp_strdiff (provider, "org.gnome.OnlineAccounts") &&
> +      tp_strv_contains (known_goa_services, service))
> +    {
> 
> I'd add a some DEBUG in this function. Would be useful when debugging from
> logs.

 done
Comment 12 Guillaume Desmottes 2011-11-03 08:49:36 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > Review of attachment 200108 [details] [review] [details]:
> > 
> > I don't like the structur of this code. You added loads of logic in the
> > factory, making it much more than just a factory. Ideally most of this should
> > in empathy-server-sasl-handler. For example you connect to the
> > SASLStatusChanged signal but we already have code in the handler doing that.
> 
> I think file structure is totally broken, it has nothing to do inside
> libempathy, we should split all this in a separate directory for each client.
> Wondering why that files is named "factory" too...

Well maybe but that's not really the point. I'm talking about the logic being
splitted in different files; not the place where those files live.

> Not sure how to make this proper tbh, all the code before the Claim() fallback
> to try password path so can't really be moved to empathy-sasl-handler. Or
> should it claim the channel right away and fail auth if something goes wrong
> instead of fallback to password?

I think fallbacking is fine. Most of the existing handling logic is in
sasl-handler so the token one should be there too.

> > ::: libempathy/empathy-auth-factory.c
> > @@ +37,3 @@
> >  #include "extensions/extensions.h"
> > 
> > +static const gchar *known_goa_services[] = {
> > 
> > Name is kinda miss leading, it doesn't include gtalk.
> 
> Because we can't connect gtalk with goa access token yet, so it uses
> traditional password auth. Do you have better name to suggest?

oauth_services?

> > @@ +739,3 @@
> > +  provider = tp_account_get_storage_provider (account);
> > +  service = tp_account_get_service (account);
> > +  if (!tp_strdiff (provider, "org.gnome.OnlineAccounts") &&
> > 
> > libgoa doesn't have a #define for this string?
> 
> It is defined in empathy's MC plugin actually, it is not GOA who create
> TpAccounts. I guess to share a #define, it must be done in configure.ac via
> config.h, no?

Yeah or in some shared header file. config.h is mostly for configure options flags.
Comment 13 Xavier Claessens 2011-11-03 13:01:21 UTC
Branch updated, I've moved all the GOA stuff into empathy-goa-auth-handler.c
Comment 14 Guillaume Desmottes 2011-11-04 08:28:26 UTC
Created attachment 200666 [details] [review]
Import Facebook and windows Live GOA accounts

Implement their auth mechanisms

Fixes bug #661068 and #652544
Comment 15 Guillaume Desmottes 2011-11-04 08:40:22 UTC
Review of attachment 200666 [details] [review]:

"make check"'s fine?

How is handled token refreshement?

::: libempathy/empathy-goa-auth-handler.c
@@ +59,3 @@
+
+static void
+empathy_goa_auth_handler_finalize (GObject *object)

should be dispose ideally.
Comment 16 Xavier Claessens 2011-11-04 09:10:54 UTC
make check passes, changed finalize en dispose. Merged. Thanks!