AzureAD / azure-activedirectory-library-for-js

The code for ADAL.js and ADAL Angular has been moved to the MSAL.js repo. Please open any issues or PRs at the link below.
https://github.com/AzureAD/microsoft-authentication-library-for-js/tree/dev/maintenance/adal-angular
Apache License 2.0
627 stars 372 forks source link

acquireToken() relies on AuthenticationContext._user being set (which isn't always true) #729

Closed mmckenz closed 4 years ago

mmckenz commented 6 years ago

We use the ADAL.js directly (no adal-angular.js) and are having issues when retrieving access tokens.

Scenario:

  1. User logs in.
    • login() is called
  2. User is redirected back after logging in with an id_token as the URL hash.
    • handleWindowCallback() is called and AuthenticationContext._user is set
  3. User is redirected back to their initial page and attempts to access a resource.
    • acquireToken() is called but AuthenticationContext._user is now null and the call fails

It seems acquireToken() assumes AuthenticationContext._user is set but in our case, it is only set in the context of the call to handleWindowCallback(). When the user is redirected back to CONSTANTS.STORAGE.LOGIN_REQUEST at the end of handleWindowCallback(), AuthenticationContext._user is never set. As a result subsequent calls to acquireToken() fail.

acquireToken() should call getCachedUser() to populate this cached object from the id_token rather than attempt to access it directly through AuthenticationContext._user.

richardspence commented 6 years ago

Calling getUser should prime the user object, this is my bootstrap code:

public login(){
  if (this._adalInstance.isCallback(document.location.hash)) {
    this._adalInstance.handleWindowCallback();
  } else {
    const user = await this.getUser();
    if (!user || new Date(user.profile.exp * 1000) < new Date()) {
      this._adalInstance.login();
    } else {
      return user;
    }
  }
}
...
  public getUser(): Promise<UserInfo> {
        return new Promise((resolve, reject) => {
           this._adalInstance.getUser((msg, user) => {
                if (msg) {
                    reject(new Error(msg));
                } else {
                    resolve(user);
                }
            });
        });
    }
mmckenz commented 6 years ago

@richardspence - getUser() does indeed populate the _user cache object as you mention. This is actually the way we work around the issue today.

This very behavior you describe actually masked the issue for us for a while. We previously had logic which called getUser() prior to any acquireToken() calls and thus the _user object was always populated when needed. Our code was then updated as the initial call to getUser() was no longer needed and things started to fail.

While I would consider calling getUser() prior to any acquireToken() calls a valid work-around here, it does not address the root of the issue. Relying on a side effect of getUser() to successfully call acquireToken() creates an implicit dependency on these methods which appear to have the intention of being pure and callable on their own.

sameerag commented 4 years ago

This is good feedback and we have a better model for this in msal js. All current authentication work from microsoft is delivered through msal js library here. adal js is still supported only for security fixes. We would recommend to move to msal js for any advanced feature asks.