spring-security-test: @WithMockOidcUser

[nenaraab]

Expected Behavior
similar to @WithMockUser:

    @Test
    @WithMockOidcUser(name = "[email protected]" )
    public void authenticateWithoutPermission_200() throws Exception {
        mockMvc.perform(get("/authenticate"))
                .andExpect(status().isOk());
    }

Current Behavior
I'm unable to use Spring WebMVC tests for my secured methods and those methods that expects the security context to be filled. They require the user id (name) from the Oidc token also to perform authorization checks with @PreAuthorize.

Context
My application requires a OidcUser as AuthenticationPrincipal and uses Method Security.

[eleftherias]

@nenaraab This functionality is available as of Spring Security 5.3.
Check our this section of the reference documentation.

[rwinch]

@eleftherias This functionality is a little different than the functionality you linked to given this ticket is annotation based. We can leave this closed as a duplicate of the PR gh-8461 There is some discussion on that ticket as to why we are waiting on the issue.

[nenaraab]

Hi @eleftherias

thanks a lot for the hint with the oidcLogin Mock MVC post processor!

Honestly i did not find it, when I looked for spring test utilities... but actually i was looking for a testing approach for methods that were secured with custom @PreAuthorize annotations and @WithMockUser didn't worked.

So, in my simple demo app i was able to leverage your suggested aproach, e.g.:

@Test
//@WithMockOidcUser(name = "[email protected]", authorities = {"read:salesOrders"})
public void readWith_Alice_salesOrders_200() throws Exception {
        mockMvc.perform(get("/salesOrders")
                    .with(oidcLogin()
                        .idToken(token -> token.claim("sub", "[email protected]"))
                        .authorities(new SimpleGrantedAuthority("read:salesOrders"))))
                .andExpect(status().isOk());
}

Not nice, but i think this becomes even nicer with an RequestPostProcessor... :-)

My main concern here - with this approach - is the limitation to Web MVC tests. Consider you want to test this:

@Service
public class DataService {
    @PreAuthorize("hasAuthority('Admin')")
    String readSensitiveData() {
        ...
    }
}

This would not work efficiently with the web mvc testing utilities and everybody would have to implement its own logic to apply a valid OidcUser to the SecurityContextHolder.

So maybe you would like to reconsider this @WithMockOidcUser approach.

Shall i rephrase my issue to clarify my concern?

Best regards,
Nena

[eleftherias]

@nenaraab Absolutely, that all makes sense to me.
It was my mistake to close this issue, as I was unaware of the discussion in gh-8461.
Please disregard my comment and continue the discussion on the PR gh-8461.

[jzheaux]

@nenaraab, to make sure we're solving the right problem here, can you explain what problems you ran into with @WithMockUser?

I ask since - in cases where the controller or service has nothing OIDC-specific in it - @WithMockUser should work just fine.

[nenaraab]

Hi @jzheaux

that's a fair question. You're right, depending on whether developers needs to provide further "userInfo" claims, the @WithMockOidcUser must be able to handle that as well.

Here some insights into my current project context:

The scenario (method i want to test):

    @GetMapping(value = "/authenticate") // redirects to login page
    public String secured(@AuthenticationPrincipal OidcUser principal) {
        String name = principal.getGivenName();

        if (name == null) {
            name = principal.getEmail();
        }
        return "Congratulation, " + name
                + "! You just logged in successfully.";
    }

I tried to implement tests with these options:

1. using @WithMockUser

    @Test
    @WithMockUser("[email protected]")
    public void authenticateWithoutPermission_200() throws Exception {
        mockMvc.perform(get("/authenticate"))
                .andExpect(status().isOk());
    }

which result into a NullPointerException for @AuthenticationPrincipal OidcUser principal:

