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

Hash stripping breaks Angular 2+ navigation events #755

Closed sliekens closed 4 years ago

sliekens commented 6 years ago

I want to use my own redirect behavior after calling handleWindowCallback, using Angular 5 routing.

this.context.callback = (errorDesc, token, error) => {
    this.router.navigate(['/home']);
};

// context is initialized with `navigateToLoginRequestUrl: false`
this.context.handleWindowCallback(hash);

This doesn't work: the navigation to /home gets canceled by handleWindowCallback.

image

The navigation is canceled because of a side-effect: handleWindowCallback modifies the window.location.hash after the token callback.

This is the problematic code: https://github.com/AzureAD/azure-activedirectory-library-for-js/blob/49ecdcc655312a111d470858cb424c3c609de4d7/lib/adal.js#L1354-L1367

rohitnarula7176 commented 6 years ago

@StevenLiekens This is done as it is not safe to leave the token in the browser url's fragment. To overcome this issue, you can set navigateToLoginRequestUrl:true and then change the key "adal.login.request" to your desired landing page('/home').

sliekens commented 6 years ago

This is done as it is not safe to leave the token in the browser url's fragment.

I agree, but why is it done after the callback instead of before? I think my issue would go away if stripping the hash is done before the callback.

sliekens commented 6 years ago

Would this code work?

// strip hash regardless of config.navigateToLoginRequestUrl 
if (window.parent === window && !isPopup) {
    window.location.hash = '';
}

// then do the callback
try {
    if (tokenReceivedCallback) {
        tokenReceivedCallback(errorDesc, token, error, tokenType);
    }

} catch (err) {
    self.error("Error occurred in user defined callback function: " + err);
}

// then do the optional redirect
if (self.config.navigateToLoginRequestUrl && window.parent === window && !isPopup) {
    window.location.href = self._getItem(self.CONSTANTS.STORAGE.LOGIN_REQUEST);
}
rohitnarula7176 commented 6 years ago

@StevenLiekens Thanks a lot for your contribution . In certain versions of IE (7 and 8), resetting the hash, results in a app reload. Please see this: https://stackoverflow.com/questions/2602260/javascript-location-hash-refreshing-in-ie. In this case, if we reset the hash before calling the callback, the page reloads and we lose the state of all callbacks and they will never be called.

The current code which first calls the callbacks and then resets the hash is still not perfect as in the current code if you try to do an asynchronous operation in the callback, the result of the operation will be lost as after we call the callback, we reset the browser location which results in a reload. The advantage with the current code is that callbacks will be called and synchronous operations inside the callbacks will succeed.

But I don't believe there is another way to overcome this problem for the public api's which solely relying on the main page redirect like login without popup and acquireTokenRedirect . To avoid this, we recommend using popup(separate window) for login and acquireToken already uses iframe so it does not have this issue.

Please let me know if you have additional questions.

sliekens commented 6 years ago

Thanks for explaining the reason but does IE 7 / 8 really matter anymore? I thought that IE11 is the only officially supported IE.

sameerag commented 4 years ago

Most of our support is on angular 5+ and IE11. 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.