search

AzureAD / microsoft-authentication-library-for-js

Microsoft Authentication Library (MSAL) for JS
http://aka.ms/aadv2
MIT License
3.64k stars 2.65k forks source link

about:blank popup intermittently stays on on acquireTokenSilent/acquireTokenPopup #3160

Closed gabriel-kohen-by closed 3 years ago

gabriel-kohen-by commented 3 years ago

Library

Framework

React 16.4

Description

Hi Folks,

I'm using the libraries with the following versions:

    "@azure/msal-browser": "^2.12.0",
    "@azure/msal-react": "^1.0.0-beta.0"

Post logout redirect URL is ${window.location.origin}/ and redirectUrl is redirectUri The as you can see seems to be failing intermittently. See the attached recording where all is working fine for several times until around the 50th second mark of the video the about:blank window stays hanging. Video clip recreating the issue Any idea what I'm might be doing wrong? Thank you for the otherwise rock solid library.

Might be related to: https://github.com/AzureAD/microsoft-authentication-library-for-js/pull/2842

FYI: @tnorling , @jo-arroyo , @jasonnutter , @pkanher617 Originally posted by @gabriel-kohen-by in https://github.com/AzureAD/microsoft-authentication-library-for-js/issues/2842#issuecomment-792864888

Error Message

MSAL Configuration

// Provide configuration values here.
// For Azure B2C issues, please include your policies.
const msalProviderConfig = {
  type: 'popup',
  msalConfig: {
    auth: {
      clientId,
      authority,
      knownAuthorities: [authority],
      redirectUri,
      postLogoutRedirectUri,
    },
    system: {
      loggerOptions: {
        loggerCallback: getLoggerCallback(),
      },
    },
  },
  silentRequestConfig: {
    scopes: [scopes, 'offline_access'],
  },
  endSessionRequestConfig: {},
  loginRequestConfig: {
    scopes: [scopes, 'offline_access'],
  },
};

Reproduction steps

Open a SPA page triggering getting of a token with

Expected behavior

Close any popup if we resolved the promise coming back from asking for an access token.

Identity Provider

Browsers/Environment

Regression

Security

Source

tnorling commented 3 years ago

@gabriel-kohen-by Is there any error shown in the console when it fails? Can you share how you are initializing PublicClientApplication and how you are invoking loginPopup?

gabriel-kohen-by commented 3 years ago

Hi, I looked and could not see any errors in the console. Just the trace messages from the MSAL library where nothing looked suspicious. The PublicClientApplication us using the config above to initialize:

new PublicClientApplication(msalProviderConfig.msalConfig);

