Closed Bug 1041814 Opened 10 years ago Closed 10 years ago

Make Loop server an FxA Oauth relier

Categories

(Hello (Loop) :: Server, defect)

Product:
Hello (Loop)
Component:
Server
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: ckarlof, Unassigned)

References

Details

(Whiteboard: [qa+]) 

This is required to support the FxA Loop integration on Desktop.

API docs: https://github.com/mozilla/fxa-oauth-server/blob/master/docs/api.md
FxA Oauth details: https://github.com/mozilla/fxa-oauth-server/wiki/oauth-design

WIP on what that may look like: https://github.com/mozilla-services/loop-server/pull/98
Server running that PR: http://loop.dev.lcip.org/

A bunch of other stuff related to this integration: https://github.com/vladikoff/fxa-docs-browser-loop
The Loop server will need a client_id and client_secret for all environments it needs to integrate with: https://github.com/mozilla/fxa-oauth-server/issues/110
For the sake of clarity, implementation of this is the responsibility of Tarek, but Vlad has a POC patch as a starting point.
Vlad's proposed API changes to the Loop server: https://github.com/vladikoff/fxa-docs-browser-loop#server-api-instructions
Whiteboard: [qa+]
Hi chris,

Can you elaborate on what the OAuth integration brings to the current workflow? I would be very interested to have that but I need to understand the implications beforehand, thanks!
Flags: needinfo?(ckarlof)
Alexis, have you had a chance to read the above linked documents on Oauth? In particular, this one (https://github.com/mozilla/fxa-oauth-server/wiki/oauth-design) provides higher level details on our Oauth flow. In have specific questions about the material in the above documents, please let me know.
Flags: needinfo?(ckarlof)
Alexis, I probably wasn't clear, but our Oauth flow is the primary way for our services to "use" FxA going forward. 

I've updated Vlad's docs with more detailed information on what changes will be required to integrate with FxA as an Oauth relier: https://github.com/vladikoff/fxa-docs-browser-loop/blob/master/docs/server.md

The outstanding PR that Vlad has against the Loop server is merely a suggestion of what the changes should look like. It'll be your prerogative to implement the actual changes in the way that you see fit.

Please let us know if you have any questions.
Do we have a schedule for this? That is, in which Fx release will this appear?
(In reply to James Bonacci [:jbonacci] from comment #7)
> Do we have a schedule for this? That is, in which Fx release will this
> appear?

Fx 34 is our target release.
Back from PTO, I finally took the time to read the documentation you linked to, and the Pull Request (demo implementation) Vlad is proposing.

As we discussed during a meeting a few weeks ago, it seems that loop-server doesn't need to handle the web-only case (e.g. using sessions stored in cookies or passed via the url).

Because we don't have this need, the session where the "state" argument is stored could be bind to an hawk session, rather than creating a new session mechanism for OAuth needs.

If we go this direction, I believe we will need to do the following (please, if anything feels wrong, don't hesitate to amend the proposal):

- Ahead of time, register loop as an RP on the Fxa-Oauth server (we'll get back the client_id and client_secret configuration values);

On the loop-server APIs, we'll need to add a few things, but a bunch of things are still unclear.

- A "redirect_uri" needs to be defined on the loop-server side. That's where the code will be sent, and where the loop-server should trade it for a token. That's "/fxa-oauth/oauth" in vlad's proposal.

- A "callback_uri" needs to be defined as well, the documentation states:

> The "RP Backend Server" delivers a web page which we call the "RP Frontend Page". This includes a "Sign In With Your Firefox Account" link."

In the case of loop, I'm unclear how this can work: Because the client is firefox itself, I was planning on not having any HTML/CSS on the server, hence no RP Frontend page (this is happening in Firefox itself, not in a webpage).

- An endpoint where the "state" nonce is generated. In vlad's proposal, I don't quite understand why we're generating this at " different locations (signin, signup and /fxa-oauth/params). I actually don't quite understand what the latter is used for.

- An endpoint to create a session without needing to pass a simple push url.

Chris, vlad, can you try to bring some light for me in here?
Flags: needinfo?(ckarlof)
(In reply to Alexis Metaireau (:alexis) from comment #9)
> As we discussed during a meeting a few weeks ago, it seems that loop-server
> doesn't need to handle the web-only case (e.g. using sessions stored in
> cookies or passed via the url).
> 
> Because we don't have this need, the session where the "state" argument is
> stored could be bind to an hawk session, rather than creating a new session
> mechanism for OAuth needs.

Yes, associating it with the hawk session is fine.

> If we go this direction, I believe we will need to do the following (please,
> if anything feels wrong, don't hesitate to amend the proposal):
> 
> - Ahead of time, register loop as an RP on the Fxa-Oauth server (we'll get
> back the client_id and client_secret configuration values);

Yes.
 
> On the loop-server APIs, we'll need to add a few things, but a bunch of
> things are still unclear.
> 
> - A "redirect_uri" needs to be defined on the loop-server side. That's where
> the code will be sent, and where the loop-server should trade it for a
> token. That's "/fxa-oauth/oauth" in vlad's proposal.
> 
> - A "callback_uri" needs to be defined as well, the documentation states:
> 
> > The "RP Backend Server" delivers a web page which we call the "RP Frontend Page". This includes a "Sign In With Your Firefox Account" link."
> 
> In the case of loop, I'm unclear how this can work: Because the client is
> firefox itself, I was planning on not having any HTML/CSS on the server,
> hence no RP Frontend page (this is happening in Firefox itself, not in a
> webpage).

"callback_uri" was a typo. That should be "redirect_uri". No RP front end page is required unless we need to log in to FxA from a web application. Does the loop-client web application need to log in to FxA? The "redirect_uri" is what the FxA OAuth page redirects the browser back to. If you logins happen only via Desktop native login, the redirect URI in this sense isn't needed because the Desktop code can talk to the Loop Server API directly.

> - An endpoint where the "state" nonce is generated. In vlad's proposal, I
> don't quite understand why we're generating this at " different locations
> (signin, signup and /fxa-oauth/params). I actually don't quite understand
> what the latter is used for.
> 
> - An endpoint to create a session without needing to pass a simple push url.
> 
> Chris, vlad, can you try to bring some light for me in here?

Does this help?

https://developer.mozilla.org/en-US/Firefox_Accounts#Login_with_the_FxA_OAuth_HTTP_API

Essentially:

1) Create a session with the client.

2) Associate a state to that session before the OAuth flow begins.

3) Receive the OAuth code, state (authenticated with the Hawk session), verify the state provided by the client matches the one associated with the session, exchange the code for the oauth token.

Please feel free to schedule a meeting with me if that would help.

FYI, we have new developer docs for our OAuth API. Please let us know if have any question or suggestions: https://developer.mozilla.org/en-US/Firefox_Accounts
Flags: needinfo?(ckarlof)
Alexis, a good question is what the redirect_uri for Loop should be if it never uses the the web-based redirect flow. 

It's entirely possible that we'll need the redirect flow on the web for Loop in the future, so it may be a good idea to define that endpoint now, since it's currently required to be a FxA OAuth relier.

If there's a product page for Loop, we could redirect requests for the redirect_uri to that product page for now.

One detail with respect to this issue is that after the user verify's her email, we show a page that says "You are now ready to use <service name>", where <service name> is a the <a> tag with the name of the service that points to the redirect_uri. For Loop, we may need to flag it in some way that doesn't create those links. Alternatively, we if the redirect_uri endpoint forwards to a product page, then this might be fine. To see what I'm talking about, create an account here: https://123done-stable.dev.lcip.org/
Alexis and team, I think that we're trying to target a prod release of Loop with FxA by Sept 2. That's pretty tight, considering that some bugs will probably arise in integration testing. Vlad and I will make it a high priority to be available to answer questions and assist where we can. Please reach out if it would be helpful for us to be available earlier during some of our mornings to help resolve any questions.
(In reply to Chris Karlof [:ckarlof] from comment #11)
> One detail with respect to this issue is that after the user verify's her
> email, we show a page that says "You are now ready to use <service name>",
> where <service name> is a the <a> tag with the name of the service that
> points to the redirect_uri. For Loop, we may need to flag it in some way
> that doesn't create those links. Alternatively, we if the redirect_uri
> endpoint forwards to a product page, then this might be fine. To see what
> I'm talking about, create an account here:
> https://123done-stable.dev.lcip.org/

This is what the "You are now ready to use <service name>" will look like for Loop (depending on how the user verifies their email): http://v14d.com/i/53f3e11669f5e.jpg
(In reply to Chris Karlof [:ckarlof] from comment #10)
> "callback_uri" was a typo. That should be "redirect_uri". No RP front end
> page is required unless we need to log in to FxA from a web application.
> Does the loop-client web application need to log in to FxA? The
> "redirect_uri" is what the FxA OAuth page redirects the browser back to. If
> you logins happen only via Desktop native login, the redirect URI in this
> sense isn't needed because the Desktop code can talk to the Loop Server API
> directly.

Okay, we could put the url of the product page here, even if there is no one defined so far (I believe this will be hello.firefox.com)
 
> 1) Create a session with the client.
> 
> 2) Associate a state to that session before the OAuth flow begins.
> 
> 3) Receive the OAuth code, state (authenticated with the Hawk session),
> verify the state provided by the client matches the one associated with the
> session, exchange the code for the oauth token.

That helps, yes, but I feel that vlad's proposal contains more complicated bits that I feel aren't needed.
In particular, there is no reason to have the link show up if the verification email link had been clicked from the same or a different browser, we don't have such use case for loop (since it's only activated in Firefox).

