1127638 - Provide a way to get an OAuth token for a set of desired scopes for the currently logged in user

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

[Chris Karlof [:ckarlof]]

We can use https://github.com/mozilla/fxa-oauth-server/blob/master/docs/api.md#post-v1authorization to get the token from a BrowserID assertion.

Comment 1

[Chris Karlof [:ckarlof]]

:zaach has a patch in progress that adds a method on FxAccounts.jsm to provide this functionality.

Comment 2

[Chris Karlof [:ckarlof]]

Reading list can use this to get an OAuth token for the currently logged in (Sync) user.

Comment 3

[Chris Karlof [:ckarlof]]

Related server feature to request limited lifetime tokens: https://github.com/mozilla/fxa-oauth-server/issues/209

Comment 4

[Zachary Carter [:zaach]]

Attachment: Provide a way to get an OAuth token for a set of desired scopes for the currently logged in user

Comment 5

[Zachary Carter [:zaach]]

Attachment: Provide a way to get an OAuth token for a set of desired scopes for the currently logged in user

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8558673 [details] [diff] [review]
Provide a way to get an OAuth token for a set of desired scopes for the currently logged in user

Review of attachment 8558673 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.  I'll leave it to you (probably in discussion with Chris) to decide whether the pref for the client_id really is what we want, and also to my suggested dropping of the 3 getters in FxAccounts.

::: browser/app/profile/firefox.js
@@ +1754,5 @@
> +// The remote URL of the FxA OAuth Server
> +pref("identity.fxaccounts.remote.oauth.uri", "https://oauth.accounts.firefox.com/v1");
> +
> +// The privileged client id that can request OAuth tokens without user interaction
> +pref("identity.fxaccounts.oauth.client_id", "5882386c6d801776");

Storing this in a pref seems a little suspect - there's a technique we use for "secrets" (such as keys used for google services etc) that's a little more painful to use - should we be using that instead?

::: services/fxaccounts/FxAccounts.jsm
@@ +937,5 @@
> +    if (!options.scope) {
> +      throw new Error("Missing 'scope' option");
> +    }
> +
> +    let uri = options.oAuthServerUrl || this.oAuthServerUrl;

If someone passes |options.oAuthServerUrl|, shouldn't the fxAccountOAuthGrantClient object be created with the same URL?

It almost seems to be that we should just always create a new fxAccountOAuthGrantClient object each request - it seems relatively light-weight and doesn't seem to carry state that needs to be reused across calls.  This would mean everything can be inline here, avoiding all 3 new getters above.

::: services/fxaccounts/FxAccountsOAuthGrantClient.jsm
@@ +143,5 @@
> +
> +        // "response.success" means status code is 200
> +        if (request.response.success) {
> +          return resolve(body);
> +        } else {

just drop the |else| given the early-return above.

Comment 7

[Chris Karlof [:ckarlof]]

Comment on attachment 8558673 [details] [diff] [review]
Provide a way to get an OAuth token for a set of desired scopes for the currently logged in user

Review of attachment 8558673 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/profile/firefox.js
@@ +1754,5 @@
> +// The remote URL of the FxA OAuth Server
> +pref("identity.fxaccounts.remote.oauth.uri", "https://oauth.accounts.firefox.com/v1");
> +
> +// The privileged client id that can request OAuth tokens without user interaction
> +pref("identity.fxaccounts.oauth.client_id", "5882386c6d801776");

This isn't a secret, but it's also something I don't expect us needing to configure at the user level. zaach, we might just put it as a constant in FxAccountsCommon.jsm.

::: services/fxaccounts/FxAccounts.jsm
@@ +937,5 @@
> +    if (!options.scope) {
> +      throw new Error("Missing 'scope' option");
> +    }
> +
> +    let uri = options.oAuthServerUrl || this.oAuthServerUrl;

I like this suggestion to create the client on demand. Simpler, less code, less state.

Comment 8

[Zachary Carter [:zaach]]

(In reply to Chris Karlof [:ckarlof] from comment #7)
> Comment on attachment 8558673 [details] [diff] [review]
> Provide a way to get an OAuth token for a set of desired scopes for the
> currently logged in user
> 
> Review of attachment 8558673 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/app/profile/firefox.js
> @@ +1754,5 @@
> > +// The remote URL of the FxA OAuth Server
> > +pref("identity.fxaccounts.remote.oauth.uri", "https://oauth.accounts.firefox.com/v1");
> > +
> > +// The privileged client id that can request OAuth tokens without user interaction
> > +pref("identity.fxaccounts.oauth.client_id", "5882386c6d801776");
> 
> This isn't a secret, but it's also something I don't expect us needing to
> configure at the user level. zaach, we might just put it as a constant in
> FxAccountsCommon.jsm.
> 

What about for self-hosting? If you can configure URLs it seems reasonable to expect that you could also configure a different client ID without building from source.

Comment 9

[Chris Karlof [:ckarlof]]

> What about for self-hosting? If you can configure URLs it seems reasonable to expect that you could also configure a different client ID without building from source.

It's not clear to me why self-hosters would need to change it. It's just a random string. Can you think of a reason why they would need to configure it?

Comment 10

[Zachary Carter [:zaach]]

(In reply to Chris Karlof [:ckarlof] from comment #9)
> > What about for self-hosting? If you can configure URLs it seems reasonable to expect that you could also configure a different client ID without building from source.
> 
> It's not clear to me why self-hosters would need to change it. It's just a
> random string. Can you think of a reason why they would need to configure it?

True, if we document it someone visible to self-hosters it'll probably be fine.

Comment 11

[Zachary Carter [:zaach]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f4fe353c248

Comment 12

[Zachary Carter [:zaach]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/14beb450d966

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/14beb450d966