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

In Test @AuthenticationPrincipal is null because ServerWebExchange is not wrapped #6598

Closed
Dav1dde opened this issue Mar 8, 2019 · 14 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Milestone

Comments

@Dav1dde
Copy link
Contributor

Dav1dde commented Mar 8, 2019

Summary

Using @WithMockUser or org.springframework.security.test.web.reactive.server.SecurityMockServerConfigurers#mockAuthentication directly in combination with @AuthenticationPrincipal in a WebFlux controller leaves the principal empty while things like @PreAuthorize('isAuthenticated()') work as expected.

Actual Behavior

Mocking the principal in a test does not work with @AuthenticaitonPrincipal.

Because my SecurityFilterChain does not match the request of the web test client (see below, only matches Requests that contain an Authorization: Basic header), the org.springframework.security.web.server.context.SecurityContextServerWebExchangeWebFilter gets never called (which is part of the SecurityFilterChain), meaning the request is never wrapped with a org.springframework.security.web.server.context.SecurityContextServerWebExchange.

org.springframework.security.web.reactive.result.method.annotation.AuthenticationPrincipalArgumentResolver#resolveArgument uses exchange.getPrincipal() to resolve the principal, which returns an empty Mono because the exchange is still the DefaultServerWebExchange (not security!).

Now the principal set in org.springframework.security.test.web.reactive.server.SecurityMockServerConfigurers.MutatorFilter is not considered.

The org.springframework.security.access.prepost.PrePostAdviceReactiveMethodInterceptor uses org.springframework.security.core.context.ReactiveSecurityContextHolder#getContext which would work for the resolver as well.

Expected Behavior

The mocked principal should be resolved correctly for the @AuthenticationPrincipal argument.

The exchange should be wrapped correctly.

Configuration

WebTestClient (minimal)

        this.webTestClient = WebTestClient.bindToApplicationContext(context)
            .apply(springSecurity())
            .configureClient()
            .mutateWith(mockUser(someUser))

Security (minimal):

    @Configuration
    public static class BasicAuthSecurity {
        @Bean
        public SecurityWebFilterChain basicAuthSecurityWebFilterChain(ServerHttpSecurity http) {
            return http
                .securityMatcher(new BasicAuthWebExchangeMatcher())
                .authorizeExchange().anyExchange().permitAll().and()
                .headers().frameOptions().disable().and()
                .csrf().disable()
                .logout().disable()
                .httpBasic().securityContextRepository(NoOpServerSecurityContextRepository.getInstance()).and()
                .exceptionHandling()
                    .accessDeniedHandler(new HttpStatusServerAccessDeniedHandler(HttpStatus.UNAUTHORIZED))
                    .authenticationEntryPoint(new HttpStatusServerEntryPoint(HttpStatus.UNAUTHORIZED)).and()
                .build();
        }
    }
public class BasicAuthWebExchangeMatcher implements ServerWebExchangeMatcher {
    @Override
    public Mono<MatchResult> matches(ServerWebExchange exchange) {
        var header = exchange.getRequest().getHeaders().getFirst("Authorization");
        return StringUtils.startsWithIgnoreCase(header, "Basic")
            ? MatchResult.match() : MatchResult.notMatch();
    }
}

The important part here is the security matcher, which makes it so my Security chain does not match, nor does any other!

Version

Spring Security 5.1.4

Sample

Company code.

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Mar 8, 2019

Proposed patch in PR, I tried a few things this seems to be the easiest and least intrusive.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 7, 2019
@eleftherias
Copy link
Contributor

@Dav1dde the behaviour you are seeing is the expected behaviour.
Your application is only applying the security configuration to requests that include the HTTP basic Authorization header (according to the custom matcher).
Adding .mutateWith(mockUser(someUser)) does not include that header in your request, therefore the security configuration is not getting applied to your request, and your @AuthenticationPrincipal is null.

If you start up your application and make a request that does not include the HTTP basic Authorization header, then you will see the same behaviour of the @AuthenticationPrincipal being null. This is not specific to testing.

In order to include the required header in your request, you would create a customMutator, similar to the UserExchangeMutator.

It would be nice to have an HttpBasicMutator as part of Spring Security, so that you can easily apply it to requests when testing, like so .mutateWith(httpBasic()).
Let me know if you would be interested in submitting a PR for that.

