Browse Source

Leave Issuer As String

Since StringOrURI is a valid issuer, MappedJwtClaimSetConverter and
JwtIssuerValidator no longer assume it.

Issue: gh-6073
Josh Cummings 6 years ago
parent
commit
19649db9ce

+ 1 - 11
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtIssuerValidator.java

@@ -15,9 +15,6 @@
  */
 package org.springframework.security.oauth2.jwt;
 
-import java.net.MalformedURLException;
-import java.net.URL;
-
 import org.springframework.security.oauth2.core.OAuth2Error;
 import org.springframework.security.oauth2.core.OAuth2ErrorCodes;
 import org.springframework.security.oauth2.core.OAuth2TokenValidator;
@@ -46,14 +43,7 @@ public final class JwtIssuerValidator implements OAuth2TokenValidator<Jwt> {
 	 */
 	public JwtIssuerValidator(String issuer) {
 		Assert.notNull(issuer, "issuer cannot be null");
-
-		try {
-			this.issuer = new URL(issuer).toString();
-		} catch (MalformedURLException ex) {
-			throw new IllegalArgumentException(
-					"Invalid Issuer URL " + issuer + " : " + ex.getMessage(),
-					ex);
-		}
+		this.issuer = issuer;
 	}
 
 	/**

+ 11 - 24
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/MappedJwtClaimSetConverter.java

@@ -16,7 +16,6 @@
 
 package org.springframework.security.oauth2.jwt;
 
-import java.net.MalformedURLException;
 import java.net.URI;
 import java.net.URL;
 import java.time.Instant;
@@ -42,7 +41,7 @@ public final class MappedJwtClaimSetConverter
 		implements Converter<Map<String, Object>, Map<String, Object>> {
 
 	private static final Converter<Object, Collection<String>> AUDIENCE_CONVERTER = new AudienceConverter();
-	private static final Converter<Object, URL> ISSUER_CONVERTER = new IssuerConverter();
+	private static final Converter<Object, String> ISSUER_CONVERTER = new IssuerConverter();
 	private static final Converter<Object, String> STRING_CONVERTER = new StringConverter();
 	private static final Converter<Object, Instant> TEMPORAL_CONVERTER = new InstantConverter();
 
@@ -157,39 +156,27 @@ public final class MappedJwtClaimSetConverter
 	 * Coerces an <a target="_blank" href="https://tools.ietf.org/html/rfc7519#section-4.1.1">Issuer</a> claim
 	 * into a {@link URL}, ignoring null values, and throwing an error if its coercion efforts fail.
 	 */
-	private static class IssuerConverter implements Converter<Object, URL> {
+	private static class IssuerConverter implements Converter<Object, String> {
 
 		@Override
-		public URL convert(Object source) {
+		public String convert(Object source) {
 			if (source == null) {
 				return null;
 			}
 
 			if (source instanceof URL) {
-				return (URL) source;
-			}
-
-			if (source instanceof URI) {
-				return toUrl((URI) source);
+				return ((URL) source).toExternalForm();
 			}
 
-			return toUrl(source.toString());
-		}
-
-		private URL toUrl(URI source) {
-			try {
-				return source.toURL();
-			} catch (MalformedURLException e) {
-				throw new IllegalStateException("Could not coerce " + source + " into a URL", e);
+			if (source instanceof String && ((String) source).contains(":")) {
+				try {
+					return URI.create((String) source).toString();
+				} catch (Exception e) {
+					throw new IllegalStateException("Could not coerce " + source + " into a URI String", e);
+				}
 			}
-		}
 
-		private URL toUrl(String source) {
-			try {
-				return new URL(source);
-			} catch (MalformedURLException e) {
-				throw new IllegalStateException("Could not coerce " + source + " into a URL", e);
-			}
+			return source.toString();
 		}
 	}
 

+ 14 - 5
oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/JwtIssuerValidatorTests.java

@@ -82,15 +82,24 @@ public class JwtIssuerValidatorTests {
 		assertThat(result.getErrors()).isNotEmpty();
 	}
 
+	// gh-6073
 	@Test
-	public void validateWhenJwtIsNullThenThrowsIllegalArgumentException() {
-		assertThatCode(() -> this.validator.validate(null))
-				.isInstanceOf(IllegalArgumentException.class);
+	public void validateWhenIssuerMatchesAndIsNotAUriThenReturnsSuccess() {
+		Jwt jwt = new Jwt(
+				MOCK_TOKEN,
+				MOCK_ISSUED_AT,
+				MOCK_EXPIRES_AT,
+				MOCK_HEADERS,
+				Collections.singletonMap(JwtClaimNames.ISS, "issuer"));
+		JwtIssuerValidator validator = new JwtIssuerValidator("issuer");
+
+		assertThat(validator.validate(jwt))
+				.isEqualTo(OAuth2TokenValidatorResult.success());
 	}
 
 	@Test
-	public void constructorWhenMalformedIssuerIsGivenThenThrowsIllegalArgumentException() {
-		assertThatCode(() -> new JwtIssuerValidator("issuer"))
+	public void validateWhenJwtIsNullThenThrowsIllegalArgumentException() {
+		assertThatCode(() -> this.validator.validate(null))
 				.isInstanceOf(IllegalArgumentException.class);
 	}
 

+ 25 - 3
oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/MappedJwtClaimSetConverterTests.java

@@ -108,7 +108,7 @@ public class MappedJwtClaimSetConverterTests {
 		assertThat(target.get(JwtClaimNames.AUD)).isEqualTo(Arrays.asList("audience"));
 		assertThat(target.get(JwtClaimNames.EXP)).isEqualTo(Instant.ofEpochSecond(2000000000L));
 		assertThat(target.get(JwtClaimNames.IAT)).isEqualTo(Instant.ofEpochSecond(1000000000L));
-		assertThat(target.get(JwtClaimNames.ISS)).isEqualTo(new URL("https://any.url"));
+		assertThat(target.get(JwtClaimNames.ISS)).isEqualTo("https://any.url");
 		assertThat(target.get(JwtClaimNames.NBF)).isEqualTo(Instant.ofEpochSecond(1000000000L));
 		assertThat(target.get(JwtClaimNames.SUB)).isEqualTo("1234");
 	}
@@ -135,7 +135,7 @@ public class MappedJwtClaimSetConverterTests {
 		assertThat(target.get(JwtClaimNames.AUD)).isEqualTo(Arrays.asList("audience"));
 		assertThat(target.get(JwtClaimNames.EXP)).isEqualTo(Instant.ofEpochSecond(2000000000L));
 		assertThat(target.get(JwtClaimNames.IAT)).isEqualTo(Instant.ofEpochSecond(1000000000L));
-		assertThat(target.get(JwtClaimNames.ISS)).isEqualTo(new URL("https://any.url"));
+		assertThat(target.get(JwtClaimNames.ISS)).isEqualTo("https://any.url");
 		assertThat(target.get(JwtClaimNames.NBF)).isEqualTo(Instant.ofEpochSecond(1000000000L));
 		assertThat(target.get(JwtClaimNames.SUB)).isEqualTo("1234");
 	}
@@ -196,7 +196,7 @@ public class MappedJwtClaimSetConverterTests {
 		MappedJwtClaimSetConverter converter = MappedJwtClaimSetConverter
 				.withDefaults(Collections.emptyMap());
 
-		Map<String, Object> badIssuer = Collections.singletonMap(JwtClaimNames.ISS, "badly-formed-iss");
+		Map<String, Object> badIssuer = Collections.singletonMap(JwtClaimNames.ISS, "https://badly formed iss");
 		assertThatCode(() -> converter.convert(badIssuer)).isInstanceOf(IllegalStateException.class);
 
 		Map<String, Object> badIssuedAt = Collections.singletonMap(JwtClaimNames.IAT, "badly-formed-iat");
@@ -209,6 +209,28 @@ public class MappedJwtClaimSetConverterTests {
 		assertThatCode(() -> converter.convert(badNotBefore)).isInstanceOf(IllegalStateException.class);
 	}
 
+	// gh-6073
+	@Test
+	public void convertWhenIssuerIsNotAUriThenConvertsToString() {
+		MappedJwtClaimSetConverter converter = MappedJwtClaimSetConverter
+				.withDefaults(Collections.emptyMap());
+
+		Map<String, Object> nonUriIssuer = Collections.singletonMap(JwtClaimNames.ISS, "issuer");
+		Map<String, Object> target = converter.convert(nonUriIssuer);
+		assertThat(target.get(JwtClaimNames.ISS)).isEqualTo("issuer");
+	}
+
+	// gh-6073
+	@Test
+	public void convertWhenIssuerIsOfTypeURLThenConvertsToString() throws Exception {
+		MappedJwtClaimSetConverter converter = MappedJwtClaimSetConverter
+				.withDefaults(Collections.emptyMap());
+
+		Map<String, Object> issuer = Collections.singletonMap(JwtClaimNames.ISS, new URL("https://issuer"));
+		Map<String, Object> target = converter.convert(issuer);
+		assertThat(target.get(JwtClaimNames.ISS)).isEqualTo("https://issuer");
+	}
+
 	@Test
 	public void constructWhenAnyParameterIsNullThenIllegalArgumentException() {
 		assertThatCode(() -> new MappedJwtClaimSetConverter(null))