883510 - Partner customization should be able to set Facebook OAuth 2 callback url completely

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

+++ This bug was initially created as a clone of Bug #882363 +++

The fix in bug 882363 is unfortunately incomplete, as it does not change the redirect Heroku URLs described in |apps/communications/manifest.webapp|

It essentially forces every registered Facebook app to set the heroku hostname to it's app domain.

Comment 1

[[:fabrice] Fabrice Desré]

I don't think we should let partners use their own redirection url, but just set it to https://www.facebook.com/connect/login_success.html and for now add a redirect rule for this url in the manifest in addition to the heroku one.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Fabrice Desré [:fabrice] (NOT READING BUGMAIL until 06/29) from comment #1)
> I don't think we should let partners use their own redirection url, but just
> set it to https://www.facebook.com/connect/login_success.html and for now
> add a redirect rule for this url in the manifest in addition to the heroku
> one.

That makes sense if we remove the URL from the customizable |parameters.js|.

Comment 3

[[:fabrice] Fabrice Desré]

I agree Tim. But I still don't know how to deal with the situation where we can't move to https://www.facebook.com/connect/login_success.html ourselves since people on 1.0.1 may not get an OTA update. Unless facebook let us specify 2 redirect urls, or if we use a second key with https://www.facebook.com/connect/login_success.html

Comment 4

[Jose Manuel Cantera]

Attachment: Pointer to GH PR

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 763423 [details]
Pointer to GH PR

This is not a proper fix.

Comment 6

[Jose Manuel Cantera]

please could you provide a constructive comment here and explain why in your opinion this is not a proper fix?

thanks

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jose M. Cantera from comment #6)
> please could you provide a constructive comment here and explain why in your
> opinion this is not a proper fix?
> 
> thanks

According to comment 2 and comment 3, the said URL need to be removed from customizable parameter.js generated from build/application-data.js.

(In reply to Fabrice Desré [:fabrice] (NOT READING BUGMAIL until 06/29) from comment #3)
> I agree Tim. But I still don't know how to deal with the situation where we
> can't move to https://www.facebook.com/connect/login_success.html ourselves
> since people on 1.0.1 may not get an OTA update. Unless facebook let us
> specify 2 redirect urls, or if we use a second key with
> https://www.facebook.com/connect/login_success.html

You don't really need to specify this URL. I believe this URL works out of box. The problem is about our abilities to set a specific set of secure redirect URLs.

Comment 8

[Jose Manuel Cantera]

The redirect URL needs to be there because it is needed to build the initial HTTP request made to FB

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jose M. Cantera from comment #8)
> The redirect URL needs to be there because it is needed to build the initial
> HTTP request made to FB

I don't understand. Could you elaborate?

Comment 10

[Jose Manuel Cantera]

The redirect URL needs to be provided in order to check that it matches to the one declared by the app at configuration time

Comment 11

[Francisco Jordano [:arcturus] [:francisco]]

+1 to Jose Manuel.

As well I would like us to take a look to the bigger picture. Now it's facebook, but we will need to do the same for Gmail and Outlook contacts import.

And in some cases, the callback url won't be an url within the service domain. So we need to have that into account, and perhaps reserve a well known partner agnostic (mozilla controlled) end point for this cases.

WDYT?

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Francisco Jordano [:arcturus] from comment #11)
> +1 to Jose Manuel.
> 
> As well I would like us to take a look to the bigger picture. Now it's
> facebook, but we will need to do the same for Gmail and Outlook contacts
> import.
> 
> And in some cases, the callback url won't be an url within the service
> domain. So we need to have that into account, and perhaps reserve a well
> known partner agnostic (mozilla controlled) end point for this cases.
> 
> WDYT?

There is no bigger picture. A packaged app by OAuth 2 definition is a client app, so we should have been using "installed app" or "embedded browser" login flow for certified apps that could utilize mozbrowser iframes. As explained here:

https://developers.google.com/accounts/docs/OAuth2InstalledApp

"The Google OAuth 2.0 endpoint supports applications that are installed on a device (e.g. Mobile, Mac, PC). These applications are distributed to individual machines, and it is assumed that these applications cannot keep secrets."

I know this is far from ideal (and a non-webby) solution, and that was the reason I bought up the discussion on dev-gaia but no one seems to be interested back then.

For this bug, without changing how the login flow, the proper fix is at least comment 1 and comment 2; so far comment 8 and comment 10 doesn't give a clear picture on how that won't work (for Facebook login, specifically).

Maybe James have something to add (about the "bigger picture")? I will surely bring back the discussion on dev-gaia.

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

Hi Tim!

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #12)
> There is no bigger picture. A packaged app by OAuth 2 definition is a client
> app, so we should have been using "installed app" or "embedded browser"
> login flow for certified apps that could utilize mozbrowser iframes. As
> explained here:
> 
> https://developers.google.com/accounts/docs/OAuth2InstalledApp

My point about the big picture was just that, take into account not just google but the problem of oauth authentication in open web apps.

> 
> "The Google OAuth 2.0 endpoint supports applications that are installed on a
> device (e.g. Mobile, Mac, PC). These applications are distributed to
> individual machines, and it is assumed that these applications cannot keep
> secrets."
> 
> I know this is far from ideal (and a non-webby) solution, and that was the
> reason I bought up the discussion on dev-gaia but no one seems to be
> interested back then.

Yep, and that's why the redirection was introduced in gecko, to keep the process as web as possible. Ideal? Maybe not, can we do better?, possible yes, so let's make that happen :)