@eleftherias eleftherias self-assigned this Oct 17, 2019
@eleftherias eleftherias added status: feedback-provided Feedback has been provided and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 17, 2019
@Dav1dde
Copy link
Contributor Author

Dav1dde commented Oct 18, 2019

@elefeint thanks for the great response.

So I cannot use @AuthenticationPrincipal without actually also changing the client/request? That was the original intent to only use the annotation without any other modifications, making it easier to write tests and also not to have the same boilerplate in every test (I cannot do it in setup, since I might not want to have the principal for all tests).

I am aware that none of my security matchers (I actually have 3, Basic Auth, JWT and SAML2) matches the request in the test, I was hoping I can skip the authentication with @AuthenticationPrincipal.

To summarize: In order to mock a principal, I have to mutate the request with @AuthenticationPrincipal (or use the mutator that gets applied by using the annotation) and then make sure the request also hits one of the security chains? This seems a bit counter intuitive especially since if I hit one of the security chains, I also need to make sure they are not failing.

@eleftherias
Copy link
Contributor

@Dav1dde can you clarify how you are using the @AuthenticationPrincipal? I am confused by the following statement

In order to mock a principal, I have to mutate the request with @AuthenticationPrincipal

I expected the @AuthenticationPrincipal annotation to be part of your controller.

Regarding the principal only being populated when it matches a security configuration, I would argue that this is reasonable behaviour.
If Spring Security is not applied to a specific endpoint, then there is no concept of a principal when making a request to that endpoint, which is why it is not populated.

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Oct 22, 2019

@eleftherias sorry, you're obviously correct. I didn't proof read what I wrote and got confused with @MockWithUser. My situation rephrased:

  • I have multiple websecurity filter chains that match on different attributes of the request (basic auth matches Authorization: Basic ..., JWT matches Authorization: Bearer ... and SAML matches SAML specific requests)
  • I have multiple controllers using @AuthenticationPrinicipal
  • During testing I want to mock my principal using mockUser and skip all my websecurity filter chains (for simplicity)

The problem now is that if you skip the chains (as you noted) the principal doesn't get attached to the "exchange" (exchange.getPrincipal()).

So if I understood you correctly in order to have a principal attached I need to go through at least one websecurity filter chain?

But then what is the purpose of mockUser and mockAuthentication, these functions attach a security context to the exchange (Flux/Mono subscriber context), but why not also to the utility method exchange.getPrincipal()?

The implementation of SecurityContextServerWebExchange uses the same context to retrieve the principal:

	public <T extends Principal> Mono<T> getPrincipal() {
		return this.context.map(c -> (T) c.getAuthentication());
	}

The only reason @AuthenticationPrincipal isn't working because instead of reading the principal from the Mono subscriber context using ReactiveSecurityContextHolder.getContext() it uses exchange.getPrincipal() - which is not in sync with ReactiveSecurityContextHolder.getContext(). Isn't that just an implementation detail and the real problem is that exchange.getPrinicpal() isnt in sync with ReactiveSecurityContextHolder.getContext()?

@eleftherias
Copy link
Contributor

@Dav1dde

During testing I want to mock my principal using mockUser and skip all my websecurity filter chains (for simplicity)

The purpose of Mutators such as mockUser is not to skip the security filter chains. They are intended to pass through the security filter chain in order to test your application security.
For example, you can use .mutateWith(mockUser().roles("ADMIN")) to test that users with role "ADMIN" have or don't have access to specific endpoints.
If a Mutator cause the request to skip the security filter chain, then you would not be able to test those scenarios, which are crucial for verifying your application security.

I have multiple websecurity filter chains that match on different attributes of the request (basic auth matches Authorization: Basic ..., JWT matches Authorization: Bearer ... and SAML matches SAML specific requests)

Because you have custom matchers, you will need to create custom Mutators that will mutate the request so that it matches on of your configurations.
For example, an HttpBasicMutator may have method that looks something like

