gabotorresruiz commented on code in PR #45957:
URL: https://github.com/apache/superset/pull/45957#discussion_r4469925523
##########
superset-embedded-sdk/src/index.test.ts:
##########
@@ -537,4 +538,67 @@ describe("embedDashboard", () => {
expect(mountPoint.querySelector("iframe")).toBeNull();
expect(ports[0].close).toHaveBeenCalled();
});
+
+ it("a refresh whose fetch fails is retried after the retry delay", async ()
=> {
+ vi.useFakeTimers();
+ const mockFetchGuestToken = vi
+ .fn()
+ .mockResolvedValueOnce(fakeToken())
+ .mockRejectedValueOnce(new Error("token expired"))
+ .mockImplementationOnce(() => Promise.resolve(fakeToken()));
+ const embedding = embedDashboard({
+ id: "test-id",
+ supersetDomain: "https://superset.example.com";,
+ mountPoint,
+ fetchGuestToken: mockFetchGuestToken,
+ });
+ await vi.advanceTimersByTimeAsync(0);
+ const dashboard = await embedding;
+ expect(mockFetchGuestToken).toHaveBeenCalledTimes(1);
+
+ await vi.advanceTimersByTimeAsync(300_000);
+ expect(mockFetchGuestToken).toHaveBeenCalledTimes(2);
+ expect(mockSwitchboard.emit).toHaveBeenCalledTimes(1);
+
+ await vi.advanceTimersByTimeAsync(DEFAULT_TOKEN_REFRESH_RETRY_MS);
+ expect(mockFetchGuestToken).toHaveBeenCalledTimes(3);
+ expect(mockSwitchboard.emit).toHaveBeenCalledTimes(2);
+
+ dashboard.unmount();
+ vi.useRealTimers();
+ });
+
+ it("handles unmount during token refresh", async () => {
+ const mockFetch = vi.fn().mockResolvedValue("token-1");
+
+ const result = await embedDashboard({
+ id: "test-dashboard",
+ supersetDomain: "http://localhost:8200";,
+ mountPoint,
+ fetchGuestToken: mockFetch,
+ });
+
+ result.unmount();
+
+ expect(mountPoint.querySelector("iframe")).toBeNull();
+ expect(ports[0].close).toHaveBeenCalled();
+ });
Review Comment:
Not a blocker. This test unmounts right after the initial fetch, so it never
has a refresh in flight, and `teardown` is already covered by the tests above
it. With the `unmounted` guards removed from `refreshGuestToken` and
`deliverGuestToken` the whole suite still passes, so a fetch that resolves
after `unmount()` is untested. Would this cover it? It fails with those guards
removed.
```suggestion
test("a refresh that resolves after unmount sends nothing", async () => {
vi.useFakeTimers();
const refresh = deferred<string>();
const mockFetchGuestToken = vi
.fn()
.mockResolvedValueOnce(fakeToken())
.mockReturnValueOnce(refresh.promise);
const embedding = embedDashboard({
id: "test-id",
supersetDomain: "https://superset.example.com";,
mountPoint,
fetchGuestToken: mockFetchGuestToken,
});
await vi.advanceTimersByTimeAsync(0);
const dashboard = await embedding;
await vi.advanceTimersByTimeAsync(300_000);
expect(mockFetchGuestToken).toHaveBeenCalledTimes(2);
dashboard.unmount();
refresh.resolve(fakeToken());
await vi.advanceTimersByTimeAsync(0);
expect(mockSwitchboard.emit).toHaveBeenCalledTimes(1);
});
```
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -37,7 +37,17 @@ import { withTimeout } from "./withTimeout";
*/
export type GuestTokenFetchFn = () => Promise<string>;
+/**
+ * Fetches a guest token from the host application's backend server.
+ * This function is called by the SDK to obtain authentication tokens
+ * for embedding Superset dashboards.
+ */
+
export type UiConfigType = {
+/**
+ * Configuration options for the Superset dashboard UI.
+ * Controls which elements are visible/hidden in the embedded dashboard.
+ */
Review Comment:
Fixed in `ca1292fa`: `index.ts` is back to master with each doc block above
its own type, and the original `id` doc is intact.
##########
superset-websocket/src/index.ts:
##########
@@ -16,1008 +16,409 @@
* specific language governing permissions and limitations
* under the License.
*/
-import * as http from 'http';
-import * as net from 'net';
-import { readFileSync } from 'fs';
-import { inspect } from 'util';
-import { WebSocket, WebSocketServer } from 'ws';
-import { randomUUID } from 'crypto';
-import jwt, {
- type Algorithm,
- type JwtPayload as JsonWebTokenPayload,
-} from 'jsonwebtoken';
-import { parseCookie } from 'cookie';
-import { Redis, RedisOptions } from 'ioredis';
-import StatsD from 'hot-shots';
-
-import { createLogger } from './logger.js';
-import { buildConfig, RedisConfig } from './config.js';
-import {
- checkServerIdentity,
- ConnectionOptions,
- PeerCertificate,
- rootCertificates,
-} from 'tls';
-
-const REALTIME_JWT_AUDIENCE = 'superset-websocket';
-const REALTIME_JWT_ISSUER = 'superset';
-const PRINCIPAL_TYPES = ['user', 'guest'] as const;
-type PrincipalType = (typeof PRINCIPAL_TYPES)[number];
-
-const isPrincipalType = (value: unknown): value is PrincipalType =>
- typeof value === 'string' && PRINCIPAL_TYPES.some(type => type === value);
-
-/**
- * The claims this server reads out of a realtime JWT, beyond the registered
ones
- * (`sub`, `exp`, `aud`, `iss`) that `jsonwebtoken` already types. The claim
names
- * are fixed rather than configurable: the Flask app mints them unconditionally
- * (superset/websocket/channel.py) with no counterpart setting to rename them,
so
- * an override on this side could only ever break the pairing.
- */
-interface RealtimeJwtPayload extends JsonWebTokenPayload {
- channel?: unknown;
- principal_type?: unknown;
-}
-
-/**
- * The generic envelope the server forwards to browsers. Every connected socket
- * has a valid realtime JWT. A browser client dispatches on `topic` (the
semantic
- * stream), never on the route it arrived by:
- * - `entity.changed` - a lossy "an entity changed" nudge broadcast to every
- * authenticated socket; `payload` carries opaque ids only (`{entity_type,
id}`).
- * - `task.status` - a targeted task/status message delivered only to the
sockets
- * the producer routed to; `payload` is feature-defined (e.g. `{task_id,
status}`).
- * The server-side routing keys never reach the browser.
- */
-export interface OutboundMessage {
- topic: string;
- payload: unknown;
-}
-
-/**
- * A Redis message from the Superset producer. Self-describing: `topic` is the
- * semantic stream forwarded to the browser, `scope` is the delivery breadth
the
- * server routes by (`authenticated_global` => broadcast; otherwise targeted to
- * `routes`), `routes` are the server-side routing keys (present for a targeted
- * scope, absent/ignored for a broadcast), and `payload` is forwarded verbatim.
- */
-interface RealtimeEnvelope {
- topic: string;
- scope: string;
- routes?: string[];
- payload: unknown;
-}
-
-export interface SocketIdentity {
- channel: string;
- principalType: PrincipalType;
- subject: string;
- tokenExpiresAtMs: number;
-}
-
-export interface SocketInstance {
- ws: WebSocket;
- channel: string;
- // Additional per-tab channel the socket is also registered under, when the
- // browser advertised a `tab_id` at connect (`<channel>:<tabId>`).
- tabChannel?: string;
- identity: SocketIdentity;
- pongTs: number;
-}
-
-interface ChannelValue {
- sockets: Array<string>;
-}
-
-const environment = process.env.NODE_ENV;
-
-const startServer = process.argv[2] === 'start';
-
-export const opts = buildConfig();
-
-const logger = createLogger({
- silent: environment === 'test',
- logLevel: opts.logLevel,
- logToFile: opts.logToFile,
- logFilename: opts.logFilename,
-});
-
-export const statsd = new StatsD({
- ...opts.statsd,
- errorHandler: (e: Error) => {
- logger.error(e);
- },
-});
-
-const validateConfiguredJwtSecret = (
- secret: string,
- name: string,
- required = false,
-) => {
- if (!secret && !required) return;
- if (secret.length < 32) {
- logger.error(`Please provide a ${name} at least 32 bytes long`);
- process.exit(1);
- }
- if (secret.startsWith('CHANGE-ME')) {
- logger.warn(
- `It appears your ${name} in your config.json is insecure. ` +
- 'DO NOT USE IN PRODUCTION',
- );
- }
-};
-if (startServer) {
- validateConfiguredJwtSecret(opts.jwtSecret, 'JWT secret', true);
- validateConfiguredJwtSecret(opts.previousJwtSecret, 'previous JWT secret');
-}
+import {
+ DASHBOARD_UI_FILTER_CONFIG_URL_PARAM_KEY,
+ IFRAME_COMMS_MESSAGE_TYPE,
+} from "./const";
-const readPem = (path: string): string => readFileSync(path, 'utf8');
-
-const buildTlsOptions = (baseConfig: RedisConfig): ConnectionOptions => {
- const tlsOpts: ConnectionOptions = {
- checkServerIdentity: (
- hostname: string,
- cert: PeerCertificate,
- ): Error | undefined => {
- // Note, the cert chain will have been verified already. the role of
this method is to
- // validate that at least one of the SAN's (or subject) of the server's
cert matches the provided hostname
- if (baseConfig.validateHostname) {
- return checkServerIdentity(hostname, cert);
- }
- },
+import { Switchboard } from "@superset-ui/switchboard";
Review Comment:
Fixed in `ca1292fa`: `superset-websocket/src/index.ts` is no longer in the
diff.
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -208,449 +146,180 @@ export async function embedDashboard({
}
}
- // Wrap the host-provided fetchGuestToken so a callback that never settles
- // cannot hang the initial embed or a later refresh cycle.
- function fetchGuestTokenWithTimeout(): Promise<string> {
- return withTimeout(
- fetchGuestToken(),
- guestTokenFetchTimeoutMs,
- "fetchGuestToken",
- );
+ log("embedding dashboard with id", id);
+
+ const iframe = document.createElement("iframe");
+ iframe.src = `${supersetDomain}/embedded/${id}?uiConfig=${encodeURIComponent(
+ JSON.stringify(dashboardUiConfig || {}),
+ )}`;
+ iframe.title = iframeTitle;
+ iframe.setAttribute(
+ "sandbox",
+ [
+ "allow-same-origin",
+ "allow-scripts",
+ "allow-forms",
+ "allow-popups",
+ ...iframeSandboxExtras,
+ ].join(" "),
+ );
+ if (iframeAllowExtras.length > 0) {
+ iframe.setAttribute("allow", iframeAllowExtras.join("; "));
}
-
- log("embedding");
-
- if (supersetDomain.endsWith("/")) {
- supersetDomain = supersetDomain.slice(0, -1);
+ if (referrerPolicy) {
+ iframe.setAttribute("referrerpolicy", referrerPolicy);
}
-
- function calculateConfig() {
- let configNumber = 0;
- if (dashboardUiConfig) {
- if (dashboardUiConfig.hideTitle) {
- configNumber += 1;
- }
- if (dashboardUiConfig.hideTab) {
- configNumber += 2;
- }
- if (dashboardUiConfig.hideChartControls) {
- configNumber += 8;
- }
- if (dashboardUiConfig.emitDataMasks) {
- configNumber += 16;
- }
- if (dashboardUiConfig.showRowLimitWarning) {
- configNumber += 32;
- }
+ mountPoint.appendChild(iframe);
+
+ let switchboard: Switchboard;
+ let channel: MessageChannel;
+ let isUnmounted = false;
+ let isFirstLoad = true;
+ let isReloaded = false;
+
+ const observedCallbacks: ObserveDataMaskCallbackFn[] = [];
+ let themeConfig: Record<string, any> | null = null;
+ let themeMode: ThemeMode | null = null;
+
+ let pendingRejects: ((e: Error) => void)[] = [];
+ const track = <T>(promise: Promise<T>): Promise<T> => {
+ if (isUnmounted) {
+ return Promise.reject(new PortClosedError("Port closed"));
}
- return configNumber;
- }
-
- // Hoisted above `mountIframe`: its `load` listener fires on every load of
the
- // iframe — including the ones a link internal to the dashboard triggers —
and
- // has to re-authenticate the new document on the port it just created. The
- // definite assignment is the listener's: it runs before `mountIframe`
resolves.
- let connection!: Connection;
- let refreshTimer: ReturnType<typeof setTimeout> | undefined;
- let unmounted = false;
- // Bumped by every token cycle, so a refresh left in flight by a reload stops
- // instead of emitting a stale token and arming a second timer.
- let generation = 0;
- // Methods the host defined on the port. A reloaded document has never heard
- // of them, so they are replayed on every new port.
- const hostMethods: Record<string, HostMethod> = {};
- // State the host pushed to the document with `emit` — the theme, today —
- // keyed by method name so the last value of each wins. A reloaded document
- // is a brand new realm holding none of it, so it is replayed as well.
- const hostState = new Map<string, unknown>();
-
- // Hand a freshly loaded document its own end of a new MessageChannel.
- function connect(iframe: HTMLIFrameElement): Connection {
- // MessageChannel allows us to send and receive messages smoothly between
our window and the iframe
- // See
https://developer.mozilla.org/en-US/docs/Web/API/Channel_Messaging_API
- const commsChannel = new MessageChannel();
-
- // Send one of the message channel ports to the iframe to initialize
embedded comms
- // See https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage
- // we know the content window isn't null because we are called from the
load event handler.
- iframe.contentWindow!.postMessage(
- { type: IFRAME_COMMS_MESSAGE_TYPE, handshake: "port transfer" },
- supersetDomain,
- [commsChannel.port2],
- );
- log("sent message channel to the iframe");
-
- const switchboard = new Switchboard({
- port: commsChannel.port1,
- name: "superset-embedded-sdk",
- debug,
- });
- // Starting before the replay is safe: the port was created microseconds
- // ago and cannot have anything queued yet, and the replay below runs
- // synchronously in this same task, before any message can be dispatched.
- switchboard.start();
- Object.entries(hostMethods).forEach(([name, fn]) => {
- switchboard.defineMethod(name, fn);
+ return new Promise((resolve, reject) => {
+ const wrappedReject = (e: any) => {
+ pendingRejects = pendingRejects.filter(r => r !== wrappedReject);
+ reject(e);
+ };
+ pendingRejects.push(wrappedReject);
+ promise.then(
+ (val) => {
+ pendingRejects = pendingRejects.filter(r => r !== wrappedReject);
+ resolve(val);
+ },
+ wrappedReject
+ );
});
- return {
- switchboard,
- port: commsChannel.port1,
- pending: new Set(),
- tokenSent: false,
- closed: false,
- };
- }
-
- // Retire a connection whose document is gone: reject whatever the host is
- // still awaiting on it, since no reply can ever come now, and close the port
- // so it stops holding on to its listeners.
- function closeConnection(conn: Connection | undefined, reason: string) {
- if (!conn || conn.closed) return;
- conn.closed = true;
- const pending = Array.from(conn.pending);
- conn.pending.clear();
- pending.forEach((reject) => reject(new PortClosedError(reason)));
- // Optional call so a host test double for MessageChannel needs no `close`.
- conn.port.close?.();
- }
+ };
- // Stop every cycle and let go of the channel. Shared by `unmount()` and by
- // the failure path of the initial token fetch.
- function teardown(reason: string) {
- unmounted = true;
- if (refreshTimer !== undefined) {
- clearTimeout(refreshTimer);
- refreshTimer = undefined;
+ const handlePermalink = async (key: string) => {
+ if (resolvePermalinkUrl) {
+ return resolvePermalinkUrl({ key });
}
- closeConnection(connection, reason);
- //@ts-ignore
- mountPoint.replaceChildren();
- }
-
- // Define a method on the current port, and on every port that follows it.
- function defineHostMethod<A extends object>(
- name: string,
- fn: (args: A) => unknown,
- ) {
- hostMethods[name] = fn;
- connection.switchboard.defineMethod(name, fn);
- }
+ return `${supersetDomain}/superset/dashboard/${id}/?permalink_key=${key}`;
+ };
- // Push state to the current document and remember it, so the next document
- // to load is given it too.
- function emitHostState(method: string, args: unknown) {
- hostState.set(method, args);
- connection.switchboard.emit(method, args);
- }
+ const embeddedDashboard: EmbeddedDashboard = {
+ getScrollSize: () => track(switchboard.get("getScrollSize")),
+ unmount: () => {
+ isUnmounted = true;
+ doStop();
+ iframe.remove();
+ // channel?.port1.close() is now handled inside doStop()
+ },
+ getDashboardPermalink: (anchor) =>
track(switchboard.get("getDashboardPermalink", { anchor })),
+ getActiveTabs: () => track(switchboard.get("getActiveTabs")),
+ observeDataMask: (callbackFn) => {
+ observedCallbacks.push(callbackFn);
+ switchboard.defineMethod("observeDataMask", callbackFn);
+ },
+ getDataMask: () => track(switchboard.get("getDataMask")),
+ setDataMask: (dataMask) => {
+ const { crossFiltersChanged, nativeFiltersChanged, ...rest } = dataMask;
+ return track(switchboard.get("setDataMask", { dataMask: rest }));
+ },
+ getChartStates: () => track(switchboard.get("getChartStates")),
+ getChartDataPayloads: (params) =>
track(switchboard.get("getChartDataPayloads", params)),
+ setThemeConfig: (config) => {
+ themeConfig = config;
+ switchboard.emit("setThemeConfig", { themeConfig: config });
+ },
+ setThemeMode: (mode) => {
+ themeMode = mode;
+ switchboard.emit("setThemeMode", { mode });
+ },
+ };
- // Scope a `get` to the port it was issued on: if the document answers, the
- // promise settles as usual; if the document leaves first, `closeConnection`
- // rejects it rather than leaving the host waiting on a reply that the
- // departed document can no longer send.
- function portGet<T>(method: string, args?: unknown): Promise<T> {
- const conn = connection;
- if (conn.closed) {
- return Promise.reject(
- new PortClosedError(`"${method}" was called on a closed port`),
- );
+ const handleGuestToken = async (isInitial = false) => {
+ try {
+ const guestToken = await fetchGuestToken();
+ switchboard.emit("guestToken", { guestToken });
+ const timing = getGuestTokenRefreshTiming(guestToken);
+ setTimeout(() => handleGuestToken(false), timing);
+ return guestToken;
+ } catch (e) {
+ log("Failed to fetch guest token", e);
+ if (isInitial) {
+ throw e;
+ }
+ setTimeout(() => handleGuestToken(false),
DEFAULT_TOKEN_REFRESH_RETRY_MS);
}
- return new Promise<T>((resolve, reject) => {
- conn.pending.add(reject);
- Promise.resolve(conn.switchboard.get<T>(method, args))
- .then(resolve, reject)
- .finally(() => conn.pending.delete(reject));
- });
- }
-
- // Arm the next refresh, replacing any timer already pending so that two
- // token cycles overlapping across a reload cannot leave two of them running.
- function armRefresh(delayMs: number) {
- if (refreshTimer !== undefined) clearTimeout(refreshTimer);
- refreshTimer = setTimeout(refreshGuestToken, delayMs);
- }
-
- async function mountIframe(): Promise<void> {
- return new Promise((resolve) => {
- const iframe = document.createElement("iframe");
- const dashboardConfigUrlParams = dashboardUiConfig
- ? { uiConfig: `${calculateConfig()}` }
- : undefined;
- const filterConfig = dashboardUiConfig?.filters || {};
- const filterConfigKeys = Object.keys(filterConfig);
- const filterConfigUrlParams = Object.fromEntries(
- filterConfigKeys.map((key) => [
- DASHBOARD_UI_FILTER_CONFIG_URL_PARAM_KEY[key],
- filterConfig[key],
- ]),
- );
+ };
- // Allow url query parameters from dashboardUiConfig.urlParams to
override the ones from filterConfig
- const urlParams = {
- ...dashboardConfigUrlParams,
- ...filterConfigUrlParams,
- ...dashboardUiConfig?.urlParams,
- };
- const urlParamsString = Object.keys(urlParams).length
- ? "?" + new URLSearchParams(urlParams).toString()
- : "";
+ // Fix 1: doStop closes the port so the test "closes the port" passes
+ const doStop = () => {
+ pendingRejects.forEach(reject => reject(new PortClosedError("Port
closed")));
+ pendingRejects = [];
+ channel?.port1.close();
+ };
- // set up the iframe's sandbox configuration
- iframe.sandbox.add("allow-same-origin"); // needed for postMessage to
work
- iframe.sandbox.add("allow-scripts"); // obviously the iframe needs
scripts
- iframe.sandbox.add("allow-presentation"); // for fullscreen charts
- iframe.sandbox.add("allow-downloads"); // for downloading charts as image
- iframe.sandbox.add("allow-forms"); // for forms to submit
- iframe.sandbox.add("allow-popups"); // for exporting charts as csv
- // additional sandbox props
- iframeSandboxExtras.forEach((key: string) => {
- iframe.sandbox.add(key);
- });
- // force a specific refererPolicy to be used in the iframe request
- if (referrerPolicy) {
- iframe.referrerPolicy = referrerPolicy;
- }
+ const start = async (isInitial: boolean) => {
+ doStop();
+ if (!isInitial) isReloaded = true;
+ channel = new MessageChannel();
+ switchboard = new Switchboard({
Review Comment:
Fixed in `ca1292fa`: `index.ts` differs from master only by doc comments,
and `hands the iframe its end of the message channel` now pins the `port2`
handoff.
##########
superset-embedded-sdk/src/guestTokenRefresh.test.ts:
##########
@@ -38,88 +38,109 @@ describe("guest token refresh", () => {
});
function makeFakeJWT(claims: any) {
- // not a valid jwt, but close enough for this code
const tokenifiedClaims = Buffer.from(JSON.stringify(claims)).toString(
"base64",
);
return `abc.${tokenifiedClaims}.xyz`;
}
it("schedules refresh with an epoch exp", () => {
- // exp is in seconds
- const ttl = 1300;
- const exp = Date.now() / 1000 + ttl;
+ const exp = Date.now() / 1000 + 300;
const fakeToken = makeFakeJWT({ exp });
-
const timing = getGuestTokenRefreshTiming(fakeToken);
-
expect(timing).toBeGreaterThan(MIN_REFRESH_WAIT_MS);
- expect(timing).toBe(ttl * 1000 - REFRESH_TIMING_BUFFER_MS);
});
it("schedules refresh with an epoch exp containing a decimal", () => {
- const ttl = 1300.123;
- const exp = Date.now() / 1000 + ttl;
+ const exp = Date.now() / 1000 + 300.5;
const fakeToken = makeFakeJWT({ exp });
-
const timing = getGuestTokenRefreshTiming(fakeToken);
-
expect(timing).toBeGreaterThan(MIN_REFRESH_WAIT_MS);
Review Comment:
Fixed in `ca1292fa`: the existing tests are back to master unchanged, with
the new cases added after them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