I login using the follows:

  const loginResponse = await this?.authProvider.loginPopup(getLoginRequest()).catch((error) => {

  getLoginRequest = () => {
    const loginHint = this.getAuthPreferredUsername();
    const domainHint = this.populateDomainHint();
    const loginRequest: PopupRequest = {
      scopes: msalProviderConfig.loginRequestConfig.scopes,
    };
    if (domainHint !== '') {
      loginRequest.domainHint = domainHint;
    }
    if (loginHint !== '') {
      loginRequest.loginHint = this.getAuthPreferredUsername();
    }
    return loginRequest;
  };

After I log in I issue the following logic to get access tokens. I may mention that I have few request for the access token coming in succession but according to the logs it shows my logic make sure they do not happen in parallel. Just in case:

  private requestToken = async (): Promise<AuthenticationResult> => {
    const authProvider = this.getAuthProvider();
    if (!this.getDefaultAccount()) {
      console.debug(`luiminate token: Default account not found. Logging in.`);
      await this.login();
    }
    const account = this.getDefaultAccount();

    if (account != null) {
      const silentTokenRequestParameters: SilentRequest = {
        scopes: this.getDefaultScopes(),
        forceRefresh: false,
        account,
      };
      const domainHint = this.populateDomainHint();
      if (domainHint !== '') {
        silentTokenRequestParameters.extraQueryParameters = {
          domainHint,
        };
      }

      const response = await authProvider
        .acquireTokenSilent(silentTokenRequestParameters)
        .catch(async (error: BrowserAuthError) => {
          if (error instanceof InteractionRequiredAuthError) {
            // fallback to interaction when silent call fails
            console.info(
              `InteractionRequiredAuthError response during getting token. Acquiring token interactive: ${stringify(
                error,
                null,
                2
              )}`
            );
            const successResponse = await authProvider.acquireTokenPopup({
              ...silentTokenRequestParameters,
              domainHint,
              prompt: 'login',
            });
            return successResponse;
          } else {
            console.error(`Error getting token: ${stringify(error)}`);
            return Promise.reject(`Unspecified error: ${error.name}`);
          }
        });
      return response as AuthenticationResult;
    }

    return Promise.reject('Could not get authorization token. Account not found because could not login.');
  };
tnorling commented 3 years ago

@gabriel-kohen-by

The PublicClientApplication us using the config above to initialize: new PublicClientApplication(msalProviderConfig.msalConfig);

Is this being called inside or outside of a react component? If it's being called inside it's possible your component is being re-rendered after calling loginPopup but before changing the url of the popup, leaving it in a hung state. If not, can you turn on verbose logging and paste or email the logs to me?

gabriel-kohen-by commented 3 years ago

The wrapper(singleton) that uses this service is called from various React components. I made sure that there is only one instance of that when the application is initialized and later on. Even if it's re-rendered it does all of it's actions through that singleton. 

import React from 'react';

import { LinearProgress } from '@material-ui/core';
import { AuthService } from './auth/authService';
import {
  AuthenticatedTemplate,
  MsalProvider,
  UnauthenticatedTemplate,
  useMsal,
  useMsalAuthentication,
} from '@azure/msal-react';
import { InteractionType } from '@azure/msal-browser';
import FallbackScreen from './components/fallbackScreen/fallbackScreen';
import App from './app';

function AzureADWrapper() {
  const authProvider = AuthService.getInstance().getAuthProvider();
  const UnauthenticatedErrorComponent = () => {
    const { login, error } = useMsalAuthentication(InteractionType.Popup, AuthService.getInstance().getLoginRequest());
    return (
      <>
        <UnauthenticatedTemplate>
          {error ? <FallbackScreen loginFunction={login} errorCode={error.errorCode} /> : <LinearProgress />}
        </UnauthenticatedTemplate>
      </>
    );
  };

  const MainApp = () => {
    const { accounts } = useMsal();
    if (accounts.length > 0) {
      return <App accountInfo={AuthService.getInstance().getDefaultAccount()} />;
    }
    return null;
  };

  return (
    <MsalProvider instance={authProvider}>
      <AuthenticatedTemplate>
        <MainApp />
      </AuthenticatedTemplate>
      <UnauthenticatedTemplate>
        <UnauthenticatedErrorComponent />
      </UnauthenticatedTemplate>
    </MsalProvider>
  );
}

export default AzureADWrapper;

My application is wrapped in an AzureADWrapper which makes sure the my actual app is not rendered unless the user has logged in. Most of it is taken from the new MSAL React documentation:

Also here are the log files. I could recreate the issue on the fifth tab I've opened. MSAL Logs upon initial login.log Second tab opened after.log Third tab opened.log Fourth tab.log Fifth-tab-caused-the-blank-pop-up-to-stay.log

tnorling commented 3 years ago

Can you turn on verbose level logging?

Example configuration: 

system: {
        loggerOptions: {
            loggerCallback: (level, message, containsPii) => {
                if (containsPii) {  
                    return; 
                }
                switch (level) {    
                    case msal.LogLevel.Error:   
                        console.error(message); 
                        return; 
                    case msal.LogLevel.Info:    
                        console.info(message);  
                        return; 
                    case msal.LogLevel.Verbose: 
                        console.debug(message); 
                        return; 
                    case msal.LogLevel.Warning: 
                        console.warn(message);  
                        return; 
                }
            },
            logLevel: LogLevel.Verbose
        }
    }
gabriel-kohen-by commented 3 years ago

Sure. the following is with Verbose logging on.

first-tab-logging-in.log second-tab-blank-popup-hangs.log

tnorling commented 3 years ago

@gabriel-kohen-by Thanks. I'm not seeing anything unexpected in the logs. I rewatched the video you shared and it appears when this happens the popup operation succeeds, the popup closes and a new popup is opened right away and it's this 2nd popup that hangs. I didn't see any indication in the logs of this 2nd popup being opened by MSAL. Can you confirm that the login succeeds when this behavior manifests and try to determine what is opening the popup? I'd be curious if you are able to reproduce this using one of our dev samples as that would be easier for me to debug.

gabriel-kohen-by commented 3 years ago

The login when we open it for the second tab (or 5th etc) uses the silent SSO login because it's already successfully logged in in the first tab. I can tell we successfully logged in in each of those because the app is already using access token possible only if the login was successful. Not to mention we get a valid token ID. As you saw some of of our React code I've based it on a the B2C sample code (as well as the type script). That's why I was confused when this showed up. This is creating increased grief for our dev teams and not sure how to further troubleshoot it.

tnorling commented 3 years ago

@gabriel-kohen-by I'm confused. Are you saying you don't expect to see a popup at all due to ssoSilent? That would be a different issue entirely? What I observed in the video (and please correct me if it's incorrect):

  1. Load app in tab 2, user is unauthenticated in top frame
  2. Call loginPopup
  3. Sign in succeeds and closes popup
  4. User is authenticated in top frame
  5. A 2nd popup is opened to about:blank and hangs

This erroneous 2nd popup is the problem and it did not appear from the logs that MSAL was opening this popup. Which means that either MSAL is not opening the popup or the 2nd popup is not being opened from the top frame where the logs are collected. Does your redirectUri automatically invoke loginPopup? It's unlikely but possible that popup1 is opening popup2 before closing. Try setting your redirectUri to a blank page that doesn't run any code, or at least doesn't run msal code.

Beyond that, without a live repro it's very hard for us to identify or troubleshoot the issue. If you can repro this using our dev samples with minimal changes we may be of better help.

gabriel-kohen-by commented 3 years ago

Sorry I misspoke about the SSO. I meant that in the second (and on) tab there is no interactive login anymore which seems because MSAL already know it's already logged in. As mentioned the redirectUrl msalConfig values are always:

const postLogoutRedirectUri = `${window.location.origin}/`;
const redirectUri = `${window.location.origin}`;

I thought that's what I've seen the the dev samples. Is that incorrect?

tnorling commented 3 years ago

Sorry I misspoke about the SSO. I meant that in the second (and on) tab there is no interactive login anymore which seems because MSAL already know it's already logged in.

The server already knows you're logged in. If you're using session storage MSAL does not know you are logged in until you ask the server, which is what the popup is doing and why it doesnt ask for creds.

I thought that's what I've seen the the dev samples. Is that incorrect?

The redirectUri can be whatever you like, it's not for me to say it's correct or incorrect.

It may be useful to understand how the auth flow works:

  1. Call loginPopup
  2. Popup navigates to AAD server to request an auth code
  3. Server prompts for credentials, or not if already signed in
  4. Server redirects to your redirectUri with the auth code in the hash of the url
  5. MSAL running in the top frame reads the hash from the popup and closes the popup
  6. MSAL makes a POST request to server to exchange the auth code from the hash for tokens

Again, it seems all of this is working correctly even in the bad case being discussed here. But what I suggested previously is that it's possible that the page you use as a redirectUri is automatically invoking loginPopup, resulting in a 2nd popup that never resolves. To confirm this theory you should try setting your redirectUri to a blank page that does not do this so that when the page is opened in the popup it merely serves up the hash for the top frame. You can do this on a per request basis: 

const request = {
    scopes: ["openid", "profile"],
    redirectUri: "/blank.html"
}
loginPopup(request)

I will note that if this is indeed what's happening, it's a regression. We should have code in place that blocks popups from opening more popups.

tnorling commented 3 years ago

@gabriel-kohen-by I was able to confirm my theory and reproduce this issue on my end. This is definitely unintended behavior and I'll try to get a fix up soon. In the meantime the mitigation is to set your redirectUri to a page that does not open a popup on page load.