fixed authority issue #452

rohitnarula7176 · 2018-10-08T01:14:17Z

#347
*Fixed issue regarding saving authorities tied to AT.
*Added storeAuthStateInCookie check for authority for IE/Edge fix.
*Other minor fixes.
*Removed doc script from package.json just for the time being. Travis build is failing with the doc script. Need to re-add it.

nehaagrawal · 2018-10-11T17:55:22Z

lib/msal-angular/package.json

@@ -92,9 +92,9 @@
    "clean": "shx rm -rf dist docs",
    "doc": "typedoc --out ./docs ./src/**/* --gitRevision dev",
    "build:modules": "tsc && tsc -m es6 --outDir lib-es6",
-    "build": "npm run clean && npm run doc && npm run build:modules && webpack",
+    "build": "npm run clean && npm run build:modules && webpack",


Why are we removing the docs here?

Please see my comment summary on the PR above. I removed the docs temporarily as the doc script was failing on travis.

It used to work. Do you know what is the issue. May be we should fix the issue instead of removing it. Also why we are making changes in msal-angular as part of this PR? This PR is only to fix the authority issue in msal-core.

nehaagrawal · 2018-10-11T17:56:20Z

lib/msal-core/src/Utils.ts

+            return url;
+        var urlObject = this.GetUrlComponents(url);
+        var pathArray = urlObject.PathSegments;
+        if (pathArray.length !== 0 && pathArray[0] === "common" || pathArray[0] === "organizations") {


please use constants for common and organizations.

This existed previously and constants need to be applied at many places in the code base still. I will leave that up to you and Prithvi to take when you guys have time.

nehaagrawal · 2018-10-11T17:57:51Z

lib/msal-core/src/UserAgentApplication.ts

          } else {
            this._logger.warning("ClientInfo not received in the response from AAD");
-            user = User.createUser(idToken, new ClientInfo(clientInfo), authority);
+            user = User.createUser(idToken, new ClientInfo(clientInfo),);


extra comma? may be a typo?

nehaagrawal · 2018-10-11T17:58:31Z

lib/msal-core/package.json

@@ -80,7 +80,7 @@
    "clean": "shx rm -rf dist docs lib-commonjs lib-es6",
    "doc": "typedoc --out ./docs ./src/**/* --gitRevision dev",
    "build:modules": "tsc && tsc -m es6 --outDir lib-es6",
-    "build": "npm run clean && npm run doc && npm run build:modules && webpack && grunt && npm test",
+    "build": "npm run clean && npm run build:modules && webpack && grunt && npm test",


why are we removing the docs here?

nehaagrawal · 2018-10-11T20:40:33Z

lib/msal-core/src/UserAgentApplication.ts

@@ -1784,7 +1758,10 @@ protected getCachedTokenInternal(scopes : Array<string> , user: User): CacheResu
      this._cacheStorage.setItem(Constants.renewStatus + tokenResponse.stateResponse, Constants.tokenRenewStatusCompleted);
      this._cacheStorage.removeAcquireTokenEntries(authorityKey, acquireTokenUserKey);
      //this is required if navigateToLoginRequestUrl=false
-      this._cacheStorage.clearCookie();
+      if (this.storeAuthStateInCookie) {
+          this._cacheStorage.setItemCookie(authorityKey, '', -1);


why are we not doing this 'this._cacheStorage.setItemCookie(authorityKey, '', -1);' inside clearcookie?

Because authority key is dynamically created with state unlike state, nonce and loginStartPage. In order to set it inside the clearCookie function, you will have to change the clearCookie api to accept a optional string.

nehaagrawal · 2018-10-11T20:44:17Z

lib/msal-core/src/UserAgentApplication.ts

-                  this._cacheStorage.setItem(authorityKey, this.authority);
-              }
-
+              this._cacheStorage.setItem(authorityKey, this.authority, this.storeAuthStateInCookie);


why are we storing authority key in cookie? In the original design of IE/Edge fix, we were storing only state and nonce. Does authority key get wiped out during changing security zone?

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.

we never saw this issue when we released IE/Edge fix. I am curious why we didn't notice it. We can discuss this offline.

The issue existed in the IE/Edge fix release but was not caught during testing. We can easily reproduce this. We can discuss this during our meeting tom.

Can you please add some test cases?

nehaagrawal · 2018-10-11T20:45:07Z

lib/msal-core/src/UserAgentApplication.ts

-              this._cacheStorage.setItem(authorityKey, this.authority);
-          }
-
+          this._cacheStorage.setItem(authorityKey, this.authority);


are we missing the storeAuthStateInCookie flag here? You are passing it on line 412.

No, we only need this flag for redirect functions like loginRedirect and acquireTokenRedirect.

We saw some issues earlier when storeAuthStateInCookie flag was added to only loginRedirect and acquireTokenRedirect. We are currently passing this flag for popup cases as well. I think we should do it everywhere for consistency.

microsoft-authentication-library-for-js/lib/msal-core/src/UserAgentApplication.ts