Also, the loop web client doesn't have any plan to support FxA for now, I suggest we implement it if and when the problem arises.
I've started an implementation atop Hawk and how we deal with sessions and data. It's at https://github.com/mozilla-services/loop-server/pull/183/files

Currently, it doesn't has any tests associated because I mostly wanted to use this as a proof of concept, and to discuss with you about the correctness of such a solution.

Vlad, Chris, can you have a look and tell me if that looks correct?

- We don't have the need of a web flow, so I removed the redirect if 
  the login flow was terminated in a different browser.

- I don't quite understand why the server need to store a list of active flows.
  I'm storing the nonce associated with the hawk session and then checking the
  nonce provided matches the one stored in the session. Is this enough?

- Not directly related to this pull request, and I'm adding this item mostly
  for Remy to discuss: I feel unconfortable with having these errno numbers
  in one file, with some numbers missing.
  What's the reference documentation for it (if any), to be sure we don't shadow
  other errno?
(In reply to Alexis Metaireau (:alexis) from comment #14)
> That helps, yes, but I feel that vlad's proposal contains more complicated
> bits that I feel aren't needed.
> In particular, there is no reason to have the link show up if the
> verification email link had been clicked from the same or a different
> browser, we don't have such use case for loop (since it's only activated in
> Firefox).

