diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/ClientRegistrations.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/ClientRegistrations.java index 071f03cf09f..57d10aaadca 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/ClientRegistrations.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/ClientRegistrations.java @@ -21,6 +21,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.function.Supplier; import com.nimbusds.oauth2.sdk.GrantType; import com.nimbusds.oauth2.sdk.ParseException; @@ -86,10 +87,7 @@ public final class ClientRegistrations { */ public static ClientRegistration.Builder fromOidcIssuerLocation(String issuer) { Assert.hasText(issuer, "issuer cannot be empty"); - Map configuration = getConfiguration(issuer, oidc(URI.create(issuer))); - OIDCProviderMetadata metadata = parse(configuration, OIDCProviderMetadata::parse); - return withProviderConfiguration(metadata, issuer) - .userInfoUri(metadata.getUserInfoEndpointURI().toASCIIString()); + return getBuilder(issuer, oidc(URI.create(issuer))); } /** @@ -137,42 +135,68 @@ public static ClientRegistration.Builder fromOidcIssuerLocation(String issuer) { public static ClientRegistration.Builder fromIssuerLocation(String issuer) { Assert.hasText(issuer, "issuer cannot be empty"); URI uri = URI.create(issuer); - Map configuration = getConfiguration(issuer, oidc(uri), oidcRfc8414(uri), oauth(uri)); - AuthorizationServerMetadata metadata = parse(configuration, AuthorizationServerMetadata::parse); - ClientRegistration.Builder builder = withProviderConfiguration(metadata, issuer); - String userinfoEndpoint = (String) configuration.get("userinfo_endpoint"); - if (userinfoEndpoint != null) { - builder.userInfoUri(userinfoEndpoint); - } - return builder; + return getBuilder(issuer, oidc(uri), oidcRfc8414(uri), oauth(uri)); } - private static URI oidc(URI issuer) { - return UriComponentsBuilder.fromUri(issuer) + private static Supplier oidc(URI issuer) { + URI uri = UriComponentsBuilder.fromUri(issuer) .replacePath(issuer.getPath() + OIDC_METADATA_PATH).build(Collections.emptyMap()); + + return () -> { + RequestEntity request = RequestEntity.get(uri).build(); + Map configuration = rest.exchange(request, typeReference).getBody(); + OIDCProviderMetadata metadata = parse(configuration, OIDCProviderMetadata::parse); + return withProviderConfiguration(metadata, issuer.toASCIIString()) + .jwkSetUri(metadata.getJWKSetURI().toASCIIString()) + .userInfoUri(metadata.getUserInfoEndpointURI().toASCIIString()); + }; } - private static URI oidcRfc8414(URI issuer) { - return UriComponentsBuilder.fromUri(issuer) + private static Supplier oidcRfc8414(URI issuer) { + URI uri = UriComponentsBuilder.fromUri(issuer) .replacePath(OIDC_METADATA_PATH + issuer.getPath()).build(Collections.emptyMap()); + return getRfc8414Builder(issuer, uri); } - private static URI oauth(URI issuer) { - return UriComponentsBuilder.fromUri(issuer) + private static Supplier oauth(URI issuer) { + URI uri = UriComponentsBuilder.fromUri(issuer) .replacePath(OAUTH_METADATA_PATH + issuer.getPath()).build(Collections.emptyMap()); + return getRfc8414Builder(issuer, uri); + } + + private static Supplier getRfc8414Builder(URI issuer, URI uri) { + return () -> { + RequestEntity request = RequestEntity.get(uri).build(); + Map configuration = rest.exchange(request, typeReference).getBody(); + AuthorizationServerMetadata metadata = parse(configuration, AuthorizationServerMetadata::parse); + ClientRegistration.Builder builder = withProviderConfiguration(metadata, issuer.toASCIIString()); + + URI jwkSetUri = metadata.getJWKSetURI(); + if (jwkSetUri != null) { + builder.jwkSetUri(jwkSetUri.toASCIIString()); + } + + String userinfoEndpoint = (String) configuration.get("userinfo_endpoint"); + if (userinfoEndpoint != null) { + builder.userInfoUri(userinfoEndpoint); + } + return builder; + }; } - private static Map getConfiguration(String issuer, URI... uris) { + @SafeVarargs + private static ClientRegistration.Builder getBuilder(String issuer, Supplier... suppliers) { String errorMessage = "Unable to resolve Configuration with the provided Issuer of \"" + issuer + "\""; - for (URI uri : uris) { + for (Supplier supplier : suppliers) { try { - RequestEntity request = RequestEntity.get(uri).build(); - return rest.exchange(request, typeReference).getBody(); + return supplier.get(); } catch (HttpClientErrorException e) { if (!e.getStatusCode().is4xxClientError()) { throw e; } // else try another endpoint + } catch (IllegalArgumentException | IllegalStateException e) { + throw e; } catch (RuntimeException e) { throw new IllegalArgumentException(errorMessage, e); } @@ -219,7 +243,6 @@ private static ClientRegistration.Builder withProviderConfiguration(Authorizatio .clientAuthenticationMethod(method) .redirectUriTemplate("{baseUrl}/{action}/oauth2/code/{registrationId}") .authorizationUri(metadata.getAuthorizationEndpointURI().toASCIIString()) - .jwkSetUri(metadata.getJWKSetURI().toASCIIString()) .providerConfigurationMetadata(configurationMetadata) .tokenUri(metadata.getTokenEndpointURI().toASCIIString()) .clientName(issuer); diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/ClientRegistrationsTest.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/ClientRegistrationsTest.java index 86f40a9410f..3897870e702 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/ClientRegistrationsTest.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/ClientRegistrationsTest.java @@ -168,6 +168,33 @@ private void assertIssuerMetadata(ClientRegistration registration, "grant_types_supported", "token_endpoint", "token_endpoint_auth_methods_supported", "userinfo_endpoint"); } + // gh-7512 + @Test + public void issuerWhenResponseMissingJwksUriThenThrowsIllegalArgumentException() throws Exception { + this.response.remove("jwks_uri"); + assertThatThrownBy(() -> registration("").build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("The public JWK set URI must not be null"); + } + + // gh-7512 + @Test + public void issuerWhenOidcFallbackResponseMissingJwksUriThenThrowsIllegalArgumentException() throws Exception { + this.response.remove("jwks_uri"); + assertThatThrownBy(() -> registrationOidcFallback("issuer1", null).build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("The public JWK set URI must not be null"); + } + + // gh-7512 + @Test + public void issuerWhenOAuth2ResponseMissingJwksUriThenThenSuccess() throws Exception { + this.response.remove("jwks_uri"); + ClientRegistration registration = registrationOAuth2("", null).build(); + ClientRegistration.ProviderDetails provider = registration.getProviderDetails(); + assertThat(provider.getJwkSetUri()).isNull(); + } + @Test public void issuerWhenContainsTrailingSlashThenSuccess() throws Exception { assertThat(registration("")).isNotNull(); diff --git a/oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtDecoderProviderConfigurationUtils.java b/oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtDecoderProviderConfigurationUtils.java index ad1b59d54f7..2496abb5b12 100644 --- a/oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtDecoderProviderConfigurationUtils.java +++ b/oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtDecoderProviderConfigurationUtils.java @@ -33,6 +33,7 @@ * issuer and method invoked. * * @author Thomas Vitale + * @author Rafiullah Hamedy * @since 5.2 */ class JwtDecoderProviderConfigurationUtils { @@ -69,7 +70,15 @@ private static Map getConfiguration(String issuer, URI... uris) try { RequestEntity request = RequestEntity.get(uri).build(); ResponseEntity> response = rest.exchange(request, typeReference); - return response.getBody(); + Map configuration = response.getBody(); + + if (configuration.get("jwks_uri") == null) { + throw new IllegalArgumentException("The public JWK set URI must not be null"); + } + + return configuration; + } catch (IllegalArgumentException e) { + throw e; } catch (RuntimeException e) { if (!(e instanceof HttpClientErrorException && ((HttpClientErrorException) e).getStatusCode().is4xxClientError())) { diff --git a/oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/JwtDecodersTests.java b/oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/JwtDecodersTests.java index e87bf555531..db29c6ba489 100644 --- a/oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/JwtDecodersTests.java +++ b/oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/JwtDecodersTests.java @@ -33,6 +33,11 @@ import org.springframework.http.MediaType; import org.springframework.web.util.UriComponentsBuilder; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.JsonMappingException; +import com.fasterxml.jackson.databind.ObjectMapper; + import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; @@ -163,6 +168,36 @@ public void issuerWhenOAuth2ResponseIsNonCompliantThenThrowsRuntimeException() { .isInstanceOf(RuntimeException.class); } + // gh-7512 + @Test + public void issuerWhenResponseDoesNotContainJwksUriThenThrowsIllegalArgumentException() + throws JsonMappingException, JsonProcessingException { + prepareConfigurationResponse(this.buildResponseWithMissingJwksUri()); + assertThatCode(() -> JwtDecoders.fromOidcIssuerLocation(this.issuer)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("The public JWK set URI must not be null"); + } + + // gh-7512 + @Test + public void issuerWhenOidcFallbackResponseDoesNotContainJwksUriThenThrowsIllegalArgumentException() + throws JsonMappingException, JsonProcessingException { + prepareConfigurationResponseOidc(this.buildResponseWithMissingJwksUri()); + assertThatCode(() -> JwtDecoders.fromIssuerLocation(this.issuer)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("The public JWK set URI must not be null"); + } + + // gh-7512 + @Test + public void issuerWhenOAuth2ResponseDoesNotContainJwksUriThenThrowsIllegalArgumentException() + throws JsonMappingException, JsonProcessingException { + prepareConfigurationResponseOAuth2(this.buildResponseWithMissingJwksUri()); + assertThatCode(() -> JwtDecoders.fromIssuerLocation(this.issuer)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("The public JWK set URI must not be null"); + } + @Test public void issuerWhenResponseIsMalformedThenThrowsRuntimeException() { prepareConfigurationResponse("malformed"); @@ -294,4 +329,12 @@ private MockResponse response(String body) { .setBody(body) .setHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE); } + + public String buildResponseWithMissingJwksUri() throws JsonMappingException, JsonProcessingException { + ObjectMapper mapper = new ObjectMapper(); + Map response = mapper.readValue(DEFAULT_RESPONSE_TEMPLATE, + new TypeReference>(){}); + response.remove("jwks_uri"); + return mapper.writeValueAsString(response); + } } diff --git a/oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/ReactiveJwtDecodersTests.java b/oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/ReactiveJwtDecodersTests.java index d4d34cf3563..3e408b465b8 100644 --- a/oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/ReactiveJwtDecodersTests.java +++ b/oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/ReactiveJwtDecodersTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -33,12 +33,18 @@ import org.springframework.http.MediaType; import org.springframework.web.util.UriComponentsBuilder; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.JsonMappingException; +import com.fasterxml.jackson.databind.ObjectMapper; + import static org.assertj.core.api.Assertions.assertThatCode; /** * Tests for {@link ReactiveJwtDecoders} * * @author Josh Cummings + * @author Rafiullah Hamedy */ public class ReactiveJwtDecodersTests { /** @@ -76,14 +82,12 @@ public class ReactiveJwtDecodersTests { private MockWebServer server; private String issuer; - private String jwkSetUri; @Before public void setup() throws Exception { this.server = new MockWebServer(); this.server.start(); this.issuer = createIssuerFromServer(); - this.jwkSetUri = this.issuer + ".well-known/jwks.json"; this.issuer += "path"; } @@ -147,6 +151,36 @@ public void issuerWhenOAuth2ResponseIsNonCompliantThenThrowsRuntimeException() { .isInstanceOf(RuntimeException.class); } + // gh-7512 + @Test + public void issuerWhenResponseDoesNotContainJwksUriThenThrowsIllegalArgumentException() + throws JsonMappingException, JsonProcessingException { + prepareConfigurationResponse(this.buildResponseWithMissingJwksUri()); + assertThatCode(() -> ReactiveJwtDecoders.fromOidcIssuerLocation(this.issuer)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("The public JWK set URI must not be null"); + } + + // gh-7512 + @Test + public void issuerWhenOidcFallbackResponseDoesNotContainJwksUriThenThrowsIllegalArgumentException() + throws JsonMappingException, JsonProcessingException { + prepareConfigurationResponseOidc(this.buildResponseWithMissingJwksUri()); + assertThatCode(() -> ReactiveJwtDecoders.fromIssuerLocation(this.issuer)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("The public JWK set URI must not be null"); + } + + // gh-7512 + @Test + public void issuerWhenOAuth2ResponseDoesNotContainJwksUriThenThrowsIllegalArgumentException() + throws JsonMappingException, JsonProcessingException { + prepareConfigurationResponseOAuth2(this.buildResponseWithMissingJwksUri()); + assertThatCode(() -> ReactiveJwtDecoders.fromIssuerLocation(this.issuer)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("The public JWK set URI must not be null"); + } + @Test public void issuerWhenResponseIsMalformedThenThrowsRuntimeException() { prepareConfigurationResponse("malformed"); @@ -281,4 +315,12 @@ private MockResponse response(String body) { .setBody(body) .setHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE); } + + public String buildResponseWithMissingJwksUri() throws JsonMappingException, JsonProcessingException { + ObjectMapper mapper = new ObjectMapper(); + Map response = mapper.readValue(DEFAULT_RESPONSE_TEMPLATE, + new TypeReference>(){}); + response.remove("jwks_uri"); + return mapper.writeValueAsString(response); + } }