Repository: SAP/cloud-security-xsuaa-integration
Pull request #385 (2020-10-06 15:35:59)

Simplifications of XsuaaAudienceValidator
Recently, I had to debug through `XsuaaAudienceValidator` and I felt that it was partially hard to read. In method `getAllowedAudiences` I had the impression that you have a hard time handling things using Java Lists, but in fact do not want to use the property of ordering at all. Instead, you are busy fixing things afterwards using Java Streams.

Therefore, I'd like to propose this PR making things simpler to read without losing any kind of functionality or quality. The general idea is to not make use of Lists, but to use a (Hash)Set instead. This also sightly will have advantages in terms of performance (in case JWTs do get larger).

Feel free to merge; I do not claim any ownership of my changes, but only want to make things better...

CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/SAP/cloud-security-xsuaa-integration?pullRequest=385) <br/>Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla-assistant.io/SAP/cloud-security-xsuaa-integration?pullRequest=385) before we can accept your contribution.<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/SAP/cloud-security-xsuaa-integration?pullRequest=385) it.</sub>
eaglerainbow: @nenaraab  might want to have a look at it.

Patch:
diff --git a/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/XsuaaCredentials.java b/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/XsuaaCredentials.java
index 0b70c1c2..fb791cab 100644
--- a/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/XsuaaCredentials.java
+++ b/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/XsuaaCredentials.java
@@ -9,6 +9,7 @@ public class XsuaaCredentials {
 	private String url;
 	private String uaaDomain;
 	private String xsAppName;
+	private String verificationKey;
 
 	public String getClientId() {
 		return clientId;
@@ -49,4 +50,12 @@ public class XsuaaCredentials {
 	public void setXsAppName(String xsAppName) {
 		this.xsAppName = xsAppName;
 	}
+
+	public String getVerificationKey() {
+		return verificationKey;
+	}
+
+	public void setVerificationKey(String verificationKey) {
+		this.verificationKey = verificationKey;
+	}
 }
diff --git a/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/XsuaaServiceConfigurationCustom.java b/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/XsuaaServiceConfigurationCustom.java
index 6a55d4d9..57a22369 100644
--- a/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/XsuaaServiceConfigurationCustom.java
+++ b/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/XsuaaServiceConfigurationCustom.java
@@ -38,6 +38,6 @@ public class XsuaaServiceConfigurationCustom implements XsuaaServiceConfiguratio
 	@Nullable
 	@Override
 	public String getVerificationKey() {
-		return null;
+		return credentials.getVerificationKey();
 	}
 }
diff --git a/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaAudienceValidator.java b/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaAudienceValidator.java
index 5da2fe04..b4f45827 100644
--- a/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaAudienceValidator.java
+++ b/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaAudienceValidator.java
@@ -1,10 +1,11 @@
 package com.sap.cloud.security.xsuaa.token.authentication;
 
-import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.stream.Collectors;
+import java.util.Set;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -21,8 +22,8 @@ import com.sap.cloud.security.xsuaa.XsuaaServicesParser;
 import com.sap.cloud.security.xsuaa.token.TokenClaims;
 
 /**
- * Validate audience using audience field content. in case this field is empty,
- * the audience is derived from the scope field
+ * Validate audience using audience field content. In case this field is empty,
+ * the audience is derived from the scope field.
  */
 public class XsuaaAudienceValidator implements OAuth2TokenValidator<Jwt> {
 	private Map<String, String> appIdClientIdMap = new HashMap<>();
@@ -44,74 +45,68 @@ public class XsuaaAudienceValidator implements OAuth2TokenValidator<Jwt> {
 	public OAuth2TokenValidatorResult validate(Jwt token) {
 		String tokenClientId = token.getClaimAsString(TokenClaims.CLAIM_CLIENT_ID);
 		if (StringUtils.isEmpty(tokenClientId)) {
-			return OAuth2TokenValidatorResult.failure(new OAuth2Error(OAuth2ErrorCodes.INVALID_CLIENT,
-					"Jwt token must contain 'cid' (client_id)", null));
+			return OAuth2TokenValidatorResult.failure(
+					new OAuth2Error(OAuth2ErrorCodes.INVALID_CLIENT, "Jwt token must contain 'cid' (client_id)", null));
 		}
-		List<String> allowedAudiences = getAllowedAudiences(token);
+
+		final Set<String> allowedAudiences = getAllowedAudiences(token);
 
 		for (Map.Entry<String, String> xsuaaConfig : appIdClientIdMap.entrySet()) {
 			if (checkMatch(xsuaaConfig.getKey(), xsuaaConfig.getValue(), tokenClientId, allowedAudiences)) {
 				return OAuth2TokenValidatorResult.success();
 			}
 		}
-		String description = String.format("Jwt token with allowed audiences %s matches none of these: %s",
+
+		final String description = String.format("Jwt token with allowed audiences %s matches none of these: %s",
 				allowedAudiences, appIdClientIdMap.keySet().toString());
 		return OAuth2TokenValidatorResult.failure(new OAuth2Error(OAuth2ErrorCodes.INVALID_CLIENT, description, null));
 	}
 
-	private boolean checkMatch(String appId, String clientId, String tokenClientId, List<String> allowedAudiences) {
+	private boolean checkMatch(String appId, String clientId, String tokenClientId, Set<String> allowedAudiences) {
 		// case 1 : token issued by own client (or master)
-		if (clientId.equals(tokenClientId)
-				|| (appId.contains("!b")
-						&& tokenClientId.endsWith("|" + appId))) {
+		if (clientId.equals(tokenClientId) || (appId.contains("!b") && tokenClientId.endsWith("|" + appId))) {
 			return true;
-		} else {
-			// case 2: foreign token
-			if (allowedAudiences.contains(appId)) {
-				return true;
-			} else {
-				return false;
-			}
 		}
+
+		// case 2: foreign token
+		return allowedAudiences.contains(appId);
 	}
 
 	/**
-	 * Retrieve audiences from token. In case the audience list is empty, take
+	 * Retrieve audiences from token. In case the audience list is empty, takes
 	 * audiences based on the scope names.
 	 *
 	 * @param token
-	 * @return (empty) list of audiences
+	 * @return (empty) set of audiences
 	 */
-	static List<String> getAllowedAudiences(Jwt token) {
-		List<String> allAudiences = new ArrayList<>();
-		List<String> tokenAudiences = token.getAudience();
+	static Set<String> getAllowedAudiences(Jwt token) {
+		final Set<String> allAudiences = new HashSet<>();
 
+		final List<String> tokenAudiences = token.getAudience();
 		if (tokenAudiences != null) {
 			for (String audience : tokenAudiences) {
-				if (audience.contains(".")) {
-					String aud = audience.substring(0, audience.indexOf('.'));
-					allAudiences.add(aud);
-				} else {
-					allAudiences.add(audience);
-				}
+				final String aud = audience.contains(".") ? audience.substring(0, audience.indexOf('.')) : audience;
+				allAudiences.add(aud);
 			}
 		}
 
 		// extract audience (app-id) from scopes
-		if (allAudiences.size() == 0) {
+		if (allAudiences.isEmpty()) {
 			for (String scope : getScopes(token)) {
 				if (scope.contains(".")) {
-					String aud = scope.substring(0, scope.indexOf('.'));
+					final String aud = scope.substring(0, scope.indexOf('.'));
 					allAudiences.add(aud);
 				}
 			}
 		}
-		return allAudiences.stream().distinct().filter(value -> !value.isEmpty()).collect(Collectors.toList());
+
+		allAudiences.remove("");
+
+		return allAudiences;
 	}
 
 	static List<String> getScopes(Jwt token) {
-		List<String> scopes = null;
-		scopes = token.getClaimAsStringList(TokenClaims.CLAIM_SCOPES);
-		return scopes != null ? scopes : new ArrayList<>();
+		List<String> scopes = token.getClaimAsStringList(TokenClaims.CLAIM_SCOPES);
+		return scopes != null ? scopes : Collections.emptyList();
 	}
 }


Test patch:
diff --git a/spring-xsuaa/src/test/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaAudienceValidatorTest.java b/spring-xsuaa/src/test/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaAudienceValidatorTest.java
index 344f3ca6..cb7de9e8 100644
--- a/spring-xsuaa/src/test/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaAudienceValidatorTest.java
+++ b/spring-xsuaa/src/test/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaAudienceValidatorTest.java
@@ -7,6 +7,7 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
+import java.util.Set;
 
 import org.junit.Assert;
 import org.junit.Before;
@@ -67,8 +68,7 @@ public class XsuaaAudienceValidatorTest {
 	public void testExtractAudiencesFromTokenScopes() {
 		Jwt token = new JwtGenerator()
 				.addScopes("test1!t1.read", "test2!t1.read", "test2!t1.write", ".scopeWithoutAppId").getToken();
-		List<String> audiences = new XsuaaAudienceValidator(serviceConfigurationSameClientId)
-				.getAllowedAudiences(token);
+		Set<String> audiences = XsuaaAudienceValidator.getAllowedAudiences(token);
 		Assert.assertThat(audiences.size(), is(2));
 		Assert.assertThat(audiences, hasItem("test1!t1"));
 		Assert.assertThat(audiences, hasItem("test2!t1"));