-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Per Spec, allow Authorization Server to -maybe- issue a new Refresh Token. #286
Conversation
Looks good to me @alexbilbie |
I don't understand your approach to this. Why not just have a |
Mostly because, as you have the client implementing your AuthServer can dictate how long the TTL is, why would we not let them dictate how long a Refresh Token can exist before needing a refresh itself? My only thought-case here would be someone who would like to issue refresh tokens after 1 day of issuance, and another who would rather refresh after 6. Personally, I would rather let that be up the client, rather than being an all or nothing. |
Alex raises a good point. If a refresh token already has a TTL, then we know it will eventually expire. If we set a simple flag that says "rotate them" then that refresh token will be replaced with a new one instead of simply turned off. That would take care of your use case perfectly. |
Ok, changed it to be a boolean that you can enable/disable that will drive the re-issuing of the refresh token. |
Could you squash your commits with |
Done. |
Looks good. Would you mind adding a unit test to cover the changes please? |
Sorry bout that, remembered reading that in the contribute file. There we are. |
|
||
$server->addGrantType($grant); | ||
|
||
$response = $server->issueAccessToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use assertArrayHasKey() surely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean here.
Granted, this is just a copy-paste extension from another test. But I'm asserting that we are returning a refresh_token, regardless of my main change. Then the different applies with the assertNotEquals, and assertEquals,, where we see the refresh_token coming out of the issueAccessToken()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? I'm saying instead of $this->assertTrue(array_key_exists())
you can use $this-> assertArrayHasKey()
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I kind of assumed.
I wasn't sure y'all desire to have mixed-methods. As the $this->assertTrue(..)
method is used in all your other tests. Wasn't sure if using the $this->assertArrayHasKey(..)
would have been desired. I can change, if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed something and provided feedback. It’s up to Alex either way. It could well be that assertTrue(array_has_key()) is used elsewhere, but it probably shouldn’t be. :)
Feel free to take this pull, or not, with rejection of implicit grant I am going to have to utilize another oauth-server which will support the basic OAuth2 grant types. |
You can still use the implicit grant with this library (it looks like you've already written a working implementation based on that PR). This library is designed to be extensible and I'm not going to remove those plugin points. I just don't want to have the implicit grant it in the core library (as explained in my response to your PR). I will merge this when I've got 5 minutes to finish going over it. |
Neat. However, we generally have a hard time getting approval to wrap our own additions around a library when upstream rejects. Even if you were to say, wrap the implicit with a |
* | ||
* @return int | ||
*/ | ||
public function setRefreshTokenRotate($refreshTokenRotate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method should be called setRefreshTokenRotation
The authorization server MAY issue a new refresh token, in which case the client MUST discard the old refresh token and replace it with the new refresh token. The authorization server MAY revoke the old refresh token after issuing a new refresh token to the client. If a new refresh token is issued, the refresh token scope MUST be identical to that of the refresh token included by the client in the request. This commit allows users to specifiy the time before the Refresh Token expire time to issue a new Refresh Token. alter method names, naming convention(?)
The authorization server MAY issue a new refresh token, in which case
the client MUST discard the old refresh token and replace it with the
new refresh token. The authorization server MAY revoke the old
refresh token after issuing a new refresh token to the client. If a
new refresh token is issued, the refresh token scope MUST be
identical to that of the refresh token included by the client in the
request.
This commit allows users to specifiy the time before the Refresh Token
expire time to issue a new Refresh Token.