Spring-security: OAuth2ResourceServerConfigurerTests should avoid MockWebServer

[jzheaux]

Now that there is support to create a JWTProcessor using a RestOperations, several of the tests in OAuth2ResourceServerConfigurerTests that relied on a MockWebServer can now rely on a mock RestOperations, making the tests faster.

Or, as an alternative, they could be configured to use a single key instead of a JWK Set, where applicable.

[govi20]

if this is a trivial change, I would like to work on it.

[rwinch]

Thanks @govi20! This should be pretty easy. The issue is yours. Please let us know if you have any questions.

[govi20]

I had a look into OAuth2ResourceServerConfigurerTests, I also read a bit of OAuth2.0 flow
There are two options suggested by @jzheaux

• Replace MockWebServer with the mock of the RestOperations: Correct me If I am wrong. MockWebServer is for an end to end testing and this can be replaced with RestOperations/RestTemplate mock

• Configure single key instead of JWT set, where applicable.

Which one should I go for? Also, if possible please give me an overview of change.

[jzheaux]

@govi20 Many of these tests use a common spring configuration, declared there in the tests class itself. In most cases, the reference to WebServerConfig will be removed along with the line to configure its mock response.

For example, in the case of this test:

@Test
public void getWhenUsingDefaultsWithValidBearerTokenThenAcceptsRequest()
        throws Exception {

    this.spring.register(WebServerConfig.class, DefaultConfig.class, BasicController.class).autowire();
    this.authz.enqueue(this.jwks("Default"));
    String token = this.token("ValidNoScopes");

    this.mvc.perform(get("/").with(bearerToken(token)))
            .andExpect(status().isOk())
            .andExpect(content().string("ok"));
}

I would change this to:

@Test
public void getWhenUsingDefaultsWithValidBearerTokenThenAcceptsRequest()
        throws Exception {

    this.spring.register(SingleKeyConfig.class, BasicController.class).autowire();
    String token = this.token("ValidNoScopes");

    this.mvc.perform(get("/").with(bearerToken(token)))
            .andExpect(status().isOk())
            .andExpect(content().string("ok"));
}

Many of the tests don't actually need to be configured with a JWK Set URI to confirm their functionality since that is not what they are testing. In those cases, a single key is preferred.

When the JWK Set endpoint is being tested, then it should use a RestOperations, as you have outlined.

For example, this test:

@Test
public void getWhenUsingDefaultsWithBadJwkEndpointThenInvalidToken()
    throws Exception {

    this.spring.register(WebServerConfig.class, DefaultConfig.class).autowire();
    this.authz.enqueue(new MockResponse().setBody("malformed"));
    String token = this.token("ValidNoScopes");

    this.mvc.perform(get("/").with(bearerToken(token)))
            .andExpect(status().isUnauthorized())
            .andExpect(invalidTokenHeader("An error occurred while attempting to decode the Jwt: Malformed Jwk set"));
}

Could change to:

@Test
public void getWhenUsingDefaultsWithBadJwkEndpointThenInvalidToken()
    throws Exception {

    this.spring.register(RestOperationsConfig.class).autowire();
    mockRestOperationsToHaveResponse("malformed");
    String token = this.token("ValidNoScopes");

    this.mvc.perform(get("/").with(bearerToken(token)))
            .andExpect(status().isUnauthorized())
            .andExpect(invalidTokenHeader("An error occurred while attempting to decode the Jwt: Malformed Jwk set"));
}

[jzheaux]

@govi20 Wanted to see if you had any other questions. And are you still wanting to work on this?

[govi20]

Yes. I will give it a try on upcoming Saturday, Sunday.

[govi20]

@jzheaux I have started working on it. are we planning to remove WebServerConfig(MockWebServer) from every test case of OAuth2ResourceServerConfigurerTests? or there are test cases for which we may need it.

[jzheaux]

@govi20 Nice!

Yes, let's keep it for just one "happy path" jwkSetUri test. The reason is that when a RestOperations is wired, it actually changes out Nimbus's ResourceRetriever for a custom one. Having one where we just wire the URI will demonstrate that we work without the RestOperations customization.

Looking through the tests just now, after your changes you might not have a "happy path" jwkSetUri test left over. If that is the case, feel free to add one like, getWhenUsingJwkSetUriThenAcceptsRequest that uses WebServerConfig and a valid JWT.

[govi20]

I have removed WebServerConfig from some test cases, from some test cases I could not remove this config. Also, I am unable to figure out the RestOperations functionality, restTemplate.exchange will need active endpoint. I know I am terribly slow, If it needs to be fixed earliest, someone else can work on it.

[jzheaux]

@govi20 Your pace is just fine, thanks for checking in.

What I'd do is mock RestOperations using Mockito. Are you having problems with that approach?

[jzheaux]

@govi20 How are things coming? Are you able to mock RestOperations?

Here is an idea if you are stuck:

private void mockRestOperationsToHaveResponse(String response) {
    RestOperations rest = this.spring.getContext().getBean(RestOperations.class);
    when(rest.exchange(any(RequestEntity.class), eq(String.class)))
            .thenReturn(new ResponseEntity<>(response, HttpStatus.OK));
}

static class RestOperationsConfig extends WebSecurityConfigurerAdapter {
    private final RestOperations rest = mock(RestOperations.class);

    // ...

    @Bean
    RestOperations rest() {
        return this.rest;
    }
}