Abstracts the method createAuthenticationToken

[kcl-co]

The implements can replace the authentication token, which will be PreAuthenticatedAuthenticationToken or its subclass, by overriding the method createAuthenticationToken.

The system may be authenticated by several external system, and if it only can return the type of the PreAuthenticatedAuthenticationToken, it is difficult to distinguish the different external system, that can have different roles or authorities.

Such as, we can check the type in the implements of the AuthenticationProvider or AbstractAccessDecisionManager, and handle additionally because of correct type.

[pivotal-cla]

@kcl-co Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@kcl-co Thank you for signing the Contributor License Agreement!

[sjohnr]

Thanks for the PR, @kcl-co! Just a heads up we are focused on releasing 5.8 and 6.0, and so may be delayed on a review.

[kcl-co]

By this submission, we can implement multiple AbstractPreAuthenticatedProcessingFilter and create different authentication when there are more than one external system, then implement specified AuthenticationProvider to handle it, specified by method supports.

The implements can replace the authentication token, which will be PreAuthenticatedAuthenticationToken or its subclass, by overriding the method createAuthenticationToken.

The system may be authenticated by several external system, and if it only can return the type of the PreAuthenticatedAuthenticationToken, it is difficult to distinguish the different external system, that can have different roles or authorities.

Such as, we can check the type in the implements of the AuthenticationProvider or AbstractAccessDecisionManager, and handle additionally because of correct type.

[kcl-co]

OK! Thank you for your reply @sjohnr

Thanks for the PR, @kcl-co! Just a heads up we are focused on releasing 5.8 and 6.0, and so may be delayed on a review.

[jzheaux]

Once we are able to come around to this, I think it's worth considering AuthenticationConverter instead. This encourages composition over inheritance and it is what AuthenticationFilter and other filters are starting to use.

[kcl-co]

Once we are able to come around to this, I think it's worth considering AuthenticationConverter instead. This encourages composition over inheritance and it is what AuthenticationFilter and other filters are starting to use.

Yes, i should use converter component, instead of overriding method by inheritance. I will push new commit record about it and test unit. Think you for your suggestion.

[kcl-co]

Once we are able to come around to this, I think it's worth considering AuthenticationConverter instead. This encourages composition over inheritance and it is what AuthenticationFilter and other filters are starting to use.

@jzheaux I review your suggestion and the code of AbstractPreAuthenticatedProcessingFilter, i think that it will nest too many layers in method of doFilter if i instead of AuthenticationConverter, and the AbstractPreAuthenticatedProcessingFilter is a abstract class which always need to be extended, so overriding the method of createAuthenticationToken is so normal. Maybe, we should use the protected convert method which used to convert Authentication.

[sjohnr]

@kcl-co

Maybe, we should use the protected convert method which used to convert Authentication.

Hopefully I'm understanding you correctly, that you're suggesting we still consider a protected method that can be overridden. If so, I think @jzheaux's suggestion could still be a better route. Introducing an AuthenticationConverter with a default implementation seems ideal. It also begins to align the AbstractPreAuthenticatedProcessingFilter with other filters which is a significant improvement for the community in terms of understanding how the filters work.

It would require a bit of refactoring, and would possibly move at least one log entry (preAuthenticatedPrincipal = %s, trying to authenticate) inside the converter. It's worth considering whether that's what we want.

It could look something like this:

private AuthenticationConverter authenticationConverter = this::getPreAuthenticatedAuthenticationToken;

private PreAuthenticatedAuthenticationToken getPreAuthenticatedAuthenticationToken(HttpServletRequest request) {
	Object principal = getPreAuthenticatedPrincipal(request);
	if (principal == null) {
		return null;
	}
	this.logger.debug(LogMessage.format("preAuthenticatedPrincipal = %s, trying to authenticate", principal));
	Object credentials = getPreAuthenticatedCredentials(request);
	return new PreAuthenticatedAuthenticationToken(principal, credentials);
}

The doAuthenticate method would need to change to handle an Authentication that can also be null and set the details only when the authentication is of type AbstractAuthenticationToken.

