Repository: SAP/cloud-security-xsuaa-integration
Pull request #331 (2020-07-10 14:57:02)

[java-security] Validator cleanup
- correct name of test
- remove duplicate test
- improve JwtIssuerValidator
- rename XsuaaJwtIssuerValidator to XsuaaJkuValidator


Patch:
diff --git a/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtIssuerValidator.java b/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtIssuerValidator.java
index 8bb94fe3..fc6a7178 100644
--- a/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtIssuerValidator.java
+++ b/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtIssuerValidator.java
@@ -59,7 +59,8 @@ class JwtIssuerValidator implements Validator<Token> {
 						issuer);
 			}
 			issuerUri = new URI(issuer);
-			if (issuerUri.getHost() != null && issuerUri.getHost().endsWith(url.getHost())) {
+			if (issuerUri.getQuery() == null && issuerUri.getFragment() == null
+					&& issuerUri.getHost() != null && issuerUri.getHost().endsWith(url.getHost())) {
 				return createValid();
 			}
 		} catch (URISyntaxException e) {
diff --git a/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtSignatureValidator.java b/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtSignatureValidator.java
index 6e5e0072..b5f16c5b 100644
--- a/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtSignatureValidator.java
+++ b/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtSignatureValidator.java
@@ -106,7 +106,7 @@ class JwtSignatureValidator implements Validator<Token> {
 			return configuration.getUrl() + "/token_keys";
 		}
 		if (configuration.getService() == Service.XSUAA && token.hasHeaderParameter(KEYS_URL_PARAMETER_NAME)) {
-			// 'jku' was validated by XsuaaJwtIssuerValidator
+			// 'jku' was validated by XsuaaJkuValidator
 			return token.getHeaderParameterAsString(KEYS_URL_PARAMETER_NAME);
 		}
 		if (configuration.getService() != Service.XSUAA && token.hasClaim(ISSUER)) {
diff --git a/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtValidatorBuilder.java b/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtValidatorBuilder.java
index 57235eb1..2f20a1de 100644
--- a/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtValidatorBuilder.java
+++ b/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtValidatorBuilder.java
@@ -187,7 +187,7 @@ public class JwtValidatorBuilder {
 
 		if (configuration.getService() == XSUAA) {
 			if (!configuration.isLegacyMode()) {
-				defaultValidators.add(new XsuaaJwtIssuerValidator(configuration.getProperty(UAA_DOMAIN)));
+				defaultValidators.add(new XsuaaJkuValidator(configuration.getProperty(UAA_DOMAIN)));
 			}
 		} else if (configuration.getService() == IAS) {
 			defaultValidators.add(new JwtIssuerValidator(configuration.getUrl()));
diff --git a/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidator.java b/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/XsuaaJkuValidator.java
similarity index 91%
rename from java-security/src/main/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidator.java
rename to java-security/src/main/java/com/sap/cloud/security/token/validation/validators/XsuaaJkuValidator.java
index 2605c963..69aed4a7 100644
--- a/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidator.java
+++ b/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/XsuaaJkuValidator.java
@@ -19,7 +19,7 @@ import com.sap.cloud.security.token.validation.Validator;
  * service. In case of XSUAA does the token key url (jku JWT header parameter)
  * must match the identity service domain.
  */
-class XsuaaJwtIssuerValidator implements Validator<Token> {
+class XsuaaJkuValidator implements Validator<Token> {
 	private final String domain;
 	protected final Logger logger = LoggerFactory.getLogger(getClass());
 
@@ -29,8 +29,8 @@ class XsuaaJwtIssuerValidator implements Validator<Token> {
 	 *            the domain of the identity service
 	 *            {@link OAuth2ServiceConfiguration#getProperty(String)}
 	 */
-	XsuaaJwtIssuerValidator(String uaaDomain) {
-		assertHasText(uaaDomain, "XsuaaJwtIssuerValidator requires uaaDomain.");
+	XsuaaJkuValidator(String uaaDomain) {
+		assertHasText(uaaDomain, "XsuaaJkuValidator requires uaaDomain.");
 		this.domain = uaaDomain;
 	}
 
@@ -77,7 +77,7 @@ class XsuaaJwtIssuerValidator implements Validator<Token> {
 		if (o == null || getClass() != o.getClass())
 			return false;
 
-		XsuaaJwtIssuerValidator that = (XsuaaJwtIssuerValidator) o;
+		XsuaaJkuValidator that = (XsuaaJkuValidator) o;
 
 		return domain.equals(that.domain);
 	}


Test patch:
diff --git a/java-security/src/test/java/com/sap/cloud/security/adapter/xs/XSUserInfoAdapterTest.java b/java-security/src/test/java/com/sap/cloud/security/adapter/xs/XSUserInfoAdapterTest.java
index bc2a43a0..8e05f25a 100644
--- a/java-security/src/test/java/com/sap/cloud/security/adapter/xs/XSUserInfoAdapterTest.java
+++ b/java-security/src/test/java/com/sap/cloud/security/adapter/xs/XSUserInfoAdapterTest.java
@@ -281,7 +281,7 @@ public class XSUserInfoAdapterTest {
 	}
 
 	@Test
-	public void testGetSubaccountIdFromSystemAttributes() throws XSUserInfoException {
+	public void testGetSubaccountIdFromExternalAttributes() throws XSUserInfoException {
 		String token = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImprdSI6Imh0dHA6Ly9sb2NhbGhvc3Q6MzMxOTUvdG9rZW5fa2V5cyIsImtpZCI6ImxlZ2FjeS10b2tlbi1rZXkifQ.ewogICJqdGkiOiAiOGU3YjNiMDAtNzc1MS00YjQ2LTliMWEtNWE0NmEyYTFkNWI4IiwKICAieHMudXNlci5hdHRyaWJ1dGVzIjogewogICAgImNvc3QtY2VudGVyIjogWwogICAgICAiMDgxNSIsCiAgICAgICI0NzExIgogICAgXSwKICAgICJjb3VudHJ5IjogWwogICAgICAiR2VybWFueSIKICAgIF0KICB9LAogICAgInhzLnN5c3RlbS5hdHRyaWJ1dGVzIjogewogICAgInhzLnNhbWwuZ3JvdXBzIjogWwogICAgICAiZzEiCiAgICBdLAogICAgInhzLnJvbGVjb2xsZWN0aW9ucyI6IFsicmMxIl0KICB9LAogICJzdWIiOiAiMTAwMjE5MSIsCiAgInNjb3BlIjogWwogICAgImphdmEtaGVsbG8td29ybGQuRGlzcGxheSIsCiAgICAib3BlbmlkIiwKICAgICJqYXZhLWhlbGxvLXdvcmxkLkRlbGV0ZSIsCiAgICAiamF2YS1oZWxsby13b3JsZC5DcmVhdGUiCiAgXSwKICAiY2xpZW50X2lkIjogInNiLWphdmEtaGVsbG8td29ybGQiLAogICJjaWQiOiAic2ItamF2YS1oZWxsby13b3JsZCIsCiAgImF6cCI6ICJzYi1qYXZhLWhlbGxvLXdvcmxkIiwKICAiZ3JhbnRfdHlwZSI6ICJhdXRob3JpemF0aW9uX2NvZGUiLAogICJ1c2VyX2lkIjogIjEwMDIxOTEiLAogICJ1c2VyX25hbWUiOiAiTXVzdGVybWFubiIsCiAgIm9yaWdpbiI6ICJ1c2VyaWRwIiwKICAiZW1haWwiOiAibWF4QGV4YW1wbGUuY29tIiwKICAiaWF0IjogMTQ0MjkxMjI0NCwKICAiZXhwIjogMTQ0Mjk1NTMyMiwKICAiaXNzIjogImh0dHA6Ly9sb2NhbGhvc3Q6ODA4MC91YWEvb2F1dGgvdG9rZW4iLAogICJ6aWQiOiAiMTEtMjItMzMiLAogICJoZGIubmFtZWR1c2VyLnNhbWwiOiAiPD94bWwgdmVyc2lvbj1cIjEuMFwiIGVuY29kaW5nPVwiVVRGLThcIj8-PHNhbWwyOkFzc2VydGlvbiB4bWxuczpzYW1sMj1cInVybjpvYXNpczpuYW1lczp0YzpTQU1MOjIuMDphc3NlcnRpb25cIiBJRD1cIl9mOTQ2YmM3Yi05MzM5LTQyMDAtYjdlYi0xYmJhNjU4MDEwNmJcIiBJc3N1ZUluc3RhbnQ9XCIyMDE1LTA5LTIyVDA4OjU1OjIyLjc1NVpcIiBWZXJzaW9uPVwiMi4wXCI-PHNhbWwyOklzc3Vlcj54czItbG9naW4tdGVzdC5jZi5zYXAtY2YuY29tLXNhbWwtbG9naW48L3NhbWwyOklzc3Vlcj48ZHM6U2lnbmF0dXJlIHhtbG5zOmRzPVwiaHR0cDovL3d3dy53My5vcmcvMjAwMC8wOS94bWxkc2lnI1wiPjxkczpTaWduZWRJbmZvPjxkczpDYW5vbmljYWxpemF0aW9uTWV0aG9kIEFsZ29yaXRobT1cImh0dHA6Ly93d3cudzMub3JnLzIwMDEvMTAveG1sLWV4Yy1jMTRuI1wiLz48ZHM6U2lnbmF0dXJlTWV0aG9kIEFsZ29yaXRobT1cImh0dHA6Ly93d3cudzMub3JnLzIwMDAvMDkveG1sZHNpZyNyc2Etc2hhMVwiLz48ZHM6UmVmZXJlbmNlIFVSST1cIiNfZjk0NmJjN2ItOTMzOS00MjAwLWI3ZWItMWJiYTY1ODAxMDZiXCI-PGRzOlRyYW5zZm9ybXM-PGRzOlRyYW5zZm9ybSBBbGdvcml0aG09XCJodHRwOi8vd3d3LnczLm9yZy8yMDAwLzA5L3htbGRzaWcjZW52ZWxvcGVkLXNpZ25hdHVyZVwiLz48ZHM6VHJhbnNmb3JtIEFsZ29yaXRobT1cImh0dHA6Ly93d3cudzMub3JnLzIwMDEvMTAveG1sLWV4Yy1jMTRuI1wiLz48L2RzOlRyYW5zZm9ybXM-PGRzOkRpZ2VzdE1ldGhvZCBBbGdvcml0aG09XCJodHRwOi8vd3d3LnczLm9yZy8yMDAwLzA5L3htbGRzaWcjc2hhMVwiLz48ZHM6RGlnZXN0VmFsdWU-bkNCRk5hUjR5ZlhIYUlDdHZaaDZUVnBYbUl3PTwvZHM6RGlnZXN0VmFsdWU-PC9kczpSZWZlcmVuY2U-PC9kczpTaWduZWRJbmZvPjxkczpTaWduYXR1cmVWYWx1ZT5DVkp2SVBnZk9ZNnZtRjg5VGZtcHhZbUlWQWFRM2JtSlRhamVOTnVNRUMxWE50SndPenoyT2VGNnhWVjFvWkk1MzZLQURNeVBmTlZwRG5tdVBaQUVzNnFJRzdLVkZlWCtQMTNHUVgyMzdobURxeDFVZHRuTTk0Q2E4eVd5YnRhSXhjWStPMjBQYkFFa1RaQVpyUCtyczBtTjc4QUlGTk1KYStzSVpOYm5MdVU9PC9kczpTaWduYXR1cmVWYWx1ZT48L2RzOlNpZ25hdHVyZT48c2FtbDI6U3ViamVjdD48c2FtbDI6TmFtZUlEIEZvcm1hdD1cInVybjpvYXNpczpuYW1lczp0YzpTQU1MOjEuMTpuYW1laWQtZm9ybWF0OnVuc3BlY2lmaWVkXCI-V09MRkdBTkc8L3NhbWwyOk5hbWVJRD48c2FtbDI6U3ViamVjdENvbmZpcm1hdGlvbiBNZXRob2Q9XCJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6Y206YmVhcmVyXCI-PHNhbWwyOlN1YmplY3RDb25maXJtYXRpb25EYXRhIE5vdE9uT3JBZnRlcj1cIjIwMTUtMDktMjJUMTI6NTU6MjIuNzU1WlwiLz48L3NhbWwyOlN1YmplY3RDb25maXJtYXRpb24-PC9zYW1sMjpTdWJqZWN0PjxzYW1sMjpDb25kaXRpb25zIE5vdEJlZm9yZT1cIjIwMTUtMDktMjJUMDg6NTU6MjIuNzU1WlwiIE5vdE9uT3JBZnRlcj1cIjIwMTUtMDktMjJUMTI6NTU6MjIuNzU1WlwiLz48c2FtbDI6QXV0aG5TdGF0ZW1lbnQgQXV0aG5JbnN0YW50PVwiMjAxNS0wOS0yMlQwODo1NToyMi43NTdaXCIgU2Vzc2lvbk5vdE9uT3JBZnRlcj1cIjIwMTUtMDktMjJUMDk6MDA6MjIuNzU3WlwiPjxzYW1sMjpBdXRobkNvbnRleHQ-PHNhbWwyOkF1dGhuQ29udGV4dENsYXNzUmVmPnVybjpvYXNpczpuYW1lczp0YzpTQU1MOjIuMDphYzpjbGFzc2VzOlBhc3N3b3JkPC9zYW1sMjpBdXRobkNvbnRleHRDbGFzc1JlZj48L3NhbWwyOkF1dGhuQ29udGV4dD48L3NhbWwyOkF1dGhuU3RhdGVtZW50Pjwvc2FtbDI6QXNzZXJ0aW9uPiIsCiAgImF1ZCI6IFsKICAgICJzYi1qYXZhLWhlbGxvLXdvcmxkIiwKICAgICJqYXZhLWhlbGxvLXdvcmxkIiwKICAgICJvcGVuaWQiCiAgXSwKICAiYXpfYXR0ciI6IHsKICAgICJleHRlcm5hbF9ncm91cCI6ICJkb21haW5cXGdyb3VwMSIsCiAgICAiZXh0ZXJuYWxfaWQiOiAiYWJjZDEyMzQiCiAgfSwKICAiZXh0X2F0dHIiOiB7CiAgICAic2VydmljZWluc3RhbmNlaWQiOiAiYWJjZDEyMzQiLAogICAgInpkbiI6ICJ0ZXN0c3ViZG9tYWluIiwKICAgICJzdWJhY2NvdW50aWQiOiAidGVzdC1zdWJhY2NvdW50IgogIH0KfQ.yi368lAzQi-mBajDN5oMzzREn3-3WKCrjQIBe7xXVkb8ms_42HAxMiZdrLbCf1mLoJ_CMfAWJkTW0M2GmpXBJPFcdKwsmxahSmP3Ir8nRzrK76FRn1C8biVibs0MZ7cIIq-g2H--tsrml2IkL4-hZ1v5-NCF0Aq-WCcPNsHMdWTrBxpBSRkdEqiViulbfAEXJGSqkXez5VKNX_e2eKYwgu0uJN-BpD8Pqufi3H9M9UKjISkLdsXs5LdpaFN7at6BgMbW2Ce7RVTGsk3ir--rZzQg1oUnWVOKTNEwcTCIJGxm90Smse_aDcb3CBBwnMS_KbQ723ABKhW_m2q-mWeNgg";
 		cut = new XSUserInfoAdapter(new XsuaaToken(token));
 		assertThat(cut.getSubaccountId()).isEqualTo("test-subaccount");
diff --git a/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/JwtIssuerValidatorTest.java b/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/JwtIssuerValidatorTest.java
index f268a39d..f118bfa7 100644
--- a/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/JwtIssuerValidatorTest.java
+++ b/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/JwtIssuerValidatorTest.java
@@ -9,9 +9,10 @@ import org.mockito.Mockito;
 import java.net.URI;
 
 import static com.sap.cloud.security.token.TokenClaims.ISSUER;
+import static com.sap.cloud.security.token.TokenHeader.JWKS_URL;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
-import static org.hamcrest.CoreMatchers.is;
-import static org.hamcrest.CoreMatchers.startsWith;
+import static org.hamcrest.CoreMatchers.*;
+import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.mockito.Mockito.when;
 
@@ -95,11 +96,29 @@ public class JwtIssuerValidatorTest {
 
 	@Test
 	public void validationFails_whenTokenIssuerIsNotAValidUri() {
-		Token token = Mockito.mock(Token.class);
 		when(token.getClaimAsString(ISSUER)).thenReturn("\0://myauth.com");
 		ValidationResult validationResult = cut.validate(token);
 		assertThat(validationResult.isErroneous(), is(true));
 		assertThat(validationResult.getErrorDescription(), startsWith("Issuer is not trusted because"));
 	}
 
+	@Test
+	public void validationFails_whenTokenIssuerContainsQueryParameters() {
+		when(token.getClaimAsString(ISSUER)).thenReturn("https://subdomain.accounts400.ondemand.com?a=b");
+		ValidationResult validationResult = cut.validate(token);
+		assertThat(validationResult.isErroneous(), is(true));
+		assertThat(validationResult.getErrorDescription(), startsWith("Issuer is not trusted because"));
+	}
+
+	@Test
+	public void validationFails_whenTokenIssuerContainsFragment() {
+		when(token.getHeaderParameterAsString(JWKS_URL))
+				.thenReturn("https://subdomain.myauth.ondemand.com/token_keys#token_keys");
+
+		when(token.getClaimAsString(ISSUER)).thenReturn("https://subdomain.accounts400.ondemand.com#anyFragment_keys");
+		ValidationResult validationResult = cut.validate(token);
+		assertThat(validationResult.isErroneous(), is(true));
+		assertThat(validationResult.getErrorDescription(), startsWith("Issuer is not trusted because"));
+	}
+
 }
diff --git a/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/JwtValidatorBuilderTest.java b/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/JwtValidatorBuilderTest.java
index 75a99cda..88cf9c0c 100644
--- a/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/JwtValidatorBuilderTest.java
+++ b/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/JwtValidatorBuilderTest.java
@@ -61,7 +61,7 @@ public class JwtValidatorBuilderTest {
 				.hasSize(4)
 				.hasAtLeastOneElementOfType(JwtTimestampValidator.class)
 				.hasAtLeastOneElementOfType(JwtAudienceValidator.class)
-				.hasAtLeastOneElementOfType(XsuaaJwtIssuerValidator.class)
+				.hasAtLeastOneElementOfType(XsuaaJkuValidator.class)
 				.hasAtLeastOneElementOfType(JwtSignatureValidator.class);
 	}
 
@@ -77,7 +77,7 @@ public class JwtValidatorBuilderTest {
 				.hasAtLeastOneElementOfType(JwtTimestampValidator.class)
 				.hasAtLeastOneElementOfType(JwtAudienceValidator.class)
 				.hasAtLeastOneElementOfType(JwtSignatureValidator.class)
-				.doesNotHaveAnyElementsOfTypes(XsuaaJwtIssuerValidator.class);
+				.doesNotHaveAnyElementsOfTypes(XsuaaJkuValidator.class);
 	}
 
 	@Test
diff --git a/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidatorTest.java b/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/XsuaaJkuValidatorTest.java
similarity index 83%
rename from java-security/src/test/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidatorTest.java
rename to java-security/src/test/java/com/sap/cloud/security/token/validation/validators/XsuaaJkuValidatorTest.java
index b3075272..a8ae93a6 100644
--- a/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidatorTest.java
+++ b/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/XsuaaJkuValidatorTest.java
@@ -12,25 +12,25 @@ import static org.hamcrest.CoreMatchers.*;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.mockito.Mockito.when;
 
-public class XsuaaJwtIssuerValidatorTest {
-	private XsuaaJwtIssuerValidator cut;
+public class XsuaaJkuValidatorTest {
+	private XsuaaJkuValidator cut;
 	private Token token;
 
 	@Before
 	public void setup() {
-		cut = new XsuaaJwtIssuerValidator("myauth.ondemand.com");
+		cut = new XsuaaJkuValidator("myauth.ondemand.com");
 		token = Mockito.mock(Token.class);
 	}
 
 	@Test
 	public void constructor_throwsOnNullValues() {
 		assertThatThrownBy(() -> {
-			new XsuaaJwtIssuerValidator(null);
-		}).isInstanceOf(IllegalArgumentException.class).hasMessageContainingAll("XsuaaJwtIssuerValidator", "uaaDomain");
+			new XsuaaJkuValidator(null);
+		}).isInstanceOf(IllegalArgumentException.class).hasMessageContainingAll("XsuaaJkuValidator", "uaaDomain");
 
 		assertThatThrownBy(() -> {
-			new XsuaaJwtIssuerValidator(" ");
-		}).isInstanceOf(IllegalArgumentException.class).hasMessageContainingAll("XsuaaJwtIssuerValidator", "uaaDomain");
+			new XsuaaJkuValidator(" ");
+		}).isInstanceOf(IllegalArgumentException.class).hasMessageContainingAll("XsuaaJkuValidator", "uaaDomain");
 	}
 
 	@Test
@@ -75,15 +75,6 @@ public class XsuaaJwtIssuerValidatorTest {
 				containsString("Jwt token does not contain a valid uri as 'jku' header parameter"));
 	}
 
-	@Test
-	public void validationFails_whenJwksContainsParameter() {
-		when(token.getHeaderParameterAsString(JWKS_URL)).thenReturn("\0://myauth.com");
-		ValidationResult validationResult = cut.validate(token);
-		assertThat(validationResult.isErroneous(), is(true));
-		assertThat(validationResult.getErrorDescription(),
-				containsString("Jwt token does not contain a valid uri as 'jku' header parameter"));
-	}
-
 	@Test
 	public void validationFails_whenJwksDoesNotContainAValidPath() {
 		when(token.getHeaderParameterAsString(JWKS_URL))
@@ -95,7 +86,7 @@ public class XsuaaJwtIssuerValidatorTest {
 	}
 
 	@Test
-	public void validationFails_whenJwksContainQueryParameters() {
+	public void validationFails_whenJwksContainsQueryParameters() {
 		when(token.getHeaderParameterAsString(JWKS_URL))
 				.thenReturn("https://subdomain.myauth.ondemand.com/token_keys?a=b");
 		ValidationResult validationResult = cut.validate(token);