fix(dashboard): use inline theme data to prevent 403 for non-admin users (#38384)

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Joe Li
2026-03-18 16:17:03 -07:00
committed by GitHub
parent 3edf75123a
commit 03de7e1ec6
16 changed files with 1136 additions and 71 deletions

1
.gitignore vendored
View File

@@ -133,6 +133,7 @@ CLAUDE.local.md
PROJECT.md
.aider*
.claude_rc*
.claude/settings.local.json
.env.local
oxc-custom-build/
*.code-workspace

View File

@@ -44,6 +44,7 @@ export interface DashboardCreatePayload {
css?: string;
json_metadata?: string;
published?: boolean;
theme_id?: number;
}
/**

View File

@@ -0,0 +1,62 @@
/**
* 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, APIResponse } from '@playwright/test';
import { apiPost, apiDelete, ApiRequestOptions } from './requests';
export const ENDPOINTS = {
THEME: 'api/v1/theme/',
} as const;
/**
* TypeScript interface for theme creation API payload.
* Both fields are required (ThemePostSchema).
*/
export interface ThemeCreatePayload {
theme_name: string;
json_data: string;
}
/**
* POST request to create a theme
* @param page - Playwright page instance (provides authentication context)
* @param requestBody - Theme configuration object
* @returns API response from theme creation
*/
export async function apiPostTheme(
page: Page,
requestBody: ThemeCreatePayload,
): Promise<APIResponse> {
return apiPost(page, ENDPOINTS.THEME, requestBody);
}
/**
* DELETE request to remove a theme
* @param page - Playwright page instance (provides authentication context)
* @param themeId - ID of the theme to delete
* @param options - Optional request options
* @returns API response from theme deletion
*/
export async function apiDeleteTheme(
page: Page,
themeId: number,
options?: ApiRequestOptions,
): Promise<APIResponse> {
return apiDelete(page, `${ENDPOINTS.THEME}${themeId}`, options);
}

View File

@@ -21,6 +21,7 @@ import { test as base } from '@playwright/test';
import { apiDeleteChart } from '../api/chart';
import { apiDeleteDashboard } from '../api/dashboard';
import { apiDeleteDataset } from '../api/dataset';
import { apiDeleteTheme } from '../api/theme';
import { apiDeleteDatabase } from '../api/database';
/**
@@ -31,6 +32,7 @@ export interface TestAssets {
trackDashboard(id: number): void;
trackChart(id: number): void;
trackDataset(id: number): void;
trackTheme(id: number): void;
trackDatabase(id: number): void;
}
@@ -42,16 +44,18 @@ export const test = base.extend<{ testAssets: TestAssets }>({
const dashboardIds = new Set<number>();
const chartIds = new Set<number>();
const datasetIds = new Set<number>();
const themeIds = new Set<number>();
const databaseIds = new Set<number>();
await use({
trackDashboard: id => dashboardIds.add(id),
trackChart: id => chartIds.add(id),
trackDataset: id => datasetIds.add(id),
trackTheme: id => themeIds.add(id),
trackDatabase: id => databaseIds.add(id),
});
// Cleanup order: dashboards → charts → datasets → databases (respects FK dependencies)
// Cleanup order: dashboards → charts → datasets → themes → databases (respects FK dependencies)
// Use failOnStatusCode: false to avoid throwing on 404 (resource already deleted by test)
// Warn on unexpected status codes (401/403/500) that may indicate leaked state
await Promise.all(
@@ -105,6 +109,21 @@ export const test = base.extend<{ testAssets: TestAssets }>({
}),
),
);
await Promise.all(
[...themeIds].map(id =>
apiDeleteTheme(page, id, { failOnStatusCode: false })
.then(response => {
if (!EXPECTED_CLEANUP_STATUSES.has(response.status())) {
console.warn(
`[testAssets] Unexpected status ${response.status()} cleaning up theme ${id}`,
);
}
})
.catch(error => {
console.warn(`[testAssets] Failed to cleanup theme ${id}:`, error);
}),
),
);
await Promise.all(
[...databaseIds].map(id =>
apiDeleteDatabase(page, id, { failOnStatusCode: false })

View File

@@ -0,0 +1,152 @@
/**
* 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 } from '@playwright/test';
import { AuthPage } from '../../../pages/AuthPage';
import { DashboardPage } from '../../../pages/DashboardPage';
import { apiPostTheme, apiDeleteTheme } from '../../../helpers/api/theme';
import {
apiPostDashboard,
apiPutDashboard,
apiDeleteDashboard,
} from '../../../helpers/api/dashboard';
import { apiGet } from '../../../helpers/api/requests';
import { TIMEOUT } from '../../../utils/constants';
/**
* Dashboard Theme E2E tests.
*
* Prerequisites:
* - Superset running with example data loaded
* - Admin user authenticated (via global-setup)
* - Non-admin test user created by `superset load-test-users`
*
* Credentials are configurable via environment variables:
* - PLAYWRIGHT_NONADMIN_USERNAME (default: 'gamma')
* - PLAYWRIGHT_NONADMIN_PASSWORD (default: 'general')
*/
const NONADMIN_USERNAME = process.env.PLAYWRIGHT_NONADMIN_USERNAME || 'gamma';
const NONADMIN_PASSWORD = process.env.PLAYWRIGHT_NONADMIN_PASSWORD || 'general';
// Clear storageState so the default page fixture starts unauthenticated.
// Admin API calls use an explicit admin context with saved auth instead.
test.use({ storageState: { cookies: [], origins: [] } });
test('non-admin user can view a themed dashboard without 403 or infinite spinner', async ({
page,
browser,
}) => {
test.setTimeout(60_000);
// --- ADMIN SETUP (explicit admin context with saved auth) ---
const adminContext = await browser.newContext({
baseURL: test.info().project.use.baseURL,
storageState: 'playwright/.auth/user.json',
});
const adminPage = await adminContext.newPage();
let themeId: number | undefined;
let dashboardId: number | undefined;
try {
// 1. Create a theme
const themeRes = await apiPostTheme(adminPage, {
theme_name: `e2e_theme_test_${Date.now()}`,
json_data: '{}',
});
expect(themeRes.ok()).toBe(true);
const themeBody = await themeRes.json();
themeId = themeBody.id;
expect(themeId).toBeTruthy();
// 2. Create a published dashboard with the theme assigned
const dashRes = await apiPostDashboard(adminPage, {
dashboard_title: `E2E Theme Test ${Date.now()}`,
published: true,
theme_id: themeId,
});
expect(dashRes.ok()).toBe(true);
const dashBody = await dashRes.json();
dashboardId = dashBody.id;
expect(dashboardId).toBeTruthy();
// 3. Grant non-admin user access by adding them as a dashboard owner
const usersRes = await apiGet(
adminPage,
`api/v1/dashboard/related/owners?q=(filter:'${NONADMIN_USERNAME}')`,
);
expect(usersRes.ok()).toBe(true);
const usersBody = await usersRes.json();
const gammaUserId = usersBody.result?.[0]?.value;
expect(gammaUserId).toBeTruthy();
const putRes = await apiPutDashboard(adminPage, dashboardId!, {
owners: [gammaUserId],
});
expect(putRes.ok()).toBe(true);
// --- NON-ADMIN USER PHASE (page has no cached auth via test.use) ---
// 4. Instrument network: track any /api/v1/theme/ requests and 403 responses
const themeApiRequests: string[] = [];
const forbiddenResponses: string[] = [];
page.on('response', response => {
const url = response.url();
if (url.includes('/api/v1/theme/')) {
themeApiRequests.push(url);
}
if (response.status() === 403 && url.includes('/api/v1/theme/')) {
forbiddenResponses.push(url);
}
});
// 5. Login as non-admin user
const authPage = new AuthPage(page);
await authPage.goto();
await authPage.waitForLoginForm();
await authPage.loginWithCredentials(NONADMIN_USERNAME, NONADMIN_PASSWORD);
await authPage.waitForLoginSuccess();
// 6. Navigate to the themed dashboard
const dashboardPage = new DashboardPage(page);
await dashboardPage.gotoById(dashboardId!);
// 7. Assert dashboard fully loads (not stuck on infinite spinner)
await dashboardPage.waitForLoad({ timeout: TIMEOUT.PAGE_LOAD });
await dashboardPage.waitForChartsToLoad();
// 8. Assert no /api/v1/theme/ requests were made (theme data comes from dashboard response)
expect(themeApiRequests).toHaveLength(0);
// Assert no 403 responses on /api/v1/theme/ (scoped to avoid login/unrelated 403 noise)
expect(forbiddenResponses).toHaveLength(0);
} finally {
// Cleanup: delete test resources using admin context
if (dashboardId) {
await apiDeleteDashboard(adminPage, dashboardId, {
failOnStatusCode: false,
}).catch(() => {});
}
if (themeId) {
await apiDeleteTheme(adminPage, themeId, {
failOnStatusCode: false,
}).catch(() => {});
}
await adminContext.close();
}
});

View File

@@ -0,0 +1,326 @@
/**
* 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 { type ReactNode } from 'react';
import { render, screen } from 'spec/helpers/testing-library';
import { logging } from '@apache-superset/core/utils';
import {
Theme,
normalizeThemeConfig,
isThemeConfigDark,
} from '@apache-superset/core/theme';
import getBootstrapData from 'src/utils/getBootstrapData';
import CrudThemeProvider from './CrudThemeProvider';
jest.mock('@apache-superset/core/theme', () => ({
...jest.requireActual('@apache-superset/core/theme'),
normalizeThemeConfig: jest.fn((config: unknown) => config),
isThemeConfigDark: jest.fn(() => false),
}));
jest.mock('src/utils/getBootstrapData', () => ({
__esModule: true,
default: jest.fn(() => ({
common: { theme: { default: {}, dark: {} } },
})),
}));
const mockGetBootstrapData = getBootstrapData as jest.MockedFunction<
typeof getBootstrapData
>;
const mockNormalizeThemeConfig = normalizeThemeConfig as jest.MockedFunction<
typeof normalizeThemeConfig
>;
const mockIsThemeConfigDark = isThemeConfigDark as jest.MockedFunction<
typeof isThemeConfigDark
>;
type BootstrapData = ReturnType<typeof getBootstrapData>;
type BootstrapThemes = BootstrapData['common']['theme'];
function mockBootstrap(themes: Partial<BootstrapThemes> = {}): BootstrapData {
return {
common: {
theme: { default: {}, dark: {}, ...themes },
},
} as unknown as BootstrapData;
}
const MockSupersetThemeProvider = ({ children }: { children: ReactNode }) => (
<div data-test="dashboard-theme-provider">{children}</div>
);
beforeEach(() => {
jest.restoreAllMocks();
jest.spyOn(Theme, 'fromConfig').mockReturnValue({
SupersetThemeProvider: MockSupersetThemeProvider,
} as unknown as Theme);
mockNormalizeThemeConfig.mockImplementation(
config => config as ReturnType<typeof normalizeThemeConfig>,
);
mockIsThemeConfigDark.mockReturnValue(false);
mockGetBootstrapData.mockReturnValue(mockBootstrap());
// Clean up font style elements from previous tests
document
.querySelectorAll('style[data-superset-fonts]')
.forEach(el => el.remove());
});
test('renders children directly when no theme is provided', () => {
render(
<CrudThemeProvider>
<div>Dashboard Content</div>
</CrudThemeProvider>,
);
expect(screen.getByText('Dashboard Content')).toBeInTheDocument();
expect(
screen.queryByTestId('dashboard-theme-provider'),
).not.toBeInTheDocument();
expect(Theme.fromConfig).not.toHaveBeenCalled();
});
test('renders children directly when theme is null', () => {
render(
<CrudThemeProvider theme={null}>
<div>Dashboard Content</div>
</CrudThemeProvider>,
);
expect(screen.getByText('Dashboard Content')).toBeInTheDocument();
expect(
screen.queryByTestId('dashboard-theme-provider'),
).not.toBeInTheDocument();
expect(Theme.fromConfig).not.toHaveBeenCalled();
});
test('wraps children with SupersetThemeProvider when valid theme data is provided', () => {
const themeConfig = { token: { colorPrimary: '#ff0000' } };
render(
<CrudThemeProvider
theme={{
id: 1,
theme_name: 'Custom Theme',
json_data: JSON.stringify(themeConfig),
}}
>
<div>Dashboard Content</div>
</CrudThemeProvider>,
);
expect(screen.getByText('Dashboard Content')).toBeInTheDocument();
expect(screen.getByTestId('dashboard-theme-provider')).toBeInTheDocument();
expect(Theme.fromConfig).toHaveBeenCalledWith(themeConfig, expect.anything());
});
test('creates theme from inline json_data via Theme.fromConfig', () => {
const themeConfig = { token: { colorPrimary: '#1677ff' } };
render(
<CrudThemeProvider
theme={{
id: 42,
theme_name: 'Branded',
json_data: JSON.stringify(themeConfig),
}}
>
<div>Dashboard Content</div>
</CrudThemeProvider>,
);
expect(screen.getByText('Dashboard Content')).toBeInTheDocument();
// No API call can happen — CrudThemeProvider has no fetch/SupersetClient imports.
// Theme is created synchronously from the inline json_data prop.
expect(Theme.fromConfig).toHaveBeenCalledWith(themeConfig, expect.anything());
});
test('falls back to rendering children without theme wrapper when json_data is invalid JSON', () => {
jest.spyOn(logging, 'warn').mockImplementation();
render(
<CrudThemeProvider
theme={{ id: 1, theme_name: 'Bad Theme', json_data: 'not-valid-json' }}
>
<div>Dashboard Content</div>
</CrudThemeProvider>,
);
expect(screen.getByText('Dashboard Content')).toBeInTheDocument();
expect(
screen.queryByTestId('dashboard-theme-provider'),
).not.toBeInTheDocument();
expect(logging.warn).toHaveBeenCalled();
});
test('falls back to rendering children without theme wrapper when Theme.fromConfig throws', () => {
(Theme.fromConfig as jest.Mock).mockImplementation(() => {
throw new Error('Invalid theme config');
});
jest.spyOn(logging, 'warn').mockImplementation();
render(
<CrudThemeProvider
theme={{ id: 1, theme_name: 'Bad Theme', json_data: '{}' }}
>
<div>Dashboard Content</div>
</CrudThemeProvider>,
);
expect(screen.getByText('Dashboard Content')).toBeInTheDocument();
expect(
screen.queryByTestId('dashboard-theme-provider'),
).not.toBeInTheDocument();
expect(logging.warn).toHaveBeenCalled();
});
test('merges dashboard theme with default base theme from bootstrap data', () => {
const bootstrapDefault = { token: { colorPrimary: '#base-default' } };
mockGetBootstrapData.mockReturnValue(
mockBootstrap({ default: bootstrapDefault }),
);
mockIsThemeConfigDark.mockReturnValue(false);
const themeConfig = { token: { colorPrimary: '#custom' } };
render(
<CrudThemeProvider
theme={{
id: 1,
theme_name: 'Custom Theme',
json_data: JSON.stringify(themeConfig),
}}
>
<div>Dashboard Content</div>
</CrudThemeProvider>,
);
// Theme.fromConfig should be called with TWO args: normalized config + base theme
expect(Theme.fromConfig).toHaveBeenCalledWith(
expect.objectContaining({
token: expect.objectContaining({ colorPrimary: '#custom' }),
}),
bootstrapDefault,
);
});
test('uses dark base theme when dashboard theme config is dark', () => {
const bootstrapDark = {
algorithm: 'dark',
token: { colorPrimary: '#base-dark' },
};
mockGetBootstrapData.mockReturnValue(mockBootstrap({ dark: bootstrapDark }));
mockIsThemeConfigDark.mockReturnValue(true);
const themeConfig = {
algorithm: 'dark',
token: { colorPrimary: '#custom-dark' },
};
render(
<CrudThemeProvider
theme={{
id: 1,
theme_name: 'Dark Theme',
json_data: JSON.stringify(themeConfig),
}}
>
<div>Dashboard Content</div>
</CrudThemeProvider>,
);
expect(Theme.fromConfig).toHaveBeenCalledWith(themeConfig, bootstrapDark);
});
test('injects font URLs as CSS @import rules', () => {
const fontUrl = 'https://fonts.example.com/custom.css';
const themeConfig = {
token: { colorPrimary: '#ff0000', fontUrls: [fontUrl] },
};
render(
<CrudThemeProvider
theme={{
id: 1,
theme_name: 'Font Theme',
json_data: JSON.stringify(themeConfig),
}}
>
<div>Dashboard Content</div>
</CrudThemeProvider>,
);
const fontStyle = document.querySelector('style[data-superset-fonts]');
expect(fontStyle).not.toBeNull();
expect(fontStyle?.textContent).toContain(`@import url("${fontUrl}")`);
});
test('does not inject fonts when Theme.fromConfig throws even if fontUrls are present', () => {
(Theme.fromConfig as jest.Mock).mockImplementation(() => {
throw new Error('Invalid theme config');
});
jest.spyOn(logging, 'warn').mockImplementation();
const themeConfig = {
token: {
colorPrimary: '#ff0000',
fontUrls: ['https://fonts.example.com/custom.css'],
},
};
render(
<CrudThemeProvider
theme={{
id: 1,
theme_name: 'Bad Theme With Fonts',
json_data: JSON.stringify(themeConfig),
}}
>
<div>Dashboard Content</div>
</CrudThemeProvider>,
);
const fontStyle = document.querySelector('style[data-superset-fonts]');
expect(fontStyle).toBeNull();
expect(logging.warn).toHaveBeenCalled();
});
test('ignores non-array fontUrls in theme config without throwing', () => {
const themeConfig = {
token: { colorPrimary: '#ff0000', fontUrls: 'not-an-array' },
};
render(
<CrudThemeProvider
theme={{
id: 1,
theme_name: 'Malformed Fonts',
json_data: JSON.stringify(themeConfig),
}}
>
<div>Dashboard Content</div>
</CrudThemeProvider>,
);
expect(screen.getByText('Dashboard Content')).toBeInTheDocument();
expect(screen.getByTestId('dashboard-theme-provider')).toBeInTheDocument();
const fontStyle = document.querySelector('style[data-superset-fonts]');
expect(fontStyle).toBeNull();
});
test('does not inject font style element when no fontUrls in config', () => {
const themeConfig = { token: { colorPrimary: '#ff0000' } };
render(
<CrudThemeProvider
theme={{
id: 1,
theme_name: 'No Fonts',
json_data: JSON.stringify(themeConfig),
}}
>
<div>Dashboard Content</div>
</CrudThemeProvider>,
);
const fontStyle = document.querySelector('style[data-superset-fonts]');
expect(fontStyle).toBeNull();
});

View File

@@ -16,68 +16,80 @@
* specific language governing permissions and limitations
* under the License.
*/
import { ReactNode, useEffect, useState } from 'react';
import { useThemeContext } from 'src/theme/ThemeProvider';
import { Theme } from '@apache-superset/core/theme';
import { Loading } from '@superset-ui/core/components';
import { ReactNode, useEffect, useMemo } from 'react';
import { logging } from '@apache-superset/core/utils';
import {
Theme,
normalizeThemeConfig,
isThemeConfigDark,
} from '@apache-superset/core/theme';
import getBootstrapData from 'src/utils/getBootstrapData';
import type { Dashboard } from 'src/types/Dashboard';
interface CrudThemeProviderProps {
children: ReactNode;
themeId?: number | null;
theme?: Dashboard['theme'];
}
/**
* CrudThemeProvider asks the ThemeController for a dashboard theme provider.
* Flow: Dashboard loads → asks controller → controller fetches theme
* returns provider → dashboard uses it.
*
* CRITICAL: This does NOT modify the global controller - it creates an isolated dashboard theme.
* CrudThemeProvider applies a dashboard-specific theme using theme data
* from the dashboard API response. Merges with the system's base theme
* (light or dark) and loads custom fonts. Falls back to the global theme
* if the theme data is missing or invalid.
*/
export default function CrudThemeProvider({
children,
themeId,
theme,
}: CrudThemeProviderProps) {
const globalThemeContext = useThemeContext();
const [dashboardTheme, setDashboardTheme] = useState<Theme | null>(null);
const { dashboardTheme, fontUrls } = useMemo(() => {
if (!theme?.json_data) {
return { dashboardTheme: null, fontUrls: undefined };
}
try {
const themeConfig = JSON.parse(theme.json_data);
const normalizedConfig = normalizeThemeConfig(themeConfig);
const isDark = isThemeConfigDark(normalizedConfig);
const {
common: { theme: bootstrapTheme },
} = getBootstrapData();
const baseTheme = isDark ? bootstrapTheme.dark : bootstrapTheme.default;
const createdTheme = Theme.fromConfig(
normalizedConfig,
baseTheme || undefined,
);
const rawUrls = themeConfig?.token?.fontUrls;
const urls = Array.isArray(rawUrls) ? (rawUrls as string[]) : undefined;
return { dashboardTheme: createdTheme, fontUrls: urls };
} catch (error) {
logging.warn('Failed to load dashboard theme:', error);
return { dashboardTheme: null, fontUrls: undefined };
}
}, [theme?.json_data]);
useEffect(() => {
if (themeId) {
// Ask the controller to create a SEPARATE dashboard theme provider
// This should NOT affect the global controller or navbar
const loadDashboardTheme = async () => {
try {
const dashboardThemeProvider =
await globalThemeContext.createDashboardThemeProvider(
String(themeId),
);
setDashboardTheme(dashboardThemeProvider);
} catch (error) {
console.error('Failed to load dashboard theme:', error);
setDashboardTheme(null);
}
};
if (!dashboardTheme || !fontUrls?.length) return undefined;
loadDashboardTheme();
} else {
setDashboardTheme(null);
}
}, [themeId, globalThemeContext]);
// JSON.stringify provides safe escaping to prevent CSS injection
const css = fontUrls
.map((url: string) => `@import url(${JSON.stringify(url)});`)
.join('\n');
const style = document.createElement('style');
style.setAttribute('data-superset-fonts', 'true');
style.textContent = css;
document.head.appendChild(style);
// If no themeId, just render children (they use global theme)
if (!themeId) {
return () => {
style.remove();
};
}, [dashboardTheme, fontUrls]);
if (!dashboardTheme) {
return <>{children}</>;
}
// If themeId exists, but theme is not loaded yet, return null to prevent re-mounting children
if (!dashboardTheme) {
return <Loading />;
}
// Render children with the dashboard theme provider from controller
return (
<dashboardTheme.SupersetThemeProvider>
{children}
</dashboardTheme.SupersetThemeProvider>
);
}
// test comment

