Add BearerTokenAuthenticationConverter

[franticticktick]

Closes gh-14750

[jzheaux]

Will you please change BearerTokenAuthenticationFilter to accept an AuthenticationConverter? Also, OAuth2ResourceServerConfigurer should configure this in the same way as BearerTokenResolver. Will you please add that?

Finally, please add tests both for the new authentication converter and corresponding tests in OAuth2ResourceServerConfigurerTests.

[franticticktick]

Hi @jzheaux! Thanks for your feedback! I would like some advice on the best way to do this. I can add AuthenticationConverter to BearerTokenAuthenticationFilter and change doFilterInternal method:

        @Override
	protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
			throws ServletException, IOException {
		try {
			Authentication authentication = authenticationConverter.convert(request);
			if (authentication == null) {
				this.logger.trace("Did not process request since did not find bearer token");
				filterChain.doFilter(request, response);
				return;
			}
			AuthenticationManager authenticationManager = this.authenticationManagerResolver.resolve(request);
			Authentication authenticationResult = authenticationManager.authenticate(authentication);
			SecurityContext context = this.securityContextHolderStrategy.createEmptyContext();
			context.setAuthentication(authenticationResult);
			this.securityContextHolderStrategy.setContext(context);
			this.securityContextRepository.saveContext(context, request, response);
			if (this.logger.isDebugEnabled()) {
				this.logger.debug(LogMessage.format("Set SecurityContextHolder to %s", authenticationResult));
			}
			filterChain.doFilter(request, response);
		} catch (AuthenticationException failed) {
			this.securityContextHolderStrategy.clearContext();
			this.logger.trace("Failed to process authentication request", failed);
			this.authenticationFailureHandler.onAuthenticationFailure(request, response, failed);
		}
	}

But there are several problems.
Firstly, it is necessary to save backward compatibility with BearerTokenResolver, which were set through OAuth2ResourceServerConfigurer, for example:

SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
			// @formatter:off
			http
					.authorizeRequests()
					.anyRequest().authenticated()
					.and()
					.oauth2ResourceServer()
					.bearerTokenResolver(customResolver())
					.jwt();
			return http.build();

Even if i declare method bearerTokenResolver as deprecated it should still work. And it seems hard to do, i could try adding a BearerTokenResolverAuthenticationConverterAdapter which would do something like this:

	private static final class BearerTokenResolverAuthenticationConverterAdapter implements AuthenticationConverter {

		private final BearerTokenResolver bearerTokenResolver;

		BearerTokenResolverAuthenticationConverterAdapter(BearerTokenResolver bearerTokenResolver) {
			Assert.notNull(bearerTokenResolver, "bearerTokenResolver cant be null");
			this.bearerTokenResolver = bearerTokenResolver;
		}

		@Override
		public Authentication convert(HttpServletRequest request) {
			String token = this.bearerTokenResolver.resolve(request);
			if (StringUtils.hasText(token)) {
				return new BearerTokenAuthenticationToken(token);
			}
			return null;
		}

	}

And use it only when a custom BearerTokenResolver is specified, for example:

BearerTokenResolver getBearerTokenResolver() {
		if (this.bearerTokenResolver == null) {
			if (this.context.getBeanNamesForType(BearerTokenResolver.class).length > 0) {
				this.bearerTokenResolver = this.context.getBean(BearerTokenResolver.class);
			} else {
				this.bearerTokenResolver = new DefaultBearerTokenResolver();
				return this.bearerTokenResolver;
			}
		}
		this.authenticationConverter = new BearerTokenResolverAuthenticationConverterAdapter(this.bearerTokenResolver);
		return this.bearerTokenResolver;
	}

Not the most beautiful method, but it partially solves the problem. Next you will need to do something with BearerTokenRequestMatcher, of course it can be changed to:

                 @Override
		public boolean matches(HttpServletRequest request) {
			try {
				Authentication authentication = authenticationConverter.convert(request);
				return authentication != null;
			} catch (OAuth2AuthenticationException ex) {
				return false;
			}
		}