What do you think about this?

[kcl-co]

@sjohnr

What do you think about this?

I'm understanding you, i have read other filter using convert component from spring security source code, and i know "Composition over inheritance". The reason why i want to implement it by overriding is that AbstractPreAuthenticatedProcessingFilter is a abstract class, extending it by overriding is a better way(Inheritance), rather than using convert component (Composition). We should not apply mechanically.

Of course, if you want to "align the AbstractPreAuthenticatedProcessingFilter with other filters which is a significant improvement for the community in terms of understanding how the filters work. ", i suggest include the detail setting, according to the define of AuthenticationConverter :

private AuthenticationConverter authenticationConverter = this::getPreAuthenticatedAuthenticationToken;

private PreAuthenticatedAuthenticationToken getPreAuthenticatedAuthenticationToken(HttpServletRequest request) {
	Object principal = getPreAuthenticatedPrincipal(request);
	if (principal == null) {
		return null;
	}
	this.logger.debug(LogMessage.format("preAuthenticatedPrincipal = %s, trying to authenticate", principal));
	Object credentials = getPreAuthenticatedCredentials(request);
	PreAuthenticatedAuthenticationToken token = new PreAuthenticatedAuthenticationToken(principal, credentials);
        token.setDetails(this.authenticationDetailsSource.buildDetails(request));
        return token;
}

However, i feel it will nest too many layers If we use this way, and i suggest remove the layer of the doAuthenticate, such as:

@Override
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
		throws IOException, ServletException {
	if (this.requiresAuthenticationRequestMatcher.matches((HttpServletRequest) request)) {
		try {
			PreAuthenticatedAuthenticationToken authenticationRequest = (PreAuthenticatedAuthenticationToken) authenticationConverter.convert(request))
			Authentication authenticationResult = this.authenticationManager.authenticate(authenticationRequest);
			successfulAuthentication(request, response, authenticationResult);
		} catch (AuthenticationException ex) {
			unsuccessfulAuthentication(request, response, ex);
			if (!this.continueFilterChainOnUnsuccessfulAuthentication) {
				throw ex;
			}
		}
	}
	chain.doFilter(request, response);
}

If no problem, i will modify and commit again. What do you think about this?

[sjohnr]

Hi @kcl-co,

i suggest include the detail setting, according to the define of AuthenticationConverter :

I think that makes sense, thanks for suggesting it!

However, i feel it will nest too many layers If we use this way, and i suggest remove the layer of the doAuthenticate, such as:

I would seek to minimize the change to only what is required for this enhancement. While it is nice to refactor the code to make it better, this type of change would actually make it more difficult to maintain the class because we may need to patch older versions prior to the refactor and such a change would potentially cause conflicts. For that reason, I would suggest not to make the refactoring change.

I would also not recommending casting back to PreAuthenticatedAuthenticationToken as I don't believe this is necessary.

Lastly, I forgot to mention but make sure to include a setAuthenticationConverter(...) on the filter when introducing the converter, so that it can be customized by users. You will want to add @since 6.1 in the javadoc for the new method.

[kcl-co]

@sjohnr OK! I try to minimize the change.

[kcl-co]

Hi @sjohnr, should I switch branch upstream/5.6.x , create new branch to commit my modified content and new merge request, that i read from CONTRIBUTING.adoc.

Currently, I commit code and merge it to origin/main by branch of 'main'.

[sjohnr]

Hi @kcl-co!

should I switch branch upstream/5.6.x

It's not really necessary. We can accept PRs against either the target branch (e.g. 5.6.x) or main. It's not very reasonable to expect our contributors to keep up with maintaining older supported versions, so we can handle it for you. :wink:

[kcl-co]

@sjohnr I also think the code is not elegant if i fix through composition. Although we can do it, we forget that the method getPreAuthenticatedPrincipal and getPreAuthenticatedCredentials is a abstract method, which will break the usage of the AbstractPreAuthenticatedProcessingFilter.

Such as, when we modify the token, we will replace the convert by setter method, and the getPreAuthenticatedPrincipal and getPreAuthenticatedCredentials will out of work.

