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?

[jzheaux]

Thanks for the next revision here and for your patience. I've been letting this one metabolize to ensure the solution is getting us closer to being able to use AuthenticationFilter while also honoring a separate initiative to make Authentication instances immutable.

So, :), in addition to my latest inline feedback, will you please update the @since attributes to 7.0 and use the 7.0 rnc?

[jzheaux]

Will you please mark this as deprecated?

[jzheaux]

It's a bit tricky to be having the authentication converter setting the details as well as the filter. Instead of doing it in both places, will you please add to BearerTokenAuthenticatinFilter a private static adapter class and hold a default bearerTokenResolver and authenticationDetailsSource as members? This adapter would be the default authenticationConverter for BearerTokenAuthenticationFilter.

If BearerTokenAuthenticationFilter#setBearerTokenResolver or #setAuthenticationDetailsSource is called, it will set the adapter's members accordingly if authenticationConverter is of the right type. Otherwise, the setters return an error like "setBearerTokenResolver and setAuthenticationConverter cannot be used together".

I think this makes sense because BearerTokenAuthenticationConverter takes care of resolving the token and populating it with details. So it's a usage error to try and do either of those here once you've specified an AuthenticationConverter.

[franticticktick]

I think we can't fully guarantee backward compatibility this way. I can make an adapter like this:

	public static final class BearerTokenAuthenticationConverterAdapter implements AuthenticationConverter {
		private final Log logger = LogFactory.getLog(this.getClass());

		private AuthenticationDetailsSource<HttpServletRequest, ?> authenticationDetailsSource = new WebAuthenticationDetailsSource();
		private BearerTokenResolver bearerTokenResolver = new DefaultBearerTokenResolver();

		@Override
		public Authentication convert(HttpServletRequest request) {
			String token = this.bearerTokenResolver.resolve(request);
			if (!StringUtils.hasText(token)) {
				this.logger.trace("Did not process request since did not find bearer token");
				return null;
			}
			AbstractAuthenticationToken authenticationRequest = new BearerTokenAuthenticationToken(token);
			authenticationRequest.setDetails(this.authenticationDetailsSource.buildDetails(request));

            return authenticationRequest;
		}

		public void setAuthenticationDetailsSource(AuthenticationDetailsSource<HttpServletRequest, ?> authenticationDetailsSource) {
			Assert.notNull(authenticationDetailsSource, "authenticationDetailsSource cannot be null");
			this.authenticationDetailsSource = authenticationDetailsSource;
		}

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

		public BearerTokenResolver getBearerTokenResolver() {
			return this.bearerTokenResolver;
		}
	}

It has both authenticationDetailsSource and bearerTokenResolver. At the same time, when calling the setAuthenticationDetailsSource method, I can check the authenticationConverter type:

	@Deprecated
	public void setAuthenticationDetailsSource(
			AuthenticationDetailsSource<HttpServletRequest, ?> authenticationDetailsSource) {
		if(this.authenticationConverter instanceof BearerTokenAuthenticationConverterAdapter adapter) {
			Assert.notNull(authenticationDetailsSource, "authenticationDetailsSource cannot be null");
			adapter.setAuthenticationDetailsSource(authenticationDetailsSource);
		} else {
			throw new IllegalStateException("authenticationDetailsSource and authenticationConverter cannot be use together");
		}
	}

But there is one problem, if the user wanted to call the setAuthenticationDetailsSource method without explicitly defining BearerTokenResolver, he will get an error. Pay attention to the test OAuth2ResourceServerConfigurerTests.requestWhenCustomAuthenticationDetailsSourceThenUsed.

	@Test
	public void requestWhenCustomAuthenticationDetailsSourceThenUsed() throws Exception {
		this.spring.register(CustomAuthenticationDetailsSource.class, JwtDecoderConfig.class, BasicController.class)
			.autowire();
		JwtDecoder decoder = this.spring.getContext().getBean(JwtDecoder.class);
		given(decoder.decode(anyString())).willReturn(JWT);
		this.mvc.perform(get("/authenticated").with(bearerToken(JWT_TOKEN)))
			.andExpect(status().isOk())
			.andExpect(content().string(JWT_SUBJECT));
		verifyBean(AuthenticationDetailsSource.class).buildDetails(any());
	}

