feat(refresh_tokens): add refresh_tokens to /token endpoint #260

seanmonstar · 2015-05-26T19:04:02Z

All good to go-ish.

Any test cases I missed with scopes?

Fixes #209

pdehaan · 2015-05-26T19:10:03Z

lib/db/mysql/index.js

@@ -148,13 +151,20 @@ const QUERY_CODE_INSERT =
  'INSERT INTO codes (clientId, userId, email, scope, authAt, code) ' +
  'VALUES (?, ?, ?, ?, ?, ?)';
 const QUERY_TOKEN_INSERT =
-  'INSERT INTO tokens (clientId, userId, email, scope, type, token) VALUES ' +
-  '(?, ?, ?, ?, ?, ?)';
+  'INSERT INTO tokens (clientId, userId, email, scope, type, expiresAt,token)' +


Add space before token?

pdehaan · 2015-05-26T19:10:50Z

lib/db/mysql/index.js

-  'INSERT INTO tokens (clientId, userId, email, scope, type, token) VALUES ' +
-  '(?, ?, ?, ?, ?, ?)';
+  'INSERT INTO tokens (clientId, userId, email, scope, type, expiresAt,token)' +
+  ' VALUES (?, ?, ?, ?, ?, ?)';


Is this missing a "?"? It seems like we added token above and have 7 fields, but only 6 ?s.

vladikoff · 2015-05-27T01:14:02Z

docs/api.md

@@ -362,7 +362,26 @@ particular user.

 - `client_id`: The id returned from client registration.
 - `client_secret`: The secret returned from client registration.
- `code`: A string that was received from the [authorization][] endpoint.
+- `ttl`: (optional) Seconds that this access_token should be valid.


nit: Seconds that this access_token should be valid. ->

The amount of time the access_token is valid for in seconds or
The auth_token expiration time in seconds

Suggest bolding Seconds here like you've done in the lists below

vladikoff · 2015-05-27T01:18:24Z

lib/config.js