> 
> For this bug, without changing how the login flow, the proper fix is at
> least comment 1 and comment 2; so far comment 8 and comment 10 doesn't give
> a clear picture on how that won't work (for Facebook login, specifically).

At this point, in contacts we tried to do a standard way of integrating external sources that provide Oauth 2.0 authentication.

Which that said, the use of the callback url in some services is a bit tricky:

- Some of them require a valid (reachable by the service at registration time) url (not the case of FB, but other providers).
- Others don't allow to register two app ids with the same callback url

This point (how to deal with the callback url) is a bit unclear in the specification so each service provider may decide to follow different rules.

What was clear for us when we implemented the Oauth 2.0 support in contacts is that we wanted to support as many services (future services) as possible, so that's why we were so generic.

Coming back to the point of not letting partners to customise the callback URL, basically I think that will be a must (with the current), since the redirection URL will be coded in the application manifest itself.

So what I think that would happen (again, with the current solution) is that we have an unique callback url provided by Moz, and partners will need to register their keys using that url as callback.

Does that make sense for you?

> 
> Maybe James have something to add (about the "bigger picture")? I will
> surely bring back the discussion on dev-gaia.

Definitely, Oauth 2.0 is something that 3rd party devs have been asking for a while and we should try to finde the best solution for everyone.

Thanks a lot!
F.

Comment 14

[James Lal [:lightsofapollo]]

Fabrice already mentioned this but upgrading people from one redirect url to another is fairly trivial by way of creating a second key. I don't know if this is true of facebook in particular but for google adding keys is very simple / easy.

I don't know if I see a greater picture here yet the window.open + redirect flow is very close to what webdev's do now and I expect they will be comfortable with that option in most cases (assuming they know about it!).

The solution I used (prior to redirect stuff existing) was the embedded browser flow (typical for mobile devices) and that worked really well for me and gives the developer much greater control over how the flow is presented and when it ends. 

I think its easy to find use cases for both methods depending on what said 3rd party developer is doing.

---

The other side of this is more of a UX issue- In theory I would like to ask the user less frequently to sign into X oauth service and agree to do X, Y, Z things... That is really easy to argue both ways (no we need security! yes we need less user prompts!).

IIRC- Andriod has a higher level oauth abstraction for google services in particular so there are less prompts.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Francisco, thank you for the detailed response.

