Merge pull request #1686 from AzureAD/timeout-error-remove-urlnavigate

Remove url from timeout error message, move to errorPii logger
AzureAD · Jun 3, 2020 · c6e60bd · c6e60bd
2 parents 955b2f9 + 8c82178
commit c6e60bd
Show file tree

Hide file tree

Showing 8 changed files with 32 additions and 23 deletions.
diff --git a/lib/msal-core/src/Logger.ts b/lib/msal-core/src/Logger.ts
@@ -74,10 +74,10 @@ export class Logger {// Singleton Class
         const timestamp = new Date().toUTCString();
         let log: string;
         if (!StringUtils.isEmpty(this.correlationId)) {
-            log = timestamp + ":" + this.correlationId + "-" + libraryVersion() + "-" + LogLevel[logLevel] + " " + logMessage;
+            log = timestamp + ":" + this.correlationId + "-" + libraryVersion() + "-" + LogLevel[logLevel] + (containsPii ? "-pii" : "") + " " + logMessage;
         }
         else {
-            log = timestamp + ":" + libraryVersion() + "-" + LogLevel[logLevel] + " " + logMessage;
+            log = timestamp + ":" + libraryVersion() + "-" + LogLevel[logLevel] + (containsPii ? "-pii" : "") + " " + logMessage;
         }
         this.executeCallback(logLevel, log, containsPii);
     }