@@ -80,10 +80,15 @@ const conf = convict({
    }
  },
  expiration: {
-    code: {
+    accessTokenS: {


nit: expiration.accessTokenInSeconds? is this format used anywhere else? (adding S and Ms)?

I don't know if we use this convention anywhere else. I think we try to use milliseconds whenever we can, but the spec claims that the expires_at value should be in seconds...

I like millisecond timestamps too, but I think it will be against the grain when interfacing with existing OAuth, JWT and other related specs, which all seem to use timestamps in seconds.

For example, this was one of the trickest parts of trying to bring the legacy BrowserID assertion format into line with the final JWT spec. It caused a pretty funny (and thankfully low-impact) security bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1096106

In the interests of consistency, I suggest we grit our teeth and use integer second timestamps throughout the public API.

vladikoff · 2015-05-27T01:23:43Z

lib/db/memory.js

+    token.scope = vals.scope;
+    token.createdAt = new Date();
+    var _token = unique.token();
+    var ret = clone(token);


nit: group vars with the top var token = {}. rename var token = {}; to something like tokenData = {} to avoid _token vs token

vladikoff · 2015-05-27T01:29:36Z

lib/error.js

@@ -217,7 +217,7 @@ AppError.invalidScopes = function invalidScopes(scopes) {
    code: 400,
    error: 'Invalid scopes',
    errno: 114,
-    message: 'Invalid scopes'
+    message: 'Scopes requested not allowed'


Nit: Scopes requested not allowed -> i.e Requested scopes are not allowed

vladikoff · 2015-05-27T02:12:56Z

lib/db/mysql/index.js

@@ -407,6 +457,7 @@ MysqlStore.prototype = {
    // TODO this should be a transaction or stored procedure


Should we file a "good first bug" for this?

I'm not sure if it'd be an "easy" bug...

vladikoff · 2015-05-27T02:15:17Z

lib/routes/token.js

+    }
+    return json;
+  });
+  //return [code.authAt, db.removeCode(code.code), db.generateToken(code)];


old code from generateToken left here.

rfk · 2015-05-27T06:52:26Z

docs/api.md

+      endpoint specifically as a refresh token.
+    - `scope`: (optional) A subset of scopes provided to this
+      refresh_token originally, to receive an access_token with less
+      permissions.


refresh tokens will be mandatory

IIUC there are cases in the OAuth spec where we're forbidden from delivering refresh tokens, e.g. when using implicit grant clients. Perhaps we can weaken this to something like "refresh tokens will be teh default behaviour".

Woops, this was the first thing I wrote in this PR. It didn't reflect that changes that we'd always expire the token now. Updated the text.

seanmonstar · 2015-05-27T18:26:25Z

woo, build finally passes

rfk · 2015-05-28T06:09:11Z

docs/api.md

+    - `refresh_token`: A string that received from the [token][]
+      endpoint specifically as a refresh token.
+    - `scope`: (optional) A subset of scopes provided to this
+      refresh_token originally, to receive an access_token with less


I was going to go all grammar pedant on this and suggest s/less/fewer/, but Wikipedia tells me [1] that it's actually fine to use "less" with count nouns and I should learn to relax and enjoy life without worrying about made-up grammatical rules dating from 1770...

[1] https://en.wikipedia.org/wiki/Fewer_vs._less

wuts grammer

rfk · 2015-05-28T06:36:50Z

docs/api.md

+- `refresh_token`: (Optional) A refresh token to fetch a new access
+  token when this one expires. Only will be present if
+  `grant_type=authorization_code`.
+- `expires_at`: **Millisecond** timestamp when this access token will no longer be valid.


Mixing seconds and milliseconds in this response seems like a recipe for trouble.

More broadly, I think it makes sense to copy the behaviour of existing oauth and/or openid-connect servers where there's precedent. In this case both specs seem to suggest using expires_in with a relative timestamp in seconds, e.g: http://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthResponse

rfk · 2015-05-28T06:50:33Z

lib/db/memory.js

+ *       token: <string>,
+ *       clientId: <client_id>,
+ *       userId: <user_id>,
+ *       scope: <string>,
 *       createdAt: <timestamp>


Do you think it would be useful to track a lastUsedAt for refresh tokens? We might want to e.g. expire tokens that have not been used in months. Doesn't have to be part of the initial landing, but something to think about.

It probably would be useful, yes. Those that ignore the refresh token could be inadvertently generating new ones every time, and that would load up the table with a bunch of refresh tokens that sit there forever, ready to be guessed?

rfk · 2015-05-28T06:53:47Z

lib/db/memory.js

+      email: vals.email,
+      scope: vals.scope,
+      createdAt: new Date(),
+      token: encrypt.hash(token)


nit: it may be clearer to name this e.g. tokenId or tokenHash rather than just token

Hmm...I see that would be inconsistent with the naming in the mysql backend, where we use token for the column name. Probably not worth it then.

rfk · 2015-06-11T11:04:06Z

Whoops, I think I accidentally commented on the commit rather than the diff; hopefully those two comments make sense.

This looks great @seanmonstar, thanks for separating the commits for review. Please rebase when ready.

seanmonstar · 2015-06-11T22:11:49Z

nits addressed, squashed into 1.

rfk · 2015-06-12T06:14:18Z

I filed #275 as a follow-on for tracking last-used-at timestamps on refresh tokens.

rfk · 2015-06-12T06:32:52Z

The access_type parameter will require support from the content-server; I filed mozilla/fxa-content-server#2589

@seanmonstar I'm sure you'd be most welcome to dive in and implement that part of it as well, if you were feeling adventurous :-)

rfk · 2015-06-12T07:07:57Z

According to puppet-config, our active production reliers are:

I've dug into these, and the only one that I think we need to be careful with is pocket. I'll send them an email and make sure we've got a clear path forward.

rfk · 2015-06-29T23:07:41Z

(Migrating from IRC discussion)

Talking to Pocket, they'd like for all of their existing access_tokens to keep working after the switch, and I think this is a fair enough request. We can achieve this by migating existing access tokens to become refresh tokens, staging the rollout like this:

Merge this code and get it out there and deployed for testing purposes
Pocket update their code, test, and start requesting refresh tokens
We do a db update to convert their previous access tokens into refresh tokens, perhaps using a timestamp-based filter to select only the ones that were created from a refresh token.

To give us enough time between (2) and (3) we could increase the default access_token lifetime to something longer, so we don't break things in the meantime. Once reliers are migrated over we can drop it back down to a shorter lifetime.

@seanmonstar thoughts?

seanmonstar · 2015-06-29T23:08:27Z

To upgrade Pocket's tokens:

INSERT INTO refreshTokens (token, clientId, userId, email, scope, createdAt)
  (SELECT token, clientId, userId, email, scope, createdAt FROM tokens WHERE clientId = '$POCKET_CLIENT_ID', createdAt < MIN(SELECT createdAt from refresh_tokens WHERE clientId = '$POCKET_CLIENT_ID'));
DELETE FROM tokens WHERE clientId = '$POCKET_CLIENT_ID', MIN(SELECT createdAt from refresh_tokens WHERE clientId = '$POCKET_CLIENT_ID');

rfk · 2015-06-29T23:14:53Z

To give us enough time between (2) and (3) we could increase the default access_token lifetime to something longer

@seanmonstar points out that this won't work, since expiry is based on token creation time. But thinking about it some more, this won't matter anyway. As long as pocket doesn't try to use the tokens between when they're created and when we switch them to refresh magic, nothing will break.

seanmonstar · 2015-06-29T23:15:38Z

Alternatively, default expiration could be set to something ridiculous at first, and then re-adjusted afterwards.

seanmonstar · 2015-06-29T23:17:42Z

rebased to master

rfk · 2015-06-29T23:18:44Z

I think, based on IRC conversation, that we can do the above deployment plan on top of the code as it currently sits. @seanmonstar can I get an official r? from you on that plan, and if so, I think this is ready to merge.

seanmonstar · 2015-06-29T23:20:46Z

Yea that plan seems fine to me.

rfk · 2015-06-29T23:25:35Z

test/api.js

+              }
+            });
+          }).then(function(res) {
+            assertRequestParam(res.result, 'ttl');


Should this assert a particular response code?

n/m, I see the helper function does this internally. That's not clear from the name, but I'll file a follow-up bug about it.

rfk · 2015-06-29T23:33:03Z

lib/routes/destroy.js

+    payload: Joi.object().keys({
+      access_token: validators.token,
+      refresh_token: validators.token
+    }).rename('token', 'access_token').xor('access_token', 'refresh_token')


I wonder if we should add a comment here about why it's being renamed, since we probably won't remember in a few month's time.

Seems obvious to me, since token is now ambiguous, more specific names were needed.

rfk · 2015-06-29T23:36:31Z

@seanmonstar my last-minute nits notwithstanding, I think we're solid here. Let's land this as soon as train-40 has been cut, and maximise testing time in the wild. r+ \o/

See docs/api.md for changes to endpoints. Closes #209

feat(refresh_tokens): add refresh_tokens to /token endpoint

seanmonstar added waffle:in progress and removed waffle:in progress labels May 26, 2015

pdehaan reviewed May 26, 2015
View changes

pdehaan added the WIP label May 26, 2015

seanmonstar force-pushed the refresh-tokens branch from 30c656c to d4e8648 May 27, 2015

seanmonstar changed the title ~~wip(refresh_tokens): add refresh_tokens to /token endpoint~~ feat(refresh_tokens): add refresh_tokens to /token endpoint May 27, 2015

seanmonstar added ❤❤❤ and removed WIP labels May 27, 2015

seanmonstar force-pushed the refresh-tokens branch from d4e8648 to 4543704 May 27, 2015

vladikoff reviewed May 27, 2015
View changes

seanmonstar force-pushed the refresh-tokens branch from 4543704 to 029352a May 27, 2015

vladikoff reviewed May 27, 2015
View changes

seanmonstar force-pushed the refresh-tokens branch from 029352a to e511eb9 May 27, 2015

vladikoff reviewed May 27, 2015
View changes

lib/routes/token.js

}

return json;

});

