AzureAD / microsoft-authentication-library-for-js

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

Msal Intermittently returning null in redirect uri on IE/Edge/Safari #347

Closed adamtay closed 5 years ago

adamtay commented 6 years ago

Hi,

I understand that this is a known issue as per the library wiki, however, Safari (MacOS X/iOS) appears to also be impacted by this issue.

Tested browsers:

Our solution:

As per https://github.com/AzureAD/microsoft-authentication-library-for-js/issues/330#issuecomment-403889919, adding navigateToLoginRedirectUri: false to the UserAgentApplication options appears to somewhat alleviate this issue on IE/Safari but not on Edge.

However, we are seeing mixed and intermittent results when testing between a normal browser session and private/incognito session for our tests browsers. i.e. Working on normal session Safari but not on private session.

As we are releasing a public facing website, we cannot be asking our end-users to be adding the websites domain to their 'trusted website' lists in order for msal authentication to function as intended.

If a work-around for this issue will not be ready within the upcoming months, please advise a suitable alternative for AADB2C integration. (perhaps hello.js?)

Thanks

nehaagrawal commented 6 years ago

@adamtay We are currently working on a fix for this issue and we are planing to release it in our future release.

sayanghosh123 commented 6 years ago

@nehaagrawal Thanks for the quick response. We would need to understand a timeline for this fix please, as we are smack dab in the middle of a SDLC programme which is dependent on this. We are happy to get into Private previews if those allow us to test out the fix. An update will be appreciated.

jburman commented 6 years ago

Also ran into this issue tonight in Firefox 61 (Windows 10).

vladkosarev commented 6 years ago

@nehaagrawal what is the timeline for the fix? Are there any other alternatives that do work with b2c and modern browsers?

Thanks.

vladkosarev commented 6 years ago

We converted our app to hellojs and that actually mostly works (other than popup+IE). So if you need to make sure that your app actually works with multiple browsers give hellojs a try. It's a but more verbose than MSAL but it's not too far off.

nehaagrawal commented 6 years ago

@vladkosarev @jburman @sayanghosh123 @adamtay This is fixed and released in MSAL 0.2.3 . Please check release notes for more details.

bh3605 commented 6 years ago

This isn't really fixed. You are storing the state in a cookie which preserves it, but when you go to retrieve it you aren't passing the storeAuthStateInCookie flag into the getItem method. This causes the code to attempt to retrieve from the cleared out sessionStorage, instead of the cookie. I'm referring specifically to line 1747 when you are testing its existence and then on line 1747 when you go to retrieve it. It's in the saveTokenFromHash in the UserAgentApplication class.

nehaagrawal commented 6 years ago

@bh3605 Could you please confirm what version of msal you are using? Also can you please send the link for the lines you are talking about. It seems the issue that you are talking about doesn't match the line number.

bh3605 commented 6 years ago

0.2.3

This is the section of code I'm referring to between lines 1744 and 1749 in the saveTokenFromHash method.

authorityKey = Constants.authority + Constants.resourceDelimeter + tokenResponse.stateResponse;
let authority: string;

if (!Utils.isEmpty(this._cacheStorage.getItem(authorityKey))) {
     authority = this._cacheStorage.getItem(authorityKey);
     authority = Utils.replaceFirstPath(authority, idToken.tenantId);
 }
nehaagrawal commented 6 years ago

@bh3605 the line number you mentioned doesn't handle state but authorityKey. We are storing and fetching the state correctly from cookie by passing the flag 'storeAuthStateInCookie'. this._cacheStorage.getItem(Constants.stateLogin, this.storeAuthStateInCookie)

why do you think that authorityKey needs to be stored in the cookie? Also what is the error you are getting?

bh3605 commented 6 years ago

I redirect to b2c to handle my sign in. In IE when I come back from b2c the sessionStorage will be cleared. You then handle the authentication response because of the presence of the hash. At this point in time nothing has been placed into sessionStorage because that code hasn't ran yet. When it gets the authority key from cache it will be null because it hasn't been written into the storageCache.

