Introduce DPoPAuthenticationFilter to avoid conflict with other AuthenticationFilter
#17186
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
jgrandja
added
type: bug
| @@ -222,4 +223,9 @@ private Authentication attemptAuthentication(HttpServletRequest request, HttpSer | |||
| return authenticationResult; | |||
| } | |||
|
|
|||
| @Override | |||
| protected String getAlreadyFilteredAttributeName() { | |||
| return getClass().getName() + ".FILTERED"; | |||
There was a problem hiding this comment.
@therepanic This does not solve the issue as 2 instances of AuthenticationFilter in the chain would return the same value from getAlreadyFilteredAttributeName(), resulting in the 2nd AuthenticationFilter never being called.
Please adjust and add a test that demonstrates the fix.
There was a problem hiding this comment.
I apologize for not accurately solving the problem. We do want each AuthenticationFilter instance to be honored in the chain. What I did I guess won't work if the filters are the same and added to the chain. Then I guess we need to use something like identityHashCode for getAlreadyFilteredAttributeName()? I just want to make sure it's right for us. Thanks.
|
I made the changes you asked for. Including adding a test. Before the changes, this test failing because |
| @@ -222,4 +223,9 @@ private Authentication attemptAuthentication(HttpServletRequest request, HttpSer | |||
| return authenticationResult; | |||
| } | |||
|
|
|||
| @Override | |||
| protected String getAlreadyFilteredAttributeName() { | |||
There was a problem hiding this comment.
I just realized that this is a public API change and therefore cannot be applied in 6.5.1 release. We'll need to address this issue with AuthenticationFilter in a separate issue targeted for 7.0.
For now, we need to address gh-17173 and I think the easiest solution is to define the following in DPoPAuthenticationConfigurer:
private static final class DPoPAuthenticationFilter extends AuthenticationFilter {
private DPoPAuthenticationFilter(AuthenticationManager authenticationManager, AuthenticationConverter authenticationConverter) {
super(authenticationManager, authenticationConverter);
}
}
I think this should solve the bug.
There was a problem hiding this comment.
@therepanic Please hold off on the proposed change in my previous comment. The root issue is in AuthenticationFilter and we still need to address it as a bug. We might still be able to get your change into 6.5.1 but let me think about this for a bit.
There was a problem hiding this comment.
Thanks for the advice. You want me to create a separate issue and link the PR to it?
therepanic
changed the title
OncePerRequestFilter#getAlreadyFilteredAttributeName in AuthenticationFilterDPoPAuthenticationFilter to avoid conflict with other AuthenticationFilter
…thenticationFilter` (spring-projects#17173) Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
|
I believe that it is #17173 that we have now solved. However I also understand the problem that I solved in the last commit as you said for merge is now unavailable? What should we do about it now? |
|
@therepanic Maybe you missed my last comment? We should fix this in @Override
protected String getAlreadyFilteredAttributeName() {
String name = getFilterName();
if (name == null) {
name = AuthenticationFilter.class.getSimpleName() + "." +
getAuthenticationConverter().getClass().getSimpleName();
}
return name + ALREADY_FILTERED_SUFFIX;
}We'll need to get this fix into all supported OSS branches so please rebase the fix off of |
|
I apologize for the misunderstanding. I really didn't see that you asked in the next comment to leave it as is. You posted how it should be overrided and I have a question. Will this work at all? In my last commit I added a test like this: @Test
public void filterWhenInFilterChainThenBothAreExecuted() throws Exception {
MockHttpServletRequest request = new MockHttpServletRequest(“GET”, “/”);
MockHttpServletResponse response = new MockHttpServletResponse();
AuthenticationFilter filter1 = new AuthenticationFilter(this.authenticationManager,
this.authenticationConverter);
AuthenticationConverter converter2 = mock(AuthenticationConverter.class);
AuthenticationFilter filter2 = new AuthenticationFilter(this.authenticationManager, converter2);
FilterChain chain = new MockFilterChain(mock(Servlet.class), filter1, filter2);
chain.doFilter(request, response);
verify(this.authenticationConverter).convert(any());
verify(converter2).convert(any());
}Tell me if it is incorrect. However, if it is correct, then the test will fail with the solution you submitted. |
This change will enable us to use multiple
AuthenticationFilter.Fix: #17173