@Override
public void afterConfigurerAdded(WebTestClient.Builder builder, @Nullable WebHttpHandlerBuilder webHttpHandlerBuilder, @Nullable ClientHttpConnector clientHttpConnector) {
	byte[] toEncode;
	toEncode = (username + ":" + password).getBytes(StandardCharsets.UTF_8);
	builder.defaultHeaders(httpHeaders -> {
		httpHeaders.add("Authorization", "Basic " + new String(Base64.getEncoder().encode(toEncode)));
	});
	configurer().afterConfigurerAdded(builder, webHttpHandlerBuilder, clientHttpConnector);
}

As a side note, regarding @PreAuthorize("isAuthenticated()"), as you mentioned in the description it is behaving differently than @AuthenticationPrincipal for reactive applications.
In a servlet application when there is no security configuration matching the request, then @PreAuthorize("isAuthenticated()") will not permit the request and @AuthenticationPrincipal is null. This is expected behaviour.
In a reactive application when there is no security configuration matching the request, then @PreAuthorize("isAuthenticated()") will permit the request and @AuthenticationPrincipal is null.
The @AuthenticationPrincipal is behaving correctly by returning null, however this leads me to believe that @PreAuthorize("isAuthenticated()") is not behaving correctly when combined with a Mutator. Thank you for bringing this to my attention. I will look into this and create a separate issue.

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Oct 24, 2019

The purpose of Mutators such as mockUser is not to skip the security filter chains. They are intended to pass through the security filter chain in order to test your application security.
For example, you can use .mutateWith(mockUser().roles("ADMIN")) to test that users with role "ADMIN" have or don't have access to specific endpoints.

But if the request goes through the security chain, shouldn't the security chain also take care of setting the correct principal?
If not then mockUser should not only set the principal/authentication on the subscriber context but also the exchange, so the endpoint matchers are free to use either, at the end of the day shouldn't exchange.getPrincipal() just be syntax sugar for ReactiveSecurityContextHolder.getContext()?

Because you have custom matchers, you will need to create custom Mutators that will mutate the request so that it matches on of your configurations.

Sure I can make a utility method somewhere that sets up everything so it passes through the security chain and sets the correct user, I was just hoping mockUser would allow me to skip that cumbersome part (and have actual dedicated tests for my security chain elsewhere).

As a side note, regarding @PreAuthorize("isAuthenticated()"), as you mentioned in the description it is behaving differently than @AuthenticationPrincipal for reactive applications.
In a servlet application when there is no security configuration matching the request, then @PreAuthorize("isAuthenticated()") will not permit the request and @AuthenticationPrincipal is null. This is expected behaviour.

Yeah this is what would be adressed with my PR.

The @AuthenticationPrincipal is behaving correctly by returning null, however this leads me to believe that @PreAuthorize("isAuthenticated()") is not behaving correctly when combined with a Mutator. Thank you for bringing this to my attention. I will look into this and create a separate issue.

No @PreAuthorize is behaving correctly, the thing with @PreAuthorize is that it cannot access the web exchange, since it can be used without a request at all (e.g. annotate services that are called periodically or from other sources than web requests) so it has to use the subscriber context.

Where as @AuthenticationPrincipal uses the exchange AuthenticationPrincipalArgumentResolver#resolveArgument:

	@Override
	public Mono<Object> resolveArgument(MethodParameter parameter, BindingContext bindingContext,
			ServerWebExchange exchange) {
		ReactiveAdapter adapter = getAdapterRegistry().getAdapter(parameter.getParameterType());
		return exchange.getPrincipal()
			.ofType(Authentication.class)
			.flatMap( a -> {
				Object p = resolvePrincipal(parameter, a.getPrincipal());
				Mono<Object> principal = Mono.justOrEmpty(p);
				return adapter == null ? principal : Mono.just(adapter.fromPublisher(principal));
			});
	}

Which comes down to mockUser/mockAuthentication only updating the security context but not the associated web exchange.

Also a big thank you @eleftherias for your input and this discussion!

@eleftherias
Copy link
Contributor

@Dav1dde

But if the request goes through the security chain, shouldn't the security chain also take care of setting the correct principal?

Yes, exactly, the security chain will set the correct principal. The requests in your tests are not going through any of the spring security filter chain because they do not match any of your custom matchers.

Yeah this is what would be adressed with my PR.

No, your PR would change the behaviour of @AuthenticationPrincipal which is behaving correctly. It would also make Reactive applications behave differently than Servlet applications.

