From 907fac9d9272444a40ff46a979de2e47b4088b71 Mon Sep 17 00:00:00 2001 From: Fidel Coria Date: Tue, 22 Dec 2020 09:45:25 -0600 Subject: [PATCH 1/5] eliminate racey code --- packages/core-api/src/lib/loginPopup.test.ts | 80 -------------------- packages/core-api/src/lib/loginPopup.ts | 18 ----- 2 files changed, 98 deletions(-) diff --git a/packages/core-api/src/lib/loginPopup.test.ts b/packages/core-api/src/lib/loginPopup.test.ts index 98541c268e..f645b75af7 100644 --- a/packages/core-api/src/lib/loginPopup.test.ts +++ b/packages/core-api/src/lib/loginPopup.test.ts @@ -135,84 +135,4 @@ describe('showLoginPopup', () => { expect(addEventListenerSpy).toBeCalledTimes(1); expect(removeEventListenerSpy).toBeCalledTimes(1); }); - - it('should fail if popup is closed', async () => { - const openSpy = jest - .spyOn(window, 'open') - .mockReturnValue({ closed: false } as Window); - const addEventListenerSpy = jest.spyOn(window, 'addEventListener'); - const removeEventListenerSpy = jest.spyOn(window, 'removeEventListener'); - const popupMock = { closed: false }; - - openSpy.mockReturnValue(popupMock as Window); - - const payloadPromise = showLoginPopup({ - url: 'url', - name: 'name', - origin: 'origin', - }); - - expect(openSpy).toBeCalledTimes(1); - expect(addEventListenerSpy).toBeCalledTimes(1); - expect(removeEventListenerSpy).toBeCalledTimes(0); - - const listener = addEventListenerSpy.mock.calls[0][1] as EventListener; - listener({ - source: popupMock, - origin: 'origin', - data: { - type: 'config_info', - targetOrigin: 'http://localhost', - }, - } as MessageEvent); - - setTimeout(() => { - popupMock.closed = true; - }, 150); - await expect(payloadPromise).rejects.toThrow( - 'Login failed, popup was closed', - ); - - expect(openSpy).toBeCalledTimes(1); - expect(addEventListenerSpy).toBeCalledTimes(1); - expect(removeEventListenerSpy).toBeCalledTimes(1); - }); - - it('should indicate if origin does not match', async () => { - const openSpy = jest - .spyOn(window, 'open') - .mockReturnValue({ closed: false } as Window); - const addEventListenerSpy = jest.spyOn(window, 'addEventListener'); - const removeEventListenerSpy = jest.spyOn(window, 'removeEventListener'); - const popupMock = { closed: false }; - - openSpy.mockReturnValue(popupMock as Window); - - const payloadPromise = showLoginPopup({ - url: 'url', - name: 'name', - origin: 'origin', - }); - - const listener = addEventListenerSpy.mock.calls[0][1] as EventListener; - listener({ - source: popupMock, - origin: 'origin', - data: { - type: 'config_info', - targetOrigin: 'http://differenthost', - }, - } as MessageEvent); - - setTimeout(() => { - popupMock.closed = true; - }, 150); - await expect(payloadPromise).rejects.toThrow( - 'Login failed, Incorrect app origin, expected http://differenthost', - ); - - expect(openSpy).toBeCalledTimes(1); - expect(addEventListenerSpy).toBeCalledTimes(1); - expect(removeEventListenerSpy).toBeCalledTimes(1); - }); }); diff --git a/packages/core-api/src/lib/loginPopup.ts b/packages/core-api/src/lib/loginPopup.ts index 2e14447882..1741d845e8 100644 --- a/packages/core-api/src/lib/loginPopup.ts +++ b/packages/core-api/src/lib/loginPopup.ts @@ -79,8 +79,6 @@ export function showLoginPopup(options: LoginPopupOptions): Promise { `menubar=no,location=no,resizable=no,scrollbars=no,status=no,width=${width},height=${height},top=${top},left=${left}`, ); - let targetOrigin = ''; - if (!popup || typeof popup.closed === 'undefined' || popup.closed) { reject(new Error('Failed to open auth popup.')); return; @@ -96,7 +94,6 @@ export function showLoginPopup(options: LoginPopupOptions): Promise { const { data } = event; if (data.type === 'config_info') { - targetOrigin = data.targetOrigin; return; } @@ -117,23 +114,8 @@ export function showLoginPopup(options: LoginPopupOptions): Promise { done(); }; - const intervalId = setInterval(() => { - if (popup.closed) { - const errMessage = `Login failed, ${ - targetOrigin !== window.location.origin - ? `Incorrect app origin, expected ${targetOrigin}` - : 'popup was closed' - }`; - const error = new Error(errMessage); - error.name = 'PopupClosedError'; - reject(error); - done(); - } - }, 100); - function done() { window.removeEventListener('message', messageListener); - clearInterval(intervalId); } window.addEventListener('message', messageListener); From 27f2af9354baf792291ab77714f2c25ea2e1cc62 Mon Sep 17 00:00:00 2001 From: Fidel Coria Date: Tue, 22 Dec 2020 09:55:51 -0600 Subject: [PATCH 2/5] change log --- .changeset/purple-turtles-float.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/purple-turtles-float.md diff --git a/.changeset/purple-turtles-float.md b/.changeset/purple-turtles-float.md new file mode 100644 index 0000000000..2e5688bf11 --- /dev/null +++ b/.changeset/purple-turtles-float.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-api': patch +--- + +Remove race condition in loginPopup From 0836691020370a127a2847cd38f16cc6356e8779 Mon Sep 17 00:00:00 2001 From: Fidel Coria Date: Tue, 22 Dec 2020 15:44:42 -0600 Subject: [PATCH 3/5] Revert "eliminate racey code" This reverts commit 907fac9d9272444a40ff46a979de2e47b4088b71. --- packages/core-api/src/lib/loginPopup.test.ts | 80 ++++++++++++++++++++ packages/core-api/src/lib/loginPopup.ts | 18 +++++ 2 files changed, 98 insertions(+) diff --git a/packages/core-api/src/lib/loginPopup.test.ts b/packages/core-api/src/lib/loginPopup.test.ts index f645b75af7..98541c268e 100644 --- a/packages/core-api/src/lib/loginPopup.test.ts +++ b/packages/core-api/src/lib/loginPopup.test.ts @@ -135,4 +135,84 @@ describe('showLoginPopup', () => { expect(addEventListenerSpy).toBeCalledTimes(1); expect(removeEventListenerSpy).toBeCalledTimes(1); }); + + it('should fail if popup is closed', async () => { + const openSpy = jest + .spyOn(window, 'open') + .mockReturnValue({ closed: false } as Window); + const addEventListenerSpy = jest.spyOn(window, 'addEventListener'); + const removeEventListenerSpy = jest.spyOn(window, 'removeEventListener'); + const popupMock = { closed: false }; + + openSpy.mockReturnValue(popupMock as Window); + + const payloadPromise = showLoginPopup({ + url: 'url', + name: 'name', + origin: 'origin', + }); + + expect(openSpy).toBeCalledTimes(1); + expect(addEventListenerSpy).toBeCalledTimes(1); + expect(removeEventListenerSpy).toBeCalledTimes(0); + + const listener = addEventListenerSpy.mock.calls[0][1] as EventListener; + listener({ + source: popupMock, + origin: 'origin', + data: { + type: 'config_info', + targetOrigin: 'http://localhost', + }, + } as MessageEvent); + + setTimeout(() => { + popupMock.closed = true; + }, 150); + await expect(payloadPromise).rejects.toThrow( + 'Login failed, popup was closed', + ); + + expect(openSpy).toBeCalledTimes(1); + expect(addEventListenerSpy).toBeCalledTimes(1); + expect(removeEventListenerSpy).toBeCalledTimes(1); + }); + + it('should indicate if origin does not match', async () => { + const openSpy = jest + .spyOn(window, 'open') + .mockReturnValue({ closed: false } as Window); + const addEventListenerSpy = jest.spyOn(window, 'addEventListener'); + const removeEventListenerSpy = jest.spyOn(window, 'removeEventListener'); + const popupMock = { closed: false }; + + openSpy.mockReturnValue(popupMock as Window); + + const payloadPromise = showLoginPopup({ + url: 'url', + name: 'name', + origin: 'origin', + }); + + const listener = addEventListenerSpy.mock.calls[0][1] as EventListener; + listener({ + source: popupMock, + origin: 'origin', + data: { + type: 'config_info', + targetOrigin: 'http://differenthost', + }, + } as MessageEvent); + + setTimeout(() => { + popupMock.closed = true; + }, 150); + await expect(payloadPromise).rejects.toThrow( + 'Login failed, Incorrect app origin, expected http://differenthost', + ); + + expect(openSpy).toBeCalledTimes(1); + expect(addEventListenerSpy).toBeCalledTimes(1); + expect(removeEventListenerSpy).toBeCalledTimes(1); + }); }); diff --git a/packages/core-api/src/lib/loginPopup.ts b/packages/core-api/src/lib/loginPopup.ts index 1741d845e8..2e14447882 100644 --- a/packages/core-api/src/lib/loginPopup.ts +++ b/packages/core-api/src/lib/loginPopup.ts @@ -79,6 +79,8 @@ export function showLoginPopup(options: LoginPopupOptions): Promise { `menubar=no,location=no,resizable=no,scrollbars=no,status=no,width=${width},height=${height},top=${top},left=${left}`, ); + let targetOrigin = ''; + if (!popup || typeof popup.closed === 'undefined' || popup.closed) { reject(new Error('Failed to open auth popup.')); return; @@ -94,6 +96,7 @@ export function showLoginPopup(options: LoginPopupOptions): Promise { const { data } = event; if (data.type === 'config_info') { + targetOrigin = data.targetOrigin; return; } @@ -114,8 +117,23 @@ export function showLoginPopup(options: LoginPopupOptions): Promise { done(); }; + const intervalId = setInterval(() => { + if (popup.closed) { + const errMessage = `Login failed, ${ + targetOrigin !== window.location.origin + ? `Incorrect app origin, expected ${targetOrigin}` + : 'popup was closed' + }`; + const error = new Error(errMessage); + error.name = 'PopupClosedError'; + reject(error); + done(); + } + }, 100); + function done() { window.removeEventListener('message', messageListener); + clearInterval(intervalId); } window.addEventListener('message', messageListener); From 29b4056f2c6fbc32029a567572669b9711806538 Mon Sep 17 00:00:00 2001 From: Fidel Coria Date: Tue, 22 Dec 2020 16:03:21 -0600 Subject: [PATCH 4/5] delay window close by 200 ms --- .changeset/purple-turtles-float.md | 2 +- plugins/auth-backend/src/lib/flow/authFlowHelpers.ts | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.changeset/purple-turtles-float.md b/.changeset/purple-turtles-float.md index 2e5688bf11..8736a7315f 100644 --- a/.changeset/purple-turtles-float.md +++ b/.changeset/purple-turtles-float.md @@ -2,4 +2,4 @@ '@backstage/core-api': patch --- -Remove race condition in loginPopup +Delay auth loginPopup close to avoid race condition with callers of authFlowHelpers. diff --git a/plugins/auth-backend/src/lib/flow/authFlowHelpers.ts b/plugins/auth-backend/src/lib/flow/authFlowHelpers.ts index 22ffc7af88..86a615e721 100644 --- a/plugins/auth-backend/src/lib/flow/authFlowHelpers.ts +++ b/plugins/auth-backend/src/lib/flow/authFlowHelpers.ts @@ -56,7 +56,9 @@ export const postMessageResponse = ( var originInfo = {'type': 'config_info', 'targetOrigin': origin}; (window.opener || window.parent).postMessage(originInfo, '*'); (window.opener || window.parent).postMessage(JSON.parse(authResponse), origin); - window.close(); + setTimeout(() => { + window.close(); + }, 100 * 2); // double the interval of the core-api lib/loginPopup.ts `; const hash = crypto.createHash('sha256').update(script).digest('base64'); From 7dfcdc1720e96f6d61d873ab550678d142f25139 Mon Sep 17 00:00:00 2001 From: Fidel Coria Date: Wed, 23 Dec 2020 11:15:36 -0600 Subject: [PATCH 5/5] reduce close timeout to 100 ms --- plugins/auth-backend/src/lib/flow/authFlowHelpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/auth-backend/src/lib/flow/authFlowHelpers.ts b/plugins/auth-backend/src/lib/flow/authFlowHelpers.ts index 86a615e721..e70aa0bfee 100644 --- a/plugins/auth-backend/src/lib/flow/authFlowHelpers.ts +++ b/plugins/auth-backend/src/lib/flow/authFlowHelpers.ts @@ -58,7 +58,7 @@ export const postMessageResponse = ( (window.opener || window.parent).postMessage(JSON.parse(authResponse), origin); setTimeout(() => { window.close(); - }, 100 * 2); // double the interval of the core-api lib/loginPopup.ts + }, 100); // same as the interval of the core-api lib/loginPopup.ts (to address race conditions) `; const hash = crypto.createHash('sha256').update(script).digest('base64');