Skip to content

Add Refresh Token grant support #128

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

Closed
wants to merge 2 commits into from

Conversation

alek-sys
Copy link
Contributor

@alek-sys alek-sys commented Oct 7, 2020

Just a draft for now, to run tests and discuss implementation.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 7, 2020
@alek-sys alek-sys marked this pull request as ready for review October 9, 2020 07:08
@alek-sys
Copy link
Contributor Author

alek-sys commented Oct 9, 2020

Hey @jgrandja, this one is ready for your review. A couple of things I'd like to discuss before implementing are:

  • Should refresh token be re-issued when a new access token is requested? The spec says "The authorization server MAY issue a new refresh token" and I see this is quite common behaviour for other authorization servers. Should I make it default behaviour or add a configuration option for that?
  • Refresh token expiration - kind of the same question, should this be configurable? I see a comment that configuring access token expiration is a TODO item, if there'll be a separate story refresh token can also be included there.

Also there is a bit of code repetition in *Provider classes when issuing an access token, I'm not changing that for OAuth2RefreshTokenAuthenticationProvider, I believe you have refactoring in mind?

@jgrandja jgrandja self-assigned this Oct 13, 2020
@jgrandja jgrandja added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 13, 2020
@jgrandja jgrandja added this to the 0.0.3 milestone Oct 13, 2020
@jgrandja
Copy link
Collaborator

@alek-sys

Should refresh token be re-issued when a new access token is requested?

Let's introduce TokenSettings.reuseRefreshTokens(boolean) with default false

Refresh token expiration - kind of the same question, should this be configurable?

Let's introduce TokenSettings.refreshTokenTimeToLive(Duration) with default 60 mins. If it's set to 0 then it's disabled.

Also there is a bit of code repetition in *Provider classes when issuing an access token...

I'm planning on introducing interface OAuth2TokenIssuer. In preparation for that, can you extract the common logic between the 2 existing providers and OAuth2RefreshTokenAuthenticationProvider into a package-private OAuth2TokenIssuerUtil. This way we can flush out the interface and refactor after we merge this PR.

@alek-sys
Copy link
Contributor Author

alek-sys commented Oct 23, 2020

Thanks @jgrandja, all done now. Feel free to review whenever you are ready.

Also, this PR only covers opaque refresh tokens. Do you think JWT tokens should be supported as a part of this PR as well?

@alek-sys alek-sys force-pushed the master branch 2 times, most recently from a19daab to 77ab3f0 Compare October 23, 2020 15:37
Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @alek-sys ! Please see review comments.

@alek-sys alek-sys force-pushed the master branch 3 times, most recently from ed9a404 to 37da4b2 Compare October 26, 2020 17:52
@alek-sys
Copy link
Contributor Author

Thanks for review @jgrandja, I think I addressed all your comments now. Also to recap what we discussed privately: enableRefreshTokens flag stays, refreshTokenTimeToLive is enforced to be greater than Duration.ZERO so that refresh tokens always have expiry date.

@jgrandja jgrandja added the status: duplicate A duplicate of another issue label Oct 30, 2020
jgrandja added a commit that referenced this pull request Oct 30, 2020
@jgrandja
Copy link
Collaborator

Thanks for all the updates @alek-sys ! This is now in master.

@jgrandja jgrandja closed this Oct 30, 2020
jgrandja added a commit that referenced this pull request Nov 3, 2020
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants