Repository: SAP/cloud-security-xsuaa-integration (java)

Issue:
Make SPI loading bulletproof
1. Decision:
A) Default implementation is NOT loaded via SPI but . Advantage: we only need to check whether there is 0 or 1 occurrence.
B) as is, default implementation is also registered as SPI. Advantage: in case of `TokenFactory`, the default implementation `HybridTokenFactory` can't be instantiated within `TokenFactory` interface as its not part of java-api. Consequently in case of more than one SP we need to filter our default implementation to find the customers implementation. There should be never more than 2 SPs (incl. our default implementation)

=> **Therefore for consistency purposes we decide for **Option B**, as we can implement it for both cases `TokenFactory` and `HttpClientFactory` the same.**

2. Decision:
A) raise exception, this would mean, the application does not come up and forces the app developer to analyse the configuration issue. May lead to consulting efforts as it is an incompatible change.
B) log error, app developer may oversee the error too late and face behavioural issues because of misconfiguration. An advantage might be in a stacked context, that an overwrite might be possible (??? unable to find documentation about it).

=> **Option A** because its a more reliable solution.







Pull request:
Prioritize SPI loading of custom implementations
closes #668 

Patch:
diff --git a/java-api/pom.xml b/java-api/pom.xml
index 3a7ef4f6..7998e7d6 100644
--- a/java-api/pom.xml
+++ b/java-api/pom.xml
@@ -69,11 +69,6 @@
 			<artifactId>commons-io</artifactId>
 			<scope>test</scope>
 		</dependency>
-		<dependency> <!-- check if it's still needed when slf4j-api 1.8 is available -->
-			<groupId>org.slf4j</groupId>
-			<artifactId>slf4j-simple</artifactId>
-			<scope>test</scope>
-		</dependency>
 	</dependencies>
 
 	<build>
diff --git a/java-api/src/main/java/com/sap/cloud/security/token/Token.java b/java-api/src/main/java/com/sap/cloud/security/token/Token.java
index 4afd8a5f..db062d54 100644
--- a/java-api/src/main/java/com/sap/cloud/security/token/Token.java
+++ b/java-api/src/main/java/com/sap/cloud/security/token/Token.java
@@ -14,12 +14,11 @@ import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import java.io.Serializable;
 import java.security.Principal;
+import java.security.ProviderException;
 import java.time.Instant;
 import java.util.*;
 
-import static com.sap.cloud.security.token.TokenClaims.AUDIENCE;
-import static com.sap.cloud.security.token.TokenClaims.AUTHORIZATION_PARTY;
-import static com.sap.cloud.security.token.TokenClaims.ISSUER;
+import static com.sap.cloud.security.token.TokenClaims.*;
 
 /**
  * Represents a JSON Web Token (JWT).
@@ -34,6 +33,8 @@ public interface Token extends Serializable {
 		}
 	};
 
+	String DEFAULT_TOKEN_FACTORY = "com.sap.cloud.security.servlet.HybridTokenFactory";
+
 	/**
 	 * Creates a token instance based on TokenFactory implementation.
 	 * 
@@ -45,6 +46,15 @@ public interface Token extends Serializable {
 		if (services.isEmpty()) {
 			throw new ProviderNotFoundException("No TokenFactory implementation found in the classpath");
 		}
+		if (services.size() > 2) {
+			throw new ProviderException("More than 1 Custom TokenFactory service provider found. There should be only one");
+		}
+		if (services.size() == 2) {
+			return services.stream()
+					.filter(tokenFactory -> !tokenFactory.getClass().getName()
+							.equals(DEFAULT_TOKEN_FACTORY))
+					.findFirst().get().create(jwt);
+		}
 		return services.get(0).create(jwt);
 	}
 
diff --git a/token-client/pom.xml b/token-client/pom.xml
index 0a9b8b28..211b4f43 100644
--- a/token-client/pom.xml
+++ b/token-client/pom.xml
@@ -97,11 +97,6 @@
 			<artifactId>spring-security-oauth2-jose</artifactId>
 			<scope>test</scope>
 		</dependency>
-		<dependency> <!-- check if it's still needed when slf4j-api 1.8 is available -->
-			<groupId>org.slf4j</groupId>
-			<artifactId>slf4j-simple</artifactId>
-			<scope>test</scope>
-		</dependency>
 	</dependencies>
 
 	<build>
diff --git a/token-client/src/main/java/com/sap/cloud/security/client/DefaultHttpClientFactory.java b/token-client/src/main/java/com/sap/cloud/security/client/DefaultHttpClientFactory.java
index 17b55f27..79bb944e 100644
--- a/token-client/src/main/java/com/sap/cloud/security/client/DefaultHttpClientFactory.java
+++ b/token-client/src/main/java/com/sap/cloud/security/client/DefaultHttpClientFactory.java
@@ -25,6 +25,7 @@ public class DefaultHttpClientFactory implements HttpClientFactory {
 
 	private static final Logger LOGGER = LoggerFactory.getLogger(DefaultHttpClientFactory.class);
 
+	@Override
 	public CloseableHttpClient createClient(ClientIdentity clientIdentity) throws HttpClientException {
 		LOGGER.warn("In productive environment provide well configured HttpClientFactory service");
 		if (clientIdentity != null && clientIdentity.isCertificateBased()) {
diff --git a/token-client/src/main/java/com/sap/cloud/security/client/HttpClientFactory.java b/token-client/src/main/java/com/sap/cloud/security/client/HttpClientFactory.java
index 57681c1e..f8d37a0b 100644
--- a/token-client/src/main/java/com/sap/cloud/security/client/HttpClientFactory.java
+++ b/token-client/src/main/java/com/sap/cloud/security/client/HttpClientFactory.java
@@ -11,6 +11,7 @@ import com.sap.cloud.security.token.ProviderNotFoundException;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.slf4j.LoggerFactory;
 
+import java.security.ProviderException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.ServiceLoader;
@@ -19,6 +20,7 @@ import java.util.ServiceLoader;
  * Represents a {@link CloseableHttpClient} creation interface.
  */
 public interface HttpClientFactory {
+
 	@SuppressWarnings("unchecked")
 	List<HttpClientFactory> services = new ArrayList() {
 		{
@@ -28,6 +30,9 @@ public interface HttpClientFactory {
 		}
 	};
 
+	String DEFAULT_HTTP_CLIENT_FACTORY = "com.sap.cloud.security.client.DefaultHttpClientFactory";
+
+
 	/**
 	 * Provides CloseableHttpClient based on ClientIdentity details. For
 	 * ClientIdentity that is certificate based it will resolve https client using
@@ -48,6 +53,16 @@ public interface HttpClientFactory {
 		if (services.isEmpty()) {
 			throw new ProviderNotFoundException("No HttpClientFactory service could be found in the classpath");
 		}
+		if (services.size() > 2) {
+			throw new ProviderException(
+					"More than 1 Custom HttpClientFactory service provider found. There should be only one");
+		}
+		if (services.size() == 2) {
+			return services.stream()
+					.filter(httpClientFactory -> !httpClientFactory.getClass().getName()
+							.equals(DEFAULT_HTTP_CLIENT_FACTORY))
+					.findFirst().get().createClient(clientIdentity);
+		}
 		return services.get(0).createClient(clientIdentity);
 	}
 


Test patch:
diff --git a/java-api/src/test/java/com/sap/cloud/security/servlet/HybridTokenFactory.java b/java-api/src/test/java/com/sap/cloud/security/servlet/HybridTokenFactory.java
new file mode 100644
index 00000000..b33357cf
--- /dev/null
+++ b/java-api/src/test/java/com/sap/cloud/security/servlet/HybridTokenFactory.java
@@ -0,0 +1,19 @@
+/**
+ * SPDX-FileCopyrightText: 2018-2021 SAP SE or an SAP affiliate company and Cloud Security Client Java contributors
+ * <p>
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package com.sap.cloud.security.servlet;
+
+import com.sap.cloud.security.token.AccessToken;
+import com.sap.cloud.security.token.Token;
+import com.sap.cloud.security.token.TokenFactory;
+import org.mockito.Mockito;
+
+public class HybridTokenFactory implements TokenFactory {
+
+	@Override
+	public Token create(String jwtToken) {
+		return Mockito.mock(AccessToken.class);
+	}
+}
diff --git a/java-api/src/test/java/com/sap/cloud/security/token/TokenTest.java b/java-api/src/test/java/com/sap/cloud/security/token/TokenTest.java
index 8aecb404..7265e60f 100644
--- a/java-api/src/test/java/com/sap/cloud/security/token/TokenTest.java
+++ b/java-api/src/test/java/com/sap/cloud/security/token/TokenTest.java
@@ -7,6 +7,7 @@ package com.sap.cloud.security.token;
 
 import org.junit.Test;
 
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 
 public class TokenTest {
@@ -18,5 +19,10 @@ public class TokenTest {
 
 		cut = Token.create("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9");
 		assertNotNull(cut);
+
+		// Assert that custom Token factory has a priority over default
+		// com.sap.cloud.security.servlet.HybridTokenFactory
+		assertFalse(cut.getClass().getName().contains("AccessToken"));
 	}
+
 }
diff --git a/java-api/src/test/resources/META-INF/services/com.sap.cloud.security.token.TokenFactory b/java-api/src/test/resources/META-INF/services/com.sap.cloud.security.token.TokenFactory
index c2cb9383..70fc90c0 100644
--- a/java-api/src/test/resources/META-INF/services/com.sap.cloud.security.token.TokenFactory
+++ b/java-api/src/test/resources/META-INF/services/com.sap.cloud.security.token.TokenFactory
@@ -1,1 +1,2 @@
+com.sap.cloud.security.servlet.HybridTokenFactory
 com.sap.cloud.security.token.test.CustomTokenFactory
\ No newline at end of file
diff --git a/token-client/src/test/java/com/sap/cloud/security/client/HttpClientFactoryTest.java b/token-client/src/test/java/com/sap/cloud/security/client/HttpClientFactoryTest.java
new file mode 100644
index 00000000..9313a1b6
--- /dev/null
+++ b/token-client/src/test/java/com/sap/cloud/security/client/HttpClientFactoryTest.java
@@ -0,0 +1,27 @@
+/**
+ * SPDX-FileCopyrightText: 2018-2021 SAP SE or an SAP affiliate company and Cloud Security Client Java contributors
+ * <p>
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package com.sap.cloud.security.client;
+
+import com.sap.cloud.security.config.ClientCredentials;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+public class HttpClientFactoryTest {
+
+	@Test
+	public void create() {
+		CloseableHttpClient cut = HttpClientFactory.create(new ClientCredentials("clientId", "secret"));
+		assertNotNull(cut);
+
+		// Assert that custom HttpClientFactory factory has a priority over default
+		// com.sap.cloud.security.client.DefaultHttpClientFactory
+		assertFalse(cut.getClass().getName().contains("InternalHttpClient"));
+		assertTrue(cut.getClass().getName().contains("CloseableHttpClient$MockitoMock"));
+	}
+
+}
\ No newline at end of file
diff --git a/token-client/src/test/java/com/sap/cloud/security/client/TestHttpClientFactory.java b/token-client/src/test/java/com/sap/cloud/security/client/TestHttpClientFactory.java
new file mode 100644
index 00000000..ee2d7f66
--- /dev/null
+++ b/token-client/src/test/java/com/sap/cloud/security/client/TestHttpClientFactory.java
@@ -0,0 +1,18 @@
+/**
+ * SPDX-FileCopyrightText: 2018-2021 SAP SE or an SAP affiliate company and Cloud Security Client Java contributors
+ * <p>
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package com.sap.cloud.security.client;
+
+import com.sap.cloud.security.config.ClientIdentity;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.mockito.Mockito;
+
+public class TestHttpClientFactory implements HttpClientFactory {
+
+	@Override
+	public CloseableHttpClient createClient(ClientIdentity clientIdentity) throws HttpClientException {
+		return Mockito.mock(CloseableHttpClient.class);
+	}
+}
diff --git a/token-client/src/test/resources/META-INF/services/com.sap.cloud.security.client.HttpClientFactory b/token-client/src/test/resources/META-INF/services/com.sap.cloud.security.client.HttpClientFactory
new file mode 100644
index 00000000..6b945a92
--- /dev/null
+++ b/token-client/src/test/resources/META-INF/services/com.sap.cloud.security.client.HttpClientFactory
@@ -0,0 +1,2 @@
+com.sap.cloud.security.client.DefaultHttpClientFactory
+com.sap.cloud.security.client.TestHttpClientFactory
\ No newline at end of file