-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Preserve existing refresh token if new refresh token not returned #6504
Preserve existing refresh token if new refresh token not returned #6504
Conversation
@farrault Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
@farrault Thank you for signing the Contributor License Agreement! |
64b3ccf
to
4ce6ba1
Compare
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.
This looks great @farrault. Thank you!
There are a couple of minor updates (renames) required and also can you update the copyright header to 2019 for all 4 files.
} | ||
|
||
@Test | ||
public void filterWhenRefreshRequiredThenRefreshButRefreshResponseHasNotNewRefreshToken() { |
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.
Let's rename to filterWhenRefreshRequiredThenRefreshAndResponseDoesNotContainRefreshToken
@@ -94,6 +96,9 @@ | |||
@Mock | |||
private ServerWebExchange serverWebExchange; | |||
|
|||
@Captor | |||
private ArgumentCaptor<OAuth2AuthorizedClient> oauth2AuthorizedClientCaptor; |
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.
Let's rename to authorizedClientCaptor
} | ||
|
||
@Test | ||
public void filterWhenRefreshRequiredThenRefreshButRefreshResponseHasNotNewRefreshToken() { |
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.
Let's rename to filterWhenRefreshRequiredThenRefreshAndResponseDoesNotContainRefreshToken
@@ -101,6 +101,8 @@ | |||
private WebClient.RequestHeadersSpec<?> spec; | |||
@Captor | |||
private ArgumentCaptor<Consumer<Map<String, Object>>> attrs; | |||
@Captor | |||
private ArgumentCaptor<OAuth2AuthorizedClient> oauth2AuthorizedClientCaptor; |
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.
Let's rename to authorizedClientCaptor
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.
Thanks for the PR @farrault! I have one additional comment to Joe's feedback.
Can you please update the commit message to align with the Spring Security conventions? Specifically can you ensure it ends with Fixes: gh-6503
4ce6ba1
to
92e0cf0
Compare
…new refresh token, preserve the existing one Fixes: spring-projectsgh-6503
92e0cf0
to
b2e5da0
Compare
@farrault Thanks so much for the PR! The commit message needed to be updated - first line should be short (50 chars or less) summary of changes. I updated it for you and merged this to master. We appreciate your contribution and look forward to the next one :) |
During a oauth2 refresh if the authorization server doesn't return a new refresh token, keep the previous one
fix #6503