We'll figure out a way to arrange for that link to not appear. 

> Also, the loop web client doesn't have any plan to support FxA for now, I
> suggest we implement it if and when the problem arises.

Sure.
(In reply to Alexis Metaireau (:alexis) from comment #15)
 
> - I don't quite understand why the server need to store a list of active
> flows.
>   I'm storing the nonce associated with the hawk session and then checking
> the
>   nonce provided matches the one stored in the session. Is this enough?

Yes, that sounds fine, but also don't know what you mean by "active flow".
> What's the reference documentation for it?

I am using this as a reference:

https://github.com/mozilla-services/msisdn-gateway/blob/master/API.md#response-format

But I know that ferjm also has the list on client side.

https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsCommon.js#91
(In reply to Rémy Hubscher (:natim) from comment #18)
> https://github.com/mozilla-services/msisdn-gateway/blob/master/API.
> md#response-format
> https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsCommon.js#91

Rémy, this points to https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format

I think we need to have these defined in the documentation somewhere and point all the places where we used them to this authoritative doc, otherwise we're just tying the projects together, and I would like to avoid that.

> Yes, that sounds fine, but also don't know what you mean by "active flow".

Chris, I'm just reusing the concept that was part of the previous PR. If that's not needed then I'll will just ignore it.
I think I now have something that should work better. However, I miss a way to test it works properly, given the fact that I don't have any client around.

I think making a little python script interacting with our APIs and checking it works properly could solve this issue (but I don't have it yet).

The code is at https://github.com/mozilla-services/loop-server/pull/183, if you want to have a look.
Since deadline are tied and :alexis is on PTO new week, I am merging this. (The code looks right, there are tests and a bug about documentation: Bug 1057361)

https://github.com/mozilla-services/loop-server/commit/6fbb560c0648e866fb83ee9f9c119e287d813011

Vlad — Can you make sure the all flow will work for the Client team?
Blocks: 1057361
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(vlad)
Resolution: --- → FIXED
Just to rectify: I'm not in PTO this week.
(In reply to Rémy Hubscher (:natim) from comment #21)
> Vlad — Can you make sure the all flow will work for the Client team?

I've made various comments on the pull request about things that don't match the plan from https://github.com/vladikoff/fxa-docs-browser-loop/blob/master/docs/server.md#endpoints

To test in Firefox Nightly, apply the patch in bug 1047617 and run the following in the Browser Console:
Cu.import("resource:///modules/loop/MozLoopService.jsm"); MozLoopService.logInToFxA()
Flags: needinfo?(vlad)
So, then, this is not quite "Resolved"?
(In reply to Rémy Hubscher (:natim) from comment #21)
> Since deadline are tied and :alexis is on PTO new week, I am merging this.
> (The code looks right, there are tests and a bug about documentation: Bug
> 1057361)
> 
> https://github.com/mozilla-services/loop-server/commit/
> 6fbb560c0648e866fb83ee9f9c119e287d813011
> 
> Vlad — Can you make sure the all flow will work for the Client team?

We have looked the merged patch and identified a few issues. Those were addressed in pull request: https://github.com/mozilla-services/loop-server/pull/188
Those changes are required to sync up with the Firefox Loop panel client.
OK, so this is Fixed when #188 gets merged. Thanks.
We will need to open Stage/Deploy tickets to get this out there once the issues have been resolved/merged...
:mattn is there a Try or Team build to test this? It would save QA a bit of time.
There is Nightly once bug 1047617 lands which should be soon™.
> We have looked the merged patch and identified a few issues. Those were addressed in pull request: https://github.com/mozilla-services/loop-server/pull/188
> Those changes are required to sync up with the Firefox Loop panel client.

Ok it will our first priority today.
Version 0.11.0 is out in Production.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.