(In reply to Francisco Jordano [:arcturus] from comment #13)
> Which that said, the use of the callback url in some services is a bit
> tricky:
> 
> - Some of them require a valid (reachable by the service at registration
> time) url (not the case of FB, but other providers).
> - Others don't allow to register two app ids with the same callback url
> 
> This point (how to deal with the callback url) is a bit unclear in the
> specification so each service provider may decide to follow different rules.
> 
> What was clear for us when we implemented the Oauth 2.0 support in contacts
> is that we wanted to support as many services (future services) as possible,
> so that's why we were so generic.
> 
> Coming back to the point of not letting partners to customise the callback
> URL, basically I think that will be a must (with the current), since the
> redirection URL will be coded in the application manifest itself.
> 
> So what I think that would happen (again, with the current solution) is that
> we have an unique callback url provided by Moz, and partners will need to
> register their keys using that url as callback.
> 
> Does that make sense for you?

Let's limit the scope of this bug to simply dealing with the Facebook case.

If you think the right patch is to allow the vender to change the URL, instead of hard-coding it to [1], we could do that here. If the security team disagree we could file another bug for it.

[1] https://www.facebook.com/connect/login_success.html

Right now, the vendors could overwrite URLs in parameters.js but not in manifest.webapp, which is very troubling. Venders should be allow to overwrite all of them at the same time or none of them. Again, patch in comment 4 does neither of those.

Comment 16

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #15)
> 
> Let's limit the scope of this bug to simply dealing with the Facebook case.
> 
> If you think the right patch is to allow the vender to change the URL,
> instead of hard-coding it to [1], we could do that here. If the security
> team disagree we could file another bug for it.
> 
> [1] https://www.facebook.com/connect/login_success.html
> 
> Right now, the vendors could overwrite URLs in parameters.js but not in
> manifest.webapp, which is very troubling. Venders should be allow to
> overwrite all of them at the same time or none of them. Again, patch in
> comment 4 does neither of those.

Right, I think Jose Manuel is amending the patch to solve what you comment. I think is worthy to aim for the solution of partners customizing everything, let's see what he come back with :)

Thanks again for your help!
F.

Comment 17

[Jose Manuel Cantera]

Attachment: Pointer to GH PR

First version

Comment 18

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 764800 [details]
Pointer to GH PR

I think this is what was requested on the bug. This way vanilla (that is, not partner) builds can still work with a default configuration provided by Mozilla and partners can customize all the service elements, including redirects URLs.

Comment 19

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 764800 [details]
Pointer to GH PR

Great!

Looking good, left a couple of messages in github.

I think with this we have the flexibility we will need for 1.0.1 (fb case), but also for the upcoming 1.1

Thanks again!
F.

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 764800 [details]
Pointer to GH PR

Thank you!

I think we could further improve this part of the build system in the future.....

Comment 21

[Jose Manuel Cantera]

https://github.com/mozilla-b2g/gaia/commit/0163b1b7c17951be17837f22542c418e0cf906c5

Comment 22

[[e-mail]]

this bug block the tef+ bug 883344 so need uplift permission

Comment 23

[John Ford [:jhford] CET/CEST Berlin Time]

This patch does not apply cleanly to either v1.0.1 or v1-train.  I'm guessing that this means that bug 883344 will also need manual intervention once this patch is ready to go.

Comment 24

[[e-mail]]

tried to patch it and find many conflicts between master and v1.0.1. I'm not sure the correct way to solve conflict in util.js

I think its better to ask Jose who have more knowledge about this patch.


after this experiment, I think bug 883344 looks fine if this issue is uplifted.

Comment 25

[Jose Manuel Cantera]

landed in v1.0.1

https://github.com/mozilla-b2g/gaia/commit/f000719da5116eca00b78aa8121f38264d3dafee

Comment 26

[Jose Manuel Cantera]

landed in v1-train

https://github.com/mozilla-b2g/gaia/commit/c9e9b09cb181fcd15d12af09f89ff87f553c06c7

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: c9e9b09cb181fcd15d12af09f89ff87f553c06c7