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

AuthorizationFilter behaves different from FilterSecurityInterceptor for requests without bearer token #12278

Closed
vpavic opened this issue Nov 23, 2022 · 9 comments
Assignees
Labels
in: config An issue in spring-security-config status: declined A suggestion or change that we don't feel we should currently apply

Comments

@vpavic
Copy link
Contributor

vpavic commented Nov 23, 2022

I'm observing this with Spring Boot 2.7.5 (Spring Security 5.7.4).

Consider the following minimal resource server application:

@SpringBootApplication
@RestController
public class SampleApplication {

    public static void main(String[] args) {
        SpringApplication.run(SampleApplication.class, args);
    }

    @RequestMapping(path = "/")
    String home() {
        return "Hello World!";
    }

    @Bean
    SecurityFilterChain securityFilterChain(HttpSecurity httpSecurity) throws Exception {
        httpSecurity.authorizeRequests(requests -> requests.anyRequest().authenticated());
        httpSecurity.oauth2ResourceServer(OAuth2ResourceServerConfigurer::opaqueToken);
        return httpSecurity.build();
    }

}

Issuing POST request to / without authorization header will result in 401, as expected.

However, if AuthorizationFilter is used by changing SecurityFilterChain bean to this:

@Bean
SecurityFilterChain securityFilterChain(HttpSecurity httpSecurity) throws Exception {
    httpSecurity.authorizeHttpRequests(requests -> requests.anyRequest().authenticated());
    httpSecurity.oauth2ResourceServer(OAuth2ResourceServerConfigurer::opaqueToken);
    return httpSecurity.build();
}

Then the same request will result in 403.

I didn't dig too deep, but I observed that in first case FilterChain#doFilter invocation in ExceptionTranslationFilter#doFilter will throw AccessDeniedException which in turn results in invocation of BearerTokenAuthenticationEntryPoint, while in second case the same FilterChain#doFilter invocation in ExceptionTranslationFilter#doFilter won't throw an exception.

@vpavic vpavic added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Nov 23, 2022
@marcusdacoregio marcusdacoregio self-assigned this Nov 24, 2022
@marcusdacoregio marcusdacoregio added in: config An issue in spring-security-config and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 24, 2022
@marcusdacoregio
Copy link
Contributor

marcusdacoregio commented Nov 24, 2022

Thanks for the report @vpavic.

Investigating a bit more, this is not only related to a resource server, if any kind of authentication is being used, like httpBasic and formLogin the behavior is not consistent between the two filters.

This is happening because when you POST without a CSRF token, the CsrfFilter will raise an error, and Spring Boot will send an ERROR dispatch to /error. When using FilterSecurityInterceptor, even if FilterSecurityInterceptor#observeOncePerRequest is true, the filter still applies to ERROR dispatch. The same is not true for AuthorizationFilter. Note that this does not happen in 6.0 because both filters apply to every request by default #11027.

If we configure it like the below, the AuthorizationFilter will apply to ERROR dispatch, therefore, returning a 401:

http
	.authorizeHttpRequests((requests) -> requests
		.shouldFilterAllDispatcherTypes(true)
		.anyRequest().authenticated()
	)
        // ...

or, with Spring Security >= 5.6.9

http
	.authorizeHttpRequests((requests) -> requests
		.anyRequest().authenticated()
                .withObjectPostProcessor(new ObjectPostProcessor<AuthorizationFilter>() {
			@Override
			public <O extends AuthorizationFilter> O postProcess(O object) {
				object.setFilterErrorDispatch(true);
				return object;
			}
		})
	)
        // ...

I wonder if we should change the filterErrorDispatch property to true for this reason. I'll reach out to @rwinch to check his opinion on this.

@marcusdacoregio marcusdacoregio added this to the 5.6.10 milestone Nov 24, 2022
@vpavic
Copy link
Contributor Author

vpavic commented Nov 24, 2022

Thanks for the feedback.

Yes, I was aware this isn't specific to resource server, sorry for being perhaps too focused on the concrete setup I was working with when facing this issue.

