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 672060 - Add Facebook client side auth flow
Add Facebook client side auth flow
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: 672066
Blocks:
 
 
Reported: 2012-03-14 14:21 UTC by Cosimo Alfarano
Modified: 2012-03-15 15:54 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Cosimo Alfarano 2012-03-14 14:21:07 UTC
I have a branch which updates the current facebook provider auth flow from the server-side to the client-side flow one.

This means that it does not requires secrets anymore, just the client-id which is any facebook registered app.

Facebook deprecated (from May 2012 it will have no effect) offline_use scope, with a new token expire system. The token will live 60days and will be automatically refreshed by FB (if the FB app enabled the token migration).

There is an endpoint to obtain a new auto-refreshing token from an old one, but I decided it's not worth to implement it since the FB provider is not extensively used so far and the client side auth is actually new. In theory no one should have an "old style" token, since there wasn't such thing for GOA before.

A related empathy bug should enable empathy to refresh/re-ask for user interaction when the token expires:
https://bugzilla.gnome.org/show_bug.cgi?id=672050

This should be enough to make it work (right? :)

I currently use my own client-id for it. I'd be glad to add GOA maintainers or who else want to be admin to the app, since it's required I think it's better to have a suggested/default one.
Its client-id is in the configure.ac and is used if nothing else is specified instead.
Comment 2 Xavier Claessens 2012-03-14 14:44:02 UTC
As a side note, why is facebook asking for "user_events" and "read_mailbox" scopes? I don't think we need them, and IMO it's wrong to ask more access than we need. A careful user should reject the app it ask for permissions it does not need.
Comment 3 Xavier Claessens 2012-03-14 15:36:04 UTC
Also, in app settings the email address is Cosimo's. I think that should be changed to a @gnome.org, so I opened bug #672066.
Comment 4 Xavier Claessens 2012-03-14 15:39:43 UTC
I've reviewed Cosimo's code and it looks fine to me.
Comment 5 Debarshi Ray 2012-03-15 01:06:53 UTC
(In reply to comment #1)
> Please review:
> 
> http://cgit.collabora.com/git/user/kalfa/gnome-online-accounts.git/log/?h=facebook_client_auth

First of all, thanks for taking time to do this. It is great that you noticed that there is a separate client-side flow.


In src/goabackend/goaoauth2provider.c, something like this is needed, else it gives criticals when there is no authorization code:

@@ -1026,7 +1026,8 @@ goa_oauth2_provider_add_account (GoaProvider *_provider,
                                                       data.account_object_path));
 
   g_variant_builder_init (&builder, G_VARIANT_TYPE_VARDICT);
