GNOME Bugzilla – Bug 652544
Add X-FACEBOOK-PLATFORM and X-MESSENGER-OAUTH2 support
Last modified: 2011-11-04 09:10:54 UTC
The empathy-auth-client should gain support for X-FACEBOOK-PLATFORM to enable single sign one integration with GNOME Online account.
We could re-use code from http://cgit.collabora.com/git/meego-facebook-plugins.git/tree/auth-observer
See also https://bugs.freedesktop.org/show_bug.cgi?id=37376
Not needed for 3.2 as GOA will only support Google accounts.
Blocked by bug #661064 and bug #661062. See WIP branch: http://cgit.collabora.com/git/user/xclaesse/empathy.git/log/?h=auth
+ 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.
right; I should probably rename to windows-live.
Branch updated: http://cgit.collabora.com/git/user/xclaesse/empathy.git/log/?h=auth
*** Bug 661068 has been marked as a duplicate of this bug. ***
Created attachment 200108 [details] [review] Import Facebook and windows Live GOA accounts Implement their auth mechanisms Fixes bug #661068 and #652544
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.
(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
(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.
Branch updated, I've moved all the GOA stuff into empathy-goa-auth-handler.c
Created attachment 200666 [details] [review] Import Facebook and windows Live GOA accounts Implement their auth mechanisms Fixes bug #661068 and #652544
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.
make check passes, changed finalize en dispose. Merged. Thanks!