Caused by: java.lang.NullPointerException
	at com.sap.cloud.security.samples.BasicController.secured(BasicController.java:36)
	at com.sap.cloud.security.samples.BasicController$$FastClassBySpringCGLIB$$de505469.invoke(<generated>)
	at org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:218)
	at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:687)
	at com.sap.cloud.security.samples.BasicController$$EnhancerBySpringCGLIB$$7f310964.secured(<generated>)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.springframework.web.method.support.InvocableHandlerMethod.doInvoke(InvocableHandlerMethod.java:190)
	at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:138)
	at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:105)
	at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:879)
	at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:793)
	at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:87)
	at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1040)
	at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:943)
	at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1006)
	... 126 more

2. using OidcLoginRequestPostProcessor.oidcLogin()

    @Test
    public void authenticateWithoutPermission_200() throws Exception {
        mockMvc.perform(get("/authenticate").with(oidcLogin()))
                .andExpect(status().isOk());
    }

3. using @WithMockOidcUser

    @Test
    @WithMockOidcUser("[email protected]")
    public void authenticateWithoutPermission_200() throws Exception {
        mockMvc.perform(get("/authenticate"))
                .andExpect(status().isOk());
    }

Happy whitsun!

[jzheaux]

Got it, thanks for the extra insight, @nenaraab.

depending on whether developers needs to provide further "userInfo" claims, the @WithMockOidcUser must be able to handle that as well.

Since the proposed PR doesn't address the use cases you've listed, maybe let's take a look together at what supporting claims in the annotation would look like.

Some things don't map well to annotations. In the case of OIDC, claims that are an epoch date or a JSON object map poorly. Both require custom annotations themselves