//return [code.authAt, db.removeCode(code.code), db.generateToken(code)];

This comment has been minimized.

Sign in to view

vladikoff May 27, 2015
Contributor

old code from generateToken left here.

rfk reviewed May 27, 2015
View changes

seanmonstar force-pushed the refresh-tokens branch 5 times, most recently from 1fc027f to e3ab7bd May 27, 2015

rfk reviewed May 28, 2015
View changes

seanmonstar force-pushed the refresh-tokens branch from 86886cd to 5239ab4 Jun 10, 2015

shane-tomlinson mentioned this pull request Jun 11, 2015

Retrieve user's email address at a later time (post login) mozilla/fxa-auth-server#962

Closed

seanmonstar force-pushed the refresh-tokens branch from 5239ab4 to cd39443 Jun 11, 2015

rfk mentioned this pull request Jun 12, 2015

Add support for "access_type" oauth parameter, for requesting refresh tokens mozilla/fxa-content-server#2589

Closed

rfk added this to the train-40 milestone Jun 12, 2015

rfk modified the milestones: train-41, train-40 Jun 29, 2015

seanmonstar force-pushed the refresh-tokens branch from cd39443 to 24e72e8 Jun 29, 2015

rfk reviewed Jun 29, 2015
View changes

feat(refresh_tokens): add refresh_tokens to /token endpoint