View File

@@ -107,7 +107,7 @@ type DashboardPropertiesUpdate = {
owners?: Owner[];
roles?: Role[];
tags?: TagType[];
themeId?: number | null;
theme?: { id: number; theme_name: string; json_data: string } | null;
css?: string;
title?: string;
};
@@ -561,7 +561,10 @@ const Header = (): JSX.Element => {
owners: updates.owners,
roles: updates.roles,
tags: updates.tags,
theme_id: updates.themeId,
// Conditional spread: omit `theme` key entirely when undefined
// to prevent the reducer from overwriting the existing theme.
// `undefined` means "not changed" (e.g., theme not in fetched list).
...(updates.theme !== undefined && { theme: updates.theme }),
css: updates.css,
});
boundActionCreators.setUnsavedChanges(true);

View File

@@ -152,10 +152,12 @@ fetchMock.get('glob:*/api/v1/theme/*', {
{
id: 1,
theme_name: 'Test Theme 1',
json_data: '{"token":{"colorPrimary":"#ff0000"}}',
},
{
id: 2,
theme_name: 'Test Theme 2',
json_data: '{"token":{"colorPrimary":"#0000ff"}}',
},
],
},
@@ -417,6 +419,74 @@ describe('PropertiesModal', () => {
});
});
test('passes full theme object with json_data to onSubmit when theme is selected', async () => {
mockedIsFeatureEnabled.mockReturnValue(false);
const props = createProps();
props.onlyApply = true;
// Dashboard has theme id=1, which exists in the fetched themes list
const propsWithTheme = {
...props,
dashboardInfo: {
...dashboardInfo,
json_metadata: mockedJsonMetadata,
theme: { id: 1, theme_name: 'Test Theme 1' },
},
};
render(<PropertiesModal {...propsWithTheme} />, {
useRedux: true,
});
expect(
await screen.findByTestId('dashboard-edit-properties-form'),
).toBeInTheDocument();
userEvent.click(screen.getByRole('button', { name: 'Apply' }));
await waitFor(() => {
expect(props.onSubmit).toHaveBeenCalledTimes(1);
const submitCall = props.onSubmit.mock.calls[0][0];
// Full theme object (including json_data) should be passed, not just the ID
expect(submitCall.theme).toEqual({
id: 1,
theme_name: 'Test Theme 1',
json_data: '{"token":{"colorPrimary":"#ff0000"}}',
});
// themeId removed — derived from theme.id at the save callsite
expect(submitCall).not.toHaveProperty('themeId');
});
});
test('does not clear theme when selected theme is missing from fetched options', async () => {
mockedIsFeatureEnabled.mockReturnValue(false);
const props = createProps();
props.onlyApply = true;
// Dashboard has theme id=99, but the theme fetch returns ids 1 and 2
const propsWithDashboardInfo = {
...props,
dashboardInfo: {
...dashboardInfo,
json_metadata: mockedJsonMetadata,
theme: { id: 99, theme_name: 'Deleted Theme' },
},
};
render(<PropertiesModal {...propsWithDashboardInfo} />, {
useRedux: true,
});
expect(
await screen.findByTestId('dashboard-edit-properties-form'),
).toBeInTheDocument();
userEvent.click(screen.getByRole('button', { name: 'Apply' }));
await waitFor(() => {
expect(props.onSubmit).toHaveBeenCalledTimes(1);
const submitCall = props.onSubmit.mock.calls[0][0];
// theme should be undefined (not null) so Redux is not overwritten
expect(submitCall.theme).toBeUndefined();
// themeId removed — derived from theme.id at the save callsite
expect(submitCall).not.toHaveProperty('themeId');
});
});
test('Empty "Certified by" should clear "Certification details"', async () => {
const props = createProps();
const noCertifiedByProps = {

View File

@@ -138,6 +138,7 @@ const PropertiesModal = ({
Array<{
id: number;
theme_name: string;
json_data?: string;
}>
>([]);
const categoricalSchemeRegistry = getCategoricalSchemeRegistry();
@@ -385,7 +386,9 @@ const PropertiesModal = ({
colorNamespace,
certifiedBy,
certificationDetails,
themeId: selectedThemeId,
theme: selectedThemeId
? themes.find(t => t.id === selectedThemeId)
: null,
css: customCss,
...moreOnSubmitProps,
};
@@ -441,7 +444,7 @@ const PropertiesModal = ({
// Fetch themes (excluding system themes)
const themeQuery = rison.encode({
columns: ['id', 'theme_name', 'is_system'],
columns: ['id', 'theme_name', 'is_system', 'json_data'],
filters: [
{
col: 'is_system',

View File

@@ -69,6 +69,7 @@ const StyledSwitchContainer = styled.div`
interface Theme {
id: number;
theme_name: string;
json_data?: string;
}
interface CssTemplate {

View File

@@ -0,0 +1,256 @@
/**
* 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 type { ReactNode } from 'react';
import { Suspense } from 'react';
import { render, screen, waitFor } from 'spec/helpers/testing-library';
import {
useDashboard,
useDashboardCharts,
useDashboardDatasets,
} from 'src/hooks/apiResources';
import { SupersetClient } from '@superset-ui/core';
import CrudThemeProvider from 'src/components/CrudThemeProvider';
import DashboardPage from './DashboardPage';
const mockTheme = {
id: 42,
theme_name: 'Branded',
json_data: '{"token":{"colorPrimary":"#1677ff"}}',
};
const mockDashboard = {
id: 1,
slug: null,
url: '/superset/dashboard/1/',
dashboard_title: 'Test Dashboard',
thumbnail_url: null,
published: true,
css: null,
json_metadata: '{}',
position_json: '{}',
changed_by_name: 'admin',
changed_by: { id: 1, first_name: 'Admin', last_name: 'User' },
changed_on: '2024-01-01T00:00:00',
charts: [],
owners: [],
roles: [],
theme: mockTheme,
};
jest.mock('src/hooks/apiResources', () => ({
useDashboard: jest.fn(),
useDashboardCharts: jest.fn(),
useDashboardDatasets: jest.fn(),
}));
jest.mock('src/dashboard/actions/hydrate', () => ({
...jest.requireActual('src/dashboard/actions/hydrate'),
hydrateDashboard: jest.fn(() => ({ type: 'MOCK_HYDRATE' })),
}));
jest.mock('src/dashboard/actions/datasources', () => ({
...jest.requireActual('src/dashboard/actions/datasources'),
setDatasources: jest.fn(() => ({ type: 'MOCK_SET_DATASOURCES' })),
}));
jest.mock('src/dashboard/actions/dashboardState', () => ({
...jest.requireActual('src/dashboard/actions/dashboardState'),
setDatasetsStatus: jest.fn(() => ({ type: 'MOCK_SET_DATASETS_STATUS' })),
}));
jest.mock('src/components/CrudThemeProvider', () => ({
__esModule: true,
default: jest.fn(({ children }: { children: ReactNode }) => (
<div>{children}</div>
)),
}));
jest.mock('src/dashboard/components/DashboardBuilder/DashboardBuilder', () => ({
__esModule: true,
default: () => <div data-testid="dashboard-builder">DashboardBuilder</div>,
}));
jest.mock('src/dashboard/components/SyncDashboardState', () => ({
__esModule: true,
default: () => null,
getDashboardContextLocalStorage: () => ({}),
}));
jest.mock('src/dashboard/containers/Dashboard', () => ({
__esModule: true,
default: ({ children }: { children: ReactNode }) => <div>{children}</div>,
}));
jest.mock('src/components/MessageToasts/withToasts', () => ({
useToasts: () => ({ addDangerToast: jest.fn(), addSuccessToast: jest.fn() }),
}));
jest.mock('src/dashboard/util/injectCustomCss', () => ({
__esModule: true,
default: () => () => {},
}));
jest.mock('src/dashboard/util/activeAllDashboardFilters', () => ({
getAllActiveFilters: () => ({}),
getRelevantDataMask: () => ({}),
}));
jest.mock('src/dashboard/util/activeDashboardFilters', () => ({
getActiveFilters: () => ({}),
}));
jest.mock('src/utils/urlUtils', () => ({
getUrlParam: () => null,
}));
jest.mock('src/dashboard/components/nativeFilters/FilterBar/keyValue', () => ({
getFilterValue: jest.fn(),
getPermalinkValue: jest.fn(),
}));
jest.mock('src/dashboard/contexts/AutoRefreshContext', () => ({
AutoRefreshProvider: ({ children }: { children: ReactNode }) => (
<>{children}</>
),
}));
const mockUseDashboard = useDashboard as jest.Mock;
const mockUseDashboardCharts = useDashboardCharts as jest.Mock;
const mockUseDashboardDatasets = useDashboardDatasets as jest.Mock;
const MockCrudThemeProvider = CrudThemeProvider as unknown as jest.Mock;
afterEach(() => {
jest.restoreAllMocks();
});
beforeEach(() => {
jest.clearAllMocks();
mockUseDashboard.mockReturnValue({
result: mockDashboard,
error: null,
});
mockUseDashboardCharts.mockReturnValue({
result: [],
error: null,
});
mockUseDashboardDatasets.mockReturnValue({
result: [],
error: null,
status: 'complete',
});
});
test('passes full theme object from dashboard API response to CrudThemeProvider', async () => {
const clientGetSpy = jest.spyOn(SupersetClient, 'get');
render(
<Suspense fallback="loading">
<DashboardPage idOrSlug="1" />
</Suspense>,
{
useRedux: true,
useRouter: true,
initialState: {
dashboardInfo: { id: 1, metadata: {} },
dashboardState: { sliceIds: [] },
nativeFilters: { filters: {} },
dataMask: {},
},
},
);
await waitFor(() => {
expect(screen.queryByText('loading')).not.toBeInTheDocument();
});
expect(MockCrudThemeProvider).toHaveBeenCalledWith(
expect.objectContaining({ theme: mockTheme }),
expect.anything(),
);
// Regression guard: theme data comes from the dashboard API response,
// not a separate /api/v1/theme/:id fetch (which would 403 for non-admin users)
const themeCalls = clientGetSpy.mock.calls.filter(([{ endpoint }]) =>
endpoint?.startsWith('/api/v1/theme/'),
);
expect(themeCalls).toHaveLength(0);
});
test('uses theme from Redux dashboardInfo when it differs from API response (Properties modal update)', async () => {
const reduxTheme = {
id: 99,
theme_name: 'Updated Theme',
json_data: '{"token":{"colorPrimary":"#ff0000"}}',
};
render(
<Suspense fallback="loading">
<DashboardPage idOrSlug="1" />
</Suspense>,
{
useRedux: true,
useRouter: true,
initialState: {
dashboardInfo: { id: 1, metadata: {}, theme: reduxTheme },
dashboardState: { sliceIds: [] },
nativeFilters: { filters: {} },
dataMask: {},
},
},
);
await waitFor(() => {
expect(screen.queryByText('loading')).not.toBeInTheDocument();
});
// Redux theme should take priority over API response theme
expect(MockCrudThemeProvider).toHaveBeenCalledWith(
expect.objectContaining({ theme: reduxTheme }),
expect.anything(),
);
});
test('passes null theme when Redux dashboardInfo.theme is explicitly null (theme removed)', async () => {
render(
<Suspense fallback="loading">
<DashboardPage idOrSlug="1" />
</Suspense>,
{
useRedux: true,
useRouter: true,
initialState: {
dashboardInfo: { id: 1, metadata: {}, theme: null },
dashboardState: { sliceIds: [] },
nativeFilters: { filters: {} },
dataMask: {},
},
},
);
await waitFor(() => {
expect(screen.queryByText('loading')).not.toBeInTheDocument();
});
// When theme is explicitly null in Redux (removed via Properties modal),
// CrudThemeProvider should receive null
expect(MockCrudThemeProvider).toHaveBeenCalledWith(
expect.objectContaining({ theme: null }),
expect.anything(),
);
});

View File

@@ -120,7 +120,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
({ dashboardInfo }) =>
dashboardInfo && Object.keys(dashboardInfo).length > 0,
);
const dashboardTheme = useSelector(
const reduxTheme = useSelector(
(state: RootState) => state.dashboardInfo.theme,
);
const { addDangerToast } = useToasts();
@@ -295,11 +295,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
<SyncDashboardState dashboardPageId={dashboardPageId} />
<DashboardPageIdContext.Provider value={dashboardPageId}>
<CrudThemeProvider
themeId={
dashboardTheme !== undefined
? dashboardTheme?.id
: dashboard?.theme?.id
}
theme={reduxTheme !== undefined ? reduxTheme : dashboard?.theme}
>
<AutoRefreshProvider>
<DashboardContainer

View File

@@ -0,0 +1,156 @@
/**
* 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 {
DASHBOARD_INFO_UPDATED,
dashboardInfoChanged,
} from '../actions/dashboardInfo';
import type { DashboardInfo } from '../types';
import dashboardInfoReducer from './dashboardInfo';
const existingTheme = {
id: 99,
theme_name: 'Corporate Theme',
json_data: '{"token":{"colorPrimary":"#003366"}}',
};
const stateWithTheme = {
id: 1,
theme: existingTheme,
slug: 'test-dash',
metadata: {},
} as Partial<DashboardInfo>;
const stateWithScopes = {
id: 1,
metadata: {
native_filter_configuration: [
{
id: 'FILTER-1',
name: 'Region',
chartsInScope: [10, 20, 30],
tabsInScope: ['TAB-A'],
targets: [{ datasetId: 1 }],
},
{
id: 'FILTER-2',
name: 'Date Range',
chartsInScope: [40],
tabsInScope: [],
targets: [{ datasetId: 2 }],
},
],
chart_customization_config: [
{
id: 'CUSTOM-1',
chartsInScope: [100],
tabsInScope: ['TAB-X'],
},
],
},
} as unknown as Partial<DashboardInfo>;
test('preserves existing theme when DASHBOARD_INFO_UPDATED payload omits theme key', () => {
// Simulates the fixed Header behavior: conditional spread omits `theme`
// key when PropertiesModal returns theme: undefined (theme not in fetched list).
const action = dashboardInfoChanged({ slug: 'new-slug' });
const result = dashboardInfoReducer(stateWithTheme, action);
expect(result.theme).toBe(existingTheme);
expect(result.slug).toBe('new-slug');
// themeId derivation (Header line 299) should produce the original ID
const themeId = result.theme ? result.theme.id : null;
expect(themeId).toBe(99);
});
test('clears theme when DASHBOARD_INFO_UPDATED payload has theme: null', () => {
// Simulates intentional theme removal via Properties modal.
const action = dashboardInfoChanged({ theme: null });
const result = dashboardInfoReducer(stateWithTheme, action);
expect(result.theme).toBeNull();
const themeId = result.theme ? result.theme.id : null;
expect(themeId).toBeNull();
});
test('overwrites theme when DASHBOARD_INFO_UPDATED payload has theme: undefined', () => {
// Documents the dangerous behavior that the Header conditional spread prevents.
// If `theme: undefined` leaks into the action payload, the reducer WILL overwrite
// the existing theme — this is why Header must omit the key entirely.
const action = {
type: DASHBOARD_INFO_UPDATED,
newInfo: { theme: undefined },
};
const result = dashboardInfoReducer(stateWithTheme, action);
// theme is overwritten to undefined — save would compute themeId as null
expect(result.theme).toBeUndefined();
const themeId = result.theme ? result.theme.id : null;
expect(themeId).toBeNull();
});
test('preserves native_filter_configuration scopes during DASHBOARD_INFO_UPDATED metadata refresh', () => {
// Simulates the save flow: onUpdateSuccess → setDashboardMetadata(serverMetadata)
// → dashboardInfoChanged({ metadata: ... }) → DASHBOARD_INFO_UPDATED.
// The server response contains filters WITHOUT chartsInScope/tabsInScope (client-only).
const serverMetadata = {
native_filter_configuration: [
{ id: 'FILTER-1', name: 'Region (updated)', targets: [{ datasetId: 1 }] },
{ id: 'FILTER-2', name: 'Date Range', targets: [{ datasetId: 2 }] },
],
} as unknown as DashboardInfo['metadata'];
const action = dashboardInfoChanged({ metadata: serverMetadata });
const result = dashboardInfoReducer(stateWithScopes, action);
const filters = result.metadata?.native_filter_configuration;
expect(filters).toHaveLength(2);
// FILTER-1: server updated the name, scopes preserved from existing state
expect(filters![0].name).toBe('Region (updated)');
expect(filters![0].chartsInScope).toEqual([10, 20, 30]);
expect(filters![0].tabsInScope).toEqual(['TAB-A']);
// FILTER-2: unchanged, scopes preserved
expect(filters![1].chartsInScope).toEqual([40]);
expect(filters![1].tabsInScope).toEqual([]);
});
test('preserves chart_customization_config scopes during DASHBOARD_INFO_UPDATED metadata refresh', () => {
const serverMetadata = {
chart_customization_config: [{ id: 'CUSTOM-1' }],
} as unknown as DashboardInfo['metadata'];
const action = dashboardInfoChanged({ metadata: serverMetadata });
const result = dashboardInfoReducer(stateWithScopes, action);
const customizations = result.metadata?.chart_customization_config;
expect(customizations).toHaveLength(1);
expect(customizations![0].chartsInScope).toEqual([100]);
expect(customizations![0].tabsInScope).toEqual(['TAB-X']);
});
test('does not affect metadata when DASHBOARD_INFO_UPDATED has no metadata key', () => {
const action = dashboardInfoChanged({ slug: 'new-slug' });
const result = dashboardInfoReducer(stateWithScopes, action);
// Metadata untouched — same reference
expect(result.metadata).toBe(stateWithScopes.metadata);
});

View File

@@ -46,9 +46,7 @@ import {
interface DashboardInfoAction {
type: string;
newInfo?: Partial<DashboardInfo> & {
theme_id?: number | null;
};
newInfo?: Partial<DashboardInfo>;
filterBarOrientation?: FilterBarOrientation;
crossFiltersEnabled?: boolean;
chartCustomization?: (ChartCustomization | ChartCustomizationDivider)[];
@@ -124,22 +122,31 @@ export default function dashboardInfoReducer(
case DASHBOARD_INFO_UPDATED: {
const dashAction = action as DashboardInfoAction;
const newInfo = dashAction.newInfo || {};
const { theme_id: themeId, ...otherInfo } = newInfo;
const updatedState: DashboardInfoState = {
const incomingMeta = newInfo.metadata;
return {
...state,
...otherInfo,
...newInfo,
// Preserve client-only scope data (chartsInScope, tabsInScope) when
// metadata is refreshed from the server, matching HYDRATE_DASHBOARD.
...(incomingMeta && {
metadata: {
...incomingMeta,
...(incomingMeta.native_filter_configuration && {
native_filter_configuration: preserveScopes(
state.metadata?.native_filter_configuration,
incomingMeta.native_filter_configuration,
),
}),
...(incomingMeta.chart_customization_config && {
chart_customization_config: preserveScopes(
state.metadata?.chart_customization_config,
incomingMeta.chart_customization_config,
),
}),
},
}),
last_modified_time: Math.round(new Date().getTime() / 1000),
};
if (themeId !== undefined) {
if (themeId === null) {
updatedState.theme = null;
} else {
updatedState.theme = { id: themeId, name: `Theme ${themeId}` };
}
}
return updatedState;
}
case DASHBOARD_INFO_FILTERS_CHANGED: {
const dashAction = action as DashboardInfoAction;

View File

@@ -207,9 +207,9 @@ export type DashboardInfo = {
pendingChartCustomizations?: Record<string, ChartCustomization>;
theme?: {
id: number;
name: string;
theme_name: string;
json_data: string;
} | null;
theme_id?: number | null;
css?: string;
slug?: string;
last_modified_time: number;