diff --git a/lib/msal-core/src/UserAgentApplication.ts b/lib/msal-core/src/UserAgentApplication.ts
@@ -574,7 +574,7 @@ export class UserAgentApplication {
                 // popUpWindow will be null for redirects, so we dont need to attempt to monitor the window
                 if (popUpWindow) {
                     try {
-                        const hash = await WindowUtils.monitorWindowForHash(popUpWindow, this.config.system.loadFrameTimeout, urlNavigate);
+                        const hash = await WindowUtils.monitorWindowForHash(popUpWindow, this.config.system.loadFrameTimeout, urlNavigate, this.logger);
 
                         this.handleAuthenticationResponse(hash);
 
@@ -876,7 +876,7 @@ export class UserAgentApplication {
     private async loadIframeTimeout(urlNavigate: string, frameName: string, requestSignature: string): Promise<void> {
         // set iframe session to pending
         const expectedState = window.activeRenewals[requestSignature];
-        this.logger.verbose("Set loading state to pending for: " + requestSignature + ":" + expectedState);
+        this.logger.verbosePii("Set loading state to pending for: " + requestSignature + ":" + expectedState);
         this.cacheStorage.setItem(`${TemporaryCacheKeys.RENEW_STATUS}${Constants.resourceDelimiter}${expectedState}`, Constants.inProgress);
 
         // render the iframe synchronously if app chooses no timeout, else wait for the set timer to expire
@@ -885,7 +885,7 @@ export class UserAgentApplication {
             WindowUtils.loadFrameSync(urlNavigate, frameName, this.logger);
 
         try {
-            const hash = await WindowUtils.monitorWindowForHash(iframe.contentWindow, this.config.system.loadFrameTimeout, urlNavigate, true);
+            const hash = await WindowUtils.monitorWindowForHash(iframe.contentWindow, this.config.system.loadFrameTimeout, urlNavigate, this.logger, true);
 
             if (hash) {
                 this.handleAuthenticationResponse(hash);
@@ -1409,7 +1409,7 @@ export class UserAgentApplication {
         WindowUtils.addHiddenIFrame(frameName, this.logger);
 
         this.updateCacheEntries(serverAuthenticationRequest, account, false);
-        this.logger.verbose("Renew token Expected state: " + serverAuthenticationRequest.state);
+        this.logger.verbosePii("Renew token Expected state: " + serverAuthenticationRequest.state);
 
         // Build urlNavigate with "prompt=none" and navigate to URL in hidden iFrame
         const urlNavigate = UrlUtils.urlRemoveQueryStringParameter(UrlUtils.createNavigateUrl(serverAuthenticationRequest), Constants.prompt) + Constants.prompt_none + Constants.response_mode_fragment;

diff --git a/lib/msal-core/src/error/ClientAuthError.ts b/lib/msal-core/src/error/ClientAuthError.ts
@@ -137,10 +137,8 @@ export class ClientAuthError extends AuthError {
         return new ClientAuthError(ClientAuthErrorMessage.popUpWindowError.code, errorMessage);
     }
 
-    static createTokenRenewalTimeoutError(urlNavigate: string): ClientAuthError {
-        const errorMessage = `URL navigated to is ${urlNavigate}, ${ClientAuthErrorMessage.tokenRenewalError.desc}`;
-        return new ClientAuthError(ClientAuthErrorMessage.tokenRenewalError.code,
-            errorMessage);
+    static createTokenRenewalTimeoutError(): ClientAuthError {
+        return new ClientAuthError(ClientAuthErrorMessage.tokenRenewalError.code, ClientAuthErrorMessage.tokenRenewalError.desc);
     }
 
     static createInvalidIdTokenError(idToken: IdToken) : ClientAuthError {

diff --git a/lib/msal-core/src/utils/WindowUtils.ts b/lib/msal-core/src/utils/WindowUtils.ts
@@ -45,13 +45,16 @@ export class WindowUtils {
      * Monitors a window until it loads a url with a hash
      * @ignore
      */
-    static monitorWindowForHash(contentWindow: Window, timeout: number, urlNavigate: string, isSilentCall?: boolean): Promise<string> {
+    static monitorWindowForHash(contentWindow: Window, timeout: number, urlNavigate: string, logger: Logger, isSilentCall?: boolean): Promise<string> {
         return new Promise((resolve, reject) => {
             const maxTicks = timeout / WindowUtils.POLLING_INTERVAL_MS;
             let ticks = 0;
 
+            logger.verbose("monitorWindowForHash polling started");
+
             const intervalId = setInterval(() => {
                 if (contentWindow.closed) {
+                    logger.error("monitorWindowForHash window closed");
                     clearInterval(intervalId);
                     reject(ClientAuthError.createUserCancelledError());
                     return;
@@ -88,11 +91,14 @@ export class WindowUtils {
                 }
 
                 if (href && UrlUtils.urlContainsHash(href)) {
+                    logger.verbose("monitorWindowForHash found url in hash");
                     clearInterval(intervalId);
                     resolve(contentWindow.location.hash);
                 } else if (ticks > maxTicks) {
+                    logger.error("monitorWindowForHash unable to find hash in url, timing out");
+                    logger.errorPii(`monitorWindowForHash polling timed out for url: ${urlNavigate}`);
                     clearInterval(intervalId);
-                    reject(ClientAuthError.createTokenRenewalTimeoutError(urlNavigate)); // better error?
+                    reject(ClientAuthError.createTokenRenewalTimeoutError());
                 }
             }, WindowUtils.POLLING_INTERVAL_MS);
         });
@@ -108,7 +114,7 @@ export class WindowUtils {
          * This trick overcomes iframe navigation in IE
          * IE does not load the page consistently in iframe
          */
-        logger.info("LoadFrame: " + frameName);
+        logger.infoPii("LoadFrame: " + frameName);
 
         return new Promise((resolve, reject) => {
             setTimeout(() => {
@@ -156,7 +162,7 @@ export class WindowUtils {
             return null;
         }
 
-        logger.info("Add msal frame to document:" + iframeId);
+        logger.infoPii("Add msal frame to document:" + iframeId);
         let adalFrame = document.getElementById(iframeId) as HTMLIFrameElement;
         if (!adalFrame) {
             if (document.createElement &&

diff --git a/lib/msal-core/test/error/ClientAuthError.spec.ts b/lib/msal-core/test/error/ClientAuthError.spec.ts
@@ -112,7 +112,7 @@ describe("ClientAuthError.ts Class", () => {
 
   it("createTokenRenewalTimeoutError creates a ClientAuthError object", () => {
 
-    const tokenRenewalTimeOutError = ClientAuthError.createTokenRenewalTimeoutError("some string");
+    const tokenRenewalTimeOutError = ClientAuthError.createTokenRenewalTimeoutError();
     let err: ClientAuthError;
 
     try {
@@ -122,8 +122,8 @@ describe("ClientAuthError.ts Class", () => {
     }
 
     expect(err.errorCode).to.equal(ClientAuthErrorMessage.tokenRenewalError.code);
-    expect(err.errorMessage).to.equal(`URL navigated to is some string, ${ClientAuthErrorMessage.tokenRenewalError.desc}`);
-    expect(err.message).to.equal(`URL navigated to is some string, ${ClientAuthErrorMessage.tokenRenewalError.desc}`);
+    expect(err.errorMessage).to.equal(ClientAuthErrorMessage.tokenRenewalError.desc);
+    expect(err.message).to.equal(ClientAuthErrorMessage.tokenRenewalError.desc);
     expect(err.name).to.equal("ClientAuthError");
     expect(err.stack).to.include("ClientAuthError.spec.ts");
   });

diff --git a/lib/msal-core/test/utils/WindowUtils.spec.ts b/lib/msal-core/test/utils/WindowUtils.spec.ts
@@ -6,6 +6,8 @@ import { TEST_CONFIG } from "../TestConstants";
 import { ClientAuthError } from "../../src/error/ClientAuthError";
 import { Logger } from "../../src";
 
+const logger = new Logger(() => {});
+
 describe("WindowUtils", () => {
     describe("monitorWindowForHash", () => {
         it("times out (popup)", done => {
@@ -19,7 +21,7 @@ describe("WindowUtils", () => {
             };
 
             // @ts-ignore
-            WindowUtils.monitorWindowForHash(iframe.contentWindow, 500)
+            WindowUtils.monitorWindowForHash(iframe.contentWindow, 500, "url", logger)
                 .catch((err: ClientAuthError) => {
                     done();
                 });
@@ -34,7 +36,7 @@ describe("WindowUtils", () => {
             };
 
             // @ts-ignore
-            WindowUtils.monitorWindowForHash(iframe.contentWindow, 500, "http://login.microsoftonline.com", true)
+            WindowUtils.monitorWindowForHash(iframe.contentWindow, 500, "http://login.microsoftonline.com", logger, true)
                 .catch((err: ClientAuthError) => {
                     done();
                 });
@@ -51,7 +53,7 @@ describe("WindowUtils", () => {
             };
 
             // @ts-ignore
-            WindowUtils.monitorWindowForHash(iframe.contentWindow, 1000)
+            WindowUtils.monitorWindowForHash(iframe.contentWindow, 1000, "url", logger)
                 .then((hash: string) => {
                     expect(hash).to.equal("#access_token=hello");
                     done();
@@ -77,7 +79,7 @@ describe("WindowUtils", () => {
             };
 
             // @ts-ignore
-            WindowUtils.monitorWindowForHash(iframe.contentWindow, 1000)
+            WindowUtils.monitorWindowForHash(iframe.contentWindow, 1000, "url", logger)
                 .catch((error: ClientAuthError) => {
                     expect(error.errorCode).to.equal('user_cancelled');
                     done();

diff --git a/samples/react-sample-app/src/AuthProvider.js b/samples/react-sample-app/src/AuthProvider.js
@@ -39,7 +39,7 @@ export default C =>
                         })
                         : msalApp.acquireTokenPopup(request);
                 } else {
-                    console.error('Non-interactive error:', error.errorCode)
+                    console.error('Non-interactive error:', error)
                 }
             });
         }

diff --git a/samples/react-sample-app/src/auth-utils.js b/samples/react-sample-app/src/auth-utils.js
@@ -1,4 +1,4 @@
-import { UserAgentApplication, Logger } from "msal";
+import { UserAgentApplication, Logger, LogLevel } from "msal";
 
 export const requiresInteraction = errorMessage => {
     if (!errorMessage || !errorMessage.length) {
@@ -75,6 +75,9 @@ export const msalApp = new UserAgentApplication({
         navigateFrameWait: 500,
         logger: new Logger((logLevel, message) => {
             console.log(message);
+        }, {
+            level: LogLevel.Verbose,
+            piiLoggingEnabled: true
         }),
         telemetry: {
             applicationName: "react-sample-app",