But I'm not sure about this solution. In addition, there may be a problem with AuthenticationDetailsSource, in BearerTokenAuthenticationFilter it can be changed, but now it must be changed only through AuthenticationConverter.

[franticticktick]

Hi @jzheaux! I have added authenticationConverter to OAuth2ResourceServerConfigurer and to BearerTokenAuthenticationFilter. To ensure backward compatibility with bearerTokenResolver, several changes had to be made. First, i changed BearerTokenRequestMatcher:

private static final class BearerTokenRequestMatcher implements RequestMatcher {

		private BearerTokenResolver bearerTokenResolver;

		private AuthenticationConverter authenticationConverter;

		@Override
		public boolean matches(HttpServletRequest request) {
			try {
				if (this.bearerTokenResolver != null) {
					return this.bearerTokenResolver.resolve(request) != null;
				}
				return this.authenticationConverter.convert(request) != null;
			}
			catch (OAuth2AuthenticationException ex) {
				return false;
			}
		}

		void setBearerTokenResolver(BearerTokenResolver tokenResolver) {
			Assert.notNull(tokenResolver, "resolver cannot be null");
			this.bearerTokenResolver = tokenResolver;
		}

		void setAuthenticationConverter(AuthenticationConverter authenticationConverter) {
			Assert.notNull(authenticationConverter, "authenticationConverter cannot be null");
			this.authenticationConverter = authenticationConverter;
		}

	}

Now it works with authenticationConverter or with bearerTokenResolver, but not with both. If a bearerTokenResolver was specified, then the BearerTokenAuthenticationFilter will be used along with the authenticationDetailsSource. By default, authenticationConverter will be used if bearerTokenResolver is not specified:

        @Override
	public void configure(H http) {
		AuthenticationManagerResolver resolver = this.authenticationManagerResolver;
		if (resolver == null) {
			AuthenticationManager authenticationManager = getAuthenticationManager(http);
			resolver = (request) -> authenticationManager;
		}
		BearerTokenAuthenticationFilter filter = new BearerTokenAuthenticationFilter(resolver);

		BearerTokenResolver bearerTokenResolver = getBearerTokenResolver();
		if (bearerTokenResolver != null) {
			this.requestMatcher.setBearerTokenResolver(bearerTokenResolver);
			filter.setBearerTokenResolver(bearerTokenResolver);
		}
		else {
			AuthenticationConverter converter = getAuthenticationConverter();
			this.requestMatcher.setAuthenticationConverter(converter);
			filter.setAuthenticationConverter(converter);
		}

		filter.setAuthenticationConverter(getAuthenticationConverter());
		filter.setAuthenticationEntryPoint(this.authenticationEntryPoint);
		filter.setSecurityContextHolderStrategy(getSecurityContextHolderStrategy());
		filter = postProcess(filter);
		http.addFilter(filter);
	}

In BearerTokenAuthenticationFilter authenticationConverter will work if bearerTokenResolver is not set:

                 Authentication authentication;
		try {
			if (this.bearerTokenResolver != null) {
				String token = this.bearerTokenResolver.resolve(request);
				if (!StringUtils.hasText(token)) {
					this.logger.trace("Did not process request since did not find bearer token");
					return;
				}
				authentication = bearerTokenAuthenticationToken(token, request);
			}
			else {
				authentication = this.authenticationConverter.convert(request);
			}
		}
		catch (OAuth2AuthenticationException invalid) {
			this.logger.trace("Sending to authentication entry point since failed to resolve bearer token", invalid);
			this.authenticationEntryPoint.commence(request, response, invalid);
			return;
		}

		if (authentication == null) {
			this.logger.trace("Failed to convert authentication request");
			filterChain.doFilter(request, response);
			return;
		}

Tests work as is. AuthenticationDetailsSource can only be set if bearerTokenResolver has been set. I think bearerTokenResolver can be noted as deprecated.

