Skip to content
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

Conversation

farrault
Copy link
Contributor

@farrault farrault commented Feb 4, 2019

During a oauth2 refresh if the authorization server doesn't return a new refresh token, keep the previous one

fix #6503

@pivotal-issuemaster
Copy link

@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.

@pivotal-issuemaster
Copy link

@farrault Thank you for signing the Contributor License Agreement!

@farrault farrault force-pushed the gh-6503-no-refresh-token-during-refresh branch 2 times, most recently from 64b3ccf to 4ce6ba1 Compare February 5, 2019 06:44
@rwinch rwinch requested a review from jgrandja February 5, 2019 20:04
Copy link
Contributor

@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.

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() {
Copy link
Contributor

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;
Copy link
Contributor

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() {
Copy link
Contributor

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;
Copy link
Contributor

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

@jgrandja jgrandja changed the title fix #6503 : during a oauth2 refresh if the authorization server doesn't return a new refresh token, keep the previous one Preserve existing refresh token if new refresh token not returned Feb 6, 2019
@jgrandja jgrandja added type: bug A general bug in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Feb 6, 2019
@jgrandja jgrandja added this to the 5.2.0.M2 milestone Feb 6, 2019
@rwinch rwinch self-requested a review February 6, 2019 18:21
Copy link
Member

@rwinch rwinch 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 @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

@farrault farrault force-pushed the gh-6503-no-refresh-token-during-refresh branch from 4ce6ba1 to 92e0cf0 Compare February 7, 2019 08:10
@farrault farrault force-pushed the gh-6503-no-refresh-token-during-refresh branch from 92e0cf0 to b2e5da0 Compare February 7, 2019 08:15
@farrault
Copy link
Contributor Author

farrault commented Feb 7, 2019

@jgrandja @rwinch : Done !
Kind regards

@jgrandja
Copy link
Contributor

jgrandja commented Feb 7, 2019

@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 :)

@jgrandja jgrandja closed this Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refreshing access token may remove refresh token from AuthorizedClient
4 participants