Generate token params #303
Conversation
|
@mjsalinger issuing the same access token in the "refresh token" grant does not seem to be allowed by the OAuth RFC:
Can you provide more detail on how you're using these parameters? I believe you would need to also pass these down to the abstract-grant in order to get that to work. |
|
I don't think it's needed in abstract grant, just for tokens. The use case I'm dealing with is primarily the mobile use case, where a single user may need access from multiple devices. If each device has a single access token and single refresh token, then you end up having to specifically link the refresh and access token. The general practice I've seen is that only one access token is issued per user/clientId combo, and a unique, single use refresh token is issued for each token endpoint access. Once the access token expires, each client will need to use it's refresh token to obtain a new access token. The first device to refresh will get a newly generated access token, then each subsequent device will also need to refresh to obtain the access token. So it does follow the spec in for the device, they are issued a new access token, since the point of a refresh token is to refresh an expired token. This, by the way, is the way Google OAuth handles it, and I've seen it in other implementations as well. |
|
Generate token params is a good thing but these params also should be passed anywhere where generateAccessToken() or generateRefreshToken() called. |
maxtruxa
left a comment
maxtruxa
left a comment
There was a problem hiding this comment.
Throwing an error rather than falling back to generating a token when model#generateAccessToken/model#generateRefreshToken return a falsy value would be safer in my opinion.
| return Promise.try(this.model.generateAccessToken); | ||
| return Promise.try(promisify(this.model.generateAccessToken, 3), [client, user, scope]) | ||
| .then(function(accessToken) { | ||
| return accessToken || tokenUtil.generateRandomToken(); |
There was a problem hiding this comment.
Do you think it's a good idea to fall back to the default generateRandomToken?
I would consider it an error if model#generateAccessToken is implemented but returns a falsy value.
| return Promise.try(this.model.generateRefreshToken); | ||
| return Promise.try(promisify(this.model.generateRefreshToken, 3), [client, user, scope]) | ||
| .then(function(refreshToken) { | ||
| return refreshToken || tokenUtil.generateRandomToken(); |
There was a problem hiding this comment.
Do you think it's a good idea to fall back to the default generateRandomToken?
I would consider it an error if model#generateRefreshToken is implemented but returns a falsy value.
|
How about adding This would require some restructuring of |
mjsalinger
force-pushed
the
Generate-Token-Params
branch
from
6c32df5 to
26f5fba
Compare
mjsalinger
force-pushed
the
Generate-Token-Params
branch
from
26f5fba to
e018193
Compare
|
@maxtruxa I'm not sure throwing an error is appropriate - the idea is if the called function does not generate a token, the library will do it for you as a fallback, which I think is reasonable behavior - as long as the value is falsy. The model function could still throw or callback an error... +1 on adding client/user/scope to generateAuthorizationCode, but let's put that out to a separate PR - I'll create an issue around that. Made some changes to clean up the code a little, and rebased. |
|
That's very true 👍 |
4 participants
Adds client, user, and scope as arguments to generateAccessToken and generateRefreshToken. This allows such things as returning the same access token on a reauth if the token is not expired, as many oauth implementations do.