Fix to allow multiple AuthenticationFilter instances to process each request

[therepanic]

This change will enable us to use multiple AuthenticationFilter.

Fix: #17173

[jgrandja]

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

[therepanic]

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.

[therepanic]

I made the changes you asked for. Including adding a test.

Before the changes, this test failing because converter2 is not called. With the changes this text runs as it should.

[jgrandja]

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.

[jgrandja]

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

[therepanic]

Thanks for the advice. You want me to create a separate issue and link the PR to it?

[jgrandja]

@therepanic See comment

[therepanic]

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?

[jgrandja]

@therepanic Maybe you missed my last comment?

We should fix this in AuthenticationFilter as it's a bug. Please go back to previous commit and override getAlreadyFilteredAttributeName() as follows:

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

[therepanic]

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.

[jgrandja]

You are right the test will fail because of how Mockito names the mocks.

Let's change the implementation as follows:

@Override
protected String getAlreadyFilteredAttributeName() {
	String name = getFilterName();
	if (name == null) {
		name = getClass().getName().concat("-" + System.identityHashCode(this));
	}
	return name + ALREADY_FILTERED_SUFFIX;
}

[therepanic]

Thanks, my previous implementation in the last commit used System.identityHashCode as well. I reverted everything and now I think we are fine.

[jgrandja]

Thanks for the updates @therepanic. This is now merged.