No @PreAuthorize is behaving correctly

@PreAuthorize is not behaving correctly. If you try out a Servlet application with the same configuration (mockUser(), but no security chain matching the request) then @PreAuthorize will prevent the request.

Sure I can make a utility method somewhere that sets up everything so it passes through the security chain and sets the correct user, I was just hoping mockUser would allow me to skip that cumbersome part (and have actual dedicated tests for my security chain elsewhere).

As an alternative, you can create an additional filter chain that will process any requests that do not match your original filter chains, and allow anonymous users.
Then you will ensure that all of your requests will have Spring Security applied.

@Bean
@Order(100)
public SecurityWebFilterChain anonymousFilterChain(ServerHttpSecurity http) {
	return http
			.anonymous()
			.and()
			.build();
}

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Oct 26, 2019

Yes, exactly, the security chain will set the correct principal. The requests in your tests are not going through any of the spring security filter chain because they do not match any of your custom matchers.

What I wanted to explain here is, if the security chain is meant to set the principal, then way does mockUser() even exist? It completely bypasses the security and sets the principal/authentication (mockAuthentication() exists) on the context. And there is no case where you should already have an authetnication before entering the security chain (at least to my knowledge).

Maybe the right thing here is to not use mockUser() at all.

@PreAuthorize is not behaving correctly. If you try out a Servlet application with the same configuration (mockUser(), but no security chain matching the request) then @PreAuthorize will prevent the request.

But that sounds like an issue with mockUser() to me, PreAuthorize does what it should, read the security context from the Mono/Flux subscriber context and prevent entry. mockUser() sets that subscriber context.

Imo the thing where you really have to be careful here is to not break PreAuthorize if you're not within a web context. It still has to work without an exchange or a security chain, simply by using the supplied security context.

@eleftherias
Copy link
Contributor

Thank you for all of your input @Dav1dde.

The only reason @AuthenticationPrincipal isn't working because instead of reading the principal from the Mono subscriber context using ReactiveSecurityContextHolder.getContext() it uses exchange.getPrincipal() - which is not in sync with ReactiveSecurityContextHolder.getContext().

As you mentioned in the comment above, @AuthenticationPrincipal is extracting the principal from the exchange instead of the ReactiveSecurityContextHolder.
We should have a consistent way of retrieving the principal and therefore should change AuthenticationPrincipalArgumentResolver to use the ReactiveSecurityContextHolder instead of the exchange.
This will cause mockUser() to behave differently in reactive applications than it does in servlet applications, but this is a limitation of the ReactiveSecurityContextHolder.

Would you be interested in submitting a PR to update AuthenticationPrincipalArgumentResolver?
Essentially we will switch to using ReactiveSecurityContextHolder.getContext().map(SecurityContext::getAuthentication) in the resolveArgument method.

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Nov 6, 2019

@eleftherias yeah I can make a PR for this!
Might take me 1-2 weeks though, currently rather busy with work.

The change to ReactiveSecurityContextHolder makes sense to me, but also raises the question why the exchange has a getPrincipal() method at all, but I assume there is/was a good reason for it.

@eleftherias
Copy link
Contributor

@Dav1dde The ServerWebExchange has a getPrincipal() method because Principal is not specific to Spring Security.
The Spring Security implementation may use ReactiveSecurityContextHolder to look up the principal, but other implementations may not.
This is similar to the HttpServletRequest that has the getUserPrincipal method.

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Dec 12, 2019

@eleftherias once again, thanks for the great discussion and the time you invested into this issue and I am glad, we together, could make Spring (Security) a tiny bit better :)

@Dav1dde Dav1dde closed this as completed Dec 12, 2019
@eleftherias eleftherias added this to the 5.3.0.M1 milestone Dec 12, 2019
@eleftherias eleftherias added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided labels Dec 12, 2019
@mindhaq
Copy link

mindhaq commented Sep 28, 2020

I came here after googling why a WebClient mutated with mockUser() still resulted in request.principal() to be null.

I'm unit testing a reactive router function, so security testing would be outside of the scope. The test is about the route returning a response depending on what kind of principal is there, but not how that would be inserted into the request (that is tested in a more integrating test).

I will try a custom mutator for my case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants