Repository: SAP/cloud-security-xsuaa-integration
Pull request #210 (2020-01-17 14:05:25)

Migration remarks
Created PR for remarks from last migration PR.


Patch:
diff --git a/java-security/src/main/java/com/sap/cloud/security/spring/adapter/SAPOfflineTokenServicesCloud.java b/java-security/src/main/java/com/sap/cloud/security/spring/adapter/SAPOfflineTokenServicesCloud.java
index 794c3623..9a09e510 100644
--- a/java-security/src/main/java/com/sap/cloud/security/spring/adapter/SAPOfflineTokenServicesCloud.java
+++ b/java-security/src/main/java/com/sap/cloud/security/spring/adapter/SAPOfflineTokenServicesCloud.java
@@ -4,6 +4,7 @@ import com.sap.cloud.security.config.OAuth2ServiceConfiguration;
 import com.sap.cloud.security.token.Token;
 import com.sap.cloud.security.token.XsuaaToken;
 import com.sap.cloud.security.token.validation.CombiningValidator;
+import com.sap.cloud.security.token.validation.ValidationResult;
 import com.sap.cloud.security.token.validation.Validator;
 import com.sap.cloud.security.token.validation.validators.JwtValidatorBuilder;
 import org.springframework.beans.factory.InitializingBean;
@@ -14,6 +15,7 @@ import org.springframework.security.oauth2.provider.AuthorizationRequest;
 import org.springframework.security.oauth2.provider.OAuth2Authentication;
 import org.springframework.security.oauth2.provider.token.ResourceServerTokenServices;
 
+import javax.annotation.Nonnull;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Set;
@@ -36,16 +38,28 @@ public class SAPOfflineTokenServicesCloud implements ResourceServerTokenServices
 	}
 
 	@Override
-	public OAuth2Authentication loadAuthentication(String accessToken)
+	public OAuth2Authentication loadAuthentication(@Nonnull String accessToken)
 			throws AuthenticationException, InvalidTokenException {
-		XsuaaToken token = new XsuaaToken(accessToken);
+		XsuaaToken token = checkAndCreateToken(accessToken);
 		Set<String> scopes = token.getScopes().stream().collect(Collectors.toSet());
+		ValidationResult validationResult = tokenValidator.validate(token);
 
-		AuthorizationRequest authorizationRequest = new AuthorizationRequest(new HashMap<>(), null,
-				serviceConfiguration.getClientId(), scopes, new HashSet<>(), null,
-				tokenValidator.validate(token).isValid(), "", "", null);
+		if (validationResult.isValid()) {
+			AuthorizationRequest authorizationRequest = new AuthorizationRequest(new HashMap<>(), null,
+					serviceConfiguration.getClientId(), scopes, new HashSet<>(), null,
+					true, "", "", null);
+			return new OAuth2Authentication(authorizationRequest.createOAuth2Request(), null);
+		} else {
+			throw new InvalidTokenException(validationResult.getErrorDescription());
+		}
+	}
 
-		return new OAuth2Authentication(authorizationRequest.createOAuth2Request(), null);
+	private XsuaaToken checkAndCreateToken(@Nonnull String accessToken) {
+		try {
+			return new XsuaaToken(accessToken);
+		} catch (Exception e) {
+			throw new InvalidTokenException(e.getMessage());
+		}
 	}
 
 	@Override