What do you think about this?

[sjohnr]

Hi @kcl-co!

Although we can do it, we forget that the method getPreAuthenticatedPrincipal and getPreAuthenticatedCredentials is a abstract method

You are correct that setting a custom authenticationConverter would replace the use of these two abstract methods.

I also think the code is not elegant if i fix through composition.

It sounds like I am not able to convince you that introducing composition into this abstract class is the way to go here. I do understand what you're saying, but I don't really want to debate preferences here. I think that given the nature of the existing abstract class heavily relying on inheritance already, we can go ahead and move forward with making the method protected.

Would you be able to rebase on the latest main and add a test that verifies you can replace the implementation of the method and receive a custom authentication (e.g. TestingAuthenticationToken)?

[kcl-co]

@sjohnr OK！

[kcl-co]

@sjohnr Hi, I have pushed my latest commit, which contain token replacement and junit test.

PS. Im sorry so late to do it, because i was busy working...

[kcl-co]

And i dont use the git operation 'rebase' to merge the latest code, instead of using merge, because of my client of github auto-sync. if the operation must be 'rebase' for merging, i can recommit in new branch. What do you think about this? @sjohnr

[sjohnr]

@kcl-co I don’t prefer to merge with merge commits and it would also be nice to squash commits. I can do this for you however, if it’s a problem for you. So you can leave the branch as-is.

[kcl-co]

@sjohnr Thanks！I'm no problem for it and you can do it by your way😊.

[jzheaux]

Thanks for all your work on this @kcl-co. Unfortunately, neither approach (an AuthenticationConverter setter nor a protected method) prevents an implementer from having to provide dummy implementations for the two abstract methods, which I feel is a code smell.

The system may be authenticated by several external system, and if it only can return the type of the PreAuthenticatedAuthenticationToken

Given that you already have a custom way to construct a PreAuthentcatedAuthenticationToken, have you already tried the following:

@Bean 
SecurityFilterChain appSecurity(HttpSecurity http, MyCustomConverter converter) throws Exception {
    ProviderManager manager = new ProviderManager(new PreAuthenticatedAuthenticationProvider());
    AuthenticationFilter filter = new AuthenticationFilter(manager, converter);
    http
        // ...
        .addFilterBefore(filter, AbstractPreAuthenticatedProcessingFilter.class);
    return http.build();
}

[jzheaux]

Unfortunately, neither approach (an AuthenticationConverter setter nor a protected method) prevents an implementer from having to provide dummy implementations for the two abstract methods, which I feel is a code smell.

Given this, I'm going to decline this PR. @kcl-co, please reach out again if it seems I've misunderstood or if you have trouble applying my recommendation in place of changing the existing filter.

[kcl-co]

Unfortunately, neither approach (an AuthenticationConverter setter nor a protected method) prevents an implementer from having to provide dummy implementations for the two abstract methods, which I feel is a code smell.

I understand what you mean is the PR doesn't prevent implementers from having to provide dummy implementations for the two abstract methods(getPreAuthenticatedPrincipal and getPreAuthenticatedCredentials). Yes, it doesn't prevent.

However, I want to switch different permission system to auth different permission rather than preventing implementers from implementing them. You can see the example:

• https://github.com/kcl-co/dynamic-threadpool/blob/main/dtp-monitor/src/main/java/com/share/co/kcl/dtp/monitor/security/WebSecurityConfig.java
• https://github.com/kcl-co/dynamic-threadpool/blob/main/dtp-monitor/src/main/java/com/share/co/kcl/dtp/monitor/security/authentication/DtpConsoleProcessingFilter.java
• https://github.com/kcl-co/dynamic-threadpool/blob/main/dtp-monitor/src/main/java/com/share/co/kcl/dtp/monitor/security/authentication/DtpMonitorProcessingFilter.java

in which I have rewrited the class AbstractPreAuthenticatedProcessingFilter and the method of createAuthenticationToken.

On the other hand, I think the class of AuthenticationFilter is not the best fit for this scenario, in which I need override a lot of method. @jzheaux