Skip to content

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