Test patch:
diff --git a/java-security-test/src/main/java/com/sap/cloud/security/test/SecurityTestRule.java b/java-security-test/src/main/java/com/sap/cloud/security/test/SecurityTestRule.java
index e6ab7805..146547b1 100644
--- a/java-security-test/src/main/java/com/sap/cloud/security/test/SecurityTestRule.java
+++ b/java-security-test/src/main/java/com/sap/cloud/security/test/SecurityTestRule.java
@@ -161,9 +161,9 @@ public class SecurityTestRule extends ExternalResource {
 	}
 
 	/**
-	 * Same as {@link SecurityTestRule#setKeys(RSAKeys)} except you can pass in the
-	 * path to the public and private keys and the {@link RSAKeys} object will be
-	 * built for you. Any checked exceptions are caught and rethrown as runtime
+	 * Overwrites the private/public key pair to be used. The private key is used to
+	 * sign the jwt token. The public key is provided by jwks endpoint (on behalf of
+	 * WireMock). Checked exceptions are caught and rethrown as runtime
 	 * exceptions for test convenience.
 	 *
 	 * @param publicKeyPath
@@ -181,22 +181,6 @@ public class SecurityTestRule extends ExternalResource {
 		return this;
 	}
 
-	/**
-	 * Overwrites the private/public key pair to be used. The private key is used to
-	 * sign the jwt token. The public key is provided by jwks endpoint (on behalf of
-	 * WireMock).
-	 * <p>
-	 * It needs to be configured before the {@link #before()} method.
-	 *
-	 * @param keys
-	 *            the private/public key pair.
-	 * @return the rule itself.
-	 */
-	public SecurityTestRule setKeys(RSAKeys keys) {
-		this.keys = keys;
-		return this;
-	}
-
 	@Override
 	protected void before() throws Exception {
 		if (useApplicationServer) {
diff --git a/java-security-test/src/test/java/com/sap/cloud/security/test/SecurityTestRuleTest.java b/java-security-test/src/test/java/com/sap/cloud/security/test/SecurityTestRuleTest.java
index 7d4e98bb..0a63c23f 100644
--- a/java-security-test/src/test/java/com/sap/cloud/security/test/SecurityTestRuleTest.java
+++ b/java-security-test/src/test/java/com/sap/cloud/security/test/SecurityTestRuleTest.java
@@ -40,13 +40,13 @@ public class SecurityTestRuleTest {
 	private static final int PORT = 8484;
 	private static final int APPLICATION_SERVER_PORT = 8383;
 	private static final String UTF_8 = StandardCharsets.UTF_8.displayName();
-
-	private static final RSAKeys RSA_KEYS = RSAKeys.generate();
+	private static final String PUBLIC_KEY_PATH = "src/main/resources/publicKey.txt";
+	private static final String PRIVATE_KEY_PATH = "src/main/resources/privateKey.txt";
 
 	@ClassRule
 	public static SecurityTestRule cut = SecurityTestRule.getInstance(XSUAA)
 			.setPort(PORT)
-			.setKeys(RSA_KEYS)
+			.setKeys(PUBLIC_KEY_PATH, PRIVATE_KEY_PATH)
 			.useApplicationServer(forService(XSUAA).usePort(APPLICATION_SERVER_PORT))
 			.addApplicationServlet(TestServlet.class, "/hi");
 
@@ -54,7 +54,15 @@ public class SecurityTestRuleTest {
 	public void getTokenKeysRequest_responseContainsExpectedTokenKeys()
 			throws IOException, InvalidKeySpecException, NoSuchAlgorithmException {
 
-		assertThatMockKeyServiceServesPublicKey(RSA_KEYS.getPublic(), PORT);
+		HttpGet httpGet = new HttpGet("http://localhost:" + PORT + "/token_keys");
+		try (CloseableHttpResponse response = HttpClients.createDefault().execute(httpGet)) {
+			assertThat(response.getStatusLine().getStatusCode()).isEqualTo(HttpStatus.SC_OK);
+			JsonWebKeySet keySet = JsonWebKeySetFactory.createFromJson(readContent(response));
+			PublicKey actualPublicKey = keySet
+					.getKeyByAlgorithmAndId(JwtSignatureAlgorithm.RS256, "default-kid").getPublicKey();
+
+			assertThat(actualPublicKey).isEqualTo(RSAKeys.loadPublicKey(PUBLIC_KEY_PATH));
+		}
 	}
 
 	@Test
@@ -139,22 +147,6 @@ public class SecurityTestRuleTest {
 
 	}
 
-	public static class SecurityTestRuleWithRSAKeysPath {
-
-		private static final String PUBLIC_KEY_PATH = "src/main/resources/publicKey.txt";
-
-		@Rule
-		public SecurityTestRule rule = SecurityTestRule.getInstance(XSUAA)
-				.setKeys(PUBLIC_KEY_PATH, "src/main/resources/privateKey.txt");
-
-		@Test
-		public void testRuleIsInitializedCorrectly()
-				throws NoSuchAlgorithmException, IOException, InvalidKeySpecException {
-			assertThatMockKeyServiceServesPublicKey(RSAKeys.loadPublicKey(PUBLIC_KEY_PATH),
-					rule.getWireMockRule().port());
-		}
-	}
-
 	public static class TestServlet extends HttpServlet {
 		@Override
 		protected void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException {
@@ -170,16 +162,4 @@ public class SecurityTestRuleTest {
 				.collect(Collectors.joining());
 	}
 
-	private static void assertThatMockKeyServiceServesPublicKey(PublicKey expectedPublicKey, int mockKeyServicePort)
-			throws IOException, NoSuchAlgorithmException, InvalidKeySpecException {
-		HttpGet httpGet = new HttpGet("http://localhost:" + mockKeyServicePort + "/token_keys");
-		try (CloseableHttpResponse response = HttpClients.createDefault().execute(httpGet)) {
-			assertThat(response.getStatusLine().getStatusCode()).isEqualTo(HttpStatus.SC_OK);
-			JsonWebKeySet keySet = JsonWebKeySetFactory.createFromJson(readContent(response));
-			PublicKey actualPublicKey = keySet
-					.getKeyByAlgorithmAndId(JwtSignatureAlgorithm.RS256, "default-kid").getPublicKey();
-
-			assertThat(actualPublicKey).isEqualTo(expectedPublicKey);
-		}
-	}
 }
diff --git a/java-security/src/test/java/com/sap/cloud/security/spring/adapter/SAPOfflineTokenServicesCloudTest.java b/java-security/src/test/java/com/sap/cloud/security/spring/adapter/SAPOfflineTokenServicesCloudTest.java
index 4d73429c..bbb03d6b 100644
--- a/java-security/src/test/java/com/sap/cloud/security/spring/adapter/SAPOfflineTokenServicesCloudTest.java
+++ b/java-security/src/test/java/com/sap/cloud/security/spring/adapter/SAPOfflineTokenServicesCloudTest.java
@@ -11,6 +11,7 @@ import org.apache.commons.io.IOUtils;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mockito;
+import org.springframework.security.oauth2.common.exceptions.InvalidTokenException;
 import org.springframework.security.oauth2.provider.OAuth2Authentication;
 
 import java.io.IOException;
@@ -35,6 +36,7 @@ public class SAPOfflineTokenServicesCloudTest {
 		cut = createSAPOfflineTokenServicesCloud((token) -> ValidationResults.createValid());
 	}
 
+
 	@Test
 	public void loadAuthentication() throws IOException {
 		cut.afterPropertiesSet();
@@ -45,19 +47,30 @@ public class SAPOfflineTokenServicesCloudTest {
 		assertThat(authentication.getOAuth2Request().getScope()).contains("openid");
 	}
 
+	@Test
+	public void loadAuthentication_tokenIsNull_throwsException() {
+		assertThatThrownBy(() -> cut.loadAuthentication(null)).isInstanceOf(InvalidTokenException.class);
+	}
+
+	@Test
+	public void loadAuthentication_tokenIsMalformed_throwsException() {
+		assertThatThrownBy(() -> cut.loadAuthentication("a.b.c")).isInstanceOf(InvalidTokenException.class);
+	}
+
+
 	@Test
 	public void readAccessToken() {
 		assertThatThrownBy(() -> cut.readAccessToken("token")).isInstanceOf(UnsupportedOperationException.class);
 	}
 
 	@Test
-	public void loadAuthentication_notAuthenticated() {
-		cut = createSAPOfflineTokenServicesCloud((token) -> ValidationResults.createInvalid("not valid"));
+	public void loadAuthentication_tokenValidationFailed_throwsException() {
+		String errorDescription = "just not valid";
+		cut = createSAPOfflineTokenServicesCloud((token) -> ValidationResults.createInvalid(errorDescription));
 		cut.afterPropertiesSet();
 
-		OAuth2Authentication authentication = cut.loadAuthentication(xsuaaToken);
+		assertThatThrownBy(() -> cut.loadAuthentication(xsuaaToken)).isInstanceOf(InvalidTokenException.class).hasMessageContaining(errorDescription);
 
-		assertThat(authentication.isAuthenticated()).isFalse();
 	}
 
 	@Test
@@ -71,14 +84,13 @@ public class SAPOfflineTokenServicesCloudTest {
 	}
 
 	@Test
-	public void createInstanceWithPublicConstructor_notAuthenticated() {
+	public void createInstancWithEmptyConfiguration_throwsException() {
 		cut =  new SAPOfflineTokenServicesCloud(Mockito.mock(OAuth2ServiceConfiguration.class));
 		cut.afterPropertiesSet();
-		OAuth2Authentication authentication = cut.loadAuthentication(xsuaaToken);
-
-		assertThat(authentication.isAuthenticated()).isFalse();
+		assertThatThrownBy(() -> cut.loadAuthentication(xsuaaToken)).isInstanceOf(InvalidTokenException.class);
 	}
 
+
 	private static class TestTokenValidatorSupplier implements Supplier<Validator<Token>> {
 		@Override public Validator<Token> get() {
 			return null;