[franticticktick]

Hi @jzheaux ! Maybe it’s worth adding BearerTokenAuthenticationConverter to the BearerTokenAuthenticationFilter as a separate issue? This issue does not seem too easy and may affect the current functionality.

[jzheaux]

@CrazyParanoid, thanks for your patience as I have been out of town.

We certainly want to remain passive and don't want to change behavior unnecessarily. Also, though, we don't typically add filter components that cannot be used by existing filters.

I'll take a look at the PR this week and see if I can find a way to simplify things. Thank you for all your research details.

[jzheaux]

@franticticktick I realize it's been a while since you opened this PR. I've reviewed it and posted a PR to your repo that you can merge if you agree to the changes. We can see from there what other changes might be needed.

[jzheaux]

Thanks for the updates, @franticticktick. This is coming together nicely. I've left some feedback inline.

[jzheaux]

Even though, bearerTokenResolver doesn't have a JavaDoc, please add one for authenticationConverter

[jzheaux]

Please add a JavaDoc here that at least has the following:

/**
  * @deprecated please use {@link #authenticationConverter} instead
  */

[jzheaux]

Typically in XML support, two properties that set the same underlying value cannot be specified together.

Will you please add a check that errors if both authentication-converter-ref and bearer-token-resolver-ref are specified? The error message would say something like "you cannot use bearer-token-ref and authentication-converter-ref in the same oauth2-resource-server element".

In the Java DSL, the rule is "whatever is the last method called"; however, which one is the "last attribute" is not always clear.

[franticticktick]

I added such a check. Also, I removed RootBeanDefinition from getAuthenticationConverter and getBearerTokenResolver methods. We need to handle the case when bearerTokenResolver and authenticationConverter are null. This will mean that the user left the default configuration and we can set authenticationConverter to RootBeanDefinition.

		BeanMetadataElement bearerTokenResolver = getBearerTokenResolver(oauth2ResourceServer);
		BeanMetadataElement authenticationConverter = getAuthenticationConverter(oauth2ResourceServer);
		if (bearerTokenResolver != null && authenticationConverter != null) {
			throw new BeanDefinitionStoreException(
					"You cannot use bearer-token-ref and authentication-converter-ref in the same oauth2-resource-server element");
		}
		if (bearerTokenResolver == null && authenticationConverter == null) {
			authenticationConverter = new RootBeanDefinition(BearerTokenAuthenticationConverter.class);
		}

After these checks we are guaranteed to have either bearerTokenResolver or authenticationConverter left. And we can create a requestMatcher and add PropertyValue to the filterBuilder.

[jzheaux]

You will also need to add this to spring-security-6.5.rnc and then run ./gradlew :spring-security-config:rncToXsd to update the equivalent XSD. After that, there are some tests that will check that you have documented the attribute.

[jzheaux]

Can this test stay? I prefer not to remove tests until the code it is testing is removed. This simplifies proving backward compatibility.

[franticticktick]

This won't work. We'll never get DefaultBearerTokenResolver, since BearerTokenAuthenticationConverter now does the main work. At the same time, we have BearerTokenResolverAuthenticationConverterAdapter, which replaces any custom BearerTokenResolver, of course, it can have DefaultBearerTokenResolver, but only if the developer defines it himself. In other words, instead of DefaultBearerTokenResolver, we now have BearerTokenAuthenticationConverter.

[jzheaux]

Please update this to be 6.5

[jzheaux]

Please change this to 6.5

[jzheaux]

Thanks for the additional test cleanup! Will you please place any tests or cleanup that aren't part of adding BearerTokenAuthenticationConverter into a previous commit?

[franticticktick]

Hi @jzheaux , i need some more time to tidy up this PR. Also, I have a few questions, I'll write a new comment a bit later.

[franticticktick]

Hey @jzheaux , thanks for the warmth. Could you review my latest changes please?