Line 494 in 5fac0e7

this._cacheStorage.setItem(Constants.loginRequest, window.location.href, this.storeAuthStateInCookie);

nehaagrawal · 2018-10-11T20:50:23Z

lib/msal-core/src/UserAgentApplication.ts

-        if (Utils.isEmpty(this._cacheStorage.getItem(authorityKey))) {
-          this._cacheStorage.setItem(authorityKey, acquireTokenAuthority.CanonicalAuthority);
-        }
+        this._cacheStorage.setItem(authorityKey, acquireTokenAuthority.CanonicalAuthority);


are we missing the storeAuthStateInCookie flag here? You are passing it on line 412.

same comment as above.

nehaagrawal · 2018-10-11T20:51:39Z

lib/msal-core/src/UserAgentApplication.ts

-      this._cacheStorage.setItem(authorityKey, authenticationRequest.authority);
-    }
-
+    this._cacheStorage.setItem(authorityKey, authenticationRequest.authority);


are we missing the storeAuthStateInCookie flag here? You are passing it on line 412.

same comment as above.

nehaagrawal · 2018-10-15T22:35:55Z

lib/msal-angular/package.json

@@ -36,7 +36,7 @@
    "@angular/platform-browser-dynamic": "~4.3.0",
    "@angular/router": "^4.3.0",
    "@types/handlebars": "4.0.33",
-    "@types/jasmine": "^2.8.6",
+    "@types/jasmine": "2.5.38",


I think we should upgrade to @types/jasmine": "2.5.41",instead of 2.5.38 as per this github issue DefinitelyTyped/DefinitelyTyped#14569. I have created a separate PR to fix this issue. Please revert.

The same link has the version 2.5.38 listed as well which worked for other users. I don't think it matters which version to use as long as the doc issue is fixed. If you want , you can change the version later on after merging this PR.

If a higher version fixes the issue we shouldn't downgrade too much. I have fixed it in a different PR and merge changes to this PR.

nehaagrawal · 2018-10-15T22:55:02Z

lib/msal-core/package.json

@@ -29,7 +29,7 @@
  },
  "devDependencies": {
    "@types/handlebars": "4.0.33",
-    "@types/jasmine": "^2.6.0",
+    "@types/jasmine": "2.5.38",


same comment as above

rohitnarula7176 requested review from nehaagrawal, pkanher617, manoj-rath and aashiman October 8, 2018 01:14

nehaagrawal reviewed Oct 11, 2018

View reviewed changes

rohitnarula7176 mentioned this pull request Oct 11, 2018

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

Closed

fixed authority issue

134d60a

rohitnarula7176 force-pushed the ronaru/Issue_347 branch from f7187f1 to 134d60a Compare October 12, 2018 23:42

nehaagrawal reviewed Oct 15, 2018

View reviewed changes

Merge branch 'dev' into ronaru/Issue_347

2278385

nehaagrawal approved these changes Oct 22, 2018

View reviewed changes

manoj-rath approved these changes Oct 22, 2018

View reviewed changes

rohitnarula7176 merged commit f65cb13 into dev Oct 22, 2018

rohitnarula7176 deleted the ronaru/Issue_347 branch October 22, 2018 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed authority issue #452

fixed authority issue #452

rohitnarula7176 commented Oct 8, 2018 •

edited

Loading

nehaagrawal Oct 11, 2018

rohitnarula7176 Oct 11, 2018

nehaagrawal Oct 11, 2018 •

edited

Loading

nehaagrawal Oct 11, 2018

rohitnarula7176 Oct 11, 2018

nehaagrawal Oct 11, 2018

rohitnarula7176 Oct 11, 2018

nehaagrawal Oct 11, 2018

nehaagrawal Oct 11, 2018

rohitnarula7176 Oct 11, 2018

nehaagrawal Oct 11, 2018

rohitnarula7176 Oct 11, 2018

nehaagrawal Oct 11, 2018

rohitnarula7176 Oct 11, 2018

nehaagrawal Oct 11, 2018

nehaagrawal Oct 11, 2018 •

edited

Loading

rohitnarula7176 Oct 11, 2018

nehaagrawal Oct 12, 2018 •

edited

Loading

nehaagrawal Oct 11, 2018

rohitnarula7176 Oct 11, 2018

nehaagrawal Oct 11, 2018

rohitnarula7176 Oct 11, 2018

nehaagrawal Oct 15, 2018

rohitnarula7176 Oct 15, 2018

nehaagrawal Oct 22, 2018

nehaagrawal Oct 15, 2018

fixed authority issue #452

fixed authority issue #452

Conversation

rohitnarula7176 commented Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nehaagrawal Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nehaagrawal Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nehaagrawal Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohitnarula7176 commented Oct 8, 2018 •

edited

Loading

nehaagrawal Oct 11, 2018 •

edited

Loading

nehaagrawal Oct 11, 2018 •

edited

Loading

nehaagrawal Oct 12, 2018 •

edited

Loading