This is an automated email from the ASF dual-hosted git repository. jli pushed a commit to branch fix-cannot-view-dashboard-if-no-theme-role in repository https://gitbox.apache.org/repos/asf/superset.git
commit 2453e1c7fc184928f8793fce9ad11b61bcfa4177 Author: Joe Li <[email protected]> AuthorDate: Tue Mar 3 23:04:30 2026 -0800 fix(dashboard): use inline theme data to prevent 403 for non-admin users CrudThemeProvider previously fetched theme by ID via a separate API call requiring ("can_read", "Theme") permission, causing infinite loading for non-admin users viewing themed dashboards. Now uses the full theme object already included in the dashboard API response, creating the theme synchronously via Theme.fromConfig(). Falls back to the global theme on invalid data instead of spinning. Known limitation: in-session theme switching from the Dashboard Properties modal does not update live. This was also broken before (the re-fetch would 403 for non-admin users). Deferred to follow-up. Co-Authored-By: Claude Opus 4.6 <[email protected]> --- .../src/components/CrudThemeProvider.test.tsx | 132 +++++++++++++++ .../src/components/CrudThemeProvider.tsx | 63 +++----- .../dashboard/containers/DashboardPage.test.tsx | 179 +++++++++++++++++++++ .../src/dashboard/containers/DashboardPage.tsx | 11 +- 4 files changed, 332 insertions(+), 53 deletions(-) diff --git a/superset-frontend/src/components/CrudThemeProvider.test.tsx b/superset-frontend/src/components/CrudThemeProvider.test.tsx new file mode 100644 index 00000000000..0a15652723e --- /dev/null +++ b/superset-frontend/src/components/CrudThemeProvider.test.tsx @@ -0,0 +1,132 @@ +/** + * 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'; +import { Theme } from '@apache-superset/core/ui'; +import CrudThemeProvider from './CrudThemeProvider'; + +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); +}); + +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); +}); + +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); +}); + +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(); +}); diff --git a/superset-frontend/src/components/CrudThemeProvider.tsx b/superset-frontend/src/components/CrudThemeProvider.tsx index 4ecc1c68ce2..d3c757804bf 100644 --- a/superset-frontend/src/components/CrudThemeProvider.tsx +++ b/superset-frontend/src/components/CrudThemeProvider.tsx @@ -16,68 +16,45 @@ * specific language governing permissions and limitations * under the License. */ -import { ReactNode, useEffect, useState } from 'react'; -import { useThemeContext } from 'src/theme/ThemeProvider'; +import { ReactNode, useMemo } from 'react'; +import { logging } from '@apache-superset/core'; import { Theme } from '@apache-superset/core/ui'; -import { Loading } from '@superset-ui/core/components'; +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. 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); - - 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); - } - }; - - loadDashboardTheme(); - } else { - setDashboardTheme(null); + const dashboardTheme = useMemo(() => { + if (!theme?.json_data) { + return null; } - }, [themeId, globalThemeContext]); - - // If no themeId, just render children (they use global theme) - if (!themeId) { - return <>{children}</>; - } + try { + const themeConfig = JSON.parse(theme.json_data); + return Theme.fromConfig(themeConfig); + } catch (error) { + logging.warn('Failed to load dashboard theme:', error); + return null; + } + }, [theme?.json_data]); - // If themeId exists, but theme is not loaded yet, return null to prevent re-mounting children if (!dashboardTheme) { - return <Loading />; + return <>{children}</>; } - // Render children with the dashboard theme provider from controller return ( <dashboardTheme.SupersetThemeProvider> {children} </dashboardTheme.SupersetThemeProvider> ); } -// test comment diff --git a/superset-frontend/src/dashboard/containers/DashboardPage.test.tsx b/superset-frontend/src/dashboard/containers/DashboardPage.test.tsx new file mode 100644 index 00000000000..e4b3310745f --- /dev/null +++ b/superset-frontend/src/dashboard/containers/DashboardPage.test.tsx @@ -0,0 +1,179 @@ +/** + * 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 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; + +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 () => { + 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(), + ); +}); diff --git a/superset-frontend/src/dashboard/containers/DashboardPage.tsx b/superset-frontend/src/dashboard/containers/DashboardPage.tsx index 6c9198ffc5f..6c3f1ab45b6 100644 --- a/superset-frontend/src/dashboard/containers/DashboardPage.tsx +++ b/superset-frontend/src/dashboard/containers/DashboardPage.tsx @@ -120,9 +120,6 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => { ({ dashboardInfo }) => dashboardInfo && Object.keys(dashboardInfo).length > 0, ); - const dashboardTheme = useSelector( - (state: RootState) => state.dashboardInfo.theme, - ); const { addDangerToast } = useToasts(); const { result: dashboard, error: dashboardApiError } = useDashboard(idOrSlug); @@ -294,13 +291,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => { <> <SyncDashboardState dashboardPageId={dashboardPageId} /> <DashboardPageIdContext.Provider value={dashboardPageId}> - <CrudThemeProvider - themeId={ - dashboardTheme !== undefined - ? dashboardTheme?.id - : dashboard?.theme?.id - } - > + <CrudThemeProvider theme={dashboard?.theme}> <AutoRefreshProvider> <DashboardContainer activeFilters={activeFilters as ActiveFilters}