-  g_variant_builder_add (&builder, "{sv}", "authorization_code", g_variant_new_string (authorization_code));
+  if (authorization_code != NULL)
+    g_variant_builder_add (&builder, "{sv}", "authorization_code", g_variant_new_string (authorization_code));
   g_variant_builder_add (&builder, "{sv}", "access_token", g_variant_new_string (access_token));
   if (access_token_expires_in > 0)
     g_variant_builder_add (&builder, "{sv}", "access_token_expires_at",
@@ -1130,7 +1131,8 @@ goa_oauth2_provider_refresh_account (GoaProvider  *_provider,
     }
 
   g_variant_builder_init (&builder, G_VARIANT_TYPE_VARDICT);
-  g_variant_builder_add (&builder, "{sv}", "authorization_code", g_variant_new_string (authorization_code));
+  if (authorization_code != NULL)
+    g_variant_builder_add (&builder, "{sv}", "authorization_code", g_variant_new_string (authorization_code));
   g_variant_builder_add (&builder, "{sv}", "access_token", g_variant_new_string (access_token));
   if (access_token_expires_in > 0)
     g_variant_builder_add (&builder, "{sv}", "access_token_expires_at",

Also, do we really need to store the authorization code? Isn't the access token enough in both the flows?


Secondly, why did you change the redirect URI to:
https://www.facebook.com/connect/login_success.html


Third, when you registered the application in Facebook's Developer App, did you accept any Terms of Service? This is important because if you did then that needs to be reviewed by a lawyer.


I simplified this to:

@@ -735,24 +800,38 @@ get_tokens_and_identity (GoaOAuth2Provider *provider,
gtk_widget_hide (GTK_WIDGET (dialog));
- /* OK, we now have the authorization code... now we need to get the
- * email address (to e.g. check if the account already exists on
- * @client).. for that we need to get a (short-lived) access token
- * and a refresh_token
- */
+ if (data.authorization_code != NULL)
+ {
+ /* OK, we now have the authorization code... now we need to get the
+ * email address (to e.g. check if the account already exists on
+ * @client).. for that we need to get a (short-lived) access token
+ * and a refresh_token
+ */
- /* TODO: run in worker thread */
- data.access_token = get_tokens_sync (provider,
- data.authorization_code,
- NULL, /* refresh_token */
- &data.refresh_token,
- &data.access_token_expires_in,
- NULL, /* GCancellable */
- &data.error);
- if (data.access_token == NULL)
+ /* TODO: run in worker thread */
+ data.access_token = get_tokens_sync (provider,
+ data.authorization_code,
+ NULL, /* refresh_token */
+ &data.refresh_token,
+ &data.access_token_expires_in,
+ NULL, /* GCancellable */
+ &data.error);
+ if (data.access_token == NULL)
+ {
+ g_prefix_error (&data.error, _("Error getting an Access Token: "));
+ goto out;
+ }
+ }
+ else if (data.access_token != NULL)
{
- g_prefix_error (&data.error, _("Error getting an Access Token: "));
- goto out;
+ /* we actually already have the access token, wow!
+ * one step less since we are following the client side flow */
+ }
+ else
+ {
+ /* we need to have at least auth code or access token. Something went
+ * wrong otherwise and we are the cause */
+ g_assert_not_reached ();
}

to a single "if" check for the authorization_code. If it is there we fetch the access_token. After that simply assert the presence of the access_token. No need for the "else if" and "else".


There were some other nitpicks, like unnecessary white space changes, and stuff which should be split out into separate patches. But I have fixed those myself, and will attribute them to you.
Comment 6 Debarshi Ray 2012-03-15 01:40:48 UTC
Interestingly, the example given in https://developers.facebook.com/docs/authentication/ seems to indicate that the response from https://graph.facebook.com/me contains "userName", but in reality it does not. Atleast not for those, like me, who log into Facebook using an email address. That is why you made the change from userName to email, right?

However, I am wondering what happens for those who log into FB using an username. FB does allow you to log in using an username other than an email too, right?
Comment 7 Xavier Claessens 2012-03-15 09:02:10 UTC
(In reply to comment #5)
> In src/goabackend/goaoauth2provider.c, something like this is needed, else it
> gives criticals when there is no authorization code:

good point. Since I see you did the change already, you could attach the diff as proper patch :)

> Also, do we really need to store the authorization code? Isn't the access token
> enough in both the flows?

I don't think it is needed, but this is not the scope of this bug. Maybe you should open another bug about this?

> Secondly, why did you change the redirect URI to:
> https://www.facebook.com/connect/login_success.html

See "Desktop apps" section near the end of https://developers.facebook.com/docs/authentication.

> Third, when you registered the application in Facebook's Developer App, did you
> accept any Terms of Service? This is important because if you did then that
> needs to be reviewed by a lawyer.

Do we really have lawyers? How many years will they take to reply with a probability of being wrong of 50% anyway? Tbh I would just not bother, do our best to follow the rules (what we do by strictly following their doc) and in the worst case facebook can contact us and/or disable the app.

> I simplified this to:
(...)
> to a single "if" check for the authorization_code. If it is there we fetch the
> access_token. After that simply assert the presence of the access_token. No
> need for the "else if" and "else".

I agree, that's cleaner that way.
Comment 8 Xavier Claessens 2012-03-15 09:04:29 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > Third, when you registered the application in Facebook's Developer App, did you
> > accept any Terms of Service? This is important because if you did then that
> > needs to be reviewed by a lawyer.
> 
> Do we really have lawyers? How many years will they take to reply with a
> probability of being wrong of 50% anyway? Tbh I would just not bother, do our
> best to follow the rules (what we do by strictly following their doc) and in
> the worst case facebook can contact us and/or disable the app.

AFAIK there are here: https://developers.facebook.com/policy/
Comment 9 Cosimo Alfarano 2012-03-15 10:55:45 UTC
(In reply to comment #6)
> However, I am wondering what happens for those who log into FB using an
> username. FB does allow you to log in using an username other than an email
> too, right?

True, didn't think about it, well spotted. 

Although, since I requested the email in the scope it will be present anyway, isn't it?
The idea is having a single/homogeneous way to identify the account.

Unless there is a reason to not use the email as presentation identity, I'd keep it, since it's an always present info.
Facebook won't allow anyone (eg not even me :) to create a new account with an email I already stated as mine in another account.
Comment 10 Cosimo Alfarano 2012-03-15 11:12:07 UTC
(In reply to comment #5)
> In src/goabackend/goaoauth2provider.c, something like this is needed, else it
> gives criticals when there is no authorization code:
> 
> @@ -1026,7 +1026,8 @@ goa_oauth2_provider_add_account (GoaProvider *_provider,

As Xavier said, can you format it to a patch and add it to this bug, please?

> Also, do we really need to store the authorization code? Isn't the access token
> enough in both the flows?

The auth token is used to form a request for refreshing the access token, IIRC, so it's needed by providers who actually use a "refresh endpoint". Not facebook case.

If you're referring to the code above, I kept the oauth2provider as closer as it was before to avoid breaking other flows. Storing it *when present* (thanks for the patch) is actually needed, AFAIK.

> Third, when you registered the application in Facebook's Developer App, did you
> accept any Terms of Service? This is important because if you did then that
> needs to be reviewed by a lawyer.

I probably did (almost surely, given the platform), I reused an app I already had, so I don't remember what it actually was.

> There were some other nitpicks, like unnecessary white space changes, and stuff
> which should be split out into separate patches. But I have fixed those myself,
> and will attribute them to you.

Thanks.
Comment 11 Cosimo Alfarano 2012-03-15 11:17:51 UTC
On a second though about the need of storing the auth code, if the refresh token is present, it should not be necessary.
I'm not sure if it's going to be used in some other circumstances.

I agree it's out of the scope of this bug, though :)
Comment 12 Cosimo Alfarano 2012-03-15 11:22:45 UTC
BTW, Xavier's questions in Comment 2 were actually question to "who in the know".
I didn't change anything in the facebook provider which didn't break the client flow, but I agree that we should ask for as less tokens as we need.

One reason I tried to give to it was that requesting now all the tokens we "potentially" need allows the user to have to re-authorize the application when we'll add "Email" or other functionalities more than "Chat". But I don't feel this reason as strong enough.
Comment 13 Cosimo Alfarano 2012-03-15 11:24:12 UTC
Out of clarity, I meant "allows the user to NOT have to re-authorize the application"
Comment 14 Debarshi Ray 2012-03-15 13:20:52 UTC
(In reply to comment #9)
> (In reply to comment #6)
> > However, I am wondering what happens for those who log into FB using an
> > username. FB does allow you to log in using an username other than an email
> > too, right?
> 
> True, didn't think about it, well spotted. 
> 
> Although, since I requested the email in the scope it will be present anyway,
> isn't it?
> The idea is having a single/homogeneous way to identify the account.
> 
> Unless there is a reason to not use the email as presentation identity, I'd
> keep it, since it's an always present info.
> Facebook won't allow anyone (eg not even me :) to create a new account with an
> email I already stated as mine in another account.

It does look like one can not create a new account without specifying an email address, so this should be ok.
Comment 15 Debarshi Ray 2012-03-15 13:41:51 UTC
>> Secondly, why did you change the redirect URI to:
>> https://www.facebook.com/connect/login_success.html
> 
> See "Desktop apps" section near the end of
> https://developers.facebook.com/docs/authentication.

Yes, I read that part. It is to help desktop application developers who do not have their own domain. But GNOME does. So can't we pick up the access_token if it redirected us to https://www.gnome.org/goa-1.0/oauth2#access_token=... ?
Comment 16 Bastien Nocera 2012-03-15 14:05:16 UTC
A couple of things:
- Yes, we do have lawyers, they're pro-bono so pay-for work gets in the way.
- Those lawyers prioritised services that didn't require secret keys, as those were the ones we could most easily enable
- I've passed on the information about the Facebook policies and the new login mechanism to board@, feel free to answer my mail
- If Debarshi is ready to commit your code, he can commit your code (in the branch scheduled for gnome 3.6 obviously), just make sure that Facebook is kept disabled by default for now.
- It would be best if the public key was under the control of the Foundation, with access given to the developers too, just not stuck with one particular personal account.

Looking forward to Facebook integration on by default!
Comment 17 Xavier Claessens 2012-03-15 14:17:21 UTC
(In reply to comment #16)
> A couple of things:
> - Those lawyers prioritised services that didn't require secret keys, as those
> were the ones we could most easily enable

For windows live and facebook they have explicitly a flow that does not require the app secret key.

> - I've passed on the information about the Facebook policies and the new login
> mechanism to board@, feel free to answer my mail

I'm not on that ML, but ok.

> - If Debarshi is ready to commit your code, he can commit your code (in the
> branch scheduled for gnome 3.6 obviously), just make sure that Facebook is kept
> disabled by default for now.

Why not 3.4? We are not in code freeze afaik, are we? Enabling by default could be considered a feature-freeze violation though, I agree.

> - It would be best if the public key was under the control of the Foundation,
> with access given to the developers too, just not stuck with one particular
> personal account.

Cosimo and myself are admin of that app, we can add anyone with a facebook account afaik. Just tell me who to add ;-)


Note that windowslive service surely has similar terms and we merged and enabled by default... I think we really are looking way too far here... But as you wish...
Comment 18 Cosimo Alfarano 2012-03-15 14:20:35 UTC
There is no redirection going on, since it's a client-side flow.
The information is just for us to parse and can be under any domain.

My choice went to facebook.com since it is self-contained, which is nice since there is no other interaction with the rest of the world anyway (just US <-> facebook), independently from any redirect URL we use.
Using it in the code we don't give the idea that there is any (read: enforces the "we don't use any of your data" policy. Whatever happens is facebook's side)

I doubt that the URL has to exist too (maybe a check is done when it's set up in the app panel), so if philosophically you prefer to use a gnome.org URL, it's absolutely OK for me :)
Comment 19 Bastien Nocera 2012-03-15 14:34:25 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > A couple of things:
> > - Those lawyers prioritised services that didn't require secret keys, as those
> > were the ones we could most easily enable
> 
> For windows live and facebook they have explicitly a flow that does not require
> the app secret key.

That wasn't the case before...

> > - I've passed on the information about the Facebook policies and the new login
> > mechanism to board@, feel free to answer my mail
> 
> I'm not on that ML, but ok.

That's because only Board members are. I CC:ed Cosimo, Debarshi and yourself on the mail.

> > - If Debarshi is ready to commit your code, he can commit your code (in the
> > branch scheduled for gnome 3.6 obviously), just make sure that Facebook is kept
> > disabled by default for now.
> 
> Why not 3.4? We are not in code freeze afaik, are we? Enabling by default could
> be considered a feature-freeze violation though, I agree.

String freeze, at the very least. And hard code freeze is Monday.

> > - It would be best if the public key was under the control of the Foundation,
> > with access given to the developers too, just not stuck with one particular
> > personal account.
> 
> Cosimo and myself are admin of that app, we can add anyone with a facebook
> account afaik. Just tell me who to add ;-)

Right. I would like it if there was an entity added, rather than individual persons.

> Note that windowslive service surely has similar terms and we merged and
> enabled by default... I think we really are looking way too far here... But as
> you wish...

The API key requirements are similar, I doubt that the terms of service match exactly.

Anyway, carry on the conversation through mail, and we'll see.
Comment 20 Debarshi Ray 2012-03-15 15:13:08 UTC
Ok. I have got all the patches in their current form in this facebook branch:
http://git.gnome.org/browse/gnome-online-accounts/log/?h=facebook

Before merging this, I would like to wait for a decision on the redirect_uri.
Comment 21 Xavier Claessens 2012-03-15 15:16:51 UTC
(In reply to comment #20)
> Before merging this, I would like to wait for a decision on the redirect_uri.

Use what make you the happier. I have no opinion :)

Note that indeed the patches changes a few error message strings. I would be sad to delay this for 6months just because of stupid error message strings. So what about changing it to be disabled by default in configure.ac (until lawyers give green light) and request a string freeze exception (or even just untranslate those few strings). Note that the other new strings are already used in other files verbatim, so they are not new.
Comment 22 Cosimo Alfarano 2012-03-15 15:31:42 UTC
Thanks Ray for splitting and formatting everything for me.

I'm just a GNOME contributor, not a developer, so I won't push for any choice about the redirect_url (I made my choice implicitly in my code, though :)

Either way is OK for me, since technically it has no impact.


Also, out of clarity: since I'm the creator of the current FB app, I give authorization to any GNOME Foundation member to remove me from the app (since I'm not a member) and take ownership of the Applicaiton itself.
Xavier, as GNOME developer is currently an admin with me.
Comment 23 Debarshi Ray 2012-03-15 15:37:06 UTC
(In reply to comment #18)
> There is no redirection going on, since it's a client-side flow.
> The information is just for us to parse and can be under any domain.

Well, if you really want to check, you can comment out the bits where we ignore the redirection request and emit the OK response. You will see a blank page with "Success" show up. ;-)

> My choice went to facebook.com since it is self-contained, which is nice since
> there is no other interaction with the rest of the world anyway (just US <->
> facebook), independently from any redirect URL we use.
> Using it in the code we don't give the idea that there is any (read: enforces
> the "we don't use any of your data" policy. Whatever happens is facebook's
> side)

I don't really see the logic behind this, but that is ok. This is not terribly important.

> I doubt that the URL has to exist too

It doesn't. Which is why http://www.gnome.org/goa-1.0/oauth2 and http://www.gnome.org/goa-1.0/oauth do not exist. :-)

> in the app panel), so if philosophically you prefer to use a gnome.org URL,
> it's absolutely OK for me :)

Since you changed the redirect URI to something else than what it was before, without any obvious benefits, I think it was a valid question for me to ask.
Comment 24 Debarshi Ray 2012-03-15 15:38:48 UTC
(In reply to comment #21)
> (In reply to comment #20)
> > Before merging this, I would like to wait for a decision on the redirect_uri.
> 
> Use what make you the happier. I have no opinion :)
> 
> Note that indeed the patches changes a few error message strings. I would be
> sad to delay this for 6months just because of stupid error message strings. So
> what about changing it to be disabled by default in configure.ac (until lawyers
> give green light) and request a string freeze exception (or even just
> untranslate those few strings). Note that the other new strings are already
> used in other files verbatim, so they are not new.

I am going to merge this into master now. There is a separate branch for gnome-3-4. Feel free to pick the patches in whichever form you feel like to the gnome-3-4 branch.
Comment 25 Debarshi Ray 2012-03-15 15:54:54 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.