@DateClaim(year=...

@AddressClaim(street=...

custom String formatting

@DateClaim("2020-08-23...

or a flattening of their attributes:

address_street="my street", address_city="...

The result is a fair amount of complexity for only a little gain - that is, I don't imagine that most testers would be setting the iat, exp, or address claims. Though, if the annotation supports claims, I believe it'll be easier to support all claims in the spec than to try and decide on which.

My hesitation to this point has been that the cost of supporting all claims outweighs the benefit of supporting both coding styles (annotations + fluent).

However, your point about method security sheds light on the fact that it's not just about supporting two different testing styles. One cannot test only a @Service with an annotation that looks like:

@PreAuthorize("hasAuthority('SCOPE_read') and principal.locale == 'en_US'")

using the MockMvc support. In that case, @WithMockOidcUser(locale = "en_US") would greatly simplify such a test.

This is also the case with @Query and tests that are testing just the @Repository layer:

@Query("FROM Sales WHERE email = ?#{principal.email}")

Doing @WithMockOidcUser(email="[email protected]") would be very simple.

So, perhaps it's reasonable to reconsider my assumption about needing to support all claims or none. I wonder if there's a rubric we could follow to determine which claims to add support for.

EDIT I'll also add that a test can always set the SecurityContext directly with SecurityContextHolder.getContext().setAuthentication(...). So, I don't mean to say that the above use cases cannot be tested at all, only that the MockMvc support doesn't help.

[nenaraab]

Hi @jzheaux

yes that's true I definitively need the option to specify further claims.

Examples (also inspired from here):

@WithMockOidcToken( 
            name = "[email protected]",
            claims={
                  @StringAttribute(name="zid", value="my_zone_id"),
                  @Attribute(name="ownedEntities", value="1,42,51", parser=CsvAttributeParser.class)})

What do you think?
Btw, is my PR a duplicate of the one which is discussed with the above mentioned issue.

Thx

[jzheaux]

I don't think we want to maintain additional annotations and specialized parsers. At that point, really it's quite a bit easier to simply populate the SecurityContext.

Generally speaking, this snippet feels like it's trying to fit a square peg into a round hole.

Btw, is my PR a duplicate of the one which is discussed with the above-mentioned issue.

Good point, I think that's just a bit of cleanup that I hadn't gotten to yet. Since at this time, we aren't going to be adding support for custom claims in an annotation-based way, I've closed that issue.

[ch4mpy]

@nenaraab, at time I contributed the first OAuth2 unit testing helpers, I had proposed a few annotations along with MockMvc request post-processors and WebTestClient configurers. Unfortunately, Spring team rejected it for more or less the same reasons they closed this ticket.

I write "unfortunately" because annotations where clearly my favorite solution for the same reasons as yours: it allows to tests any secured methods and @component (outside of the context of an http request which is acceptable just for @controllers exposed endpoints).

So, I kept and improved the code I had proposed and I might have something addressing your use case out of the box.

The clomplete project you can clone to hack tests (which contain a few sample boot apps) and figure out how it's working at runtime

The lib is published on maven central:

	<dependencies>
		<dependency>
			<groupId>com.c4-soft.springaddons</groupId>
			<artifactId>spring-security-oauth2-test-addons</artifactId>
			<version>2.1.0</version>
			<scope>test</scope>
		</dependency>
	</dependencies>

[nenaraab]

Hi @ch4mpy, oh cool - thanks a lot! Will give it a try. It comes with a remark, that Java 11 is a prerequisite. Is that true for spring-security-oauth2-test-addons?

[nenaraab]

Hi @jzheaux,

not sure whether i've understood your point.

Generally speaking, this snippet feels like it's trying to fit a square peg into a round hole.

👍

So, perhaps it's reasonable to reconsider my assumption about needing to support all claims or none. I wonder if there's a rubric we could follow to determine which claims to add support for.

With that you like to start a discussion about the claims that should be supported?
Maybe a selection of some of the claims that are specified here:
https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims

But in our case we already learnt that it would be nice to specify the one or the other specific claim that is provided by our Identity Service...

[ch4mpy]

It comes with a remark, that Java 11 is a prerequisite. Is that true for spring-security-oauth2-test-addons?

Yes, I do use java 11 enhancements to streams and collections (and, as stated on lib main README, don't wan't to bother maintain backward compatibility with Java versions published more than 6 years ago)

[jzheaux]

@nenaraab

But in our case we already learnt that it would be nice to specify the one or the other specific claim that is provided by our Identity Service...

Can you describe this use case a bit more? The use cases I've understood from you thus far are either easily addressed by the MockMvc support or aren't OIDC-specific.

For example, if I'm understanding you correctly, would it work to do:

this.mockMvc.perform(get("/request")
        .with(oidcLogin().userInfoToken(info -> info
                .claim("zid", "my_zone_id")
                .claim("ownedEntities", Arrays.asList("1", "42", "51")))
        .andExpect()...

So, perhaps it's reasonable to reconsider my assumption about needing to support all claims or none. I wonder if there's a rubric we could follow to determine which claims to add support for.

What I mean here is "perhaps it's reasonable to reconsider my assumption about needing to support all the standard claims or none. I wonder if there's a rubric we could follow to determine which claims to add support for."

But again, if what you ultimately need is custom claims, then my recommendation is to use the MockMvc support, take a look at @ch4mpy's library, or construct a SecurityContext manually.

[nenaraab]

Hi @jzheaux

that's fine with me. Thanks.

IMHO, the absolut minimum set of configurable standard claims in context of @WithMockOidcToken could be:

• email
• given_name and
• family_name

The rest can be enhanced on demand... what do you think?

[jzheaux]

Sure, I think those could be reasonable, @nenaraab.

Let's wait and see if someone has a concrete use case where the MockMvc support doesn't help, but OIDC-based annotation support would.

[ch4mpy]

Let's wait and see if someone has a concrete use case where the MockMvc support doesn't help, but OIDC-based annotation support would.

@jzheaux do you mean something more than unit-testing a secured @Component that isn't a @Controller (such as a @Service) ?

[jzheaux]

Sorry, I wasn't clear. I agree that testing a @Service is a sensible idea and that a @WithMockOidcUser annotation would be helpful there.

What I'm trying to ascertain specifically is what is the minimum set of claims to support for a feature like this to be valuable to the community. It seems like listening for real-world use cases is a good way to figure out what that minimum set is.

Does that clarify what I'm looking for?

[ch4mpy]

The app I'm working on now is accessing sub, email and preferred_username.

In my lib, I chose to map all standard claims (from org.springframework.security.oauth2.core.oidc.IdTokenClaimAccessor and org.springframework.security.oauth2.core.oidc.StandardClaimAccessor). I don't think it's much of a problem to maintain.