Allow AuthenticationConverter to be settable in BasicAuthenticationFilter #13989
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
marcusdacoregio
added
type: enhancement
There was a problem hiding this comment.
Thanks @mmoayyed, I've left some feedback inline.
When you are done, can you please squash your commits? Thanks!
| protected AuthenticationEntryPoint authenticationEntryPoint; | ||
|
|
||
| private AuthenticationManager authenticationManager; | ||
| protected final AuthenticationManager authenticationManager; |
There was a problem hiding this comment.
Can you please leave the PR with changes related only to the AuthenticationConverter?
| this.filter.setAuthenticationConverter(new BasicAuthenticationConverter() { | ||
| @Override | ||
| public UsernamePasswordAuthenticationToken convert(final HttpServletRequest request) { | ||
| if (request.getServletPath().equalsIgnoreCase("/ignored-request")) { | ||
| return null; | ||
| } | ||
| return super.convert(request); | ||
| } | ||
| }); |
There was a problem hiding this comment.
You can set the AuthenticationConverter into the filter in a specific test, there is no need to add it for every test method in @BeforeEach.
There was a problem hiding this comment.
In addition to that, we recommend using a delegate than extend the existing BasicAuthenticationConverter, something like:
static class MyAuthenticationConverter implements AuthenticationConverter {
private final BasicAuthenticationConverter delegate = new BasicAuthenticationConverter();
@Override
public UsernamePasswordAuthenticationToken convert(final HttpServletRequest request) {
// check if request matches, maybe use RequestMatcher here
return this.delegate.convert(request);
}
}Sometimes people refer to the tests as implementation examples, so it is better to align with what the team recommends and uses.
There was a problem hiding this comment.
A delegate is not possible here because the setter is:
public void setAuthenticationConverter(BasicAuthenticationConverter authenticationConverter) {}It's not possible to switch to:
public void setAuthenticationConverter(AuthenticationConverter authenticationConverter) {}...and change the field type to AuthenticationConverter because that breaks other things in the filter.
How would you like to proceed?
|
Thank you for the feedback. I will try to get to this tomorrow but if that is too late for the release you are more than welcome to take over from here or otherwise I was see what I can do in the coming days. |
|
Closed via 7e9d707 |
|
Thanks, @mmoayyed. I took over from where you left it and added the support into |
|
Thank you very much! |
Closes #13988