16e787f

See docs/api.md for changes to endpoints. Closes #209

seanmonstar force-pushed the refresh-tokens branch from 24e72e8 to 16e787f Jun 30, 2015

seanmonstar added a commit that referenced this pull request Jun 30, 2015

Merge pull request #260 from mozilla/refresh-tokens

52854d6

feat(refresh_tokens): add refresh_tokens to /token endpoint

seanmonstar merged commit 52854d6 into master Jun 30, 2015
1 check was pending

1 check was pending

continuous-integration/travis-ci/push The Travis CI build is in progress
Details

seanmonstar removed the waffle:in review label Jun 30, 2015

seanmonstar deleted the refresh-tokens branch Jun 30, 2015

chilts mentioned this pull request Jul 6, 2015

feat(log): Add activity event logging #278

Closed

		@@ -407,6 +457,7 @@ MysqlStore.prototype = {
		// TODO this should be a transaction or stored procedure

mozilla / fxa-oauth-server Archived

feat(refresh_tokens): add refresh_tokens to /token endpoint #260

feat(refresh_tokens): add refresh_tokens to /token endpoint #260

Conversation

seanmonstar commented May 26, 2015

This comment has been minimized.

pdehaan May 26, 2015 Contributor

This comment has been minimized.

pdehaan May 26, 2015 Contributor

This comment has been minimized.

vladikoff May 27, 2015 Contributor

This comment has been minimized.

rfk Jun 2, 2015 Member

This comment has been minimized.

vladikoff May 27, 2015 Contributor

This comment has been minimized.

seanmonstar May 27, 2015 Author Member

This comment has been minimized.

rfk May 28, 2015 Member

This comment has been minimized.

vladikoff May 27, 2015 Contributor

This comment has been minimized.

vladikoff May 27, 2015 Contributor

This comment has been minimized.

vladikoff May 27, 2015 Contributor

This comment has been minimized.

seanmonstar May 27, 2015 Author Member

This comment has been minimized.

vladikoff May 27, 2015 Contributor

This comment has been minimized.

rfk May 27, 2015 Member

This comment has been minimized.

seanmonstar May 27, 2015 Author Member

seanmonstar commented May 27, 2015

This comment has been minimized.

rfk May 28, 2015 Member

This comment has been minimized.

seanmonstar Jun 1, 2015 Author Member

This comment has been minimized.

rfk May 28, 2015 Member

This comment has been minimized.

rfk May 28, 2015 Member

This comment has been minimized.

seanmonstar Jun 1, 2015 Author Member

This comment has been minimized.

rfk May 28, 2015 Member

This comment has been minimized.

rfk May 28, 2015 Member

rfk commented Jun 11, 2015

seanmonstar commented Jun 11, 2015

rfk commented Jun 12, 2015

rfk commented Jun 12, 2015

rfk commented Jun 12, 2015

rfk commented Jun 29, 2015

seanmonstar commented Jun 29, 2015

rfk commented Jun 29, 2015

seanmonstar commented Jun 29, 2015

seanmonstar commented Jun 29, 2015

rfk commented Jun 29, 2015

seanmonstar commented Jun 29, 2015

This comment has been minimized.

rfk Jun 29, 2015 Member

This comment has been minimized.

rfk Jun 29, 2015 Member

This comment has been minimized.

rfk Jun 29, 2015 Member

This comment has been minimized.

seanmonstar Jun 29, 2015 Author Member

rfk commented Jun 29, 2015

pdehaan May 26, 2015
Contributor

pdehaan May 26, 2015
Contributor

vladikoff May 27, 2015
Contributor

rfk Jun 2, 2015
Member

vladikoff May 27, 2015
Contributor

seanmonstar May 27, 2015
Author Member

rfk May 28, 2015
Member

vladikoff May 27, 2015
Contributor

vladikoff May 27, 2015
Contributor

vladikoff May 27, 2015
Contributor

seanmonstar May 27, 2015
Author Member

vladikoff May 27, 2015
Contributor

rfk May 27, 2015
Member

seanmonstar May 27, 2015
Author Member

rfk May 28, 2015
Member

seanmonstar Jun 1, 2015
Author Member

rfk May 28, 2015
Member

rfk May 28, 2015
Member

seanmonstar Jun 1, 2015
Author Member

rfk May 28, 2015
Member

rfk May 28, 2015
Member

rfk Jun 29, 2015
Member

rfk Jun 29, 2015
Member

rfk Jun 29, 2015
Member

seanmonstar Jun 29, 2015
Author Member