mozilla / fxa-auth-server

DEPRECATED - Migrated to https://github.com/mozilla/fxa
Mozilla Public License 2.0
399 stars 121 forks source link

feat(oauth): Use an assertion to directly grant tokens at /token. #2969

Closed rfk closed 5 years ago

rfk commented 5 years ago

Fixes #2962. Requires #2946 as it's based off that commit.

Clients could previously use an FxA assertion to grant themselves OAuth access tokens via the /authorization endpoint in a style modelled after OAuth's "implicit grant" flow. But it's not really the implicit grant flow, the way we use it in practice much more closely resembles the "resource owner password credentials" flow. In particular, existing clients use it to directly create tokens for their own use, rather than to authorize tokens for another client.

This commit makes that functionality available on the /token endpoint instead, using grant_type=fxa-assertion. This is better aligned with the way that the rest of OAuth works, closely mirroring the grant_type=password flow and keeping a clear distinction between obtaining tokens for ones own use (always use the /token endpoint) versus authorizing them for someone else (always use the /authorization endpoint).

It will hopefully help us avoid future footguns if we want to allow assertion-bearing clients to create things for their own use (such as refresh tokens) that are forbidden in the "implicit grant" flow proper.

rfk commented 5 years ago

It needs tests for the new grant_type, but here's what I was thinking for #2962. Key points:

@shane-tomlinson what do you think, is this heading in a good direction?

For full context, the plan would be that #2954 uses grant_type=fxa-assertion to do direct oauth token grants from a sessionToken.

vladikoff commented 5 years ago

Creates a new file grant.js containing shared logic for validating grant requests (is this client allowed that scope? is the user sufficiently authenticated?) and generating the corresponding tokens. Refactors the /authorization route to make it a bit easier to follow, and to use the shared logic. Adds a new grant_type=fxa-assertion in the freshly-refactored /token endpoint, which takes an identity assertion and directly grants the requested tokens. Also updates the /key-data endpoint to use the shared logic, for completeness.

This makes sense to me. I'm trying to see how I can implement https://github.com/mozilla/fxa-auth-server/issues/2880 and also have this landed. I guess a bit of rebasing!

vladikoff commented 5 years ago

Draft looks very good! 👍

rfk commented 5 years ago

I've added a bunch of tests, I think this is ready for review!

vladikoff commented 5 years ago

Filed https://github.com/mozilla/fxa-auth-server/pull/2984 to unblock ci

vladikoff commented 5 years ago

cc @shane-tomlinson r?

vladikoff commented 5 years ago

re: key-data

Do we have to worry about any clients depending on the previous behavior?

Firefox 67 and Content-server would fail the flow.

rfk commented 5 years ago

Would it have failed with a missing code had this not been sent?

Yes, because of the order in which fields are validated; I'll add a comment.

rfk commented 5 years ago

@vladikoff I think I've addressed all the review comments, could you please take a final look and merge if you're happy with it?