From ffcc8a2c539f3e1b07a2135e87e49c74d38e2f01 Mon Sep 17 00:00:00 2001 From: "JUST.in DO IT" Date: Wed, 16 Jul 2025 13:36:03 -0700 Subject: [PATCH] fix(explore): Display missing dataset for denied access (#34129) (cherry picked from commit 96cb6030c800976fa73772de98df09ffab3a9af4) --- .../superset-ui-chart-controls/src/types.ts | 1 + .../DatasourceControl.test.tsx | 27 +++++ .../controls/DatasourceControl/index.jsx | 80 ++++++++----- .../DndFilterSelect.tsx | 5 +- superset-frontend/src/explore/fixtures.tsx | 4 +- superset-frontend/src/explore/types.ts | 2 +- .../src/pages/Chart/Chart.test.tsx | 91 +++++++++++++- superset-frontend/src/pages/Chart/index.tsx | 112 ++++++++++++------ superset/explore/api.py | 6 + superset/security/manager.py | 1 + 10 files changed, 258 insertions(+), 71 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts index 62ac9446885..f8058a3da86 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts @@ -90,6 +90,7 @@ export interface Dataset { database?: Record; normalize_columns?: boolean; always_filter_main_dttm?: boolean; + extra?: object | string; } export interface ControlPanelState { diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx index e84fb4500eb..d4676edb38c 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx @@ -474,3 +474,30 @@ test('should show missing dataset state', () => { ), ).toBeVisible(); }); + +test('should show forbidden dataset state', () => { + // @ts-ignore + delete window.location; + // @ts-ignore + window.location = { search: '?slice_id=152' }; + const error = { + error_type: 'TABLE_SECURITY_ACCESS_ERROR', + statusText: 'FORBIDDEN', + message: 'You do not have access to the following tables: blocked_table', + extra: { + datasource: 152, + datasource_name: 'forbidden dataset', + }, + }; + const props = createProps({ + datasource: { + ...fallbackExploreInitialData.dataset, + extra: { + error, + }, + }, + }); + render(, { useRedux: true, useRouter: true }); + expect(screen.getByText(error.message)).toBeInTheDocument(); + expect(screen.getByText(error.statusText)).toBeVisible(); +}); diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx index 4e5063b1e4a..80dc4e9c44d 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx @@ -47,6 +47,7 @@ import { isUserAdmin, } from 'src/dashboard/util/permissionUtils'; import ModalTrigger from 'src/components/ModalTrigger'; +import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace'; import ViewQueryModalFooter from 'src/explore/components/controls/ViewQueryModalFooter'; import ViewQuery from 'src/explore/components/controls/ViewQuery'; import { SaveDatasetModal } from 'src/SqlLab/components/SaveDatasetModal'; @@ -275,7 +276,17 @@ class DatasourceControl extends PureComponent { showSaveDatasetModal, } = this.state; const { datasource, onChange, theme } = this.props; - const isMissingDatasource = !datasource?.id; + let extra; + if (datasource?.extra) { + if (typeof datasource.extra === 'string') { + try { + extra = JSON.parse(datasource.extra); + } catch {} // eslint-disable-line no-empty + } else { + extra = datasource.extra; // eslint-disable-line prefer-destructuring + } + } + const isMissingDatasource = !datasource?.id || Boolean(extra?.error); let isMissingParams = false; if (isMissingDatasource) { const datasourceId = getUrlParam(URL_PARAMS.datasourceId); @@ -380,20 +391,10 @@ class DatasourceControl extends PureComponent { const { health_check_message: healthCheckMessage } = datasource; - let extra; - if (datasource?.extra) { - if (typeof datasource.extra === 'string') { - try { - extra = JSON.parse(datasource.extra); - } catch {} // eslint-disable-line no-empty - } else { - extra = datasource.extra; // eslint-disable-line prefer-destructuring - } - } - - const titleText = isMissingDatasource - ? t('Missing dataset') - : getDatasourceTitle(datasource); + const titleText = + isMissingDatasource && !datasource.name + ? t('Missing dataset') + : getDatasourceTitle(datasource); const tooltip = titleText; @@ -439,23 +440,38 @@ class DatasourceControl extends PureComponent { )} {isMissingDatasource && !isMissingParams && (
- - {t('The dataset linked to this chart may have been deleted.')} - - - } - /> + {extra?.error ? ( +
+ +
+ ) : ( + + {t( + 'The dataset linked to this chart may have been deleted.', + )} + + + } + /> + )}
)} {showEditDatasourceModal && ( diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx index d15289db8c4..d52beba64b6 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx @@ -90,7 +90,10 @@ const DndFilterSelect = (props: DndFilterSelectProps) => { let extra = {}; if (datasource?.extra) { try { - extra = JSON.parse(datasource.extra); + extra = + typeof datasource.extra === 'string' + ? JSON.parse(datasource.extra) + : datasource.extra; } catch {} // eslint-disable-line no-empty } return extra; diff --git a/superset-frontend/src/explore/fixtures.tsx b/superset-frontend/src/explore/fixtures.tsx index eb1d0d6d662..015e4470ea7 100644 --- a/superset-frontend/src/explore/fixtures.tsx +++ b/superset-frontend/src/explore/fixtures.tsx @@ -158,8 +158,8 @@ export const fallbackExploreInitialData: ExplorePageInitialData = { verbose_map: {}, main_dttm_col: '', owners: [], - datasource_name: 'missing_datasource', - name: 'missing_datasource', + datasource_name: '', + name: '', description: null, }, slice: null, diff --git a/superset-frontend/src/explore/types.ts b/superset-frontend/src/explore/types.ts index 708ffa9dfa5..f045c8e2951 100644 --- a/superset-frontend/src/explore/types.ts +++ b/superset-frontend/src/explore/types.ts @@ -69,7 +69,7 @@ export type Datasource = Dataset & { catalog?: string | null; schema?: string; is_sqllab_view?: boolean; - extra?: string; + extra?: string | object; }; export interface ExplorePageInitialData { diff --git a/superset-frontend/src/pages/Chart/Chart.test.tsx b/superset-frontend/src/pages/Chart/Chart.test.tsx index 2fe7599211f..87991d36cb2 100644 --- a/superset-frontend/src/pages/Chart/Chart.test.tsx +++ b/superset-frontend/src/pages/Chart/Chart.test.tsx @@ -30,7 +30,7 @@ import { LocalStorageKeys } from 'src/utils/localStorageHelpers'; import getFormDataWithExtraFilters from 'src/dashboard/util/charts/getFormDataWithExtraFilters'; import { URL_PARAMS } from 'src/constants'; import { JsonObject, VizType } from '@superset-ui/core'; - +import { getParsedExploreURLParams } from 'src/explore/exploreUtils/getParsedExploreURLParams'; import ChartPage from '.'; jest.mock('re-resizable', () => ({ @@ -46,6 +46,9 @@ jest.mock( ), ); jest.mock('src/dashboard/util/charts/getFormDataWithExtraFilters'); +jest.mock('src/explore/exploreUtils/getParsedExploreURLParams', () => ({ + getParsedExploreURLParams: jest.fn(), +})); describe('ChartPage', () => { afterEach(() => { @@ -75,6 +78,92 @@ describe('ChartPage', () => { ); }); + test('displays the dataset name and error when it is prohibited', async () => { + const chartApiRoute = `glob:*/api/v1/chart/*`; + const exploreApiRoute = 'glob:*/api/v1/explore/*'; + const expectedDatasourceName = 'failed datasource name'; + (getParsedExploreURLParams as jest.Mock).mockReturnValue( + new Map([['datasource_id', 1]]), + ); + fetchMock.get(exploreApiRoute, () => { + class Extra { + datasource = 123; + + datasource_name = expectedDatasourceName; + } + class SupersetSecurityError { + message = 'You do not have a permission to the table'; + + extra = new Extra(); + } + throw new SupersetSecurityError(); + }); + fetchMock.get(chartApiRoute, 200); + const { getByTestId } = render(, { + useRouter: true, + useRedux: true, + useDnd: true, + }); + await waitFor( + () => + expect(getByTestId('mock-explore-chart-panel')).toHaveTextContent( + JSON.stringify({ datasource_name: expectedDatasourceName }).slice( + 1, + -1, + ), + ), + { + timeout: 5000, + }, + ); + expect(fetchMock.calls(chartApiRoute).length).toEqual(0); + expect(fetchMock.calls(exploreApiRoute).length).toBeGreaterThanOrEqual(1); + }); + + test('fetches the chart api when explore metadata is prohibited and access from the chart link', async () => { + const expectedChartId = 7; + const expectedChartName = 'Unauthorized dataset owned chart name'; + (getParsedExploreURLParams as jest.Mock).mockReturnValue( + new Map([['slice_id', expectedChartId]]), + ); + const chartApiRoute = `glob:*/api/v1/chart/${expectedChartId}`; + const exploreApiRoute = 'glob:*/api/v1/explore/*'; + + fetchMock.get(exploreApiRoute, () => { + class Extra { + datasource = 123; + } + class SupersetSecurityError { + message = 'You do not have a permission to the table'; + + extra = new Extra(); + } + throw new SupersetSecurityError(); + }); + fetchMock.get(chartApiRoute, { + result: { + id: expectedChartId, + slice_name: expectedChartName, + url: 'chartid', + owners: [{ userId: 1 }], + }, + }); + const { getByTestId, getByText } = render(, { + useRouter: true, + useRedux: true, + useDnd: true, + }); + await waitFor(() => expect(fetchMock.calls(chartApiRoute).length).toBe(1), { + timeout: 5000, + }); + expect(fetchMock.calls(exploreApiRoute).length).toBeGreaterThanOrEqual(1); + expect(getByTestId('mock-explore-chart-panel')).toBeInTheDocument(); + expect(getByTestId('mock-explore-chart-panel')).toHaveTextContent( + JSON.stringify({ datasource: 123 }).slice(1, -1), + ); + expect(getByText(expectedChartName)).toBeInTheDocument(); + }); + describe('with dashboardContextFormData', () => { const dashboardPageId = 'mockPageId'; diff --git a/superset-frontend/src/pages/Chart/index.tsx b/superset-frontend/src/pages/Chart/index.tsx index 5bbeb5fa86c..3ae68d7502c 100644 --- a/superset-frontend/src/pages/Chart/index.tsx +++ b/superset-frontend/src/pages/Chart/index.tsx @@ -41,6 +41,7 @@ import { ExploreResponsePayload, SaveActionType } from 'src/explore/types'; import { fallbackExploreInitialData } from 'src/explore/fixtures'; import { getItem, LocalStorageKeys } from 'src/utils/localStorageHelpers'; import { getFormDataWithDashboardContext } from 'src/explore/controlUtils/getFormDataWithDashboardContext'; +import type Chart from 'src/types/Chart'; const isValidResult = (rv: JsonObject): boolean => rv?.result?.form_data && rv?.result?.dataset; @@ -49,41 +50,31 @@ const hasDatasetId = (rv: JsonObject): boolean => isDefined(rv?.result?.dataset?.id); const fetchExploreData = async (exploreUrlParams: URLSearchParams) => { - try { - const rv = await makeApi<{}, ExploreResponsePayload>({ - method: 'GET', - endpoint: 'api/v1/explore/', - })(exploreUrlParams); - if (isValidResult(rv)) { - if (hasDatasetId(rv)) { - return rv; - } - // Since there's no dataset id but the API responded with a valid payload, - // we assume the dataset was deleted, so we preserve some values from previous - // state so if the user decide to swap the datasource, the chart config remains - fallbackExploreInitialData.form_data = { - ...rv.result.form_data, - ...fallbackExploreInitialData.form_data, - }; - if (rv.result?.slice) { - fallbackExploreInitialData.slice = rv.result.slice; - } + const rv = await makeApi<{}, ExploreResponsePayload>({ + method: 'GET', + endpoint: 'api/v1/explore/', + })(exploreUrlParams); + if (isValidResult(rv)) { + if (hasDatasetId(rv)) { + return rv; } - let message = t('Failed to load chart data'); - const responseError = rv?.result?.message; - if (responseError) { - message = `${message}:\n${responseError}`; + // Since there's no dataset id but the API responded with a valid payload, + // we assume the dataset was deleted, so we preserve some values from previous + // state so if the user decide to swap the datasource, the chart config remains + fallbackExploreInitialData.form_data = { + ...rv.result.form_data, + ...fallbackExploreInitialData.form_data, + }; + if (rv.result?.slice) { + fallbackExploreInitialData.slice = rv.result.slice; } - throw new Error(message); - } catch (err) { - // todo: encapsulate the error handler - const clientError = await getClientErrorObject(err); - throw new Error( - clientError.message || - clientError.error || - t('Failed to load chart data.'), - ); } + let message = t('Failed to load chart data'); + const responseError = rv?.result?.message; + if (responseError) { + message = `${message}:\n${responseError}`; + } + throw new Error(message); }; const getDashboardPageContext = (pageId?: string | null) => { @@ -161,9 +152,62 @@ export default function ExplorePage() { }), ); }) - .catch(err => { + .catch(err => Promise.all([getClientErrorObject(err), err])) + .then(resolved => { + const [clientError, err] = resolved || []; + if (!err) { + return Promise.resolve(); + } + const errorMesage = + clientError?.message || + clientError?.error || + t('Failed to load chart data.'); + dispatch(addDangerToast(errorMesage)); + + if (err.extra?.datasource) { + const exploreData = { + ...fallbackExploreInitialData, + dataset: { + ...fallbackExploreInitialData.dataset, + id: err.extra?.datasource, + name: err.extra?.datasource_name, + extra: { + error: err, + }, + }, + }; + const chartId = exploreUrlParams.get('slice_id'); + return ( + chartId + ? makeApi({ + method: 'GET', + endpoint: `api/v1/chart/${chartId}`, + })() + : Promise.reject() + ) + .then( + ({ result: { id, url, owners, form_data: _, ...data } }) => { + const slice = { + ...data, + datasource: err.extra?.datasource_name, + slice_id: id, + slice_url: url, + owners: owners?.map(({ id }) => id), + }; + dispatch( + hydrateExplore({ + ...exploreData, + slice, + }), + ); + }, + ) + .catch(() => { + dispatch(hydrateExplore(exploreData)); + }); + } dispatch(hydrateExplore(fallbackExploreInitialData)); - dispatch(addDangerToast(err.message)); + return Promise.resolve(); }) .finally(() => { setIsLoaded(true); diff --git a/superset/explore/api.py b/superset/explore/api.py index 7e7813beca6..e16b083feb8 100644 --- a/superset/explore/api.py +++ b/superset/explore/api.py @@ -27,6 +27,7 @@ from superset.commands.temporary_cache.exceptions import ( TemporaryCacheResourceNotFoundError, ) from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP +from superset.exceptions import SupersetSecurityException from superset.explore.exceptions import DatasetAccessDeniedError, WrongEndpointError from superset.explore.permalink.exceptions import ExplorePermalinkGetFailedError from superset.explore.schemas import ExploreContextSchema @@ -118,6 +119,11 @@ class ExploreRestApi(BaseSupersetApi): return self.response(200, result=result) except ValueError as ex: return self.response(400, message=str(ex)) + except SupersetSecurityException as ex: + return self.response( + 403, + **ex.to_dict(), + ) except DatasetAccessDeniedError as ex: return self.response( 403, diff --git a/superset/security/manager.py b/superset/security/manager.py index a48aadb5fe2..40a44161ab2 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -632,6 +632,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods extra={ "link": self.get_datasource_access_link(datasource), "datasource": datasource.id, + "datasource_name": datasource.name, }, )