This test will not pass. authenticationDetailsSource via withObjectPostProcessor, but the bearerTokenResolver was not explicitly set, which means that BearerTokenAuthenticationFilter will use BearerTokenAuthenticationConverter as the converter. In OAuth2ResourceServerConfigurer:

	AuthenticationConverter getAuthenticationConverter() {
		if (this.authenticationConverter != null) {
			return this.authenticationConverter;
		}
		if (this.context.getBeanNamesForType(AuthenticationConverter.class).length > 0) {
			this.authenticationConverter = this.context.getBean(AuthenticationConverter.class);
		}
		else if (this.context.getBeanNamesForType(BearerTokenResolver.class).length > 0) {
			BearerTokenResolver bearerTokenResolver = this.context.getBean(BearerTokenResolver.class);
			BearerTokenAuthenticationFilter.BearerTokenAuthenticationConverterAdapter converter = new BearerTokenAuthenticationFilter.BearerTokenAuthenticationConverterAdapter();
			converter.setBearerTokenResolver(bearerTokenResolver);
			this.authenticationConverter = converter;
		}
		else {
			this.authenticationConverter = new BearerTokenAuthenticationConverter();
		}
		return this.authenticationConverter;
	}

Simply put, authenticationDetailsSource can only be set if bearerTokenResolver is explicitly specified via bean or via configurer. Otherwise, BearerTokenAuthenticationFilter will use BearerTokenAuthenticationConverter. Without explicitly specified bearerTokenResolver, setting authenticationDetailsSource is simply pointless. But we can do this:

	@Deprecated
	public void setBearerTokenResolver(BearerTokenResolver bearerTokenResolver) {
		logger.warn("If you specify an authenticationConverter it will not be used.");
		BearerTokenAuthenticationConverterAdapter adapter = new BearerTokenAuthenticationConverterAdapter();
		adapter.setBearerTokenResolver(bearerTokenResolver);
		this.authenticationConverter = adapter;
	}

	@Deprecated
	public void setAuthenticationDetailsSource(
			AuthenticationDetailsSource<HttpServletRequest, ?> authenticationDetailsSource) {
		logger.warn("If you specify an authenticationConverter it will not be used. " +
				"Instead the DefaultBearerTokenResolver will be used in combination with the authenticationDetailsSource you specify.");
		BearerTokenAuthenticationConverterAdapter adapter = new BearerTokenAuthenticationConverterAdapter();
		adapter.setAuthenticationDetailsSource(authenticationDetailsSource);
		this.authenticationConverter = adapter;
	}

At the same time, we will preserve backward compatibility as much as possible. All tests will remain as is, we will warn the user that authenticationConverter will not be used. @jzheaux what do you think about this?

[franticticktick]

The correct solution would be like this:

	@Deprecated
	public void setAuthenticationDetailsSource(
			AuthenticationDetailsSource<HttpServletRequest, ?> authenticationDetailsSource) {
		if(this.authenticationConverter instanceof BearerTokenAuthenticationConverterAdapter adapter) {
			adapter.setAuthenticationDetailsSource(authenticationDetailsSource);
		} else {
			logger.warn("If you specify an authenticationConverter it will not be used. " +
					"Instead the DefaultBearerTokenResolver will be used in combination with the authenticationDetailsSource you specify.");
			BearerTokenAuthenticationConverterAdapter adapter = new BearerTokenAuthenticationConverterAdapter();
			adapter.setAuthenticationDetailsSource(authenticationDetailsSource);
			this.authenticationConverter = adapter;
		}
	}

And:

	@Deprecated
	public void setBearerTokenResolver(BearerTokenResolver bearerTokenResolver) {
		if(this.authenticationConverter instanceof BearerTokenAuthenticationConverterAdapter adapter) {
			adapter.setBearerTokenResolver(bearerTokenResolver);
		} else {
			logger.warn("If you specify an authenticationConverter it will not be used.");
			BearerTokenAuthenticationConverterAdapter adapter = new BearerTokenAuthenticationConverterAdapter();
			adapter.setBearerTokenResolver(bearerTokenResolver);
			this.authenticationConverter = adapter;	
		}
	}

[jzheaux]

I see, thanks, @franticticktick, for the research. Let's proceed with that, though please have the log be a TRACE log and use the language:

LogMessage.of(() -> String.format("Will override %s with %s", this.authenticationConverter, bearerTokenResolver))

and

LogMessage.of(() -> String.format("Will override %s with %s", this.authenticationConverter, authenticationDetailsSource))

The nice thing about log messages like this is that they read more like a sentence: "BearerTokenAuthenticationFilter will override ..."

I agree that the log, in combination with a deprecation message directing them to call BearerTokenAuthenticationConverter#setAuthenticationDetailsSource, should be a strong enough signal.

[jzheaux]

Will you please deprecate this method since BearerTokenAuthenticationConverter takes care of this?