I've switched frameworks by now because my app is about to be deployed soon so I forget the exact error I was getting. I think I was getting constantly redirected to b2c because it wasn't able to create a user object and when I check to get a user if it's null I redirect the user to b2c to sign in.

adamtay commented 6 years ago

@bh3605 Which library have you converted to (presumably hello.js?) and how have your experiences been with it so far?

rohitnarula7176 commented 6 years ago

@bh3605 We already have a fix for this authority issue in the following PR. It will soon be merged and released. Can you please try the branch and see if it resolves your issue. https://github.com/AzureAD/microsoft-authentication-library-for-js/pull/452

bh3605 commented 6 years ago

It's neat you presume hello.js because that's the library our Microsoft Rep recommended and I was all for msal.js until this problem. After realizing I had to switch, I evaluated hello.js, angular-oauth2-oidc, oidc-client.js, and angular-auth-oidc-client. I ended up using oidc-client.js. I made a service to interact with it and it's really come along really well. Everything you call from oidc-client returns a promise so I make assumptions to determine if you're signed in and store the user in the service so not everything has to deal with a promise. Session storage provides better security over cookies so after someone completes sign in I take the user object, clear cookies, and tell the framework to use session storage. Odic-client.js is just enough, provides the ability to use a custom storage implementation (like an obj that stores cookies) and the ability to switch out configurations like when I switch from cookie storage to session storage. There are events you can tie in to. It doesn't go overboard with trying to do everything for you or completely fail at certain things like angular-oauth or angular-auth. Hello.js's problem is it doesn't do enough out of the box and I would have to do A LOT of stuff that all the other frameworks automatically do that I just didn't have time to implement but it is a great starting point if you have to deal with multiple IDPs

darrelmiller commented 6 years ago

@rohitnarula7176 Can you explain how the PR fixes this issue? As @bh3605 points out in his code snippet, when you retrieve the authorityKey it does not appear to be getting the authority from the cookie.

In the PR comments you state:

The entire storage can get wiped out during the transition between zones. Since the tokens are tied to authority, we need a way to restore the authority from cookie as well otherwise all the tokens will be saved with authority set to undefined.

When you store the authorityKey you do this:

 this._cacheStorage.setItem(authorityKey, this.authority, this.storeAuthStateInCookie);

but when you retrieve it, you do this

  authority = this._cacheStorage.getItem(authorityKey);

i.e. you don't pass in this.storeAuthStateInCookie.

I'm really just reiterating what @bh3605 previously said, but I don't see where we have addressed his concern.

rohitnarula7176 commented 6 years ago

@darrelmiller Are you referring to the PR or some other branch: We have applied this fix to loginRedirect and acquireTokenRedirect api's in the PR. Please see below: https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/708d41314d6f99ef68314b118cce80562d47726f/lib/msal-core/src/UserAgentApplication.ts#L412 https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/708d41314d6f99ef68314b118cce80562d47726f/lib/msal-core/src/UserAgentApplication.ts#L1064

Above is set before we initiate the request and below is checked when we verify the response: https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/708d41314d6f99ef68314b118cce80562d47726f/lib/msal-core/src/UserAgentApplication.ts#L1676 https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/708d41314d6f99ef68314b118cce80562d47726f/lib/msal-core/src/UserAgentApplication.ts#L1720

All these 4 places are passed a storeAuthStateInCookie flag.

darrelmiller commented 6 years ago

@rohitnarula7176 Thank you for the details. I was sure I had looked at the PR and I was convinced the this.storeAuthStateInCookie parameter wasn't provided in line 1676. But I was wrong. It does look to me like the PR will address @bh3605 's concerns.

bh3605 commented 6 years ago

@darrelmiller You're confusion is not unfounded. I was looking at the PR on mobile yesterday and was confused as to why I saw he was passing in the flag, then he wasn't, and then he was again. As far as I could tell, this was the only thing blocking IE and Edge from working. You can get IE working if you have the dev console opened. Maybe a tester had their console open?

Thank you @rohitnarula7176 for the fix!

