spring-security-test @WithMockOidcuser gh-8459

[nenaraab]

Adds @WithMockOidcUser to spring-security-test library that allows you to fill the SecurityContext properly when using oauth2Login as I was used to with @WithMockUser

Example Web Mvc test:

@Test
@WithMockOidcUser(name = "[email protected]", authorities = {"read:salesOrders"})
public void readWith_Alice_salesOrders_200() {
        mockMvc.perform(get("/salesOrders"))
                .andExpect(status().isOk());
}

Example other Spring Tests:

@Autowired
DataService dataService;
    
@Test
@WithMockOidcUser(authorities = {"Admin"})
public void accessService_authorizedUser() {
        String data = dataService.readSensitiveData();
        assertFalse(data.isEmpty());
}

Github issue

Find further details discussed here: ##8459

[rwinch]

Thanks for the PR @nenaraab!

When we did gh-7618, I think we avoided annotation support for OIDC because it was not very easy for users to customize the OidcUser and OAuth2AuthenticationToken objects (the attributes are rich objects which cannot easily be modified by an annotation). Instead, we felt that user's could set SecurityContextHolder directly for method support or use the oidcLogin support for MockMvc style testing.

Given you find value in this, it is worth us reconsidering. However, I want to ensure that we remember all the concerns we had before we merge this. I'm assigning this to @jzheaux since he worked on the test support for some of the other OAuth bits and he might remember better what we discussed. As a heads up he is out on PTO so his response will be delayed.

Thanks again for your PR!

[nenaraab]

Hi @rwinch and @jzheaux, that's cool, thanks for initial feedback and reconsidering.

In our context the usage of @WithMockOidcUser is quite meaningful and elegant :-), have a look at this sample:
https://github.com/SAP/cloud-security-xsuaa-integration/blob/cas/samples/spring-security-cas/src/test/java/com/sap/cloud/security/samples/SalesOrderControllerTest.java

Btw, to make the configuration of @WithMockOidcUser more generic, there is the option to enhance @WithMockOidcUser with custom claims, as documented here:
https://stackoverflow.com/questions/13569079/can-java-annotation-have-complex-return-type-like-hashmap

Enjoy your day!

[rwinch]

Here is a link to some of our previous discussions around annotation based testing with more complex objects #6557

[jzheaux]

Thanks for the PR, @nenaraab! I agree that it'd be good to introduce simple annotation support without claims.

I've left some feedback inline.

[jzheaux]

Go ahead and change this to 5.4 since that's the next minor release

[nenaraab]

sure, thanks for the detailed feedback!

[jzheaux]

Did you already consider adding a scopes attribute as well? Doing scopes={ "openid", "profile" } would mirror roles in @WithMockUser

[nenaraab]

mhhh...

• I dislike mutually exclusive options, which would be the case when introducing scopes in addition to authorities.
  Then we have to implement it similar to @WithMockUser where scopes gets "ignored" in case authorities are defined.

• scopes only is not a good option, as I need the flexibility to add authorities without Spring's prefix.

• authorities only might confuse the one or the other consumer, because he/she has to add "SCOPE_" prefix, which exposes Spring Security implementation detail.

[jzheaux]

No problem, I don't have strong feelings about it, and we can always wait for a community member to request it.

The rest is just for future reference:

mutual exclusivity

Btw, while I see your point about mutual exclusivity, I don't see it as a problem as you do. The DSL allows oauth2ResourceServer().jwt() OR oauth2ResourceServer().opaqueToken(), for example.

scopes only

agreed

authorities only ... add "SCOPE_" prefix

The application only has to do this if it's using the "SCOPE_" prefix in its authorization rules, in which case the tester already knows about it. I don't see this as an issue.

[nenaraab]

then - lets provide both :-)

[jzheaux]

I think let's leave out any additional claims for the time being.

[nenaraab]

yes, for sure!

[jzheaux]

Can you clarify why this is mandatory?

[nenaraab]

No, its not required (any more) it can be removed. Thanks!

[jzheaux]

Is a separate class necessary? I'm wondering if you'd get the same outcome using methods.

[jzheaux]

To align this with the OidcUser that OidcUserService creates, I think an OidcUserAuthority should also be added.

[nenaraab]

The only thing it does now: it adds another GrantedAuthority with "ROLE_USER", not sure why required...

[jzheaux]

It might be easier to maintain if these used the claim names defined in IdTokenClaimNames.

[jzheaux]

Is there a reason that you are using new GregorianCalendar().toInstant() instead of Instant.now()?

[nenaraab]

👍

[jzheaux]

Would you please re-arrange the method names so that they look like valueWhenUserNameIsNullThenDefaultsUserId? In new classes, the team has standardized on methodNameWhenCircumstanceThenResult.

[jzheaux]

Reminder to remove this @since, which probably came over from a copy-paste

[nenaraab]

@jzheaux
I'm done with the changes, happy to receive further feedback, in case there are further findings :-)
Thanks again for your valuable review and explanations!

[jzheaux]

@nenaraab thanks so much for this contribution and for responding to my change requests. After letting this sit for some time now, I feel like we don't want to add claims to the annotation.

Without claims, it provides no information that isn't already available in Authentication:

public String method(Authentication authentication) {
    String name = authentication.getName();
    Collection<GrantedAuthority> authorities = authentication.getAuthorities();
}

in which case tying the application to OidcUser is needless. I'd prefer not to simplify the use case where folks are tying their applications needlessly to OidcUser.

Given that, I'm going to close this, but please feel free to comment further if I've missed a use case of yours.