From 0ee4bbcd814f7725f3ad7331806790c47870c6ea Mon Sep 17 00:00:00 2001 From: Avery-Dunn Date: Wed, 26 Jul 2023 13:45:42 -0700 Subject: [PATCH 1/4] Remove default timeouts and improve exception messages --- .../HttpClientIT.java | 34 +++++++++++++++++-- ...AcquireTokenByInteractiveFlowSupplier.java | 7 ++-- .../aad/msal4j/AuthenticationErrorCode.java | 12 ++++++- .../aad/msal4j/DefaultHttpClient.java | 13 ++++--- 4 files changed, 57 insertions(+), 9 deletions(-) diff --git a/msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/HttpClientIT.java b/msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/HttpClientIT.java index edc82ef3..ad6eafb2 100644 --- a/msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/HttpClientIT.java +++ b/msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/HttpClientIT.java @@ -8,10 +8,14 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.api.BeforeAll; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; import java.util.Collections; +import java.util.concurrent.ExecutionException; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; @TestInstance(TestInstance.Lifecycle.PER_CLASS) class HttpClientIT { @@ -34,6 +38,14 @@ void acquireToken_apacheHttpClient() throws Exception { assertAcquireTokenCommon(user, new ApacheHttpClientAdapter()); } + @Test + void acquireToken_readTimeout() throws Exception { + User user = labUserProvider.getDefaultUser(); + + //Set a 1ms read timeout, which will almost certainly occur before the service can respond + assertAcquireTokenCommon_WithTimeout(user, 1); + } + private void assertAcquireTokenCommon(User user, IHttpClient httpClient) throws Exception { PublicClientApplication pca = PublicClientApplication.builder( @@ -54,4 +66,22 @@ private void assertAcquireTokenCommon(User user, IHttpClient httpClient) assertNotNull(result.idToken()); assertEquals(user.getUpn(), result.account().username()); } + + private void assertAcquireTokenCommon_WithTimeout(User user, int readTimeout) + throws Exception { + PublicClientApplication pca = PublicClientApplication.builder( + user.getAppId()). + authority(TestConstants.ORGANIZATIONS_AUTHORITY). + readTimeoutForDefaultHttpClient(readTimeout). + build(); + + ExecutionException ex = assertThrows(ExecutionException.class, () -> pca.acquireToken(UserNamePasswordParameters. + builder(Collections.singleton(TestConstants.GRAPH_DEFAULT_SCOPE), + user.getUpn(), + user.getPassword().toCharArray()) + .build()) + .get()); + + assertTrue(ex.getMessage().contains("Timeout while waiting for response from service.")); + } } diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenByInteractiveFlowSupplier.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenByInteractiveFlowSupplier.java index 8029c5d8..9e98e6ef 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenByInteractiveFlowSupplier.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenByInteractiveFlowSupplier.java @@ -209,8 +209,11 @@ private AuthorizationResult getAuthorizationResultFromHttpListener() { expirationTime = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis()) + 1; } - while (result == null && !interactiveRequest.futureReference().get().isCancelled() && - TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis()) < expirationTime) { + while (result == null && !interactiveRequest.futureReference().get().isCancelled()) { + if (TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis()) > expirationTime) { + LOG.warn(String.format("Listener timed out after %S seconds, no authorization code was returned from the server during that time.", timeFromParameters)); + break; + } result = authorizationResultQueue.poll(100, TimeUnit.MILLISECONDS); } diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationErrorCode.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationErrorCode.java index 78f5260c..22e8299f 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationErrorCode.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationErrorCode.java @@ -115,9 +115,19 @@ public class AuthenticationErrorCode { * A JWT parsing failure, indicating the JWT provided to MSAL is of invalid format. */ public final static String INVALID_JWT = "invalid_jwt"; + /** * Indicates that a Broker implementation is missing from the device, such as when an app developer * does not include one of our broker packages as a dependency in their project, or otherwise cannot - * be accessed by MSAL Java*/ + * be accessed by MSAL Java + */ public final static String MISSING_BROKER = "missing_broker"; + + /** + * Indicates that a timeout occurred during an HTTP call. If this was thrown in relation to a connection timeout error, + * there is likely a network issue preventing the library from reaching a service, such as being blocked by a firewall. + * If this was thrown in relation to a read timeout error, there is likely an issue in the service itself causing a + * slow response, and this may be resolvable by increasing timeouts. For more details, see https://aka.ms/msal4j-http-client + */ + public final static String HTTP_TIMEOUT = "http_timeout"; } diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultHttpClient.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultHttpClient.java index a563b369..73135740 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultHttpClient.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultHttpClient.java @@ -5,8 +5,10 @@ import java.io.DataOutputStream; import java.io.IOException; import java.io.InputStream; +import java.net.ConnectException; import java.net.HttpURLConnection; import java.net.Proxy; +import java.net.SocketTimeoutException; import java.net.URL; import java.nio.charset.StandardCharsets; import java.util.Map; @@ -15,11 +17,10 @@ class DefaultHttpClient implements IHttpClient { private final Proxy proxy; private final SSLSocketFactory sslSocketFactory; - public int DEFAULT_CONNECT_TIMEOUT = 10000; - public int DEFAULT_READ_TIMEOUT = 15000; - private int connectTimeout = DEFAULT_CONNECT_TIMEOUT; - private int readTimeout = DEFAULT_READ_TIMEOUT; + //By default, rely on the timeout behavior of the services requests are sent to + private int connectTimeout = 0; + private int readTimeout = 0; DefaultHttpClient(Proxy proxy, SSLSocketFactory sslSocketFactory, Integer connectTimeout, Integer readTimeout) { this.proxy = proxy; @@ -117,6 +118,10 @@ private HttpResponse readResponseFromConnection(final HttpsURLConnection conn) t httpResponse.addHeaders(conn.getHeaderFields()); httpResponse.body(inputStreamToString(is)); return httpResponse; + } catch (SocketTimeoutException readException) {conn.getURL(); + throw new MsalServiceException("Timeout while waiting for response from service. If custom timeouts were set, increasing them may resolve this issue. See https://aka.ms/msal4j-http-client for more information and solutions.", AuthenticationErrorCode.HTTP_TIMEOUT); + } catch (ConnectException timeoutException) { + throw new MsalServiceException("Exception while connecting to service, there may be network issues preventing MSAL Java from connecting. See https://aka.ms/msal4j-http-client for more information and solutions.", AuthenticationErrorCode.HTTP_TIMEOUT); } finally { if (is != null) { is.close(); From e94f2153f9d5671d06e428abb4292c93a632dd69 Mon Sep 17 00:00:00 2001 From: Avery-Dunn Date: Wed, 26 Jul 2023 13:54:56 -0700 Subject: [PATCH 2/4] Fix NPE for on-prem ADFS scenario --- .../TokenCacheIT.java | 26 +++++++++++++++++++ .../com/microsoft/aad/msal4j/TokenCache.java | 2 +- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/TokenCacheIT.java b/msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/TokenCacheIT.java index 9576eaef..d1192f2c 100644 --- a/msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/TokenCacheIT.java +++ b/msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/TokenCacheIT.java @@ -9,6 +9,7 @@ import org.junit.jupiter.api.TestInstance; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import java.util.Collections; import java.util.HashMap; @@ -170,6 +171,31 @@ void twoAccountsInCache_SameUserDifferentTenants_RemoveAccountTest() throws Exce "/cache_data/remove-account-test-cache.json"); } + @Test + void retrieveAccounts_ADFSOnPrem() throws Exception { + UserQueryParameters query = new UserQueryParameters(); + query.parameters.put(UserQueryParameters.FEDERATION_PROVIDER, FederationProvider.ADFS_2019); + query.parameters.put(UserQueryParameters.USER_TYPE, UserType.ON_PREM); + + User user = labUserProvider.getLabUser(query); + + PublicClientApplication pca = PublicClientApplication.builder( + TestConstants.ADFS_APP_ID). + authority(TestConstants.ADFS_AUTHORITY). + build(); + + pca.acquireToken(UserNamePasswordParameters. + builder(Collections.singleton(TestConstants.ADFS_SCOPE), + user.getUpn(), + user.getPassword().toCharArray()) + .build()) + .get(); + + assertNotNull(pca.getAccounts().join().iterator().next()); + assertEquals(pca.getAccounts().join().size(), 1); + } + + private static class TokenPersistence implements ITokenCacheAccessAspect { String data; diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenCache.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenCache.java index d12dc186..b5032d7c 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenCache.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenCache.java @@ -346,7 +346,7 @@ Set getAccounts(String clientId) { ((Account) rootAccounts.get(accCached.homeAccountId())).tenantProfiles.put(accCached.realm(), profile); } - if (accCached.homeAccountId().contains(accCached.localAccountId())) { + if (accCached.localAccountId() != null && accCached.homeAccountId().contains(accCached.localAccountId())) { ((Account) rootAccounts.get(accCached.homeAccountId())).username(accCached.username()); } } From 9ab97351df85dad23f9c5735d138d4df4db414ea Mon Sep 17 00:00:00 2001 From: Avery-Dunn Date: Fri, 28 Jul 2023 08:52:21 -0700 Subject: [PATCH 3/4] Log MSAL message but re-throw exception --- .../com.microsoft.aad.msal4j/HttpClientIT.java | 3 +-- .../microsoft/aad/msal4j/DefaultHttpClient.java | 14 +++++++++++--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/HttpClientIT.java b/msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/HttpClientIT.java index ad6eafb2..66f55986 100644 --- a/msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/HttpClientIT.java +++ b/msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/HttpClientIT.java @@ -15,7 +15,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; @TestInstance(TestInstance.Lifecycle.PER_CLASS) class HttpClientIT { @@ -82,6 +81,6 @@ private void assertAcquireTokenCommon_WithTimeout(User user, int readTimeout) .build()) .get()); - assertTrue(ex.getMessage().contains("Timeout while waiting for response from service.")); + assertEquals("com.microsoft.aad.msal4j.MsalClientException: java.net.SocketTimeoutException: Read timed out", ex.getMessage()); } } diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultHttpClient.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultHttpClient.java index 73135740..ee60eccc 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultHttpClient.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultHttpClient.java @@ -1,5 +1,8 @@ package com.microsoft.aad.msal4j; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import javax.net.ssl.HttpsURLConnection; import javax.net.ssl.SSLSocketFactory; import java.io.DataOutputStream; @@ -14,6 +17,7 @@ import java.util.Map; class DefaultHttpClient implements IHttpClient { + private final static Logger LOG = LoggerFactory.getLogger(DefaultHttpClient.class); private final Proxy proxy; private final SSLSocketFactory sslSocketFactory; @@ -118,10 +122,14 @@ private HttpResponse readResponseFromConnection(final HttpsURLConnection conn) t httpResponse.addHeaders(conn.getHeaderFields()); httpResponse.body(inputStreamToString(is)); return httpResponse; - } catch (SocketTimeoutException readException) {conn.getURL(); - throw new MsalServiceException("Timeout while waiting for response from service. If custom timeouts were set, increasing them may resolve this issue. See https://aka.ms/msal4j-http-client for more information and solutions.", AuthenticationErrorCode.HTTP_TIMEOUT); + } catch (SocketTimeoutException readException) { + LOG.error("Timeout while waiting for response from service. If custom timeouts were set, increasing them may resolve this issue. See https://aka.ms/msal4j-http-client for more information and solutions."); + + throw readException; } catch (ConnectException timeoutException) { - throw new MsalServiceException("Exception while connecting to service, there may be network issues preventing MSAL Java from connecting. See https://aka.ms/msal4j-http-client for more information and solutions.", AuthenticationErrorCode.HTTP_TIMEOUT); + LOG.error("Exception while connecting to service, there may be network issues preventing MSAL Java from connecting. See https://aka.ms/msal4j-http-client for more information and solutions."); + + throw timeoutException; } finally { if (is != null) { is.close(); From e5f3ae7ce37e40b5db528d505de54170d5fb00d3 Mon Sep 17 00:00:00 2001 From: Avery-Dunn Date: Fri, 28 Jul 2023 10:47:32 -0700 Subject: [PATCH 4/4] Update vulnerable test dependency --- msal4j-sdk/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/msal4j-sdk/pom.xml b/msal4j-sdk/pom.xml index cc2b0aba..9a3c8e3e 100644 --- a/msal4j-sdk/pom.xml +++ b/msal4j-sdk/pom.xml @@ -131,7 +131,7 @@ com.google.guava guava - 32.0.0-jre + 32.1.1-jre test