AzureAD / microsoft-authentication-library-for-java

Microsoft Authentication Library (MSAL) for Java http://aka.ms/aadv2
MIT License
285 stars 142 forks source link

[Bug] HttpListener (HTTP-Dispatcher thread) stays alive for 120 seconds after client timeout occurs #741

Closed samsan-nasa closed 9 months ago

samsan-nasa commented 11 months ago

Library version used

1.14.0

Java version

Java 11

Scenario

PublicClient (AcquireTokenInteractive, AcquireTokenByUsernamePassword)

Is this a new or an existing app?

This is a new app or experiment

Issue description and reproduction steps

This example is using OAuth2 authentication. I am using the Interactive method using the default system browser to acquire a token. When I use a timeout value with the call to acquire the token with the .orTImeout() or .completeOnTimeout() methods, the HttpListener thread stays alive after the client timeout. If you attempt a retry you will get an exception regarding the localhost port being in use because the HttpListener thread stays alive on that port.

ClientTimeout-HttpListenerStaysAlive

Relevant code snippets

// Client code adds a timeout to the acquireToken call:

PublicClientApplication publicClientApplication =
                    PublicClientApplication
                            .builder(CLIENT_ID)
                            .authority(AUTHORITY)
                            .logPii(true)
                            .build();

            InteractiveRequestParameters parameters = InteractiveRequestParameters
                    .builder(new URI("http://localhost:8888"))
                    .scopes(SCOPE)
                    .build();

            IAuthenticationResult result = publicClientApplication.acquireToken(parameters)
                .completeOnTimeout(null, 2, TimeUnit.SECONDS)
                .join();

//AcquireTokenByInteractiveFlowSupplier class hard codes a 120
//second timeout for the HttpListener:

try {
            LOG.debug("Listening for authorization result");
            long expirationTime = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis()) + 120;

            while (result == null && !interactiveRequest.futureReference().get().isCancelled() &&
                    TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis()) < expirationTime) {

                result = authorizationResultQueue.poll(100, TimeUnit.MILLISECONDS);
            }
        } catch (Exception e) {
            throw new MsalClientException(e);
        }

// Full client code below

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.microsoft.aad.msal4j.*;

import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.util.Base64;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.TimeUnit;

public class JavaDesktopClient {

    // 1)  Set the Client ID
    private final static String CLIENT_ID = "";

    // 2) Enter the URL for the AUTHORITY
    private final static String AUTHORITY = "";

    // 3) Set the Scope if needed.
    private final static Set<String> SCOPE = Collections.singleton("");

    public static void main(String args[]) throws Exception {
        try {
            PublicClientApplication publicClientApplication =
                    PublicClientApplication
                            .builder(CLIENT_ID)
                            .authority(AUTHORITY)
                            .logPii(true)
                            .build();

            InteractiveRequestParameters parameters = InteractiveRequestParameters
                    .builder(new URI("http://localhost:8888"))
                    .scopes(SCOPE)
                    .build();

            IAuthenticationResult result = publicClientApplication.acquireToken(parameters)
                .orTimeout(2, TimeUnit.SECONDS)
                //.completeOnTimeout(null, 2, TimeUnit.SECONDS)
                .join();

            if (result == null) {
                // Add breakpoint here if you use completeOnTimeout() to examine
                // threads and see the HTTP-Dispatcher (HttpListener class) stay
                // active for 120 seconds.
                System.out.println("\n\nAuthentication result was NULL.");
                return;
            }

            System.out.println("\n\nAccess token: " + result.accessToken());
            System.out.println("\n\nId token: " + result.idToken());
            System.out.println("\n\nAccount username: " + result.account().username());

            String[] jwtChunks = result.idToken().split("\\.");

            byte[] decodedBytes = Base64.getDecoder().decode(jwtChunks[1]);
            String jsonStr = new String(decodedBytes);

            ObjectMapper mapper = new ObjectMapper();
            JsonNode jsonNode = mapper.readTree(jsonStr);

            System.out.println(jsonNode);

        } catch (Exception msalexp) {
            // Add breakpoint here if you use orTimeout() to examine
            // threads and see the HTTP-Dispatcher (HttpListener class) stay
            // active for 120 seconds.
            System.out.println("\n\n******************************\nException: \n");
            msalexp.printStackTrace();
        }
    }

}

Expected behavior

When the client timeout is smaller than 120 seconds (the length of the HttpListener timout) the HttpListener stays alive after the acquireToken() call has completed.

If you add retry logic to the code example, you will get an exception that the localhost port is aleady in use.

Id you add a breakpoint in the catch clause you will see the HttpListener thread (HTTP-Dispatcher thread) stay alive until the 120 second timeout.

Identity provider

Other

Regression

No response

Solution and workarounds

Do not retry authentication for 120 seconds to allow HttpListener to die.

Avery-Dunn commented 11 months ago

Hello @samsan-nasa : I believe this part of the polling/waiting logic occurs in the getAuthorizationResultFromHttpListener method of 'AcquireTokenByInteractiveFlowSupplier'.

There, it waits until there's a result, the future is canceled, or it reaches an expiration time based on the httpPollingTimeoutInSeconds value for InteractiveRequestParameters, which has a default timeout of 120 seconds: https://github.com/AzureAD/microsoft-authentication-library-for-java/blob/d4b5096f9e7926fb491978b45599dc96b46f8f83/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/InteractiveRequestParameters.java#L100

It does seems like there's bad behavior here: that thread does end when you 'cancel' the future, but doesn't seem to when the future completes 'exceptionally' because of a timeout.

However, setting that value to your desired timeout should cause the thread to close correctly for now, and we'll investigate a good way way to improve this behavior or make it more clear.

samsan-nasa commented 10 months ago

Thank you. Yes, I copied the wrong code where I saw the 120 value hardcoded. Perhaps you can expose this value as a settable builder value or change the httpPollingTimeoutInSeconds value to match the value specified by the parameter passed to the orTimeout() function for the PublicClientApplication class.

Avery-Dunn commented 9 months ago

As of the recent 1.14.1 release, the threads should now shut down when the futures time out. The httpPollingTimeoutInSeconds value will be used to end the loop if you don't set a timeout in the future itself, but if you set a timeout in the future then that will be followed instead.