Copilot commented on code in PR #39300: URL: https://github.com/apache/superset/pull/39300#discussion_r3211790482
########## tests/integration_tests/superset_test_config.py: ########## @@ -78,6 +78,15 @@ WEBDRIVER_BASEURL = "http://0.0.0.0:8081/"; +# Enable CORS for embedded dashboard E2E tests (test app on port 9000) +ENABLE_CORS = True +CORS_OPTIONS: dict = { + "origins": [ + "http://localhost:9000";, + ], + "supports_credentials": True, Review Comment: CORS config is hard-coded to allow only `http://localhost:9000`, but the embedded test app server in `embedded-dashboard.spec.ts` binds to `127.0.0.1` on an ephemeral port. If CORS is actually needed for the embedding flow, this mismatch will cause requests from the test app origin to be rejected. Either align the test server to use `localhost:9000` consistently, or widen `CORS_OPTIONS['origins']` to include the actual origin pattern (e.g. localhost/127.0.0.1 with dynamic ports). ########## superset-frontend/playwright/embedded-app/index.html: ########## @@ -0,0 +1,96 @@ +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> +<!DOCTYPE html> +<html lang="en"> +<head> + <meta charset="UTF-8" /> + <meta name="viewport" content="width=device-width, initial-scale=1.0" /> + <title>Embedded Dashboard Test App</title> + <style> + html, body { margin: 0; padding: 0; height: 100%; } + #superset-container { width: 100%; height: 100vh; } + #superset-container iframe { width: 100%; height: 100%; border: none; } + #error { color: red; padding: 20px; display: none; } + #status { padding: 10px; font-family: monospace; font-size: 12px; } + </style> +</head> +<body> + <div id="status">Initializing embedded dashboard...</div> + <div id="error"></div> + <div id="superset-container" data-test="embedded-container"></div> + + <script src="/sdk/index.js"></script> + <script> + (async function () { + const params = new URLSearchParams(window.location.search); + const uuid = params.get('uuid'); + const supersetDomain = params.get('supersetDomain'); + + if (!uuid || !supersetDomain) { + document.getElementById('error').style.display = 'block'; + document.getElementById('error').textContent = + 'Missing required query params: uuid, supersetDomain'; + return; + } + + const statusEl = document.getElementById('status'); + + // fetchGuestToken is injected by Playwright via page.exposeFunction() + // Falls back to window.__guestToken for simple/static token injection + async function fetchGuestToken() { + if (typeof window.__fetchGuestToken === 'function') { + statusEl.textContent = 'Fetching guest token...'; + const token = await window.__fetchGuestToken(); + statusEl.textContent = 'Guest token received, loading dashboard...'; + return token; + } + if (window.__guestToken) { + return window.__guestToken; + } + throw new Error('No guest token source available'); + } + + try { + // Parse optional UI config from query params + const uiConfig = {}; + if (params.get('hideTitle') === 'true') uiConfig.hideTitle = true; + if (params.get('hideTab') === 'true') uiConfig.hideTab = true; + if (params.get('hideChartControls') === 'true') uiConfig.hideChartControls = true; + + const dashboard = await supersetEmbeddedSdk.embedDashboard({ + id: uuid, + supersetDomain: supersetDomain, + mountPoint: document.getElementById('superset-container'), + fetchGuestToken: fetchGuestToken, + dashboardUiConfig: Object.keys(uiConfig).length > 0 ? uiConfig : undefined, + debug: params.get('debug') === 'true', + }); + + statusEl.textContent = 'Dashboard embedded successfully'; + // Expose dashboard API on window for Playwright assertions + window.__embeddedDashboard = dashboard; + } catch (err) { + document.getElementById('error').style.display = 'block'; + document.getElementById('error').textContent = 'Embed failed: ' + err.message; Review Comment: The `catch (err)` handler assumes `err` is an `Error` and uses `err.message`, but browser exceptions can be strings or other values. This can lead to `Embed failed: undefined` and lose the real error. Use a safer message extraction (e.g. `err instanceof Error ? err.message : String(err)`). ########## superset-frontend/playwright/tests/embedded/embedded-dashboard.spec.ts: ########## @@ -0,0 +1,350 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { test, expect, Browser, BrowserContext, Page } from '@playwright/test'; +import { createServer, IncomingMessage, ServerResponse, Server } from 'http'; +import { AddressInfo, Socket } from 'net'; +import { readFileSync, existsSync } from 'fs'; +import { join } from 'path'; +import { + apiEnableEmbedding, + getAccessToken, + getGuestToken, +} from '../../helpers/api/embedded'; +import { getDashboardBySlug } from '../../helpers/api/dashboard'; +import { EmbeddedPage } from '../../pages/EmbeddedPage'; + +/** + * Superset domain (Flask server) — set by CI or defaults to local dev + */ +const SUPERSET_DOMAIN = (() => { + const url = process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8088'; + return url.replace(/\/+$/, ''); +})(); + +const SUPERSET_BASE_URL = SUPERSET_DOMAIN.endsWith('/') + ? SUPERSET_DOMAIN + : `${SUPERSET_DOMAIN}/`; + +/** + * Path to the SDK bundle built from superset-embedded-sdk/ + */ +const SDK_BUNDLE_PATH = join( + __dirname, + '../../../../superset-embedded-sdk/bundle/index.js', +); + +/** + * Path to the embedded test app static files + */ +const EMBED_APP_DIR = join(__dirname, '../../embedded-app'); + +/** + * Create a minimal static file server for the embedded test app. + * Serves only a fixed allowlist of routes — the test app references just + * its index.html and the SDK bundle, so anything else is 404. + */ +const INDEX_HTML_PATH = join(EMBED_APP_DIR, 'index.html'); + +interface EmbedAppServer { + server: Server; + url: string; + close: () => Promise<void>; +} + +/** + * Start the static test app on an OS-assigned ephemeral port. Tracks open + * sockets so close() doesn't hang on iframe keep-alive connections, and so + * different workers/retries never collide on a fixed port. + */ +async function startEmbedAppServer(): Promise<EmbedAppServer> { + const sockets = new Set<Socket>(); + const server = createServer((req: IncomingMessage, res: ServerResponse) => { + const urlPath = req.url?.split('?')[0] || '/'; + + if (urlPath === '/sdk/index.js') { + if (!existsSync(SDK_BUNDLE_PATH)) { + res.writeHead(404); + res.end( + 'SDK bundle not found. Run: cd superset-embedded-sdk && npm ci && npm run build', + ); + return; + } + res.writeHead(200, { 'Content-Type': 'text/javascript' }); + res.end(readFileSync(SDK_BUNDLE_PATH)); + return; + } + + if (urlPath === '/' || urlPath === '/index.html') { + res.writeHead(200, { 'Content-Type': 'text/html' }); + res.end(readFileSync(INDEX_HTML_PATH)); + return; + } + + res.writeHead(404); + res.end('Not found'); + }); + + server.on('connection', socket => { + sockets.add(socket); + socket.once('close', () => sockets.delete(socket)); + }); + + await new Promise<void>((resolve, reject) => { + server.once('error', reject); + server.listen(0, '127.0.0.1', () => { + server.removeListener('error', reject); + resolve(); + }); + }); + + const address = server.address() as AddressInfo; + const url = `http://127.0.0.1:${address.port}`; + + return { + server, + url, + close: () => + new Promise<void>(resolve => { + for (const socket of sockets) socket.destroy(); + sockets.clear(); + server.close(() => resolve()); + }), + }; +} + +/** + * Create a browser context authenticated as admin for API-only work + * (enabling embedding, restoring config). Caller is responsible for closing. + */ +function createAdminContext(browser: Browser): Promise<BrowserContext> { + return browser.newContext({ + storageState: 'playwright/.auth/user.json', + baseURL: SUPERSET_BASE_URL, + }); +} + +// ─── Test Suite ──────────────────────────────────────────────────────────── + +// Describe wrapper is needed for shared server state and serial execution: +// all tests share a static file server and must not run in parallel. +test.describe('Embedded Dashboard E2E', () => { + test.describe.configure({ mode: 'serial' }); + + // The full embedded chain (login → guest token → iframe → dashboard render + // → chart render) routinely exceeds the 30s default on cold CI starts. + test.setTimeout(60000); + + let appServer: EmbedAppServer; + let appUrl: string; + let accessToken: string; + let embedUuid: string; + let dashboardId: number; + + /** + * Set up a page to render the default embedded dashboard. + * Tests that need a different UUID or UI config should not use this helper. + */ + async function setupEmbeddedPage(page: Page): Promise<EmbeddedPage> { + const embeddedPage = new EmbeddedPage(page); + await embeddedPage.exposeTokenFetcher(async () => + getGuestToken(page, dashboardId, { accessToken }), + ); + await embeddedPage.goto({ + appUrl, + uuid: embedUuid, + supersetDomain: SUPERSET_DOMAIN, + }); + await embeddedPage.waitForIframe(); + await embeddedPage.waitForDashboardContent(); + return embeddedPage; + } + + test.beforeAll(async ({ browser }) => { + // Skip all tests if the SDK bundle hasn't been built + test.skip( + !existsSync(SDK_BUNDLE_PATH), + 'Embedded SDK bundle not found. Build it with: cd superset-embedded-sdk && npm ci && npm run build', + ); + + appServer = await startEmbedAppServer(); + appUrl = appServer.url; + + // Use a fresh context with auth to set up test data via API + const context = await createAdminContext(browser); + const setupPage = await context.newPage(); + + try { + const dashboard = await getDashboardBySlug(setupPage, 'world_health'); + if (!dashboard) { + throw new Error( + 'Dashboard "world_health" not found. Ensure load_examples ran in CI setup.', + ); + } + dashboardId = dashboard.id; + + // Enable embedding on the dashboard (empty allowed_domains = allow all) + const embedded = await apiEnableEmbedding(setupPage, dashboardId); + embedUuid = embedded.uuid; + + // Cache the JWT access token so tests don't re-login per guest token. + accessToken = await getAccessToken(setupPage); + } finally { + await context.close(); + } + }); + + test.afterAll(async ({ browser }) => { + // Defensive restore in case the allowed_domains test failed mid-flight. + if (dashboardId !== undefined) { + const context = await createAdminContext(browser); + try { + const setupPage = await context.newPage(); + await apiEnableEmbedding(setupPage, dashboardId, []); + } catch { + // Best-effort cleanup — never fail teardown. + } finally { + await context.close(); + } + } + + if (appServer) await appServer.close(); + }); + + test('dashboard renders in embedded iframe', async ({ page }) => { + const embeddedPage = await setupEmbeddedPage(page); + + // Verify the iframe src points to Superset's /embedded/ endpoint + await expect( + page.locator('iframe[title="Embedded Dashboard"]'), + ).toHaveAttribute('src', new RegExp(`/embedded/${embedUuid}`)); + + // Verify no errors in the test app + const error = await embeddedPage.getError(); + expect(error).toBe(''); + + // Baseline: title should be visible when hideTitle is not set + await expect(embeddedPage.titleLocator).toBeVisible(); + }); Review Comment: PR description mentions a native-filters-in-embedded-mode test case, but this spec currently covers rendering, UI config, allowed_domains, and chart/data access only. If native filters are intended to be validated here, add an explicit test exercising filter interaction inside the embedded iframe (or update the PR description to match the implemented coverage). ########## superset-frontend/playwright/helpers/api/embedded.ts: ########## @@ -0,0 +1,136 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Page } from '@playwright/test'; +import { apiPost, apiPut, getCsrfToken } from './requests'; +import { ENDPOINTS as DASHBOARD_ENDPOINTS } from './dashboard'; + +export const ENDPOINTS = { + SECURITY_LOGIN: 'api/v1/security/login', + GUEST_TOKEN: 'api/v1/security/guest_token/', +} as const; + +export interface EmbeddedConfig { + uuid: string; + allowed_domains: string[]; + dashboard_id: number; +} + +/** + * Enable embedding on a dashboard and return the embedded UUID. + * Uses PUT (upsert) to preserve UUID across repeated calls. + * @param page - Playwright page instance (provides authentication context) + * @param dashboardIdOrSlug - Numeric dashboard id or slug + * @param allowedDomains - Domains allowed to embed; empty array allows all + * @returns Embedded config with UUID, allowed_domains, and dashboard_id + */ +export async function apiEnableEmbedding( + page: Page, + dashboardIdOrSlug: number | string, + allowedDomains: string[] = [], +): Promise<EmbeddedConfig> { + const response = await apiPut( + page, + `${DASHBOARD_ENDPOINTS.DASHBOARD}${dashboardIdOrSlug}/embedded`, + { allowed_domains: allowedDomains }, + ); + const body = await response.json(); + return body.result as EmbeddedConfig; +} + +/** + * Login as admin and return the JWT access token used by the guest_token + * endpoint. Cache the result at suite level (`beforeAll`) and pass it into + * `getGuestToken` to avoid a fresh login on every test. + * + * Defaults match `playwright/global-setup.ts` so credentials come from one + * source (env vars). Overrides via `options` win. + */ +export async function getAccessToken( + page: Page, + options?: { username?: string; password?: string }, +): Promise<string> { + const username = + options?.username ?? process.env.PLAYWRIGHT_ADMIN_USERNAME ?? 'admin'; + const password = + options?.password ?? process.env.PLAYWRIGHT_ADMIN_PASSWORD ?? 'general'; + const loginResponse = await apiPost( + page, + ENDPOINTS.SECURITY_LOGIN, + { + username, + password, + provider: 'db', + refresh: true, + }, + { allowMissingCsrf: true }, + ); + const loginBody = await loginResponse.json(); + return loginBody.access_token; +} + +/** + * Get a guest token for an embedded dashboard. + * If `accessToken` is provided, the login round-trip is skipped — preferred + * for tests that fetch many tokens from a single suite. + * @returns Signed guest token string + */ +export async function getGuestToken( + page: Page, + dashboardId: number | string, + options?: { + accessToken?: string; + username?: string; + password?: string; + rls?: Array<{ dataset: number; clause: string }>; + }, +): Promise<string> { + const accessToken = + options?.accessToken ?? + (await getAccessToken(page, { + username: options?.username, + password: options?.password, + })); + const rls = options?.rls ?? []; + + // The guest_token endpoint authenticates via JWT Bearer, but if the + // request also carries a session cookie (which page.request inherits from + // storageState), Flask-WTF still requires a matching X-CSRFToken. Send it + // unconditionally so this works whether the caller is authenticated via + // session, JWT, or both. + const { token: csrfToken } = await getCsrfToken(page); + const guestResponse = await page.request.post(ENDPOINTS.GUEST_TOKEN, { + data: { + user: { + username: 'embedded_test_user', + first_name: 'Embedded', + last_name: 'TestUser', + }, + resources: [{ type: 'dashboard', id: String(dashboardId) }], + rls, + }, + headers: { + 'Content-Type': 'application/json', + Authorization: `Bearer ${accessToken}`, + ...(csrfToken ? { 'X-CSRFToken': csrfToken } : {}), + }, + }); Review Comment: `getGuestToken()` bypasses the shared `apiPost()` helper and (1) does not set `failOnStatusCode`, so a 4xx/5xx response will silently return `undefined` token and fail later with a less actionable error, and (2) when including `X-CSRFToken` it does not include the matching `Referer` header that other mutation helpers add for Flask-WTF CSRF protection. Consider reusing `apiPost()` with an Authorization header, or explicitly set `failOnStatusCode: true` and include the appropriate `Referer` when sending a CSRF token. -- 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]
