Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -180,4 +180,29 @@ describe('OAuth2RedirectMessage Component', () => {

expect(reRunQuery).not.toHaveBeenCalled();
});

test('dispatches only once when both BroadcastChannel and storage signals arrive', async () => {
render(setup());

simulateBroadcastMessage({ tabId: 'tabId' });
simulateStorageMessage({ tabId: 'tabId' });

await waitFor(() => {
expect(reRunQuery).toHaveBeenCalledTimes(1);
});
});

test('falls back to storage events when BroadcastChannel construction throws', async () => {
(global as any).BroadcastChannel = jest.fn().mockImplementation(() => {
throw new Error('blocked');
});

render(setup());

simulateStorageMessage({ tabId: 'tabId' });

await waitFor(() => {
expect(reRunQuery).toHaveBeenCalledWith({ sql: 'SELECT * FROM table' });
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { useEffect } from 'react';
import { useEffect, useRef } from 'react';

import { useDispatch, useSelector } from 'react-redux';
import { QueryEditor, SqlLabRootState } from 'src/SqlLab/types';
Expand Down Expand Up @@ -99,29 +99,61 @@ export function OAuth2RedirectMessage({

const dispatch = useDispatch();

// `chartList` is rebuilt from `Object.keys` on every store update; keeping
// the listener state in a ref avoids tearing down/recreating the
// BroadcastChannel on each render and the race window that comes with it.
const latestStateRef = useRef({
source,
query,
chartId,
chartList,
dashboardId,
});
latestStateRef.current = {
source,
query,
chartId,
chartList,
dashboardId,
};

useEffect(() => {
// Guard against duplicate dispatches if both the BroadcastChannel and the
// storage fallback ever deliver the same completion.
let handled = false;
const handleOAuthComplete = (tabId?: string) => {
if (tabId !== extra.tab_id) {
if (tabId !== extra.tab_id || handled) {
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));
handled = true;
const {
source: src,
query: q,
chartId: cId,
chartList: cList,
dashboardId: dId,
} = latestStateRef.current;
if (src === 'sqllab' && q) {
dispatch(reRunQuery(q));
} else if (src === 'explore' && cId) {
dispatch(triggerQuery(true, cId));
} else if (src === 'dashboard') {
dispatch(onRefresh(cList.map(Number), true, 0, dId));
}
};

const channel =
typeof BroadcastChannel !== 'undefined'
? new BroadcastChannel(OAUTH_CHANNEL_NAME)
: null;

if (channel) {
channel.onmessage = event => {
handleOAuthComplete(event.data?.tabId);
};
// `BroadcastChannel` may exist on `window` but throw at construction time
// in restricted contexts; fall back to the storage listener if so.
let channel: BroadcastChannel | null = null;
if (typeof BroadcastChannel !== 'undefined') {
try {
channel = new BroadcastChannel(OAUTH_CHANNEL_NAME);
channel.onmessage = event => {
handleOAuthComplete(event.data?.tabId);
};
} catch {
channel = null;
}
}

const handleStorage = (event: StorageEvent) => {
Expand All @@ -143,7 +175,7 @@ export function OAuth2RedirectMessage({
window.removeEventListener('storage', handleStorage);
channel?.close();
};
}, [source, extra.tab_id, dispatch, query, chartId, chartList, dashboardId]);
}, [extra.tab_id, dispatch]);

const body = (
<p>
Expand Down
22 changes: 17 additions & 5 deletions superset/templates/superset/oauth2.html
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,25 @@
<body>
<script nonce="{{ macros.get_nonce() }}">
const message = { tabId: '{{ tab_id }}' };
let broadcasted = false;
if (typeof BroadcastChannel !== 'undefined') {
const channel = new BroadcastChannel('oauth');
channel.postMessage(message);
channel.close();
try {
const channel = new BroadcastChannel('oauth');
channel.postMessage(message);
channel.close();
broadcasted = true;
} catch (e) {
// fall through to the localStorage fallback below
}
}
if (!broadcasted) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Richard's agent here: the receiver-side handled guard and the BroadcastChannel construction try/catch both make sense to me.

One thing I’d double-check: could we still write the localStorage signal even when BroadcastChannel.postMessage() succeeds? A successful postMessage() only tells us the callback page sent the message; it does not guarantee the original tab had its listener attached at that exact moment. The storage event gives us a second delivery path for that race.

Since the receiver now dedupes with handled, sending both signals should not reintroduce duplicate reruns. I think we can keep the try/catch around both mechanisms, but avoid making storage conditional on broadcasted, e.g.:

try {
  const channel = new BroadcastChannel('oauth');
  channel.postMessage(message);
  channel.close();
} catch (e) {}

try {
  localStorage.setItem('oauth2_auth_complete', JSON.stringify(message));
  localStorage.removeItem('oauth2_auth_complete');
} catch (e) {}

try {
localStorage.setItem('oauth2_auth_complete', JSON.stringify(message));
localStorage.removeItem('oauth2_auth_complete');
} catch (e) {
// storage may be unavailable (e.g. blocked or restricted); ignore
}
}
localStorage.setItem('oauth2_auth_complete', JSON.stringify(message));
localStorage.removeItem('oauth2_auth_complete');
window.close();
</script>
<p>You can close this window and re-run the query.</p>
Expand Down
Loading