1

The context: I created a test annotation @WithMockAuthentication to populate test security context with an Authentication instance, much like @WithMockUser does. The main difference being, in my case, the instance is a Mockito mock.

What I experience: As soon as I replace an actual instance with a mock, the Authentication instance provided as controller method parameter is null in annotated tests: in the WithSecurityContextFactory, if I replace:

    public Authentication workingAuthentication(WithMockAuthentication annotation) {
        return new TestAuthentication(annotation.name(), Stream.of(annotation.authorities()).map(SimpleGrantedAuthority::new).collect(Collectors.toSet()));
    }

with

    public Authentication bogousAuthentication(WithMockAuthentication annotation) {
        var auth = mock(Authentication.class);
        when(auth.getName()).thenReturn(annotation.name());
        when(auth.getAuthorities()).thenReturn((Collection) Stream.of(annotation.authorities()).map(SimpleGrantedAuthority::new).collect(Collectors.toSet()));
        when(auth.isAuthenticated()).thenReturn(true);
        return auth;
    }

Then I get NPE in the controller tests at

    @RequestMapping("/method")
    @PreAuthorize("hasRole('ROLE_AUTHORIZED')")
    public ResponseEntity<String> securedMethod(Authentication auth) {
        // Here, auth is null if Authentication is a mock
        return ResponseEntity.ok(String.format("Hey %s, how are you?", auth.getName()));
    }

I've created a minimal sample to reproduce. Run the test to see the failure.

I'm pretty sure I face a bug. Enough to create an issue in spring-security project, but it seems that Spring team team has no time to investigate...

[EDIT] This last statement is uselessly offensive and completely wrong as the answer is provided by Rob Winch, who is a major member of spring-security :/ My Bad

ch4mp
  • 6,622
  • 6
  • 29
  • 49

1 Answers1

2

The argument for your SampleController is of type Authentication which is an instance of Principal and thus the ServletRequestMethodArgumentResolver will attempt to resolve the argument from HttpServletRequest.getUserPrincipal().

The mock that you are creating did not stub the Authentication.getPrincipal() method.

public Authentication bogousAuthentication(WithMockAuthentication annotation) {
    var auth = mock(Authentication.class);
    when(auth.getName()).thenReturn(annotation.name());
    when(auth.getAuthorities()).thenReturn((Collection) Stream.of(annotation.authorities()).map(SimpleGrantedAuthority::new).collect(Collectors.toSet()));
    when(auth.isAuthenticated()).thenReturn(true);
    return auth;
}

For that reason, Authentication.getPrincipal() is null and thus SecurityContextHolderAwareRequestWrapper.getUserPrincipal() returns null. Why does it return null when the principal is null? I cannot be certain the original intention as the code was added before I was a team member. However, it makes sense in Spring Security's model. Authentication can represent both an authenticated user and credentials used for authenticating. The Javadoc of Authentication.getPrincipal() states (emphasis mine):

The identity of the principal being authenticated. In the case of an authentication request with username and password, this would be the username. Callers are expected to populate the principal for an authentication request.

The AuthenticationManager implementation will often return an Authentication containing richer information as the principal for use by the application. Many of the authentication providers will create a UserDetails object as the principal.

The null check is to ensure that the Authentication is indeed representing an authenticated user.

To fix it you must stub out the getPrincipal() method with something like when(auth.getPrincipal()).thenReturn("bogus");. The change can be seen below and in my pull request.

public Authentication bogousAuthentication(WithMockAuthentication annotation) {
    var auth = mock(Authentication.class);
    when(auth.getPrincipal()).thenReturn("bogus");
    when(auth.getName()).thenReturn(annotation.name());
    when(auth.getAuthorities()).thenReturn((Collection) Stream.of(annotation.authorities()).map(SimpleGrantedAuthority::new).collect(Collectors.toSet()));
    when(auth.isAuthenticated()).thenReturn(true);
    return auth;
}
Rob Winch
  • 21,440
  • 2
  • 59
  • 76
  • Also, I steel need to reproduce, but I'm almost sure the Authentication argument is provided in webflux. Shouldn't the behavior be the same on both sides of the force? – ch4mp Mar 31 '20 at 04:30
  • I updated the answer to indicate why a getPrincipal check is performed. – Rob Winch Mar 31 '20 at 13:43
  • I have a different understanding of the spec. `Authentication` is a (extends) `Principal`. Shouldn't "Callers are expected to populate the principal for an authentication request" be implemented with a check on `auth.getName() == null` (rather than `auth.getPrincipal() == null`) as this is the method specified in `Principal` interface and principal is not a `Principal` but an `Object` ? – ch4mp Mar 31 '20 at 14:00
  • I'd turn the question around and ask why would you have an Authentication where getPrincipal is null? I understand where you are coming from, but this is how Spring Security chose to map an Authentication object to Principal. Honestly, I'm not sure I would have chosen to do it that way, but changing it at this point is a bit out of question given the impact it would have. – Rob Winch Mar 31 '20 at 14:05
  • I don't use `getPrincipal()` return value. Granted authorities are enough most of the time, sometimes, user name (Authentication.getName() should answer that) to retrieve user-details and if I need to use more of `Authentication` internals, then I use an implementation exposing something else than `Object`. So the only reason I'd use an Authentication where getPrincipal(), getDetails() or getCredentials() is not null is because I'm forced to by the framework (no use in my security expressions or methods body) – ch4mp Apr 01 '20 at 18:01