Added setter to make RequestCache injectable #8392
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
|
Hi @jgrandja, FYI: the same integration test (gradlew spring-security-config:integrationTest) ran just fine at my end. Let me know, if you envision any change that I can help with. Attached snap: Thank you. |
|
@parikshitdutta Apologies for the delay with this review. I'm currently working on some priority items but will get to this tomorrow or early next week. Thanks for your patience. |
jgrandja
added
in: oauth2
There was a problem hiding this comment.
Thanks for the PR @parikshitdutta. Please see my comments.
In addition, we need to ensure that a configured RequestCache is actually supplied to OAuth2AuthorizationCodeGrantFilter.setRequestCache(). Take a look at OAuth2LoginConfigurer to see how it configures OAuth2AuthorizationRequestRedirectFilter.setRequestCache() and apply the same.
| @@ -134,6 +134,17 @@ public final void setAuthorizationRequestRepository(AuthorizationRequestReposito | |||
| this.authorizationRequestRepository = authorizationRequestRepository; | |||
| } | |||
|
|
|||
| /** | |||
| * Sets the {@link RequestCache} used for storing the current request | |||
| * before redirecting the OAuth 2.0 Authorization Request. | |||
There was a problem hiding this comment.
The javadoc needs to be updated here as it does not save the current request. Instead it loads a saved request and replays it.
| @@ -130,6 +130,12 @@ public void setAuthorizationRequestRepositoryWhenAuthorizationRequestRepositoryI | |||
| .isInstanceOf(IllegalArgumentException.class); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
We should also have a test that asserts the supplied RequestCache is actually used.
|
Hi @jgrandja, please check as you find time, I will look forward to your feedback, if any. |
|
@parikshitdutta Looks like this was missed:
|
|
Hi @jgrandja, I regret that I missed to correct that part. Can you please suggest where you exactly want me to configure OAuth2AuthorizationCodeGrantFilter that way? I didn't get this part. |
It should be configured in |
|
Hi @jgrandja, please review as you find time, I will look forward to your feedback. |
|
Hi @jgrandja , if you envision any change for this PR that I can help with, please let me know while I look at some other open issues. Thank you. |
There was a problem hiding this comment.
Thanks for the updates @parikshitdutta. Please see review comments.
| @@ -218,6 +218,10 @@ private void configure(B builder) { | |||
| OAuth2AuthorizationRequestRedirectFilter authorizationRequestRedirectFilter = createAuthorizationRequestRedirectFilter(builder); | |||
| builder.addFilter(postProcess(authorizationRequestRedirectFilter)); | |||
| OAuth2AuthorizationCodeGrantFilter authorizationCodeGrantFilter = createAuthorizationCodeGrantFilter(builder); | |||
| RequestCache requestCache = builder.getSharedObject(RequestCache.class); | |||
There was a problem hiding this comment.
Can you move this logic to createAuthorizationCodeGrantFilter()
| @@ -218,6 +218,10 @@ private void configure(B builder) { | |||
| OAuth2AuthorizationRequestRedirectFilter authorizationRequestRedirectFilter = createAuthorizationRequestRedirectFilter(builder); | |||
| builder.addFilter(postProcess(authorizationRequestRedirectFilter)); | |||
| OAuth2AuthorizationCodeGrantFilter authorizationCodeGrantFilter = createAuthorizationCodeGrantFilter(builder); | |||
| RequestCache requestCache = builder.getSharedObject(RequestCache.class); | |||
| if (requestCache != null) { | |||
| authorizationCodeGrantFilter.setRequestCache(requestCache); | |||
There was a problem hiding this comment.
Please add a test in OAuth2ClientConfigurerTests to verify(requestCache).getRequest()
|
Thanks @jgrandja for your time reviewing changes, please validate my latest changes done based on your comments. |
|
Thanks for the PR @parikshitdutta ! This is now in master. |
|
@parikshitdutta I just logged #8536, which is the equivalent of the changes you applied in this PR but for the WebFlux stack. Let me know if you're interested and free to take #8536. |
|
@jgrandja I would love to, thanks for asking. Already asked for permission there, just seeing the message here now. |
updated requestCache member variable to non-final
added setter to make RequestCache injectable
added respective test to validate requestCache can not be set to null
added test that asserts the supplied RequestCache is actually used
updated OAuth2ClientConfigurer to set the configured requestCache
added test that verifies provided RequestCache in OAuth2ClientConfigurer is used
closes gh-8120