Honestly, while I was troubleshooting this I completely forgot about #shouldFilterAllDispatcherTypes - that should do it. Regarding changing the default, that would be potentially quite an impactful change late in the cycle for 5.x.

@marcusdacoregio
Copy link
Contributor

Regarding changing the default, that would be potentially quite an impactful change late in the cycle for 5.x.

Yes, I agree. That's why I prefer Rob's input on this first. He is on PTO this week but we will talk when he is back.

@marcusdacoregio marcusdacoregio removed this from the 5.6.10 milestone Nov 24, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Nov 28, 2022

Though this ticket is focused on 5.7, this portion of the 5.8 migration guide may be helpful for additional context.

@marcusdacoregio
Copy link
Contributor

Hi @vpavic, I've talked to @rwinch, and indeed we should not change the default given that we have ways to configure the filter to behave as we want.

I'm only curious what has made you switch from authorizeRequests to authorizeHttpRequests? Asking this because we might find something that we can improve in the docs or the migration guides.

@marcusdacoregio marcusdacoregio added status: invalid An issue that we don't feel is valid and removed type: bug A general bug labels Nov 29, 2022
@blaenk
Copy link

blaenk commented Nov 29, 2022

I'm only curious what has made you switch from authorizeRequests to authorizeHttpRequests? Asking this because we might find something that we can improve in the docs or the migration guides.

I am an unrelated observer/subscriber to this issue, but for what it's worth, I myself switched because after upgrading to Spring Boot 3 and Spring Security 6, I noticed that my IDE stated that authorizeRequests was deprecated and the deprecation message said to use authorizeHttpRequests instead.

I wouldn't be surprised if others have done the same.

Thank you all for working on this project!

(Also unrelated: the most confusing/time-consuming aspect of the migration was trying to figure out why some routes were failing authorization even though clearly authorized, logs would show that authorization passed but then the filter would somehow re-run for the same request and that one failed authorization. It ended up being related to the change that the filter runs for all dispatcher types now, I believe, and "fixed" it by setting .shouldFilterAllDispatcherTypes(false). The docs state that it is instead preferred to explicitly specify which dispatcher types to filter on, but I had considerable trouble figuring out what "dispatcher types" even were let alone which ones I should allow, and I did dedicate a decent amount of time trying to figure that out before giving up and simply specifying that option).

@marcusdacoregio
Copy link
Contributor

marcusdacoregio commented Nov 30, 2022

Thanks for your input @blaenk.

Yes, we are aware that most users will migrate because of the deprecation notice.

Did you see the Preparing for 6.0 section of Spring Security 5.8 documentation? There you have all the steps necessary to prepare for 6.0 changes, including the filter all dispatcher types.

You might want to permit forward dispatch if you are using Spring MVC to resolve view names, or async dispatch if using WebClient. If you have any suggestions on how to improve the documentation to help figure out the dispatcher types, that would be nice.

@marcusdacoregio marcusdacoregio added status: declined A suggestion or change that we don't feel we should currently apply and removed status: invalid An issue that we don't feel is valid labels Nov 30, 2022
@blaenk
Copy link

blaenk commented Nov 30, 2022

Did you see the Preparing for 6.0 section of Spring Security 5.8 documentation? There you have all the steps necessary to prepare for 6.0 changes, including the filter all dispatcher types.

Ah I'm sorry I had not, that's on me. Thank you for pointing that out, I will go through it.

You might want to permit forward dispatch if you are using Spring MVC to resolve view names, or async dispatch if using WebClient. If you have any suggestions on how to improve the documentation to help figure out the dispatcher types, that would be nice.

Thank you so much for this advice. I will read the "preparing for 6.0" section you linked and then re-read your advice.

I think the documentation is really great. I think it was more that I missed that note/link, or I think I saw it but mistakenly interpreted that it was not relevant to "Migrating to 6.0". I assumed the section was self-contained.

Thanks again for that info and for working on this project!

@marcusdacoregio
Copy link
Contributor

I'll close this as solved but feel free to continue the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants