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

Support for override AuthenticationEntryPoint #152

Merged
merged 4 commits into from
Nov 5, 2023
Merged

Support for override AuthenticationEntryPoint #152

merged 4 commits into from
Nov 5, 2023

Conversation

nexus061
Copy link
Contributor

@nexus061 nexus061 commented Nov 2, 2023

I have added possibility to ovveride AuthenticationEntryPoint for excpetion handing and and oauth2ResourceServer

Copy link
Owner

@ch4mpy ch4mpy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to use @ConditionalOnMissingBean for the default values and inject a complete Customizer<ExceptionHandlingConfigurer<HttpSecurity>> in configureResourceServer method.

@@ -40,6 +41,7 @@ public static HttpSecurity configureResourceServer(
HttpSecurity http,
ServerProperties serverProperties,
SpringAddonsOidcResourceServerProperties addonsResourceServerProperties,
AuthenticationEntryPoint exceptionHandlerAuthenticationEntryPoint,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should we inject Customizer<ExceptionHandlingConfigurer<HttpSecurity>> instead of just the AuthenticationEntryPoint (see below for default beans). This would give more flexibility to framework users.


if (serverProperties.getSsl() != null && serverProperties.getSsl().isEnabled()) {
http.requiresChannel(channel -> channel.anyRequest().requiresSecure());
}

return httpPostProcessor.process(http);
}


public static HttpSecurity configureResourceServer(
Copy link
Owner

@ch4mpy ch4mpy Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this new method, in SpringAddonsOidcResourceServerBeans, expose beans as follow:

@ConditionalOnMissingBean
@Bean
AuthenticationEntryPoint authenticationEntryPoint() {
	return (request, response, authException) -> {
		response.addHeader(HttpHeaders.WWW_AUTHENTICATE, "Bearer realm=\"Restricted Content\"");
		response.sendError(HttpStatus.UNAUTHORIZED.value(), HttpStatus.UNAUTHORIZED.getReasonPhrase());
	};
}
	
@ConditionalOnMissingBean
@Bean
Customizer<ExceptionHandlingConfigurer<HttpSecurity>> exceptionHandlingCustomizer(AuthenticationEntryPoint authenticationEntryPoint) {
	return exceptionHandling -> exceptionHandling.authenticationEntryPoint(authenticationEntryPoint);
}

If an AuthenticationEntryPoint or Customizer<ExceptionHandlingConfigurer<HttpSecurity>> are provided in an application configuration, this beans would back off (because of @ConditionalOnMissingBean) and the custom beans will be picked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful that in this way we also overwrite with the same implementation the authenticationEntryPoint bean property of the BearerTokenAuthenticationFilter, for this reason I had declared 2 of them so as not to silently change BearerTokenAuthenticationFilter, can it go if we use BearerTokenAuthenticationFilter as default for the two cases?

Copy link
Owner

@ch4mpy ch4mpy Nov 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent a all day in transports with my 10 months old daughter and had no time to give feedback yesterday (thanks to a tempest in France :/). sorry about that.

I hadn't noticed that we can configure authenticationEntryPoint and accessDeniedHandler in both the oauth2ResourceServer and the exceptionHandling. My bad.

@nexus061
Copy link
Contributor Author

nexus061 commented Nov 4, 2023

Hi, I think so:

we have 3 cases:

AccessDeniedHandler -> responds to the FORBIDDEN 403 case

Customizer<ExceptionHandlingConfigurer> -> intercepts the missing token

authenticationEntryPoint at the BearerTokenAuthenticationFilter level intercepts an invalid token

I would let you customize the configuration for all three. With the default set to what it uses from the factory
Oaut2, I try to jot down this implementation

@ch4mpy
Copy link
Owner

ch4mpy commented Nov 5, 2023

As a side note, 401 is the expected status for both missing and invalid tokens.

Actually, AccessDeniedHandler can respond more than 403: if you have a look at the reactive implementation, it is slightly different from the servlet one and uses the ServerAccessDeniedHandler instead of the authenticationEntryPoint to return 401 in case of missing or invalid token and 403 in case of access rule violation:

response.setStatusCode(principal instanceof AnonymousAuthenticationToken ? HttpStatus.UNAUTHORIZED : HttpStatus.FORBIDDEN);

This means that you can process missing tokens with both authenticationEntryPoint and accessDeniedHandler, even if the first should be preferred as it is made to process that case => the reactive implementation should be changed to (I will do after this PR for servlet is merged):

  • use authenticationEntryPoint instead of accessDeniedHandler to return 401 in case of missing or invalid token (to replace Spring default which is 302 redirect to login but makes no sense on a resource server)
  • set a WWW-Authenticate header as it should

I'm not sure Customizer<ExceptionHandlingConfigurer> is designed specifically to intercept the missing token. I think it is made to configure things like authenticationEntryPoint and accessDeniedHandler, as you do in the resourceServer configurer, but for the all SecurityFilterChain. As we have only Bearer tokens security in the filter-chain we are configuring, doing it as you do in the resourceServer or doing it in the exceptionHandling is the same and we don't need to do both.

I suggest that we choose between one of the two:

  • inject just a Customizer<ExceptionHandlingConfigurer> in the resource server filter-chains with a default being:
@ConditionalOnMissingBean
@Bean
AuthenticationEntryPoint authenticationEntryPoint() {
	return (request, response, authException) -> {
		response.addHeader(HttpHeaders.WWW_AUTHENTICATE, "Bearer realm=\"Restricted Content\"");
		response.sendError(HttpStatus.UNAUTHORIZED.value(), HttpStatus.UNAUTHORIZED.getReasonPhrase());
	};
}

@ConditionalOnMissingBean
@Bean
Customizer<ExceptionHandlingConfigurer<HttpSecurity>> exceptionHandlingCustomizer(
		AuthenticationEntryPoint authenticationEntryPoint,
		Optional<AccessDeniedHandler> accessDeniedHandler) {
	return exceptionHandling -> {
		exceptionHandling.authenticationEntryPoint(authenticationEntryPoint);
		accessDeniedHandler.ifPresent(exceptionHandling::accessDeniedHandler);
	};
}

@Conditional(IsJwtDecoderResourceServerCondition.class)
@Order(Ordered.LOWEST_PRECEDENCE)
@Bean
SecurityFilterChain springAddonsJwtResourceServerSecurityFilterChain(
		HttpSecurity http,
		ServerProperties serverProperties,
		SpringAddonsOidcProperties addonsProperties,
		ResourceServerExpressionInterceptUrlRegistryPostProcessor authorizePostProcessor,
		ResourceServerHttpSecurityPostProcessor httpPostProcessor,
		AuthenticationManagerResolver<HttpServletRequest> authenticationManagerResolver,
		Customizer<ExceptionHandlingConfigurer<HttpSecurity>> exceptionHandlingCustomizer)
		throws Exception {
	http.oauth2ResourceServer(server -> {
		server.authenticationManagerResolver(authenticationManagerResolver);
	});
	
	http.exceptionHandling(exceptionHandlingCustomizer);

	ServletConfigurationSupport
			.configureResourceServer(http, serverProperties, addonsProperties.getResourceserver(), authorizePostProcessor, httpPostProcessor);

	return http.build();
}
  • inject both authenticationEntryPoint and accessDeniedHandler in the resource server filter-chains, keeping the default exceptionHandling (remove Customizer<ExceptionHandlingConfigurer> from what is injected into the resource server filter-chains), and configure authenticationEntryPoint and accessDeniedHandler inside resourceServer configurer
@ConditionalOnMissingBean
@Bean
AuthenticationEntryPoint authenticationEntryPoint() {
	return (request, response, authException) -> {
		response.addHeader(HttpHeaders.WWW_AUTHENTICATE, "Bearer realm=\"Restricted Content\"");
		response.sendError(HttpStatus.UNAUTHORIZED.value(), HttpStatus.UNAUTHORIZED.getReasonPhrase());
	};
}

@Conditional(IsJwtDecoderResourceServerCondition.class)
@Order(Ordered.LOWEST_PRECEDENCE)
@Bean
SecurityFilterChain springAddonsJwtResourceServerSecurityFilterChain(
		HttpSecurity http,
		ServerProperties serverProperties,
		SpringAddonsOidcProperties addonsProperties,
		ResourceServerExpressionInterceptUrlRegistryPostProcessor authorizePostProcessor,
		ResourceServerHttpSecurityPostProcessor httpPostProcessor,
		AuthenticationManagerResolver<HttpServletRequest> authenticationManagerResolver,
		AuthenticationEntryPoint authenticationEntryPoint,
		Optional<AccessDeniedHandler> accessDeniedHandler)
		throws Exception {
	http.oauth2ResourceServer(server -> {
		server.authenticationManagerResolver(authenticationManagerResolver);
		server.authenticationEntryPoint(authenticationEntryPoint);
		accessDeniedHandler.ifPresent(server::accessDeniedHandler);	
	});

	ServletConfigurationSupport
			.configureResourceServer(http, serverProperties, addonsProperties.getResourceserver(), authorizePostProcessor, httpPostProcessor);

	return http.build();
}

@nexus061
Copy link
Contributor Author

nexus061 commented Nov 5, 2023

I think the second one is better, I implement it and push it

@nexus061
Copy link
Contributor Author

nexus061 commented Nov 5, 2023

I think the second one is better, I implement it and push it​

@ch4mpy push done

Copy link
Owner

@ch4mpy ch4mpy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the many quick updates

@ch4mpy ch4mpy merged commit f442ec6 into ch4mpy:master Nov 5, 2023
1 check passed
@ch4mpy
Copy link
Owner

ch4mpy commented Nov 5, 2023

I have already pushed the same feature for reactive applications. Will release with a bump to Spring Boot 3.1.5 as transient dependency.

@nexus061
Copy link
Contributor Author

nexus061 commented Nov 5, 2023

I hope to be able to help again in the future!

@ch4mpy
Copy link
Owner

ch4mpy commented Nov 5, 2023

Don not hesitate to open more tickets or PRs.

I don't know why the 7.1.11 is not available yet from https://repo1.maven.org/maven2/com/c4-soft/springaddons/spring-addons-starter-oidc/ (the maven release to maven central ended successfully).

If it isn't there tomorrow morning, I'll publish a new version with the same content.

Repository owner deleted a comment from nexus061 Nov 6, 2023
Repository owner deleted a comment from nexus061 Nov 6, 2023
Repository owner deleted a comment from nexus061 Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants