Unified cache #418
Unified cache #418
Conversation
| reject(Constants_1.ErrorCodes.userLoginError + "|" + Constants_1.ErrorDescription.userLoginError); | ||
| return; | ||
| } | ||
| //if user didn't passes the login_hint and adal's idtoken is present and no userobject, user the login_hint gfrom adal's idToken | ||
| else if (!userObject_1 && !extraQueryParameters && !Utils_1.Utils.isEmpty(adalIdToken)) { |
There was a problem hiding this comment.
so, if any extraQueryParameters is present, adal id_token will be skipped? What if it's something like slice=testslice?
There was a problem hiding this comment.
I will remove the !extraQueryParameters from this condition. Just wanted to mention that all your review comments are on the generated code, The actual code sits in src/UserAgentApplication.ts
| @@ -2577,13 +2587,22 @@ var UserAgentApplication = /** @class */ (function () { | |||
| } | |||
| var scope_1 = scopes.join(" ").toLowerCase(); | |||
| var userObject_1 = user ? user : _this.getUser(); | |||
| if (!userObject_1) { | |||
| var adalIdToken = _this._cacheStorage.getItem(Constants_1.Constants.adalIdToken); | |||
There was a problem hiding this comment.
Should it be responsibility of getUser to check for both MSAL and ADAL id tokens and create user? I'd prefer this method to not have the responsibility of tracking ADAL id token.
There was a problem hiding this comment.
I agree. When I created this PR, I made the changes for both option 2(don't change getUser()) and option 3(create MSAL object from adal's idToken). I just wanted to demonstrate how the changes will look like if we go with one option or the other. Since we are going ahead with option 3, I don't need this code.
| @@ -2577,13 +2587,22 @@ var UserAgentApplication = /** @class */ (function () { | |||
| } | |||
| var scope_1 = scopes.join(" ").toLowerCase(); | |||
| var userObject_1 = user ? user : _this.getUser(); | |||
| if (!userObject_1) { | |||
| var adalIdToken = _this._cacheStorage.getItem(Constants_1.Constants.adalIdToken); | |||
| if (!userObject_1 && (extraQueryParameters && (extraQueryParameters.indexOf('login_hint') == -1 || extraQueryParameters.indexOf('sid') == -1)) && Utils_1.Utils.isEmpty(adalIdToken)) { | |||
There was a problem hiding this comment.
Consider moving this check out to a variable or helper function.
(extraQueryParameters && (extraQueryParameters.indexOf('login_hint') == -1 || extraQueryParameters.indexOf('sid') == -1))
| else if (!userObject_1 && !extraQueryParameters && !Utils_1.Utils.isEmpty(adalIdToken)) { | ||
| var idTokenObject = Utils_1.Utils.extractIdToken(adalIdToken); | ||
| console.log("ADAL's idToken exists. Extracting login information from ADAL's idToken "); | ||
| extraQueryParameters = "&login_hint=" + idTokenObject.upn; |
There was a problem hiding this comment.
There's no guarantee that upn will be there in AAD v1 id_token. Consider adding a check to verify that upn is there.
| var authenticationRequest_1; | ||
| var newAuthority = authority ? AuthorityFactory_1.AuthorityFactory.CreateInstance(authority, _this.validateAuthority) : _this.authorityInstance; | ||
| if (Utils_1.Utils.compareObjects(userObject_1, _this.getUser())) { | ||
| var msalIdToken = _this._cacheStorage.getItem(Constants_1.Constants.idTokenKey); | ||
| if (Utils_1.Utils.compareObjects(userObject_1, _this.getUser()) && !Utils_1.Utils.isEmpty(msalIdToken)) { //TODO revisit this change if getUser() is not null |
There was a problem hiding this comment.
What if we have adal id token, but not msal id token?
There was a problem hiding this comment.
It will go to the else block and get both idToken and accesstoken. Notice that the responsetype is idtoken and token
else {
authenticationRequest = new AuthenticationRequestParameters(newAuthority, this.clientId, scopes, ResponseTypes.id_token_token, this._redirectUri, this._state);
}
| this._user = User_1.User.createUser(idToken, clientInfo, this.authority); | ||
| return this._user; | ||
| var adalIdToken = this._cacheStorage.getItem(Constants_1.Constants.adalIdToken); | ||
| if (!Utils_1.Utils.isEmpty(adalIdToken)) { |
There was a problem hiding this comment.
I'd prefer to first check for msal id token. we should only fallback to adal id token, if msal one is missing
There was a problem hiding this comment.
If MSAL's idtoken exists then this._user object will be present and it will return before only. At this line
if (this._user) {
return this._user;
}
| clientInfoObject["utid"] = utid; | ||
| var rawClientInfo = Utils_1.Utils.base64EncodeStringUrlSafe(JSON.stringify(clientInfoObject)); | ||
| var clientInfo = new ClientInfo_1.ClientInfo(rawClientInfo); | ||
| this._user = User_1.User.createUser(idToken, clientInfo, this.authority); |
There was a problem hiding this comment.
we shouldn't return adal id token for msal user
There was a problem hiding this comment.
We have agreed on passing null/empty for unified cache. I will make this change.
Unified cache first commit