Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow one to customize the AuthenticationConverter in BasicAuthenticationFilter #13988

Closed
mmoayyed opened this issue Oct 10, 2023 · 5 comments
Closed
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Milestone

Comments

@mmoayyed
Copy link

mmoayyed commented Oct 10, 2023

Expected Behavior

Presently in version 6.2.0-M3, there is no possibility to customize the authenticationConverter that is provided by BasicAuthenticationFilter, which means post-processing of the filter does not allow one to control the authentication conversion. Customizing the authentication converter allows one to decide where, when and how the filter should actually process a given request, i.e.:

// A custom implementation of authenticationConverter can return null here
var authRequest = authenticationConverter.convert(request);
if (authRequest == null) {
    this.logger.trace("Did not process authentication request");
    chain.doFilter(request, response);
    return;
}

The proposal in summary is,

  1. Provide a setter for authenticationConverter in BasicAuthenticationFilter
  2. Similar to OidcLogoutAuthenticationConverter, allow one to customize the request matching functionality.

Current Behavior

Not possible to customize the authentication conversion process for this filter without reflection or a brand new filter. The main driver for this is to allow the filter match on certain requests, letting SS to handle those, while ignoring other (authenticated) requests and letting the app handle those.

Context

If HttpSecurity is configured for basic-authentication, it is not possible (or at least seems this way) to decide when and for which requests the basic auth filter should execute. Compared with OidcLogoutAuthenticationConverter one is given a customizable request matcher. In contrast, the BasicAuthenticationFilter, matches on everything and anything that is able to produce the right kind of credentials. It seems impossible for the filter to back away, when a request contains credentials, allowing the app to handle that request. Perhaps that can be done using multiple filter chains, web customizers, etc all of which seem somewhat unnecessarily complex compared to the option here.

As ever, thank you!

@mmoayyed
Copy link
Author

PS I am happy to provide a PR here if you feel like this proposal might be appropriate to implement.

@marcusdacoregio marcusdacoregio self-assigned this Oct 10, 2023
@marcusdacoregio marcusdacoregio added in: web An issue in web modules (web, webmvc) and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 10, 2023
@marcusdacoregio
Copy link
Contributor

Hi, @mmoayyed. Thanks for the report.

I agree that the AuthenticationConverter should be customizable via a setter allowing the user to specify an ObjectPostProcessor.

Are you able to create a PR that targets the main branch and add the setter and related tests?
6.2.0-RC1 will be released on Monday. Ideally, this PR should come at maximum on Wednesday so we have time to add it to the release. If not, we will only be able to add such support in 6.3. If you do not have time to provide the PR in time, please let me know so I can work on it.

@mmoayyed
Copy link
Author

Are you able to create a PR that targets the main branch and add the setter and related tests?

Certainly, sure. Thank you. I'll try to put together something later today to be reviewed.

@mmoayyed
Copy link
Author

Please see #13989

@mmoayyed
Copy link
Author

Extra note: the PR I put together allows AuthenticationConverter to be customizable via a setter. I would still maintain that the filter or (the converter) should allow one to customize the request matching functionality. I can of course do that separately if you think that might be a good idea. (I handle this atm with a custom converter).

marcusdacoregio added a commit to marcusdacoregio/spring-security that referenced this issue Oct 10, 2023
@marcusdacoregio marcusdacoregio added this to the 6.2.0-RC1 milestone Oct 11, 2023
@marcusdacoregio marcusdacoregio added the status: duplicate A duplicate of another issue label Oct 11, 2023
jzheaux added a commit that referenced this issue Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants