Assorted fixes (#684)

* Remove default timeouts and improve exception messages

* Fix NPE for on-prem ADFS scenario

* Log MSAL message but re-throw exception

* Update vulnerable test dependency

Author: Avery-Dunn

msal4j-sdk/pom.xml

@@ -131,7 +131,7 @@
         <dependency>
             <groupId>com.google.guava</groupId>
             <artifactId>guava</artifactId>
-            <version>32.0.0-jre</version>
+            <version>32.1.1-jre</version>
             <scope>test</scope>
         </dependency>
         <dependency>

msal4j-sdk/src/integrationtest/java/com.microsoft.aad.msal4j/HttpClientIT.java

@@ -8,10 +8,13 @@
 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;
 
 @TestInstance(TestInstance.Lifecycle.PER_CLASS)
 class HttpClientIT {
@@ -34,6 +37,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 +65,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());
+
+        assertEquals("com.microsoft.aad.msal4j.MsalClientException: java.net.SocketTimeoutException: Read timed out", ex.getMessage());
+    }
 }

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;
 

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);
             }

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";
 }

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultHttpClient.java

@@ -1,25 +1,30 @@
 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;
 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;
 
 class DefaultHttpClient implements IHttpClient {
+    private final static Logger LOG = LoggerFactory.getLogger(DefaultHttpClient.class);
 
     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 +122,14 @@ private HttpResponse readResponseFromConnection(final HttpsURLConnection conn) t
             httpResponse.addHeaders(conn.getHeaderFields());
             httpResponse.body(inputStreamToString(is));
             return httpResponse;
+        } 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) {
+            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();

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenCache.java

@@ -346,7 +346,7 @@ Set<IAccount> 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());
                     }
                 }