baltuonis commented 5 years ago

So this was not actually fixed was it? We use 0.2.3 and we still sometimes get "invalid_state" with state=null in latest Firefox

adamtay commented 5 years ago

@baltuonis Have you initialized UserAgentApplication with the storeAuthStateInCookie flag set to true? The fix as part of 0.2.3 was to store state as part of the cookies.

baltuonis commented 5 years ago

Yeah but we can still reproduce the issue

MarcusPenn commented 5 years ago

This issue is not fixed. I am using version 0.2.4 and need to support Chrome, IE11, Firefox and Edge. I am testing all changes I make with a new InPrivate browsing session after every change.

Using loginPopup, Chrome is the only browser that works 100% of the time. Firefox and IE11 require a refresh after logging in to acquire the access token. Edge always redirects to the /null in the popup.

Switching over to use loginRedirect, Edge still always redirects to /null

Adding the storeAuthStateInCookie property and setting to true no longer redirects to /null. However no id token is set in local storage. This is the same for both loginRedirect and loginPopup.

@nehaagrawal can you reopen this issue to reflect this. I will continue to update this comment as I investigate in detail.

Looking into the the Microsoft docs to see how they suggest handling IE and Edge doesn't provide any answers.

Spent some more time looking at this. I can get the id token from the redirect. Following this I then try to get the access token on redirect. Coming back from the redirect for the access token the UserAgentApplication then crashes on setup as there is no id token.

With the logger attached:

logged: Thu, 31 Jan 2019 15:22:05 GMT:0.2.4-Info Processing the callback from redirect response eval code (2327) (28,5)

logged: Thu, 31 Jan 2019 15:22:05 GMT:0.2.4-Info State status:true; Request type:RENEW_TOKEN eval code (2327) (28,5)

logged: Thu, 31 Jan 2019 15:22:05 GMT:0.2.4-Info State is right eval code (2327) (28,5)

logged: Thu, 31 Jan 2019 15:22:05 GMT:0.2.4-Info Fragment has access token eval code (2327) (28,5)

SCRIPT5022: null or empty raw idtoken IdToken.js (32,1)

sameerag commented 5 years ago

@MarcusPenn we cannot store idTokens in cookies. The length is a problem and so is the security. This specific case for IE/Edge is a browser limitation that is beyond MSALs reach. We have our guidance documented here: Known Issues for IE/Edge

One option is to save the idToken once it is emitted at the application end and append the response with it. This is more easier probably with our new API surface which has a fleshed out 'response' object.

Please download our latest preview package or pull the dev branch and try updating your code and see if the issue still persists.

We are also now throwing error stack traces which may help us for any other issues you are facing.

If you would like guidance on how to use the new version of the library, please review our wiki page here.

RichardPergament commented 5 years ago

Feels a bit funky though, that even though one selects a full Microsoft stack, to cater enterprise users who default to Edge/IE, the pieces still don't interoperate.

MubashirEbad commented 5 years ago

@nehaagrawal Hi, so I am facing an issue, after I successfully login using the msal loginpopup, I should be redirected to a the URL I have specified, and in most of the cases and OS, it redirects and works perfectly fine. But I have some rare cases such as an MSI laptop with windows OS, which does not redirect and returns www.my_url/null. I have used Chrome, Firefox, Edge, but only on this laptop I have this certain problem.

I am not able to debug it somehow.

AndrewCraswell commented 5 years ago

My app which is about to launch to production is currently in pilot, but a few customers are reporting on Edge this is still occurring, where the site is attempting to redirect to [sitename].com/null. This is a blocker for our site to release at the moment... Doesn't seem like this issue should be closed unless it's being tracked somewhere else.

MubashirEbad commented 5 years ago

Yeah this issue still exists, it returns .com/null and throw the content not found Error. This is really a blocker and occurs more frequently in Incognito mode too. @nehaagrawal Please have a look.

jasonnutter commented 5 years ago

We'll be tracking this issue in #1039 (please leave any future comments on that issue), and we'll investigate to get a fix in for this soon.