From 13be2127fab88e2255e958b5bea0691ba057bd77 Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Tue, 11 Aug 2020 16:22:56 -0700 Subject: [PATCH 01/14] Change loginPopup to sync, add openSizedPopup function to handle opening popup --- .../src/app/PublicClientApplication.ts | 17 +++--- .../src/interaction_handler/PopupHandler.ts | 53 ++++++++++++------- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/lib/msal-browser/src/app/PublicClientApplication.ts b/lib/msal-browser/src/app/PublicClientApplication.ts index 866ad37874..ebd8a17fcc 100644 --- a/lib/msal-browser/src/app/PublicClientApplication.ts +++ b/lib/msal-browser/src/app/PublicClientApplication.ts @@ -249,8 +249,13 @@ export class PublicClientApplication implements IPublicClientApplication { * * @returns {Promise.} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object */ - async loginPopup(request: PopupRequest): Promise { - return this.acquireTokenPopup(request); + loginPopup(request: PopupRequest): Promise { + if (false) { // flag is set to true: Office Add-Ins + return this.acquireTokenPopup(request); + } else { // flag false: all browsers + const popup = PopupHandler.openSizedPopup(); + return this.acquireTokenPopup(request, popup); + } } /** @@ -259,7 +264,7 @@ export class PublicClientApplication implements IPublicClientApplication { * * @returns {Promise.} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object */ - async acquireTokenPopup(request: PopupRequest): Promise { + async acquireTokenPopup(request: PopupRequest, popup?: Window|null): Promise { try { // Preflight request const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request); @@ -274,7 +279,7 @@ export class PublicClientApplication implements IPublicClientApplication { const navigateUrl = await authClient.getAuthCodeUrl(validRequest); // Acquire token with popup - return await this.popupTokenHelper(navigateUrl, authCodeRequest, authClient); + return await this.popupTokenHelper(navigateUrl, authCodeRequest, authClient, popup); } catch (e) { this.browserStorage.cleanRequest(); throw e; @@ -285,11 +290,11 @@ export class PublicClientApplication implements IPublicClientApplication { * Helper which acquires an authorization code with a popup from given url, and exchanges the code for a set of OAuth tokens. * @param navigateUrl */ - private async popupTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient): Promise { + private async popupTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient, popup?: Window|null): Promise { // Create popup interaction handler. const interactionHandler = new PopupHandler(authClient, this.browserStorage); // Show the UI once the url has been created. Get the window handle for the popup. - const popupWindow: Window = interactionHandler.initiateAuthRequest(navigateUrl, authCodeRequest); + const popupWindow = interactionHandler.initiateAuthRequest(navigateUrl, authCodeRequest, popup); // Monitor the window for the hash. Return the string value and close the popup when the hash is received. Default timeout is 60 seconds. const hash = await interactionHandler.monitorPopupForHash(popupWindow, this.config.system.windowHashTimeout); // Handle response from hash string. diff --git a/lib/msal-browser/src/interaction_handler/PopupHandler.ts b/lib/msal-browser/src/interaction_handler/PopupHandler.ts index d42284bd95..889c89f118 100644 --- a/lib/msal-browser/src/interaction_handler/PopupHandler.ts +++ b/lib/msal-browser/src/interaction_handler/PopupHandler.ts @@ -27,7 +27,7 @@ export class PopupHandler extends InteractionHandler { * Opens a popup window with given request Url. * @param requestUrl */ - initiateAuthRequest(requestUrl: string, authCodeRequest: AuthorizationCodeRequest): Window { + initiateAuthRequest(requestUrl: string, authCodeRequest: AuthorizationCodeRequest, popup?: Window|null): Window { // Check that request url is not empty. if (!StringUtils.isEmpty(requestUrl)) { // Save auth code request @@ -36,7 +36,7 @@ export class PopupHandler extends InteractionHandler { this.browserStorage.setItem(this.browserStorage.generateCacheKey(BrowserConstants.INTERACTION_STATUS_KEY), BrowserConstants.INTERACTION_IN_PROGRESS_VALUE, CacheSchemaType.TEMPORARY); this.authModule.logger.infoPii("Navigate to:" + requestUrl); // Open the popup window to requestUrl. - return this.openPopup(requestUrl, Constants.LIBRARY_NAME, BrowserConstants.POPUP_WIDTH, BrowserConstants.POPUP_HEIGHT); + return this.openPopup(requestUrl, popup); } else { // Throw error if request URL is empty. this.authModule.logger.error("Navigate url is empty"); @@ -112,25 +112,19 @@ export class PopupHandler extends InteractionHandler { * @ignore * @hidden */ - private openPopup(urlNavigate: string, title: string, popUpWidth: number, popUpHeight: number): Window { + private openPopup(urlNavigate: string, popup?: Window|null): Window { try { - /** - * adding winLeft and winTop to account for dual monitor - * using screenLeft and screenTop for IE8 and earlier - */ - const winLeft = window.screenLeft ? window.screenLeft : window.screenX; - const winTop = window.screenTop ? window.screenTop : window.screenY; - /** - * window.innerWidth displays browser window"s height and width excluding toolbars - * using document.documentElement.clientWidth for IE8 and earlier - */ - const width = window.innerWidth || document.documentElement.clientWidth || document.body.clientWidth; - const height = window.innerHeight || document.documentElement.clientHeight || document.body.clientHeight; - const left = Math.max(0, ((width / 2) - (popUpWidth / 2)) + winLeft); - const top = Math.max(0, ((height / 2) - (popUpHeight / 2)) + winTop); - - // open the window - const popupWindow = window.open(urlNavigate, title, "width=" + popUpWidth + ", height=" + popUpHeight + ", top=" + top + ", left=" + left); + let popupWindow; + // Popup window passed in, setting url to navigate to + if (popup) { + popupWindow = popup; + popupWindow.location.assign(urlNavigate); + } else if (typeof popup === "undefined") { + // Popup will be undefined if it was not passed in + popupWindow = PopupHandler.openSizedPopup(urlNavigate); + } + + // Popup will be null if popups are blocked if (!popupWindow) { throw BrowserAuthError.createEmptyWindowCreatedError(); } @@ -148,6 +142,25 @@ export class PopupHandler extends InteractionHandler { } } + static openSizedPopup(urlNavigate: string = "about:blank"): Window|null { + /** + * adding winLeft and winTop to account for dual monitor + * using screenLeft and screenTop for IE8 and earlier + */ + const winLeft = window.screenLeft ? window.screenLeft : window.screenX; + const winTop = window.screenTop ? window.screenTop : window.screenY; + /** + * window.innerWidth displays browser window"s height and width excluding toolbars + * using document.documentElement.clientWidth for IE8 and earlier + */ + const width = window.innerWidth || document.documentElement.clientWidth || document.body.clientWidth; + const height = window.innerHeight || document.documentElement.clientHeight || document.body.clientHeight; + const left = Math.max(0, ((width / 2) - (BrowserConstants.POPUP_WIDTH / 2)) + winLeft); + const top = Math.max(0, ((height / 2) - (BrowserConstants.POPUP_HEIGHT / 2)) + winTop); + + return window.open(urlNavigate, Constants.LIBRARY_NAME, "width=" + BrowserConstants.POPUP_WIDTH + ", height=" + BrowserConstants.POPUP_HEIGHT + ", top=" + top + ", left=" + left) + } + /** * Event callback to unload main window. */ From e4bfa933a9dacac09ad62f19c6d401f6996a2aee Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Wed, 12 Aug 2020 15:58:25 -0700 Subject: [PATCH 02/14] Add delayOpenPopup to configuration --- lib/msal-browser/src/app/PublicClientApplication.ts | 6 ++++-- lib/msal-browser/src/config/Configuration.ts | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/msal-browser/src/app/PublicClientApplication.ts b/lib/msal-browser/src/app/PublicClientApplication.ts index 8503dbf546..b85b775fa0 100644 --- a/lib/msal-browser/src/app/PublicClientApplication.ts +++ b/lib/msal-browser/src/app/PublicClientApplication.ts @@ -292,9 +292,11 @@ export class PublicClientApplication implements IPublicClientApplication { * @returns {Promise.} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object */ loginPopup(request: PopupRequest): Promise { - if (false) { // flag is set to true: Office Add-Ins + // delayOpenPopup flag is true. Acquires token without first opening popup. + if (this.config.system.delayOpenPopup) { return this.acquireTokenPopup(request || DEFAULT_REQUEST); - } else { // flag false: all browsers + } else { + // delayOpenPopup flag it set to false. Opens popup before acquiring token. const popup = PopupHandler.openSizedPopup(); return this.acquireTokenPopup(request || DEFAULT_REQUEST, popup); } diff --git a/lib/msal-browser/src/config/Configuration.ts b/lib/msal-browser/src/config/Configuration.ts index c7765cc28f..1b6fe46279 100644 --- a/lib/msal-browser/src/config/Configuration.ts +++ b/lib/msal-browser/src/config/Configuration.ts @@ -51,6 +51,7 @@ export type CacheOptions = { * - windowHashTimeout - sets the timeout for waiting for a response hash in a popup * - iframeHashTimeout - sets the timeout for waiting for a response hash in an iframe * - loadFrameTimeout - maximum time the library should wait for a frame to load + * - delayOpenPopup - Sets whether popup is open as soon as loginPopup is called. By default, this flag is set to false. Set to true for native apps and PWA. */ export type BrowserSystemOptions = SystemOptions & { loggerOptions?: LoggerOptions; @@ -58,6 +59,7 @@ export type BrowserSystemOptions = SystemOptions & { windowHashTimeout?: number; iframeHashTimeout?: number; loadFrameTimeout?: number; + delayOpenPopup?: boolean; }; /** @@ -104,7 +106,8 @@ const DEFAULT_BROWSER_SYSTEM_OPTIONS: BrowserSystemOptions = { networkClient: BrowserUtils.getBrowserNetworkClient(), windowHashTimeout: DEFAULT_POPUP_TIMEOUT_MS, iframeHashTimeout: DEFAULT_IFRAME_TIMEOUT_MS, - loadFrameTimeout: BrowserUtils.detectIEOrEdge() ? 500 : 0 + loadFrameTimeout: BrowserUtils.detectIEOrEdge() ? 500 : 0, + delayOpenPopup: false }; /** From 61a1173a2cb96170b8898973c7ea3ede435cffa2 Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Wed, 12 Aug 2020 16:02:55 -0700 Subject: [PATCH 03/14] Change files --- ...r-2020-08-12-16-02-55-browser-popup-window-change.json | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 change/@azure-msal-browser-2020-08-12-16-02-55-browser-popup-window-change.json diff --git a/change/@azure-msal-browser-2020-08-12-16-02-55-browser-popup-window-change.json b/change/@azure-msal-browser-2020-08-12-16-02-55-browser-popup-window-change.json new file mode 100644 index 0000000000..4d86f8f6ef --- /dev/null +++ b/change/@azure-msal-browser-2020-08-12-16-02-55-browser-popup-window-change.json @@ -0,0 +1,8 @@ +{ + "type": "patch", + "comment": "Change msal-browser loginPopup and openPopup, add ability to configure popup delay (#2132)", + "packageName": "@azure/msal-browser", + "email": "joarroyo@microsoft.com", + "dependentChangeType": "patch", + "date": "2020-08-12T23:02:55.249Z" +} From 05d59d766c77bfb99610ad4f8db3d82a019fc442 Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Thu, 13 Aug 2020 15:32:59 -0700 Subject: [PATCH 04/14] Add unit tests for loginPopup and configuraiton --- .../test/app/PublicClientApplication.spec.ts | 33 +++++++++++++++++++ .../test/config/Configuration.spec.ts | 5 ++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index bf548dbd92..b9d713b64f 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -893,6 +893,13 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { describe("Popup Flow Unit tests", () => { describe("loginPopup", () => { + beforeEach(() => { + sinon.stub(window, "open").returns(window); + }); + + afterEach(() => { + sinon.restore(); + }); it("throws error if interaction is in progress", async () => { window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${BrowserConstants.INTERACTION_STATUS_KEY}`, BrowserConstants.INTERACTION_IN_PROGRESS_VALUE); @@ -911,6 +918,32 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { pca.loginPopup(); }); + it("opens popup window by default", () => { + sinon.stub(pca, "acquireTokenPopup").callsFake((request, popup) => { + expect(popup).to.exist; + expect(popup.location.href).to.exist; + expect(popup.location.href).to.eql(TEST_URIS.TEST_REDIR_URI); + return null; + }); + + pca.loginPopup(); + }); + + it("delays opening popup if configured", () => { + pca = new PublicClientApplication({ + system: { + delayOpenPopup: true + } + }); + + sinon.stub(pca, "acquireTokenPopup").callsFake((request, popup) => { + expect(popup).to.be.undefined; + return null; + }); + + pca.loginPopup(); + }); + it("resolves the response successfully", async () => { const testServerTokenResponse = { token_type: TEST_CONFIG.TOKEN_TYPE_BEARER, diff --git a/lib/msal-browser/test/config/Configuration.spec.ts b/lib/msal-browser/test/config/Configuration.spec.ts index f522692711..6b14ea25a2 100644 --- a/lib/msal-browser/test/config/Configuration.spec.ts +++ b/lib/msal-browser/test/config/Configuration.spec.ts @@ -47,6 +47,7 @@ describe("Configuration.ts Class Unit Tests", () => { expect(emptyConfig.system.windowHashTimeout).to.be.not.null.and.not.undefined; expect(emptyConfig.system.windowHashTimeout).to.be.eq(DEFAULT_POPUP_TIMEOUT_MS); expect(emptyConfig.system.tokenRenewalOffsetSeconds).to.be.eq(300); + expect(emptyConfig.system.delayOpenPopup).to.be.false; }); it("Tests logger", () => { @@ -114,7 +115,8 @@ describe("Configuration.ts Class Unit Tests", () => { loggerOptions: { loggerCallback: testLoggerCallback, piiLoggingEnabled: true - } + }, + delayOpenPopup: true } }); // Auth config checks @@ -139,5 +141,6 @@ describe("Configuration.ts Class Unit Tests", () => { expect(newConfig.system.loggerOptions).to.be.not.null; expect(newConfig.system.loggerOptions.loggerCallback).to.be.not.null; expect(newConfig.system.loggerOptions.piiLoggingEnabled).to.be.true; + expect(newConfig.system.delayOpenPopup).to.be.true; }); }); From 789f84ac5535f3e06cd6cf5d01271322c8c449b6 Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Thu, 13 Aug 2020 15:50:45 -0700 Subject: [PATCH 05/14] Fix linting issue --- lib/msal-browser/src/interaction_handler/PopupHandler.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/msal-browser/src/interaction_handler/PopupHandler.ts b/lib/msal-browser/src/interaction_handler/PopupHandler.ts index 5569a2220d..cff74a65a1 100644 --- a/lib/msal-browser/src/interaction_handler/PopupHandler.ts +++ b/lib/msal-browser/src/interaction_handler/PopupHandler.ts @@ -157,7 +157,7 @@ export class PopupHandler extends InteractionHandler { const left = Math.max(0, ((width / 2) - (BrowserConstants.POPUP_WIDTH / 2)) + winLeft); const top = Math.max(0, ((height / 2) - (BrowserConstants.POPUP_HEIGHT / 2)) + winTop); - return window.open(urlNavigate, Constants.LIBRARY_NAME, "width=" + BrowserConstants.POPUP_WIDTH + ", height=" + BrowserConstants.POPUP_HEIGHT + ", top=" + top + ", left=" + left) + return window.open(urlNavigate, Constants.LIBRARY_NAME, "width=" + BrowserConstants.POPUP_WIDTH + ", height=" + BrowserConstants.POPUP_HEIGHT + ", top=" + top + ", left=" + left); } /** From c4be0de65a5894461a90e483a375f258598b15ad Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Fri, 14 Aug 2020 16:52:12 -0700 Subject: [PATCH 06/14] Update loginPopup tests, add tests for PopupHandler --- .../src/app/PublicClientApplication.ts | 18 +++- .../test/app/PublicClientApplication.spec.ts | 77 ++++++++++----- .../interaction_handler/PopupHandler.spec.ts | 97 +++++++++++++++++++ 3 files changed, 162 insertions(+), 30 deletions(-) diff --git a/lib/msal-browser/src/app/PublicClientApplication.ts b/lib/msal-browser/src/app/PublicClientApplication.ts index b85b775fa0..93459b297a 100644 --- a/lib/msal-browser/src/app/PublicClientApplication.ts +++ b/lib/msal-browser/src/app/PublicClientApplication.ts @@ -292,23 +292,33 @@ export class PublicClientApplication implements IPublicClientApplication { * @returns {Promise.} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object */ loginPopup(request: PopupRequest): Promise { + return this.acquireTokenPopup(request || DEFAULT_REQUEST); + } + + /** + * Use when you want to obtain an access_token for your API via opening a popup window in the user's browser + * @param {@link (PopupRequest:type)} + * + * @returns {Promise.} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object + */ + acquireTokenPopup(request: PopupRequest): Promise { // delayOpenPopup flag is true. Acquires token without first opening popup. if (this.config.system.delayOpenPopup) { - return this.acquireTokenPopup(request || DEFAULT_REQUEST); + return this.acquireTokenPopupAsync(request); } else { // delayOpenPopup flag it set to false. Opens popup before acquiring token. const popup = PopupHandler.openSizedPopup(); - return this.acquireTokenPopup(request || DEFAULT_REQUEST, popup); + return this.acquireTokenPopupAsync(request, popup); } } /** - * Use when you want to obtain an access_token for your API via opening a popup window in the user's browser + * Helper which obtains an access_token for your API via opening a popup window in the user's browser * @param {@link (PopupRequest:type)} * * @returns {Promise.} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object */ - async acquireTokenPopup(request: PopupRequest, popup?: Window|null): Promise { + private async acquireTokenPopupAsync(request: PopupRequest, popup?: Window|null): Promise { // Preflight request const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request, InteractionType.POPUP); const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.acquireTokenPopup, validRequest.correlationId); diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index b9d713b64f..8be8e1988b 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -918,32 +918,6 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { pca.loginPopup(); }); - it("opens popup window by default", () => { - sinon.stub(pca, "acquireTokenPopup").callsFake((request, popup) => { - expect(popup).to.exist; - expect(popup.location.href).to.exist; - expect(popup.location.href).to.eql(TEST_URIS.TEST_REDIR_URI); - return null; - }); - - pca.loginPopup(); - }); - - it("delays opening popup if configured", () => { - pca = new PublicClientApplication({ - system: { - delayOpenPopup: true - } - }); - - sinon.stub(pca, "acquireTokenPopup").callsFake((request, popup) => { - expect(popup).to.be.undefined; - return null; - }); - - pca.loginPopup(); - }); - it("resolves the response successfully", async () => { const testServerTokenResponse = { token_type: TEST_CONFIG.TOKEN_TYPE_BEARER, @@ -1025,6 +999,13 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { }); describe("acquireTokenPopup", () => { + beforeEach(() => { + sinon.stub(window, "open").returns(window); + }); + + afterEach(() => { + sinon.restore(); + }); it("throws error if interaction is in progress", async () => { window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${BrowserConstants.INTERACTION_STATUS_KEY}`, BrowserConstants.INTERACTION_IN_PROGRESS_VALUE); @@ -1038,6 +1019,50 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { })).rejectedWith(BrowserAuthError); }); + it("opens popup window by default", () => { + const request: AuthorizationUrlRequest = { + redirectUri: TEST_URIS.TEST_REDIR_URI, + scopes: ["scope"], + loginHint: "AbeLi@microsoft.com", + state: TEST_STATE_VALUES.USER_STATE + }; + + sinon.stub(CryptoOps.prototype, "generatePkceCodes").resolves({ + challenge: TEST_CONFIG.TEST_CHALLENGE, + verifier: TEST_CONFIG.TEST_VERIFIER + }); + + const popupSpy = sinon.stub(PopupHandler, "openSizedPopup"); + + pca.acquireTokenPopup(request); + expect(popupSpy.calledWith()).to.be.true; + }); + + it("delays opening popup if configured", () => { + pca = new PublicClientApplication({ + system: { + delayOpenPopup: true + } + }); + + sinon.stub(CryptoOps.prototype, "generatePkceCodes").resolves({ + challenge: TEST_CONFIG.TEST_CHALLENGE, + verifier: TEST_CONFIG.TEST_VERIFIER + }); + + const request: AuthorizationUrlRequest = { + redirectUri: TEST_URIS.TEST_REDIR_URI, + scopes: ["scope"], + loginHint: "AbeLi@microsoft.com", + state: TEST_STATE_VALUES.USER_STATE + }; + + const popupSpy = sinon.stub(PopupHandler, "openSizedPopup"); + + pca.acquireTokenPopup(request); + expect(popupSpy.calledWith()).to.be.false; + }); + it("resolves the response successfully", async () => { const testServerTokenResponse = { token_type: TEST_CONFIG.TOKEN_TYPE_BEARER, diff --git a/lib/msal-browser/test/interaction_handler/PopupHandler.spec.ts b/lib/msal-browser/test/interaction_handler/PopupHandler.spec.ts index 5fe1847c86..d51bc31fca 100644 --- a/lib/msal-browser/test/interaction_handler/PopupHandler.spec.ts +++ b/lib/msal-browser/test/interaction_handler/PopupHandler.spec.ts @@ -242,4 +242,101 @@ describe("PopupHandler.ts Unit Tests", () => { }, 500); }); }); + + describe("openPopup", () => { + afterEach(() => { + sinon.restore(); + }); + + it("assigns urlNavigate if popup passed in", () => { + const assignSpy = sinon.spy(); + const focusSpy = sinon.spy(); + + let windowObject = { + location: { + assign: assignSpy + }, + focus: focusSpy + }; + + const testRequest: AuthorizationCodeRequest = { + redirectUri: "", + code: "thisIsATestCode", + scopes: TEST_CONFIG.DEFAULT_SCOPES, + codeVerifier: TEST_CONFIG.TEST_VERIFIER, + authority: `${Constants.DEFAULT_AUTHORITY}/`, + correlationId: RANDOM_TEST_GUID + }; + + // @ts-ignore + const popupWindow = popupHandler.initiateAuthRequest("http://localhost/#/code=hello", testRequest, windowObject); + + expect(assignSpy.calledWith("http://localhost/#/code=hello")).to.be.true; + expect(popupWindow).to.equal(windowObject); + }); + + it("opens popup if no popup window is passed in", () => { + sinon.stub(window, "open").returns(window); + sinon.stub(window, "focus"); + + const testRequest: AuthorizationCodeRequest = { + redirectUri: "", + code: "thisIsATestCode", + scopes: TEST_CONFIG.DEFAULT_SCOPES, + codeVerifier: TEST_CONFIG.TEST_VERIFIER, + authority: `${Constants.DEFAULT_AUTHORITY}/`, + correlationId: RANDOM_TEST_GUID + }; + + const popupWindow = popupHandler.initiateAuthRequest("http://localhost/#/code=hello", testRequest); + + expect(popupWindow).to.equal(window); + }); + + it("throws error if no popup passed in but window.open returns null", () => { + sinon.stub(window, "open").returns(null); + + const testRequest: AuthorizationCodeRequest = { + redirectUri: `${TEST_URIS.DEFAULT_INSTANCE}/`, + code: "thisIsATestCode", + scopes: TEST_CONFIG.DEFAULT_SCOPES, + codeVerifier: TEST_CONFIG.TEST_VERIFIER, + authority: `${Constants.DEFAULT_AUTHORITY}/`, + correlationId: RANDOM_TEST_GUID + }; + + expect(() => popupHandler.initiateAuthRequest("http://localhost/#/code=hello", testRequest)).to.throw(BrowserAuthErrorMessage.emptyWindowError.desc); + expect(() => popupHandler.initiateAuthRequest("http://localhost/#/code=hello", testRequest)).to.throw(BrowserAuthError); + }); + + it("throws error if popup passed in is null", () => { + const testRequest: AuthorizationCodeRequest = { + redirectUri: `${TEST_URIS.DEFAULT_INSTANCE}/`, + code: "thisIsATestCode", + scopes: TEST_CONFIG.DEFAULT_SCOPES, + codeVerifier: TEST_CONFIG.TEST_VERIFIER, + authority: `${Constants.DEFAULT_AUTHORITY}/`, + correlationId: RANDOM_TEST_GUID + }; + + expect(() => popupHandler.initiateAuthRequest("http://localhost/#/code=hello", testRequest, null)).to.throw(BrowserAuthErrorMessage.emptyWindowError.desc); + expect(() => popupHandler.initiateAuthRequest("http://localhost/#/code=hello", testRequest, null)).to.throw(BrowserAuthError); + }); + }); + + describe("openSizedPopup", () => { + it("opens a popup with urlNavigate", () => { + const windowOpenSpy = sinon.stub(window, "open"); + PopupHandler.openSizedPopup("http://localhost/"); + + expect(windowOpenSpy.calledWith("http://localhost/")).to.be.true; + }); + + it("opens a popup with about:blank if no urlNavigate passed in", () => { + const windowOpenSpy = sinon.stub(window, "open"); + PopupHandler.openSizedPopup(); + + expect(windowOpenSpy.calledWith("about:blank")).to.be.true; + }); + }); }); From 1de86c905d6be832d30ba5901f6eed56295e3e72 Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Fri, 14 Aug 2020 16:58:25 -0700 Subject: [PATCH 07/14] Restores optional request to loginPopup --- lib/msal-browser/src/app/PublicClientApplication.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/msal-browser/src/app/PublicClientApplication.ts b/lib/msal-browser/src/app/PublicClientApplication.ts index 93459b297a..6a08e493ae 100644 --- a/lib/msal-browser/src/app/PublicClientApplication.ts +++ b/lib/msal-browser/src/app/PublicClientApplication.ts @@ -291,7 +291,7 @@ export class PublicClientApplication implements IPublicClientApplication { * * @returns {Promise.} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object */ - loginPopup(request: PopupRequest): Promise { + loginPopup(request?: PopupRequest): Promise { return this.acquireTokenPopup(request || DEFAULT_REQUEST); } From 6a9be4674d46ffe2a3c4e4fff3eeadbd5dd3c26b Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Mon, 17 Aug 2020 15:04:01 -0700 Subject: [PATCH 08/14] Address feedback --- lib/msal-browser/src/app/PublicClientApplication.ts | 6 +++--- lib/msal-browser/src/config/Configuration.ts | 12 ++++++------ .../test/app/PublicClientApplication.spec.ts | 6 +++--- lib/msal-browser/test/config/Configuration.spec.ts | 6 +++--- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/lib/msal-browser/src/app/PublicClientApplication.ts b/lib/msal-browser/src/app/PublicClientApplication.ts index 6a08e493ae..50bc08f224 100644 --- a/lib/msal-browser/src/app/PublicClientApplication.ts +++ b/lib/msal-browser/src/app/PublicClientApplication.ts @@ -302,11 +302,11 @@ export class PublicClientApplication implements IPublicClientApplication { * @returns {Promise.} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object */ acquireTokenPopup(request: PopupRequest): Promise { - // delayOpenPopup flag is true. Acquires token without first opening popup. - if (this.config.system.delayOpenPopup) { + // asyncPopup flag is true. Acquires token without first opening popup. Popup will be opened later asynchronously. + if (this.config.system.asyncPopup) { return this.acquireTokenPopupAsync(request); } else { - // delayOpenPopup flag it set to false. Opens popup before acquiring token. + // asyncPopup flag is set to false. Opens popup before acquiring token. const popup = PopupHandler.openSizedPopup(); return this.acquireTokenPopupAsync(request, popup); } diff --git a/lib/msal-browser/src/config/Configuration.ts b/lib/msal-browser/src/config/Configuration.ts index 1b6fe46279..7c083bdfe1 100644 --- a/lib/msal-browser/src/config/Configuration.ts +++ b/lib/msal-browser/src/config/Configuration.ts @@ -48,10 +48,10 @@ export type CacheOptions = { * - tokenRenewalOffsetSeconds - Sets the window of offset needed to renew the token before expiry * - loggerOptions - Used to initialize the Logger object (See ClientConfiguration.ts) * - networkClient - Network interface implementation - * - windowHashTimeout - sets the timeout for waiting for a response hash in a popup - * - iframeHashTimeout - sets the timeout for waiting for a response hash in an iframe - * - loadFrameTimeout - maximum time the library should wait for a frame to load - * - delayOpenPopup - Sets whether popup is open as soon as loginPopup is called. By default, this flag is set to false. Set to true for native apps and PWA. + * - windowHashTimeout - Sets the timeout for waiting for a response hash in a popup + * - iframeHashTimeout - Sets the timeout for waiting for a response hash in an iframe + * - loadFrameTimeout - Maximum time the library should wait for a frame to load + * - asyncPopup - When set to false, blank popup is opened before anything else happens. When set to true, popup is opened when making the network request. */ export type BrowserSystemOptions = SystemOptions & { loggerOptions?: LoggerOptions; @@ -59,7 +59,7 @@ export type BrowserSystemOptions = SystemOptions & { windowHashTimeout?: number; iframeHashTimeout?: number; loadFrameTimeout?: number; - delayOpenPopup?: boolean; + asyncPopup?: boolean; }; /** @@ -107,7 +107,7 @@ const DEFAULT_BROWSER_SYSTEM_OPTIONS: BrowserSystemOptions = { windowHashTimeout: DEFAULT_POPUP_TIMEOUT_MS, iframeHashTimeout: DEFAULT_IFRAME_TIMEOUT_MS, loadFrameTimeout: BrowserUtils.detectIEOrEdge() ? 500 : 0, - delayOpenPopup: false + asyncPopup: false }; /** diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index 8be8e1988b..7282496cba 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -1019,7 +1019,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { })).rejectedWith(BrowserAuthError); }); - it("opens popup window by default", () => { + it("opens popup window before network request by default", () => { const request: AuthorizationUrlRequest = { redirectUri: TEST_URIS.TEST_REDIR_URI, scopes: ["scope"], @@ -1038,10 +1038,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { expect(popupSpy.calledWith()).to.be.true; }); - it("delays opening popup if configured", () => { + it("opens popup asynchronously if configured", () => { pca = new PublicClientApplication({ system: { - delayOpenPopup: true + asyncPopup: true } }); diff --git a/lib/msal-browser/test/config/Configuration.spec.ts b/lib/msal-browser/test/config/Configuration.spec.ts index 6b14ea25a2..eff107e99a 100644 --- a/lib/msal-browser/test/config/Configuration.spec.ts +++ b/lib/msal-browser/test/config/Configuration.spec.ts @@ -47,7 +47,7 @@ describe("Configuration.ts Class Unit Tests", () => { expect(emptyConfig.system.windowHashTimeout).to.be.not.null.and.not.undefined; expect(emptyConfig.system.windowHashTimeout).to.be.eq(DEFAULT_POPUP_TIMEOUT_MS); expect(emptyConfig.system.tokenRenewalOffsetSeconds).to.be.eq(300); - expect(emptyConfig.system.delayOpenPopup).to.be.false; + expect(emptyConfig.system.asyncPopup).to.be.false; }); it("Tests logger", () => { @@ -116,7 +116,7 @@ describe("Configuration.ts Class Unit Tests", () => { loggerCallback: testLoggerCallback, piiLoggingEnabled: true }, - delayOpenPopup: true + asyncPopup: true } }); // Auth config checks @@ -141,6 +141,6 @@ describe("Configuration.ts Class Unit Tests", () => { expect(newConfig.system.loggerOptions).to.be.not.null; expect(newConfig.system.loggerOptions.loggerCallback).to.be.not.null; expect(newConfig.system.loggerOptions.piiLoggingEnabled).to.be.true; - expect(newConfig.system.delayOpenPopup).to.be.true; + expect(newConfig.system.asyncPopup).to.be.true; }); }); From 543980bfcac776badd218d9d313a96c50364a84d Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Mon, 17 Aug 2020 15:05:06 -0700 Subject: [PATCH 09/14] Address feedback --- lib/msal-browser/src/config/Configuration.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/msal-browser/src/config/Configuration.ts b/lib/msal-browser/src/config/Configuration.ts index 7c083bdfe1..187a9451ef 100644 --- a/lib/msal-browser/src/config/Configuration.ts +++ b/lib/msal-browser/src/config/Configuration.ts @@ -51,7 +51,7 @@ export type CacheOptions = { * - windowHashTimeout - Sets the timeout for waiting for a response hash in a popup * - iframeHashTimeout - Sets the timeout for waiting for a response hash in an iframe * - loadFrameTimeout - Maximum time the library should wait for a frame to load - * - asyncPopup - When set to false, blank popup is opened before anything else happens. When set to true, popup is opened when making the network request. + * - asyncPopup - Sets whether popup is open as soon as loginPopup is called. By default, this flag is set to false. When set to false, blank popup is opened before anything else happens. When set to true, popup is opened when making the network request. */ export type BrowserSystemOptions = SystemOptions & { loggerOptions?: LoggerOptions; From 398705cb99b004a5acd34b722e6b057864a7c18f Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Mon, 17 Aug 2020 15:06:12 -0700 Subject: [PATCH 10/14] Update comment --- lib/msal-browser/src/config/Configuration.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/msal-browser/src/config/Configuration.ts b/lib/msal-browser/src/config/Configuration.ts index 187a9451ef..d8e6a3cbe5 100644 --- a/lib/msal-browser/src/config/Configuration.ts +++ b/lib/msal-browser/src/config/Configuration.ts @@ -51,7 +51,7 @@ export type CacheOptions = { * - windowHashTimeout - Sets the timeout for waiting for a response hash in a popup * - iframeHashTimeout - Sets the timeout for waiting for a response hash in an iframe * - loadFrameTimeout - Maximum time the library should wait for a frame to load - * - asyncPopup - Sets whether popup is open as soon as loginPopup is called. By default, this flag is set to false. When set to false, blank popup is opened before anything else happens. When set to true, popup is opened when making the network request. + * - asyncPopup - Sets whether popup is opened asynchronously. By default, this flag is set to false. When set to false, blank popup is opened before anything else happens. When set to true, popup is opened when making the network request. */ export type BrowserSystemOptions = SystemOptions & { loggerOptions?: LoggerOptions; From b0fdf772d93702883a5599f8e910894336d616f2 Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Mon, 17 Aug 2020 15:07:08 -0700 Subject: [PATCH 11/14] Remove unnecessary tests --- lib/msal-browser/test/interaction_handler/PopupHandler.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/msal-browser/test/interaction_handler/PopupHandler.spec.ts b/lib/msal-browser/test/interaction_handler/PopupHandler.spec.ts index d51bc31fca..4691416601 100644 --- a/lib/msal-browser/test/interaction_handler/PopupHandler.spec.ts +++ b/lib/msal-browser/test/interaction_handler/PopupHandler.spec.ts @@ -306,7 +306,6 @@ describe("PopupHandler.ts Unit Tests", () => { }; expect(() => popupHandler.initiateAuthRequest("http://localhost/#/code=hello", testRequest)).to.throw(BrowserAuthErrorMessage.emptyWindowError.desc); - expect(() => popupHandler.initiateAuthRequest("http://localhost/#/code=hello", testRequest)).to.throw(BrowserAuthError); }); it("throws error if popup passed in is null", () => { From 8e52e977376ad832407bf5b9f4fd706cda4640db Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Tue, 18 Aug 2020 14:30:31 -0700 Subject: [PATCH 12/14] Update asyncPopups config name --- lib/msal-browser/src/app/PublicClientApplication.ts | 6 +++--- lib/msal-browser/src/config/Configuration.ts | 6 +++--- lib/msal-browser/test/app/PublicClientApplication.spec.ts | 4 ++-- lib/msal-browser/test/config/Configuration.spec.ts | 6 +++--- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/msal-browser/src/app/PublicClientApplication.ts b/lib/msal-browser/src/app/PublicClientApplication.ts index 50bc08f224..d6720f9483 100644 --- a/lib/msal-browser/src/app/PublicClientApplication.ts +++ b/lib/msal-browser/src/app/PublicClientApplication.ts @@ -302,11 +302,11 @@ export class PublicClientApplication implements IPublicClientApplication { * @returns {Promise.} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object */ acquireTokenPopup(request: PopupRequest): Promise { - // asyncPopup flag is true. Acquires token without first opening popup. Popup will be opened later asynchronously. - if (this.config.system.asyncPopup) { + // asyncPopups flag is true. Acquires token without first opening popup. Popup will be opened later asynchronously. + if (this.config.system.asyncPopups) { return this.acquireTokenPopupAsync(request); } else { - // asyncPopup flag is set to false. Opens popup before acquiring token. + // asyncPopups flag is set to false. Opens popup before acquiring token. const popup = PopupHandler.openSizedPopup(); return this.acquireTokenPopupAsync(request, popup); } diff --git a/lib/msal-browser/src/config/Configuration.ts b/lib/msal-browser/src/config/Configuration.ts index d8e6a3cbe5..522804b99c 100644 --- a/lib/msal-browser/src/config/Configuration.ts +++ b/lib/msal-browser/src/config/Configuration.ts @@ -51,7 +51,7 @@ export type CacheOptions = { * - windowHashTimeout - Sets the timeout for waiting for a response hash in a popup * - iframeHashTimeout - Sets the timeout for waiting for a response hash in an iframe * - loadFrameTimeout - Maximum time the library should wait for a frame to load - * - asyncPopup - Sets whether popup is opened asynchronously. By default, this flag is set to false. When set to false, blank popup is opened before anything else happens. When set to true, popup is opened when making the network request. + * - asyncPopups - Sets whether popups are opened asynchronously. By default, this flag is set to false. When set to false, blank popups are opened before anything else happens. When set to true, popups are opened when making the network request. */ export type BrowserSystemOptions = SystemOptions & { loggerOptions?: LoggerOptions; @@ -59,7 +59,7 @@ export type BrowserSystemOptions = SystemOptions & { windowHashTimeout?: number; iframeHashTimeout?: number; loadFrameTimeout?: number; - asyncPopup?: boolean; + asyncPopups?: boolean; }; /** @@ -107,7 +107,7 @@ const DEFAULT_BROWSER_SYSTEM_OPTIONS: BrowserSystemOptions = { windowHashTimeout: DEFAULT_POPUP_TIMEOUT_MS, iframeHashTimeout: DEFAULT_IFRAME_TIMEOUT_MS, loadFrameTimeout: BrowserUtils.detectIEOrEdge() ? 500 : 0, - asyncPopup: false + asyncPopups: false }; /** diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index 7282496cba..22bf78ecfd 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -1038,10 +1038,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { expect(popupSpy.calledWith()).to.be.true; }); - it("opens popup asynchronously if configured", () => { + it("opens popups asynchronously if configured", () => { pca = new PublicClientApplication({ system: { - asyncPopup: true + asyncPopups: true } }); diff --git a/lib/msal-browser/test/config/Configuration.spec.ts b/lib/msal-browser/test/config/Configuration.spec.ts index eff107e99a..86973a72b8 100644 --- a/lib/msal-browser/test/config/Configuration.spec.ts +++ b/lib/msal-browser/test/config/Configuration.spec.ts @@ -47,7 +47,7 @@ describe("Configuration.ts Class Unit Tests", () => { expect(emptyConfig.system.windowHashTimeout).to.be.not.null.and.not.undefined; expect(emptyConfig.system.windowHashTimeout).to.be.eq(DEFAULT_POPUP_TIMEOUT_MS); expect(emptyConfig.system.tokenRenewalOffsetSeconds).to.be.eq(300); - expect(emptyConfig.system.asyncPopup).to.be.false; + expect(emptyConfig.system.asyncPopups).to.be.false; }); it("Tests logger", () => { @@ -116,7 +116,7 @@ describe("Configuration.ts Class Unit Tests", () => { loggerCallback: testLoggerCallback, piiLoggingEnabled: true }, - asyncPopup: true + asyncPopups: true } }); // Auth config checks @@ -141,6 +141,6 @@ describe("Configuration.ts Class Unit Tests", () => { expect(newConfig.system.loggerOptions).to.be.not.null; expect(newConfig.system.loggerOptions.loggerCallback).to.be.not.null; expect(newConfig.system.loggerOptions.piiLoggingEnabled).to.be.true; - expect(newConfig.system.asyncPopup).to.be.true; + expect(newConfig.system.asyncPopups).to.be.true; }); }); From 873179af18f64bc249b51434b308acd9e57a841d Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Wed, 19 Aug 2020 17:19:45 -0700 Subject: [PATCH 13/14] Update configuration.md --- lib/msal-browser/docs/configuration.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/msal-browser/docs/configuration.md b/lib/msal-browser/docs/configuration.md index 0c035ec98c..cc3c2def31 100644 --- a/lib/msal-browser/docs/configuration.md +++ b/lib/msal-browser/docs/configuration.md @@ -48,7 +48,8 @@ const msalConfig = { }, windowHashTimeout: 60000, iframeHashTimeout: 6000, - loadFrameTimeout: 0 + loadFrameTimeout: 0, + asyncPopups: false }; } @@ -81,6 +82,7 @@ const msalInstance = new PublicClientApplication(msalConfig); | `windowHashTimeout` | Timeout in milliseconds to wait for popup authentication to resolve. | integer (milliseconds) | `60000` | | `iframeHashTimeout` | Timeout in milliseconds to wait for iframe authentication to resolve | integer (milliseconds) | `6000` | | `loadFrameTimeout` | Timeout in milliseconds to wait for the iframe to load in the window. | integer (milliseconds) | In IE or Edge: `500`, in all other browsers: `0` | +| `asyncPopups` | Sets whether popups are opened asynchronously. When set to false, blank popups are opened before anything else happens. When set to true, popups are opened when making the network request. Can be set to true for scenarios that do not support popups, e.g. desktop apps or progressive web apps | boolean | `false` | ### Logger Config Options | Option | Description | Format | Default Value | From abda11c56c83fe58e718855185ddd3ac449d7dce Mon Sep 17 00:00:00 2001 From: Jo Arroyo Date: Thu, 20 Aug 2020 10:25:04 -0700 Subject: [PATCH 14/14] Update lib/msal-browser/docs/configuration.md Co-authored-by: Jason Nutter --- lib/msal-browser/docs/configuration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/msal-browser/docs/configuration.md b/lib/msal-browser/docs/configuration.md index cc3c2def31..98a877294e 100644 --- a/lib/msal-browser/docs/configuration.md +++ b/lib/msal-browser/docs/configuration.md @@ -82,7 +82,7 @@ const msalInstance = new PublicClientApplication(msalConfig); | `windowHashTimeout` | Timeout in milliseconds to wait for popup authentication to resolve. | integer (milliseconds) | `60000` | | `iframeHashTimeout` | Timeout in milliseconds to wait for iframe authentication to resolve | integer (milliseconds) | `6000` | | `loadFrameTimeout` | Timeout in milliseconds to wait for the iframe to load in the window. | integer (milliseconds) | In IE or Edge: `500`, in all other browsers: `0` | -| `asyncPopups` | Sets whether popups are opened asynchronously. When set to false, blank popups are opened before anything else happens. When set to true, popups are opened when making the network request. Can be set to true for scenarios that do not support popups, e.g. desktop apps or progressive web apps | boolean | `false` | +| `asyncPopups` | Sets whether popups are opened asynchronously. When set to false, blank popups are opened before anything else happens. When set to true, popups are opened when making the network request. Can be set to true for scenarios where `about:blank` is not supported, e.g. desktop apps or progressive web apps | boolean | `false` | ### Logger Config Options | Option | Description | Format | Default Value |