Closed Bug 1127638 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Firefox Accounts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox38 --- fixed

People

(Reporter: ckarlof, Assigned: zaach)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
:zaach has a patch in progress that adds a method on FxAccounts.jsm to provide this functionality.
Reading list can use this to get an OAuth token for the currently logged in (Sync) user.
Related server feature to request limited lifetime tokens: https://github.com/mozilla/fxa-oauth-server/issues/209
Blocks: 1125233
Blocks: 1127540
Attachment #8558655 - Flags: review?(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.
Attachment #8558673 - Flags: review?(mhammond) → review+
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.
(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.
> 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?
(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.
Flags: firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/14beb450d966
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Iteration: --- → 38.2 - 9 Feb
Flags: qe-verify?
See Also: → 1131409
Flags: qe-verify? → qe-verify-
Product: Core → Firefox
Target Milestone: mozilla38 → Firefox 38
You need to log in before you can comment on or make changes to this bug.