From 1cb0f3d28b6d6411d630c45211450a33b2d9573a Mon Sep 17 00:00:00 2001 From: mikebender Date: Wed, 21 Jun 2023 12:19:01 -0400 Subject: [PATCH 1/2] fix: AuthPluginParent wasn't working when embedded in an iframe - Check for window.opener _or_ window.parent when using AuthPluginParent - Reorder plugin priority to use AuthPluginParent first if specified - Tested using an example html page that both embeds and pops open a new tab --- .../src/components/AuthBootstrap.tsx | 2 +- .../src/AuthPluginParent.test.tsx | 33 +++++++- .../auth-plugins/src/AuthPluginParent.tsx | 3 +- packages/jsapi-utils/src/MessageUtils.test.ts | 80 ++++++++++++++++++- packages/jsapi-utils/src/MessageUtils.ts | 17 +++- 5 files changed, 125 insertions(+), 10 deletions(-) diff --git a/packages/app-utils/src/components/AuthBootstrap.tsx b/packages/app-utils/src/components/AuthBootstrap.tsx index d8558c5961..e9b52d6635 100644 --- a/packages/app-utils/src/components/AuthBootstrap.tsx +++ b/packages/app-utils/src/components/AuthBootstrap.tsx @@ -21,8 +21,8 @@ export type AuthBootstrapProps = { /** Core auth plugins that are always loaded */ const CORE_AUTH_PLUGINS = new Map([ - ['@deephaven/auth-plugins.AuthPluginPsk', AuthPluginPsk], ['@deephaven/auth-plugins.AuthPluginParent', AuthPluginParent], + ['@deephaven/auth-plugins.AuthPluginPsk', AuthPluginPsk], ['@deephaven/auth-plugins.AuthPluginAnonymous', AuthPluginAnonymous], ]); diff --git a/packages/auth-plugins/src/AuthPluginParent.test.tsx b/packages/auth-plugins/src/AuthPluginParent.test.tsx index ac8703b019..4e32e92fa7 100644 --- a/packages/auth-plugins/src/AuthPluginParent.test.tsx +++ b/packages/auth-plugins/src/AuthPluginParent.test.tsx @@ -3,11 +3,13 @@ import { act, render, screen } from '@testing-library/react'; import { ApiContext, ClientContext } from '@deephaven/jsapi-bootstrap'; import { dh } from '@deephaven/jsapi-shim'; import type { CoreClient, LoginOptions } from '@deephaven/jsapi-types'; +import { TestUtils } from '@deephaven/utils'; import AuthPluginParent from './AuthPluginParent'; import { AuthConfigMap } from './AuthPlugin'; let mockParentResponse: Promise; jest.mock('@deephaven/jsapi-utils', () => ({ + ...jest.requireActual('@deephaven/jsapi-utils'), LOGIN_OPTIONS_REQUEST: 'mock-login-options-request', requestParentResponse: jest.fn(() => mockParentResponse), })); @@ -49,8 +51,29 @@ function renderComponent( describe('availability tests', () => { const authHandlers = []; + it('is available when window opener is set', () => { - window.opener = { postMessage: jest.fn() }; + const oldWindowOpener = window.opener; + // Can't use a spy because window.opener isn't set by default + // Still using a var to set the old value, in case that behaviour ever changes + window.opener = TestUtils.createMockProxy({ + postMessage: jest.fn(), + }); + window.history.pushState( + {}, + 'Test Title', + `/test.html?authProvider=parent` + ); + expect(AuthPluginParent.isAvailable(authHandlers, authConfigMap)).toBe( + true + ); + window.opener = oldWindowOpener; + }); + + it('is available when window parent is set', () => { + const parentSpy = jest.spyOn(window, 'parent', 'get').mockReturnValue( + TestUtils.createMockProxy({ postMessage: jest.fn() }) + ); window.history.pushState( {}, 'Test Title', @@ -59,12 +82,16 @@ describe('availability tests', () => { expect(AuthPluginParent.isAvailable(authHandlers, authConfigMap)).toBe( true ); + parentSpy.mockRestore(); }); - it('is not available when window opener not set', () => { - delete window.opener; + + it('is not available when window opener and parent are not set', () => { + const oldWindowOpener = window.opener; + window.opener = null; expect(AuthPluginParent.isAvailable(authHandlers, authConfigMap)).toBe( false ); + window.opener = oldWindowOpener; }); }); diff --git a/packages/auth-plugins/src/AuthPluginParent.tsx b/packages/auth-plugins/src/AuthPluginParent.tsx index 70700f0020..811f795e86 100644 --- a/packages/auth-plugins/src/AuthPluginParent.tsx +++ b/packages/auth-plugins/src/AuthPluginParent.tsx @@ -1,6 +1,7 @@ import React from 'react'; import type { LoginOptions } from '@deephaven/jsapi-types'; import { + getWindowParent, LOGIN_OPTIONS_REQUEST, requestParentResponse, } from '@deephaven/jsapi-utils'; @@ -49,7 +50,7 @@ function Component({ children }: AuthPluginProps): JSX.Element { const AuthPluginParent: AuthPlugin = { Component, isAvailable: () => - window.opener != null && getWindowAuthProvider() === 'parent', + getWindowParent() != null && getWindowAuthProvider() === 'parent', }; export default AuthPluginParent; diff --git a/packages/jsapi-utils/src/MessageUtils.test.ts b/packages/jsapi-utils/src/MessageUtils.test.ts index 8c3fa78bdd..7d3dda8f25 100644 --- a/packages/jsapi-utils/src/MessageUtils.test.ts +++ b/packages/jsapi-utils/src/MessageUtils.test.ts @@ -1,3 +1,4 @@ +import { TestUtils } from '@deephaven/utils'; import { makeMessage, makeResponse, @@ -7,11 +8,11 @@ import { it('Throws an exception if called on a window without parent', async () => { await expect(requestParentResponse('request')).rejects.toThrow( - 'window.opener is null, unable to send request.' + 'window parent is null, unable to send request.' ); }); -describe('requestParentResponse', () => { +describe('requestParentResponse with opener', () => { let addListenerSpy: jest.SpyInstance; let removeListenerSpy: jest.SpyInstance; let listenerCallback; @@ -80,3 +81,78 @@ describe('requestParentResponse', () => { jest.useRealTimers(); }); }); + +describe('requestParentResponse with parent', () => { + let addListenerSpy: jest.SpyInstance; + let removeListenerSpy: jest.SpyInstance; + let windowParentSpy: jest.SpyInstance; + let listenerCallback; + let messageId; + const mockPostMessage = jest.fn((data: Message) => { + messageId = data.id; + }); + beforeEach(() => { + addListenerSpy = jest + .spyOn(window, 'addEventListener') + .mockImplementation((event, cb) => { + listenerCallback = cb; + }); + removeListenerSpy = jest.spyOn(window, 'removeEventListener'); + windowParentSpy = jest.spyOn(window, 'parent', 'get').mockReturnValue( + TestUtils.createMockProxy({ + postMessage: mockPostMessage, + }) + ); + }); + + afterEach(() => { + addListenerSpy.mockRestore(); + removeListenerSpy.mockRestore(); + windowParentSpy.mockRestore(); + mockPostMessage.mockClear(); + messageId = undefined; + }); + + it('Posts message to parent and subscribes to response', async () => { + requestParentResponse('request'); + expect(mockPostMessage).toHaveBeenCalledWith( + expect.objectContaining(makeMessage('request', messageId)), + '*' + ); + expect(addListenerSpy).toHaveBeenCalledWith( + 'message', + expect.any(Function) + ); + }); + + it('Resolves with the payload from the parent window response and unsubscribes', async () => { + const PAYLOAD = 'PAYLOAD'; + const promise = requestParentResponse('request'); + listenerCallback({ + data: makeResponse(messageId, PAYLOAD), + }); + const result = await promise; + expect(result).toBe(PAYLOAD); + expect(removeListenerSpy).toHaveBeenCalledWith('message', listenerCallback); + }); + + it('Ignores unrelated response, rejects on timeout', async () => { + jest.useFakeTimers(); + const promise = requestParentResponse('request'); + listenerCallback({ + data: makeMessage('wrong-id'), + }); + jest.runOnlyPendingTimers(); + await expect(promise).rejects.toThrow('Request timed out'); + jest.useRealTimers(); + }); + + it('Times out if no response', async () => { + jest.useFakeTimers(); + const promise = requestParentResponse('request'); + jest.runOnlyPendingTimers(); + expect(removeListenerSpy).toHaveBeenCalled(); + await expect(promise).rejects.toThrow('Request timed out'); + jest.useRealTimers(); + }); +}); diff --git a/packages/jsapi-utils/src/MessageUtils.ts b/packages/jsapi-utils/src/MessageUtils.ts index 8b672c8b0d..b2f2900225 100644 --- a/packages/jsapi-utils/src/MessageUtils.ts +++ b/packages/jsapi-utils/src/MessageUtils.ts @@ -95,6 +95,16 @@ export function makeResponse(messageId: string, payload: T): Response { return { id: messageId, payload }; } +export function getWindowParent(): Window | null { + if (window.opener != null) { + return window.opener; + } + if (window.parent != null && window.parent !== window) { + return window.parent; + } + return null; +} + /** * Request data from the parent window and wait for response * @param request Request message to send to the parent window @@ -105,8 +115,9 @@ export async function requestParentResponse( request: string, timeout = 30000 ): Promise { - if (window.opener == null) { - throw new Error('window.opener is null, unable to send request.'); + const parent = getWindowParent(); + if (parent == null) { + throw new Error('window parent is null, unable to send request.'); } return new Promise((resolve, reject) => { let timeoutId: number; @@ -131,6 +142,6 @@ export async function requestParentResponse( window.removeEventListener('message', listener); reject(new TimeoutError('Request timed out')); }, timeout); - window.opener.postMessage(makeMessage(request, id), '*'); + parent.postMessage(makeMessage(request, id), '*'); }); } From 7d41761f49da191cda2b22b8b2deeea8e159ec1b Mon Sep 17 00:00:00 2001 From: mikebender Date: Wed, 21 Jun 2023 13:50:04 -0400 Subject: [PATCH 2/2] Refactor MessageUtils unit tests to share code - Now it doesn't duplicate the same code --- packages/jsapi-utils/src/MessageUtils.test.ts | 226 ++++++++---------- 1 file changed, 94 insertions(+), 132 deletions(-) diff --git a/packages/jsapi-utils/src/MessageUtils.test.ts b/packages/jsapi-utils/src/MessageUtils.test.ts index 7d3dda8f25..a7a390c64e 100644 --- a/packages/jsapi-utils/src/MessageUtils.test.ts +++ b/packages/jsapi-utils/src/MessageUtils.test.ts @@ -12,147 +12,109 @@ it('Throws an exception if called on a window without parent', async () => { ); }); -describe('requestParentResponse with opener', () => { - let addListenerSpy: jest.SpyInstance; - let removeListenerSpy: jest.SpyInstance; - let listenerCallback; - let messageId; - const mockPostMessage = jest.fn((data: Message) => { - messageId = data.id; - }); +/** + * Set up the mock for window.parent or window.opener, and return a cleanup function. + * @param type Whether to mock window.parent or window.opener + * @param mockPostMessage The mock postMessage function to use + * @returns Cleanup function + */ +function setupWindowParentMock( + type: string, + mockPostMessage: jest.Mock +): () => void { + if (type !== 'parent' && type !== 'opener') { + throw new Error(`Invalid type ${type}`); + } + if (type === 'parent') { + const windowParentSpy = jest.spyOn(window, 'parent', 'get').mockReturnValue( + TestUtils.createMockProxy({ + postMessage: mockPostMessage, + }) + ); + return () => { + windowParentSpy.mockRestore(); + }; + } + const originalWindowOpener = window.opener; - beforeEach(() => { - addListenerSpy = jest - .spyOn(window, 'addEventListener') - .mockImplementation((event, cb) => { - listenerCallback = cb; - }); - removeListenerSpy = jest.spyOn(window, 'removeEventListener'); - window.opener = { postMessage: mockPostMessage }; - }); - afterEach(() => { - addListenerSpy.mockRestore(); - removeListenerSpy.mockRestore(); - mockPostMessage.mockClear(); + window.opener = { postMessage: mockPostMessage }; + return () => { window.opener = originalWindowOpener; - messageId = undefined; - }); + }; +} - it('Posts message to parent and subscribes to response', async () => { - requestParentResponse('request'); - expect(mockPostMessage).toHaveBeenCalledWith( - expect.objectContaining(makeMessage('request', messageId)), - '*' - ); - expect(addListenerSpy).toHaveBeenCalledWith( - 'message', - expect.any(Function) - ); - }); - - it('Resolves with the payload from the parent window response and unsubscribes', async () => { - const PAYLOAD = 'PAYLOAD'; - const promise = requestParentResponse('request'); - listenerCallback({ - data: makeResponse(messageId, PAYLOAD), +describe.each([['parent'], ['opener']])( + `requestParentResponse with %s`, + type => { + let parentCleanup: () => void; + let addListenerSpy: jest.SpyInstance; + let removeListenerSpy: jest.SpyInstance; + let listenerCallback; + let messageId; + const mockPostMessage = jest.fn((data: Message) => { + messageId = data.id; }); - const result = await promise; - expect(result).toBe(PAYLOAD); - expect(removeListenerSpy).toHaveBeenCalledWith('message', listenerCallback); - }); - - it('Ignores unrelated response, rejects on timeout', async () => { - jest.useFakeTimers(); - const promise = requestParentResponse('request'); - listenerCallback({ - data: makeMessage('wrong-id'), + beforeEach(() => { + addListenerSpy = jest + .spyOn(window, 'addEventListener') + .mockImplementation((event, cb) => { + listenerCallback = cb; + }); + removeListenerSpy = jest.spyOn(window, 'removeEventListener'); + parentCleanup = setupWindowParentMock(type, mockPostMessage); + }); + afterEach(() => { + addListenerSpy.mockRestore(); + removeListenerSpy.mockRestore(); + mockPostMessage.mockClear(); + parentCleanup(); + messageId = undefined; }); - jest.runOnlyPendingTimers(); - await expect(promise).rejects.toThrow('Request timed out'); - jest.useRealTimers(); - }); - it('Times out if no response', async () => { - jest.useFakeTimers(); - const promise = requestParentResponse('request'); - jest.runOnlyPendingTimers(); - expect(removeListenerSpy).toHaveBeenCalled(); - await expect(promise).rejects.toThrow('Request timed out'); - jest.useRealTimers(); - }); -}); + it('Posts message to parent and subscribes to response', async () => { + requestParentResponse('request'); + expect(mockPostMessage).toHaveBeenCalledWith( + expect.objectContaining(makeMessage('request', messageId)), + '*' + ); + expect(addListenerSpy).toHaveBeenCalledWith( + 'message', + expect.any(Function) + ); + }); -describe('requestParentResponse with parent', () => { - let addListenerSpy: jest.SpyInstance; - let removeListenerSpy: jest.SpyInstance; - let windowParentSpy: jest.SpyInstance; - let listenerCallback; - let messageId; - const mockPostMessage = jest.fn((data: Message) => { - messageId = data.id; - }); - beforeEach(() => { - addListenerSpy = jest - .spyOn(window, 'addEventListener') - .mockImplementation((event, cb) => { - listenerCallback = cb; + it('Resolves with the payload from the parent window response and unsubscribes', async () => { + const PAYLOAD = 'PAYLOAD'; + const promise = requestParentResponse('request'); + listenerCallback({ + data: makeResponse(messageId, PAYLOAD), }); - removeListenerSpy = jest.spyOn(window, 'removeEventListener'); - windowParentSpy = jest.spyOn(window, 'parent', 'get').mockReturnValue( - TestUtils.createMockProxy({ - postMessage: mockPostMessage, - }) - ); - }); - - afterEach(() => { - addListenerSpy.mockRestore(); - removeListenerSpy.mockRestore(); - windowParentSpy.mockRestore(); - mockPostMessage.mockClear(); - messageId = undefined; - }); - - it('Posts message to parent and subscribes to response', async () => { - requestParentResponse('request'); - expect(mockPostMessage).toHaveBeenCalledWith( - expect.objectContaining(makeMessage('request', messageId)), - '*' - ); - expect(addListenerSpy).toHaveBeenCalledWith( - 'message', - expect.any(Function) - ); - }); - - it('Resolves with the payload from the parent window response and unsubscribes', async () => { - const PAYLOAD = 'PAYLOAD'; - const promise = requestParentResponse('request'); - listenerCallback({ - data: makeResponse(messageId, PAYLOAD), + const result = await promise; + expect(result).toBe(PAYLOAD); + expect(removeListenerSpy).toHaveBeenCalledWith( + 'message', + listenerCallback + ); }); - const result = await promise; - expect(result).toBe(PAYLOAD); - expect(removeListenerSpy).toHaveBeenCalledWith('message', listenerCallback); - }); - it('Ignores unrelated response, rejects on timeout', async () => { - jest.useFakeTimers(); - const promise = requestParentResponse('request'); - listenerCallback({ - data: makeMessage('wrong-id'), + it('Ignores unrelated response, rejects on timeout', async () => { + jest.useFakeTimers(); + const promise = requestParentResponse('request'); + listenerCallback({ + data: makeMessage('wrong-id'), + }); + jest.runOnlyPendingTimers(); + await expect(promise).rejects.toThrow('Request timed out'); + jest.useRealTimers(); }); - jest.runOnlyPendingTimers(); - await expect(promise).rejects.toThrow('Request timed out'); - jest.useRealTimers(); - }); - it('Times out if no response', async () => { - jest.useFakeTimers(); - const promise = requestParentResponse('request'); - jest.runOnlyPendingTimers(); - expect(removeListenerSpy).toHaveBeenCalled(); - await expect(promise).rejects.toThrow('Request timed out'); - jest.useRealTimers(); - }); -}); + it('Times out if no response', async () => { + jest.useFakeTimers(); + const promise = requestParentResponse('request'); + jest.runOnlyPendingTimers(); + expect(removeListenerSpy).toHaveBeenCalled(); + await expect(promise).rejects.toThrow('Request timed out'); + jest.useRealTimers(); + }); + } +);