From 4e6d5b57c6681543fabdee49d56ab667d2f4e1d7 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Wed, 13 May 2026 15:30:04 -0400 Subject: [PATCH] fix: OAuth2 trigger --- .../OAuth2RedirectMessage.test.tsx | 60 ++++++++------- .../ErrorMessage/OAuth2RedirectMessage.tsx | 76 +++++-------------- superset/templates/superset/oauth2.html | 4 +- 3 files changed, 57 insertions(+), 83 deletions(-) diff --git a/superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.test.tsx b/superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.test.tsx index a69089b91ae..3152d81e5c3 100644 --- a/superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.test.tsx +++ b/superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.test.tsx @@ -20,7 +20,7 @@ import * as reduxHooks from 'react-redux'; import { Provider } from 'react-redux'; import { createStore } from 'redux'; -import { render, fireEvent, waitFor } from 'spec/helpers/testing-library'; +import { render, waitFor } from 'spec/helpers/testing-library'; import { ErrorLevel, ErrorSource, ErrorTypeEnum } from '@superset-ui/core'; import { reRunQuery } from 'src/SqlLab/actions/sqlLab'; import { triggerQuery } from 'src/components/Chart/chartAction'; @@ -58,25 +58,24 @@ jest.mock('src/dashboard/actions/dashboardState', () => ({ const mockDispatch = jest.fn(); jest.spyOn(reduxHooks, 'useDispatch').mockReturnValue(mockDispatch); -// Mock global window functions -const mockOpen = jest.spyOn(window, 'open').mockImplementation(() => null); -const mockAddEventListener = jest.spyOn(window, 'addEventListener'); -const mockRemoveEventListener = jest.spyOn(window, 'removeEventListener'); - -// Mock window.postMessage -const originalPostMessage = window.postMessage; +// Capture the channel instance created by the component so tests can drive its +// onmessage handler and assert it gets closed on unmount. +let capturedChannel: { + onmessage: ((event: any) => void) | null; + close: jest.Mock; +}; +const channelCloseMock = jest.fn(); beforeEach(() => { - window.postMessage = jest.fn(); + jest.clearAllMocks(); + capturedChannel = { onmessage: null, close: channelCloseMock }; + (global as any).BroadcastChannel = jest + .fn() + .mockImplementation(() => capturedChannel); }); -afterEach(() => { - window.postMessage = originalPostMessage; -}); - -function simulateMessageEvent(data: any, origin: string) { - const messageEvent = new MessageEvent('message', { data, origin }); - window.dispatchEvent(messageEvent); +function simulateBroadcastMessage(data: any) { + capturedChannel.onmessage?.({ data }); } const defaultProps = { @@ -108,27 +107,26 @@ describe('OAuth2RedirectMessage Component', () => { expect(getByText(/provide authorization/i)).toBeInTheDocument(); }); - test('opens a new window with the correct URL when the link is clicked', () => { + test('renders the authorization link pointing at the OAuth2 URL', () => { const { getByText } = render(setup()); - const linkElement = getByText(/provide authorization/i); - fireEvent.click(linkElement); - - expect(mockOpen).toHaveBeenCalledWith('https://example.com', '_blank'); + const linkElement = getByText(/provide authorization/i).closest('a'); + expect(linkElement).toHaveAttribute('href', 'https://example.com'); + expect(linkElement).toHaveAttribute('target', '_blank'); }); - test('cleans up the message event listener on unmount', () => { + test('closes the BroadcastChannel on unmount', () => { const { unmount } = render(setup()); - expect(mockAddEventListener).toHaveBeenCalled(); + expect((global as any).BroadcastChannel).toHaveBeenCalledWith('oauth'); unmount(); - expect(mockRemoveEventListener).toHaveBeenCalled(); + expect(channelCloseMock).toHaveBeenCalled(); }); test('dispatches reRunQuery action when a message with correct tab ID is received for SQL Lab', async () => { render(setup()); - simulateMessageEvent({ tabId: 'tabId' }, 'https://redirect.example.com'); + simulateBroadcastMessage({ tabId: 'tabId' }); await waitFor(() => { expect(reRunQuery).toHaveBeenCalledWith({ sql: 'SELECT * FROM table' }); @@ -138,7 +136,7 @@ describe('OAuth2RedirectMessage Component', () => { test('dispatches triggerQuery action for explore source upon receiving a correct message', async () => { render(setup({ source: 'explore' })); - simulateMessageEvent({ tabId: 'tabId' }, 'https://redirect.example.com'); + simulateBroadcastMessage({ tabId: 'tabId' }); await waitFor(() => { expect(triggerQuery).toHaveBeenCalledWith(true, 123); @@ -148,11 +146,19 @@ describe('OAuth2RedirectMessage Component', () => { test('dispatches onRefresh action for dashboard source upon receiving a correct message', async () => { render(setup({ source: 'dashboard' })); - simulateMessageEvent({ tabId: 'tabId' }, 'https://redirect.example.com'); + simulateBroadcastMessage({ tabId: 'tabId' }); await waitFor(() => { // Chart IDs are converted to numbers by the component via chartList.map(Number) expect(onRefresh).toHaveBeenCalledWith([1, 2], true, 0, 'dashboard-id'); }); }); + + test('ignores messages with a mismatched tab ID', () => { + render(setup()); + + simulateBroadcastMessage({ tabId: 'someOtherTab' }); + + expect(reRunQuery).not.toHaveBeenCalled(); + }); }); diff --git a/superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx b/superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx index 2dc655fdec0..fd010a04c8c 100644 --- a/superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx +++ b/superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { useEffect, useRef, MouseEvent } from 'react'; +import { useEffect } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { QueryEditor, SqlLabRootState } from 'src/SqlLab/types'; @@ -34,7 +34,6 @@ import { ErrorAlert } from './ErrorAlert'; interface OAuth2RedirectExtra { url: string; tab_id: string; - redirect_uri: string; } /* @@ -52,29 +51,20 @@ interface OAuth2RedirectExtra { * be used in subsequent connections. If a refresh token is also present in the response, * it will also be stored. * - * After the token has been stored, the opened tab will send a message to the original - * tab and close itself. This component, running on the original tab, will listen for - * message events, and once it receives the success message from the opened tab it will - * re-run the query for the user, be it in SQL Lab, Explore, or a dashboard. In order to - * communicate securely, both tabs share a "tab ID", which is a UUID that is generated - * by the backend and sent from the opened tab to the original tab. For extra security, - * we also check that the source of the message is the opened tab via a ref. + * After the token has been stored, the opened tab will broadcast a message to the + * original tab and close itself. This component, running on the original tab, listens + * on a same-origin BroadcastChannel and re-runs the query for the user once it + * receives the success message — be it in SQL Lab, Explore, or a dashboard. Both tabs + * share a "tab ID" (a UUID generated by the backend) which is echoed back through the + * channel so the original tab only reacts to its own OAuth2 flow. */ export function OAuth2RedirectMessage({ error, source, closable, }: ErrorMessageComponentProps) { - const oAuthTab = useRef(null); const { extra, level } = error; - // store a reference to the OAuth2 browser tab, so we can check that the success - // message is coming from it - const handleOAuthClick = (event: MouseEvent) => { - event.preventDefault(); - oAuthTab.current = window.open(extra.url, '_blank'); - }; - // state needed for re-running the SQL Lab query const queries = useSelector< SqlLabRootState, @@ -107,43 +97,24 @@ export function OAuth2RedirectMessage({ const dispatch = useDispatch(); useEffect(() => { - /* Listen for messages from the OAuth2 tab. - * - * After OAuth2 is successful the opened tab will send a message before - * closing itself. Once we receive the message we can retrigger the - * original query in SQL Lab, explore, or in a dashboard. - */ - const redirectUrl = new URL(extra.redirect_uri); - const handleMessage = (event: MessageEvent) => { - if ( - event.origin === redirectUrl.origin && - event.data.tabId === extra.tab_id && - event.source === oAuthTab.current - ) { - if (source === 'sqllab' && query) { - dispatch(reRunQuery(query)); - } else if (source === 'explore' && chartId) { - dispatch(triggerQuery(true, chartId)); - } else if (source === 'dashboard') { - dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId)); - } + const channel = new BroadcastChannel('oauth'); + channel.onmessage = event => { + if (event.data.tabId !== extra.tab_id) { + return; + } + if (source === 'sqllab' && query) { + dispatch(reRunQuery(query)); + } else if (source === 'explore' && chartId) { + dispatch(triggerQuery(true, chartId)); + } else if (source === 'dashboard') { + dispatch(onRefresh(chartList.map(Number), true, 0, dashboardId)); } }; - window.addEventListener('message', handleMessage); return () => { - window.removeEventListener('message', handleMessage); + channel.close(); }; - }, [ - source, - extra.redirect_uri, - extra.tab_id, - dispatch, - query, - chartId, - chartList, - dashboardId, - ]); + }, [source, extra.tab_id, dispatch, query, chartId, chartList, dashboardId]); const body = (

@@ -155,12 +126,7 @@ export function OAuth2RedirectMessage({ const subtitle = ( <> {t('You need to')}{' '} - + {t('provide authorization')} {' '} {t('in order to run this operation.')} diff --git a/superset/templates/superset/oauth2.html b/superset/templates/superset/oauth2.html index e3562758db8..47fb9a20ecf 100644 --- a/superset/templates/superset/oauth2.html +++ b/superset/templates/superset/oauth2.html @@ -23,7 +23,9 @@ under the License.

You can close this window and re-run the query.