diff --git a/UPDATING.md b/UPDATING.md index de26c5ba2a57..2100aef2671f 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -32,6 +32,7 @@ assists people when migrating to a new version. - [32317](https://github.com/apache/superset/pull/32317) The horizontal filter bar feature is now out of testing/beta development and its feature flag `HORIZONTAL_FILTER_BAR` has been removed. - [31590](https://github.com/apache/superset/pull/31590) Marks the begining of intricate work around supporting dynamic Theming, and breaks support for [THEME_OVERRIDES](https://github.com/apache/superset/blob/732de4ac7fae88e29b7f123b6cbb2d7cd411b0e4/superset/config.py#L671) in favor of a new theming system based on AntD V5. Likely this will be in disrepair until settling over the 5.x lifecycle. - [32432](https://github.com/apache/superset/pull/31260) Moves the List Roles FAB view to the frontend and requires `FAB_ADD_SECURITY_API` to be enabled in the configuration and `superset init` to be executed. +- [34319](https://github.com/apache/superset/pull/34319) Drill to Detail and Drill By is now supported in Embedded mode, and also with the `DASHBOARD_RBAC` FF. If you don't want to expose these features in Embedded / `DASHBOARD_RBAC`, make sure the roles used for Embedded / `DASHBOARD_RBAC`don't have the required permissions to perform D2D actions. ## 5.0.0 diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/drillby.test.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/drillby.test.ts index b8ded5ee1b53..edc44a6fdb38 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/drillby.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/drillby.test.ts @@ -31,6 +31,52 @@ import { interceptFormDataKey, } from '../explore/utils'; +const interceptDrillInfo = () => { + cy.intercept('GET', '**/api/v1/dataset/*/drill_info/*', { + statusCode: 200, + body: { + result: { + id: 1, + changed_on_humanized: '2 days ago', + created_on_humanized: 'a week ago', + table_name: 'birth_names', + changed_by: { + first_name: 'Admin', + last_name: 'User', + }, + created_by: { + first_name: 'Admin', + last_name: 'User', + }, + owners: [ + { + first_name: 'Admin', + last_name: 'User', + }, + ], + columns: [ + { + column_name: 'gender', + verbose_name: null, + }, + { + column_name: 'state', + verbose_name: null, + }, + { + column_name: 'name', + verbose_name: null, + }, + { + column_name: 'ds', + verbose_name: null, + }, + ], + }, + }, + }).as('drillInfo'); +}; + const closeModal = () => { cy.get('body').then($body => { if ($body.find('[data-test="close-drill-by-modal"]').length) { @@ -62,6 +108,7 @@ const drillBy = (targetDrillByColumn: string, isLegacy = false) => { cy.get( '.ant-dropdown-menu-submenu:not(.ant-dropdown-menu-submenu-hidden) [data-test="drill-by-submenu"]', + { timeout: 15000 }, ) .should('be.visible') .find('[role="menuitem"]') @@ -235,12 +282,14 @@ describe('Drill by modal', () => { closeModal(); }); before(() => { + interceptDrillInfo(); cy.visit(SUPPORTED_CHARTS_DASHBOARD); }); describe('Modal actions + Table', () => { before(() => { closeModal(); + interceptDrillInfo(); openTopLevelTab('Tier 1'); SUPPORTED_TIER1_CHARTS.forEach(waitForChartLoad); }); @@ -389,6 +438,7 @@ describe('Drill by modal', () => { describe('Tier 1 charts', () => { before(() => { closeModal(); + interceptDrillInfo(); openTopLevelTab('Tier 1'); SUPPORTED_TIER1_CHARTS.forEach(waitForChartLoad); }); @@ -552,6 +602,7 @@ describe('Drill by modal', () => { describe('Tier 2 charts', () => { before(() => { closeModal(); + interceptDrillInfo(); openTopLevelTab('Tier 2'); SUPPORTED_TIER2_CHARTS.forEach(waitForChartLoad); }); diff --git a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx index 4e3369429c77..b5362e578e0b 100644 --- a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx +++ b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx @@ -22,7 +22,9 @@ import { ReactNode, RefObject, useCallback, + useEffect, useImperativeHandle, + useMemo, useState, } from 'react'; import ReactDOM from 'react-dom'; @@ -35,7 +37,9 @@ import { ensureIsArray, FeatureFlag, getChartMetadataRegistry, + getExtensionsRegistry, isFeatureEnabled, + logging, QueryFormData, t, useTheme, @@ -48,6 +52,10 @@ import { updateDataMask } from 'src/dataMask/actions'; import DrillByModal from 'src/components/Chart/DrillBy/DrillByModal'; import { useVerboseMap } from 'src/hooks/apiResources/datasets'; import { Dataset } from 'src/components/Chart/types'; +import { + cachedSupersetGet, + supersetGetCache, +} from 'src/utils/cachedSupersetGet'; import { DrillDetailMenuItems } from '../DrillDetail'; import { getMenuAdjustedY } from '../utils'; import { MenuItemTooltip } from '../DisabledMenuItemTooltip'; @@ -99,6 +107,9 @@ const ChartContextMenu = ( const crossFiltersEnabled = useSelector( ({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled, ); + const dashboardId = useSelector( + ({ dashboardInfo }) => dashboardInfo.id, + ); const [openKeys, setOpenKeys] = useState([]); const [modalFilters, setFilters] = useState( @@ -121,6 +132,7 @@ const ChartContextMenu = ( const [drillByColumn, setDrillByColumn] = useState(); const [showDrillByModal, setShowDrillByModal] = useState(false); const [dataset, setDataset] = useState(); + const [isLoadingDataset, setIsLoadingDataset] = useState(false); const verboseMap = useVerboseMap(dataset); const closeContextMenu = useCallback(() => { @@ -129,12 +141,15 @@ const ChartContextMenu = ( onClose(); }, [onClose]); - const handleDrillBy = useCallback((column: Column, dataset: Dataset) => { + const handleDrillBy = useCallback((column: Column) => { setDrillByColumn(column); - setDataset(dataset); // Save dataset when drilling setShowDrillByModal(true); }, []); + const loadDrillByOptionsExtension = getExtensionsRegistry().get( + 'load.drillby.options', + ); + const handleCloseDrillByModal = useCallback(() => { setShowDrillByModal(false); }, []); @@ -151,6 +166,75 @@ const ChartContextMenu = ( canDrillBy && isDisplayed(ContextMenuItem.DrillBy); + useEffect(() => { + async function fetchDataset() { + if (!visible || dataset || (!showDrillBy && !showDrillToDetail)) return; + + const datasetId = Number(formData.datasource.split('__')[0]); + try { + setIsLoadingDataset(true); + let response; + + if (loadDrillByOptionsExtension) { + response = await loadDrillByOptionsExtension(datasetId, formData); + } else { + const endpoint = `/api/v1/dataset/${datasetId}/drill_info/?q=(dashboard_id:${dashboardId})`; + response = await cachedSupersetGet({ endpoint }); + } + + const { json } = response; + const { result } = json; + + setDataset(result); + } catch (error) { + logging.error('Failed to load dataset:', error); + supersetGetCache.delete(`/api/v1/dataset/${datasetId}/drill_info/`); + } finally { + setIsLoadingDataset(false); + } + } + + fetchDataset(); + }, [ + visible, + showDrillBy, + showDrillToDetail, + formData.datasource, + loadDrillByOptionsExtension, + dashboardId, + ]); + + // Compute filteredDataset with all columns returned + a filtered list of valid drillable options + const filteredDataset = useMemo(() => { + if (!dataset || !showDrillBy) return dataset; + + const filteredColumns = ensureIsArray(dataset.columns).filter( + column => + // If using an extension, also filter by column.groupby since the extension might not do this + (!loadDrillByOptionsExtension || column.groupby) && + !ensureIsArray( + formData[filters?.drillBy?.groupbyFieldName ?? ''], + ).includes(column.column_name) && + column.column_name !== formData.x_axis && + ensureIsArray(additionalConfig?.drillBy?.excludedColumns)?.every( + excludedCol => excludedCol.column_name !== column.column_name, + ), + ); + + return { + ...dataset, + drillable_columns: filteredColumns, + }; + }, [ + dataset, + showDrillBy, + filters?.drillBy?.groupbyFieldName, + formData.x_axis, + formData[filters?.drillBy?.groupbyFieldName ?? ''], + additionalConfig?.drillBy?.excludedColumns, + loadDrillByOptionsExtension, + ]); + const showCrossFilters = isDisplayed(ContextMenuItem.CrossFilter); const isCrossFilteringSupportedByChart = getChartMetadataRegistry() @@ -254,6 +338,8 @@ const ChartContextMenu = ( onSelection={onSelection} submenuIndex={showCrossFilters ? 2 : 1} setShowModal={setDrillModalIsOpen} + dataset={filteredDataset} + isLoadingDataset={isLoadingDataset} {...(additionalConfig?.drillToDetail || {})} />, ); @@ -277,6 +363,8 @@ const ChartContextMenu = ( open={openKeys.includes('drill-by-submenu')} key="drill-by-submenu" onDrillBy={handleDrillBy} + dataset={filteredDataset} + isLoadingDataset={isLoadingDataset} {...(additionalConfig?.drillBy || {})} />, ); @@ -359,6 +447,7 @@ const ChartContextMenu = ( onHideModal={() => { setDrillModalIsOpen(false); }} + dataset={filteredDataset} /> )} {showDrillByModal && drillByColumn && dataset && filters?.drillBy && ( @@ -367,7 +456,7 @@ const ChartContextMenu = ( drillByConfig={filters?.drillBy} formData={formData} onHideModal={handleCloseDrillByModal} - dataset={{ ...dataset!, verbose_map: verboseMap }} + dataset={{ ...filteredDataset!, verbose_map: verboseMap }} canDownload={canDownload} /> )} diff --git a/superset-frontend/src/components/Chart/ChartContextMenu/useContextMenu.test.tsx b/superset-frontend/src/components/Chart/ChartContextMenu/useContextMenu.test.tsx index e1db166269b5..db812201f104 100644 --- a/superset-frontend/src/components/Chart/ChartContextMenu/useContextMenu.test.tsx +++ b/superset-frontend/src/components/Chart/ChartContextMenu/useContextMenu.test.tsx @@ -64,6 +64,7 @@ const setup = ({ ['can_explore', 'Superset'], ['can_samples', 'Datasource'], ['can_write', 'ExploreFormDataRestApi'], + ['can_get_drill_info', 'Dataset'], ], }, }, @@ -92,12 +93,13 @@ test('Context menu contains all displayed items only', () => { expect(screen.getByText('Drill by')).toBeInTheDocument(); }); -test('Context menu shows "Drill by" with `can_explore` & `can_write` perms', () => { +test('Context menu shows "Drill by" with `can_drill`, `can_write` & `can_get_drill_info` perms', () => { const result = setup({ roles: { Admin: [ ['can_write', 'ExploreFormDataRestApi'], - ['can_explore', 'Superset'], + ['can_drill', 'Dashboard'], + ['can_get_drill_info', 'Dataset'], ], }, }); @@ -105,12 +107,14 @@ test('Context menu shows "Drill by" with `can_explore` & `can_write` perms', () expect(screen.getByText('Drill by')).toBeInTheDocument(); }); -test('Context menu shows "Drill by" with `can_drill` & `can_write` perms', () => { +test('Context menu shows "Drill by" with `can_drill`, `can_get_drill_info` & `can_explore` + `can_write` perms', () => { const result = setup({ roles: { Admin: [ ['can_write', 'ExploreFormDataRestApi'], + ['can_explore', 'Superset'], ['can_drill', 'Dashboard'], + ['can_get_drill_info', 'Dataset'], ], }, }); @@ -118,46 +122,60 @@ test('Context menu shows "Drill by" with `can_drill` & `can_write` perms', () => expect(screen.getByText('Drill by')).toBeInTheDocument(); }); -test('Context menu shows "Drill by" with `can_drill` & `can_explore` + `can_write` perms', () => { +test('Context menu does not show "Drill by" with neither of required perms', () => { const result = setup({ roles: { - Admin: [ - ['can_write', 'ExploreFormDataRestApi'], - ['can_explore', 'Superset'], - ['can_drill', 'Dashboard'], - ], + Admin: [['invalid_permission', 'Dashboard']], }, }); result.current.onContextMenu(0, 0, {}); - expect(screen.getByText('Drill by')).toBeInTheDocument(); + expect(screen.queryByText('Drill by')).not.toBeInTheDocument(); }); -test('Context menu does not show "Drill by" with neither of required perms', () => { +test('Context menu does not show "Drill by" with just `can_dril` perm', () => { const result = setup({ roles: { - Admin: [['invalid_permission', 'Dashboard']], + Admin: [['can_drill', 'Dashboard']], }, }); result.current.onContextMenu(0, 0, {}); expect(screen.queryByText('Drill by')).not.toBeInTheDocument(); }); -test('Context menu does not show "Drill by" with just `can_dril` perm', () => { +test('Context menu does not show "Drill by" with just `can_dril` & `can_write` perms', () => { const result = setup({ roles: { - Admin: [['can_drill', 'Dashboard']], + Admin: [ + ['can_drill', 'Dashboard'], + ['can_write', 'ExploreFormDataRestApi'], + ], + }, + }); + result.current.onContextMenu(0, 0, {}); + expect(screen.queryByText('Drill by')).not.toBeInTheDocument(); +}); + +test('Context menu does not show "Drill by" with just `can_drill`, `can_explore` & `can_write` perms', () => { + const result = setup({ + roles: { + Admin: [ + ['can_write', 'ExploreFormDataRestApi'], + ['can_explore', 'Superset'], + ['can_drill', 'Dashboard'], + ], }, }); result.current.onContextMenu(0, 0, {}); expect(screen.queryByText('Drill by')).not.toBeInTheDocument(); }); -test('Context menu shows "Drill to detail" with `can_samples` and `can_explore` perms', () => { +test('Context menu shows "Drill to detail" with `can_samples`, `can_explore` & `can_get_drill_info` perms', () => { const result = setup({ roles: { Admin: [ ['can_samples', 'Datasource'], ['can_explore', 'Superset'], + ['can_get_drill_info', 'Dataset'], ], }, }); @@ -165,12 +183,13 @@ test('Context menu shows "Drill to detail" with `can_samples` and `can_explore` expect(screen.getByText('Drill to detail')).toBeInTheDocument(); }); -test('Context menu shows "Drill to detail" with `can_drill` & `can_samples` perms', () => { +test('Context menu shows "Drill to detail" with `can_drill`, `can_samples` & `can_get_drill_info` perms', () => { const result = setup({ roles: { Admin: [ ['can_samples', 'Datasource'], ['can_drill', 'Dashboard'], + ['can_get_drill_info', 'Dataset'], ], }, }); @@ -178,13 +197,14 @@ test('Context menu shows "Drill to detail" with `can_drill` & `can_samples` perm expect(screen.getByText('Drill to detail')).toBeInTheDocument(); }); -test('Context menu shows "Drill to detail" with `can_drill` & `can_explore` + `can_write` perms', () => { +test('Context menu shows "Drill to detail" with `can_drill`, `can_get_drill_info` & `can_explore` + `can_samples` perms', () => { const result = setup({ roles: { Admin: [ ['can_samples', 'Datasource'], ['can_explore', 'Superset'], ['can_drill', 'Dashboard'], + ['can_get_drill_info', 'Dataset'], ], }, }); @@ -202,7 +222,7 @@ test('Context menu does not show "Drill to detail" with neither of required perm expect(screen.queryByText('Drill to detail')).not.toBeInTheDocument(); }); -test('Context menu does not show "Drill to detail" with just `can_dril` perm', () => { +test('Context menu does not show "Drill to detail" with just `can_drill` perm', () => { const result = setup({ roles: { Admin: [['can_drill', 'Dashboard']], @@ -211,3 +231,43 @@ test('Context menu does not show "Drill to detail" with just `can_dril` perm', ( result.current.onContextMenu(0, 0, {}); expect(screen.queryByText('Drill to detail')).not.toBeInTheDocument(); }); + +test('Context menu does not show "Drill to detail" with just `can_drill` & `can_samples` perms', () => { + const result = setup({ + roles: { + Admin: [ + ['can_drill', 'Dashboard'], + ['can_samples', 'Datasource'], + ], + }, + }); + result.current.onContextMenu(0, 0, {}); + expect(screen.queryByText('Drill to detail')).not.toBeInTheDocument(); +}); + +test('Context menu does not show "Drill to detail" with `can_samples` & `can_explore` perms', () => { + const result = setup({ + roles: { + Admin: [ + ['can_samples', 'Datasource'], + ['can_explore', 'Superset'], + ], + }, + }); + result.current.onContextMenu(0, 0, {}); + expect(screen.queryByText('Drill to detail')).not.toBeInTheDocument(); +}); + +test('Context menu does not show "Drill to detail" with `can_drill`, `can_explore` + `can_samples` perms', () => { + const result = setup({ + roles: { + Admin: [ + ['can_samples', 'Datasource'], + ['can_explore', 'Superset'], + ['can_drill', 'Dashboard'], + ], + }, + }); + result.current.onContextMenu(0, 0, {}); + expect(screen.queryByText('Drill to detail')).not.toBeInTheDocument(); +}); diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx index 9d2a9360a185..13410f803164 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.test.tsx @@ -36,9 +36,6 @@ import { DrillByMenuItems, DrillByMenuItemsProps } from './DrillByMenuItems'; /* eslint jest/expect-expect: ["warn", { "assertFunctionNames": ["expect*"] }] */ -const DATASET_ENDPOINT = 'glob:*/api/v1/dataset/7*'; -const CHART_DATA_ENDPOINT = 'glob:*/api/v1/chart/data*'; -const FORM_DATA_KEY_ENDPOINT = 'glob:*/api/v1/explore/form_data'; const { form_data: defaultFormData } = chartQueries[sliceId]; jest.mock('lodash/debounce', () => (fn: Function & { debounce: Function }) => { @@ -61,6 +58,19 @@ const defaultColumns = [ { column_name: 'col11', groupby: true }, ]; +const mockDataset = { + id: 7, + table_name: 'test_table', + columns: defaultColumns, + drillable_columns: defaultColumns, + changed_on_humanized: '1 day ago', + created_on_humanized: '2 days ago', + description: 'Test dataset', + owners: [], + changed_by: { first_name: 'Test', last_name: 'User' }, + created_by: { first_name: 'Test', last_name: 'User' }, +}; + const defaultFilters = [ { col: 'filter_col', @@ -72,6 +82,7 @@ const defaultFilters = [ const renderMenu = ({ formData = defaultFormData, drillByConfig = { filters: defaultFilters, groupbyFieldName: 'groupby' }, + dataset = mockDataset, ...rest }: Partial) => render( @@ -79,6 +90,7 @@ const renderMenu = ({ @@ -151,20 +163,20 @@ test('render enabled menu item for supported chart, no filters', async () => { }); test('render disabled menu item for supported chart, no columns', async () => { - fetchMock.get(DATASET_ENDPOINT, { result: { columns: [] } }); - renderMenu({}); - await waitFor(() => fetchMock.called(DATASET_ENDPOINT)); + const emptyDataset = { ...mockDataset, columns: [], drillable_columns: [] }; + renderMenu({ dataset: emptyDataset }); await expectDrillByEnabled(); screen.getByText('No columns found'); }); test('render menu item with submenu without searchbox', async () => { const slicedColumns = defaultColumns.slice(0, 9); - fetchMock.get(DATASET_ENDPOINT, { - result: { columns: slicedColumns }, - }); - renderMenu({}); - await waitFor(() => fetchMock.called(DATASET_ENDPOINT)); + const datasetWithSlicedColumns = { + ...mockDataset, + columns: slicedColumns, + drillable_columns: slicedColumns, + }; + renderMenu({ dataset: datasetWithSlicedColumns }); await expectDrillByEnabled(); // Check that each column appears in the drill-by submenu @@ -180,11 +192,7 @@ test('render menu item with submenu without searchbox', async () => { jest.setTimeout(20000); test('render menu item with submenu and searchbox', async () => { - fetchMock.get(DATASET_ENDPOINT, { - result: { columns: defaultColumns }, - }); - renderMenu({}); - await waitFor(() => fetchMock.called(DATASET_ENDPOINT)); + renderMenu({ dataset: mockDataset }); await expectDrillByEnabled(); // Wait for all columns to be visible @@ -236,18 +244,21 @@ test('render menu item with submenu and searchbox', async () => { }); test('Do not display excluded column in the menu', async () => { - fetchMock.get(DATASET_ENDPOINT, { - result: { columns: defaultColumns }, - }); - const excludedColNames = ['col3', 'col5']; + const filteredColumns = defaultColumns.filter( + col => !excludedColNames.includes(col.column_name), + ); + const datasetWithFilteredColumns = { + ...mockDataset, + drillable_columns: filteredColumns, + }; renderMenu({ + dataset: datasetWithFilteredColumns, excludedColumns: excludedColNames.map(colName => ({ column_name: colName, })), }); - await waitFor(() => fetchMock.called(DATASET_ENDPOINT)); await expectDrillByEnabled(); // Wait for menu items to be loaded @@ -274,19 +285,12 @@ test('Do not display excluded column in the menu', async () => { }); test('When menu item is clicked, call onSelection with clicked column and drill by filters', async () => { - fetchMock - .get(DATASET_ENDPOINT, { - result: { columns: defaultColumns }, - }) - .post(FORM_DATA_KEY_ENDPOINT, {}) - .post(CHART_DATA_ENDPOINT, {}); - const onSelectionMock = jest.fn(); renderMenu({ + dataset: mockDataset, onSelection: onSelectionMock, }); - await waitFor(() => fetchMock.called(DATASET_ENDPOINT)); await expectDrillByEnabled(); // Wait for col1 to be visible before clicking diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx index ec0d4e1c2180..3d944c496a7f 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx @@ -32,25 +32,16 @@ import { Behavior, Column, ContextMenuFilters, - JsonResponse, css, ensureIsArray, getChartMetadataRegistry, - getExtensionsRegistry, - logging, t, useTheme, } from '@superset-ui/core'; import { Constants, Input, Loading } from '@superset-ui/core/components'; -import rison from 'rison'; import { debounce } from 'lodash'; import { FixedSizeList as List } from 'react-window'; import { Icons } from '@superset-ui/core/components/Icons'; -import { useToasts } from 'src/components/MessageToasts/withToasts'; -import { - cachedSupersetGet, - supersetGetCache, -} from 'src/utils/cachedSupersetGet'; import { InputRef } from 'antd'; import { MenuItemTooltip } from '../DisabledMenuItemTooltip'; import { getSubmenuYOffset } from '../utils'; @@ -73,27 +64,10 @@ export interface DrillByMenuItemsProps { excludedColumns?: Column[]; open: boolean; onDrillBy?: (column: Column, dataset: Dataset) => void; + dataset?: Dataset; + isLoadingDataset?: boolean; } -const loadDrillByOptions = getExtensionsRegistry().get('load.drillby.options'); - -const queryString = rison.encode({ - columns: [ - 'table_name', - 'owners.first_name', - 'owners.last_name', - 'created_by.first_name', - 'created_by.last_name', - 'created_on_humanized', - 'changed_by.first_name', - 'changed_by.last_name', - 'changed_on_humanized', - 'columns.column_name', - 'columns.verbose_name', - 'columns.groupby', - ], -}); - export const DrillByMenuItems = ({ drillByConfig, formData, @@ -106,18 +80,16 @@ export const DrillByMenuItems = ({ openNewModal = true, open, onDrillBy, + dataset, + isLoadingDataset = false, ...rest }: DrillByMenuItemsProps) => { const theme = useTheme(); - const { addDangerToast } = useToasts(); - const [isLoadingColumns, setIsLoadingColumns] = useState(true); const [searchInput, setSearchInput] = useState(''); const [debouncedSearchInput, setDebouncedSearchInput] = useState(''); - const [dataset, setDataset] = useState(); - const [columns, setColumns] = useState([]); const ref = useRef(null); - const showSearch = - loadDrillByOptions || columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD; + const columns = dataset ? ensureIsArray(dataset.drillable_columns) : []; + const showSearch = columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD; const handleSelection = useCallback( (event, column) => { @@ -151,56 +123,6 @@ export const DrillByMenuItems = ({ [formData.viz_type], ); - useEffect(() => { - async function loadOptions() { - const datasetId = Number(formData.datasource.split('__')[0]); - try { - setIsLoadingColumns(true); - let response: JsonResponse; - if (loadDrillByOptions) { - response = await loadDrillByOptions(datasetId, formData); - } else { - response = await cachedSupersetGet({ - endpoint: `/api/v1/dataset/${datasetId}?q=${queryString}`, - }); - } - const { json } = response; - const { result } = json; - setDataset(result); - setColumns( - ensureIsArray(result.columns) - .filter(column => column.groupby) - .filter( - column => - !ensureIsArray( - formData[drillByConfig?.groupbyFieldName ?? ''], - ).includes(column.column_name) && - column.column_name !== formData.x_axis && - ensureIsArray(excludedColumns)?.every( - excludedCol => excludedCol.column_name !== column.column_name, - ), - ), - ); - } catch (error) { - logging.error(error); - supersetGetCache.delete(`/api/v1/dataset/${datasetId}`); - addDangerToast(t('Failed to load dimensions for drill by')); - } finally { - setIsLoadingColumns(false); - } - } - if (handlesDimensionContextMenu && hasDrillBy) { - loadOptions(); - } - }, [ - addDangerToast, - drillByConfig?.groupbyFieldName, - excludedColumns, - formData, - handlesDimensionContextMenu, - hasDrillBy, - ]); - const debouncedSetSearchInput = useMemo( () => debounce((value: string) => { @@ -316,7 +238,7 @@ export const DrillByMenuItems = ({ value={searchInput} /> )} - {isLoadingColumns ? ( + {isLoadingDataset ? (
({ + isEmbedded: jest.fn(() => false), +})); + const CHART_DATA_ENDPOINT = 'glob:*/api/v1/chart/data*'; const FORM_DATA_KEY_ENDPOINT = 'glob:*/api/v1/explore/form_data'; @@ -67,9 +72,14 @@ const dataset = { columns: [ { column_name: 'gender', + verbose_name: null, + }, + { + column_name: 'name', + verbose_name: null, }, - { column_name: 'name' }, ], + verbose_map: {}, }; const renderModal = async ( @@ -159,7 +169,7 @@ test('should render alert banner when results fail to load', async () => { test('should generate Explore url', async () => { await renderModal({ - column: { column_name: 'name' }, + column: { column_name: 'name', verbose_name: null }, drillByConfig: { filters: [{ col: 'gender', op: '==', val: 'boy' }], groupbyFieldName: 'groupby', @@ -222,7 +232,7 @@ test('should render radio buttons', async () => { test('render breadcrumbs', async () => { await renderModal({ - column: { column_name: 'name' }, + column: { column_name: 'name', verbose_name: null }, drillByConfig: { filters: [{ col: 'gender', op: '==', val: 'boy' }], groupbyFieldName: 'groupby', @@ -270,3 +280,79 @@ test('should render "Edit chart" enabled with can_explore permission', async () ); expect(screen.getByRole('button', { name: 'Edit chart' })).toBeEnabled(); }); + +describe('Embedded mode behavior', () => { + // eslint-disable-next-line global-require, @typescript-eslint/no-var-requires + const { isEmbedded } = require('src/dashboard/util/isEmbedded'); + + beforeEach(() => { + (isEmbedded as jest.Mock).mockClear(); + }); + + afterEach(() => { + (isEmbedded as jest.Mock).mockReturnValue(false); + }); + + test('should not render "Edit chart" button in embedded mode', async () => { + (isEmbedded as jest.Mock).mockReturnValue(true); + + await renderModal(); + + expect( + screen.queryByRole('button', { name: 'Edit chart' }), + ).not.toBeInTheDocument(); + const footerCloseButton = screen.getByTestId('close-drill-by-modal'); + expect(footerCloseButton).toHaveTextContent('Close'); + }); + + test('should not call postFormData API in embedded mode', async () => { + (isEmbedded as jest.Mock).mockReturnValue(true); + + await renderModal({ + column: { column_name: 'name', verbose_name: null }, + drillByConfig: { + filters: [{ col: 'gender', op: '==', val: 'boy' }], + groupbyFieldName: 'groupby', + }, + }); + + await waitFor(() => fetchMock.called(CHART_DATA_ENDPOINT)); + + expect(fetchMock.called(FORM_DATA_KEY_ENDPOINT)).toBe(false); + }); + + test('should render "Edit chart" button in non-embedded mode', async () => { + (isEmbedded as jest.Mock).mockReturnValue(false); + + await renderModal(); + + expect( + screen.getByRole('button', { name: 'Edit chart' }), + ).toBeInTheDocument(); + }); + + test('should call postFormData API in non-embedded mode', async () => { + (isEmbedded as jest.Mock).mockReturnValue(false); + + await renderModal({ + column: { column_name: 'name', verbose_name: null }, + drillByConfig: { + filters: [{ col: 'gender', op: '==', val: 'boy' }], + groupbyFieldName: 'groupby', + }, + }); + + await waitFor(() => fetchMock.called(CHART_DATA_ENDPOINT)); + + await waitFor(() => { + expect(fetchMock.called(FORM_DATA_KEY_ENDPOINT)).toBe(true); + }); + + expect( + await screen.findByRole('link', { name: 'Edit chart' }), + ).toHaveAttribute( + 'href', + '/explore/?form_data_key=123&dashboard_page_id=1', + ); + }); +}); diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx index 302e39e409ef..f118d34c7de1 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx @@ -56,6 +56,7 @@ import { } from 'src/logger/LogUtils'; import { findPermission } from 'src/utils/findPermission'; import { getQuerySettings } from 'src/explore/exploreUtils'; +import { isEmbedded } from 'src/dashboard/util/isEmbedded'; import { Dataset, DrillByType } from '../types'; import DrillByChart from './DrillByChart'; import { ContextMenuItem } from '../ChartContextMenu/ChartContextMenu'; @@ -93,6 +94,8 @@ const ModalFooter = ({ formData, closeModal }: ModalFooterProps) => { const [datasource_id, datasource_type] = formData.datasource.split('__'); useEffect(() => { + // short circuit if the user is embedded as explore is not available + if (isEmbedded()) return; postFormData(Number(datasource_id), datasource_type, formData, 0) .then(key => { setUrl( @@ -113,28 +116,30 @@ const ModalFooter = ({ formData, closeModal }: ModalFooterProps) => { return ( <> - + + {t('Edit chart')} + + + )} + {!isEmbedded() && ( + + )}
); - }, [datasetProps, result, status, theme.sizeUnit]); + }, [dataset, theme.sizeUnit]); return { metadataBar, - status, }; }; diff --git a/superset-frontend/src/hooks/usePermissions.ts b/superset-frontend/src/hooks/usePermissions.ts index 1811c0a96d54..79636dfa3d2b 100644 --- a/superset-frontend/src/hooks/usePermissions.ts +++ b/superset-frontend/src/hooks/usePermissions.ts @@ -36,8 +36,13 @@ export const usePermissions = () => { const canDrill = useSelector((state: RootState) => findPermission('can_drill', 'Dashboard', state.user?.roles), ); - const canDrillBy = (canExplore || canDrill) && canWriteExploreFormData; - const canDrillToDetail = (canExplore || canDrill) && canDatasourceSamples; + const canGetDrillInfo = useSelector((state: RootState) => + findPermission('can_get_drill_info', 'Dataset', state.user?.roles), + ); + const canDrillBy = + (canExplore || canDrill) && canWriteExploreFormData && canGetDrillInfo; + const canDrillToDetail = + (canExplore || canDrill) && canDatasourceSamples && canGetDrillInfo; const canViewQuery = useSelector((state: RootState) => findPermission('can_view_query', 'Dashboard', state.user?.roles), ); diff --git a/superset/config.py b/superset/config.py index d42aadbba390..bb63d0d8b528 100644 --- a/superset/config.py +++ b/superset/config.py @@ -279,10 +279,11 @@ def _try_json_readsha(filepath: str, length: int) -> str | None: # Add endpoints that need to be exempt from CSRF protection WTF_CSRF_EXEMPT_LIST = [ - "superset.views.core.log", - "superset.views.core.explore_json", "superset.charts.data.api.data", "superset.dashboards.api.cache_dashboard_screenshot", + "superset.views.core.explore_json", + "superset.views.core.log", + "superset.views.datasource.views.samples", ] # Whether to run the web server in debug mode or not diff --git a/superset/datasets/api.py b/superset/datasets/api.py index 098e0f682895..0af6ecca4bdb 100644 --- a/superset/datasets/api.py +++ b/superset/datasets/api.py @@ -35,7 +35,7 @@ from jinja2.exceptions import TemplateSyntaxError from marshmallow import ValidationError -from superset import event_logger, is_feature_enabled +from superset import event_logger, is_feature_enabled, security_manager from superset.commands.dataset.create import CreateDatasetCommand from superset.commands.dataset.delete import DeleteDatasetCommand from superset.commands.dataset.duplicate import DuplicateDatasetCommand @@ -58,17 +58,20 @@ from superset.commands.importers.v1.utils import get_contents_from_bundle from superset.connectors.sqla.models import SqlaTable from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod +from superset.daos.dashboard import DashboardDAO from superset.daos.dataset import DatasetDAO from superset.databases.filters import DatabaseFilter from superset.datasets.filters import DatasetCertifiedFilter, DatasetIsNullOrEmptyFilter from superset.datasets.schemas import ( DatasetCacheWarmUpRequestSchema, DatasetCacheWarmUpResponseSchema, + DatasetDrillInfoSchema, DatasetDuplicateSchema, DatasetPostSchema, DatasetPutSchema, DatasetRelatedObjectsResponse, get_delete_ids_schema, + get_drill_info_schema, get_export_ids_schema, GetOrCreateDatasetSchema, openapi_spec_methods_override, @@ -110,6 +113,7 @@ class DatasetRestApi(BaseSupersetModelRestApi): "duplicate", "get_or_create_dataset", "warm_up_cache", + "get_drill_info", } list_columns = [ "id", @@ -1174,6 +1178,104 @@ def get(self, pk: int, **kwargs: Any) -> Response: return self.response_400(message=str(ex)) return self.response(200, **response) + @expose("//drill_info/", methods=("GET",)) + @protect() + @rison(get_drill_info_schema) + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, + *args, + **kwargs: f"{self.__class__.__name__}.get_drill_info", + log_to_statsd=False, + ) + def get_drill_info(self, pk: int, **kwargs: Any) -> Response: + """Get dataset drill info. + --- + get: + summary: Get dataset drill info + parameters: + - in: path + schema: + type: integer + name: pk + description: The dataset ID + responses: + 200: + description: Dataset drill info + content: + application/json: + schema: + type: object + properties: + result: + type: object + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + dashboard_id = kwargs["rison"].get("dashboard_id") + drill_info_select_columns = [ + "id", + "table_name", + "owners.first_name", + "owners.last_name", + "created_by.first_name", + "created_by.last_name", + "created_on_humanized", + "changed_by.first_name", + "changed_by.last_name", + "changed_on_humanized", + "columns.column_name", + "columns.verbose_name", + "columns.groupby", + ] + dataset_schema = DatasetDrillInfoSchema() + + # First try with regular access + dataset: SqlaTable | None = self.datamodel.get( + pk, + self._base_filters, + drill_info_select_columns, + self.show_outer_default_load, + ) + if dataset: + return self.response(200, result=dataset_schema.dump(dataset)) + + # Embedded user must pass a dash ID + if not dashboard_id and security_manager.is_guest_user(): + return self.response_403() + # RBAC user must pass a dash ID for fallback validation + if not dashboard_id: + return self.response_404() + + # Lazy load the dashboard and dataset for RBAC/embedded access check + dashboard = DashboardDAO.find_by_id(dashboard_id, skip_base_filter=True) + dataset_ = DatasetDAO.find_by_id(pk, skip_base_filter=True) + if not (dashboard and dataset_): + return self.response_404() + if not security_manager.can_drill_dataset_via_dashboard_access( + dataset_, + dashboard, + ): + return self.response_403() + # Load dataset again skipping base filters + # We don't use `dataset_` to avoid lazy loading columns + dataset = self.datamodel.get( + pk, + None, + drill_info_select_columns, + self.show_outer_default_load, + ) + return self.response(200, result=dataset_schema.dump(dataset)) + @staticmethod def render_dataset_fields( data: dict[str, Any], processor: BaseTemplateProcessor diff --git a/superset/datasets/schemas.py b/superset/datasets/schemas.py index aba2283ef2fd..3b781f4b1c1d 100644 --- a/superset/datasets/schemas.py +++ b/superset/datasets/schemas.py @@ -19,14 +19,29 @@ from dateutil.parser import isoparse from flask_babel import lazy_gettext as _ -from marshmallow import fields, pre_load, Schema, validates_schema, ValidationError +from marshmallow import ( + fields, + post_dump, + pre_load, + Schema, + validates_schema, + ValidationError, +) from marshmallow.validate import Length, OneOf +from superset import security_manager +from superset.connectors.sqla.models import SqlaTable from superset.exceptions import SupersetMarshmallowValidationError from superset.utils import json get_delete_ids_schema = {"type": "array", "items": {"type": "integer"}} get_export_ids_schema = {"type": "array", "items": {"type": "integer"}} +get_drill_info_schema = { + "type": "object", + "properties": { + "dashboard_id": {"type": "integer"}, + }, +} openapi_spec_methods_override = { "get_list": { @@ -373,3 +388,48 @@ class DatasetCacheWarmUpResponseSchema(Schema): "description": "A list of each chart's warmup status and errors if any" }, ) + + +class DatasetColumnDrillInfoSchema(Schema): + column_name = fields.String(required=True) + verbose_name = fields.String(required=False) + + +class UserSchema(Schema): + first_name = fields.String() + last_name = fields.String() + + +class DatasetDrillInfoSchema(Schema): + id = fields.Integer() + columns = fields.List(fields.Nested(DatasetColumnDrillInfoSchema)) + table_name = fields.String() + owners = fields.List(fields.Nested(UserSchema)) + created_by = fields.Nested(UserSchema) + created_on_humanized = fields.String() + changed_by = fields.Nested(UserSchema) + changed_on_humanized = fields.String() + + # pylint: disable=unused-argument + @post_dump(pass_original=True) + def post_dump( + self, serialized: dict[str, Any], obj: SqlaTable, **kwargs: Any + ) -> dict[str, Any]: + """ + Clear API response to avoid exposing sensitive information for embedded users, + and filter columns to only include those with groupby=True for drill operations. + """ + dimensions = { + col.column_name + for col in getattr(obj, "columns", []) + if getattr(col, "groupby", False) + } + serialized["columns"] = [ + col + for col in serialized.get("columns", []) + if col["column_name"] in dimensions + ] + + if security_manager.is_guest_user(): + return {"id": serialized["id"], "columns": serialized["columns"]} + return serialized diff --git a/superset/security/manager.py b/superset/security/manager.py index 5bb2ac303233..378bf55aceb5 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -331,6 +331,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods "can_external_metadata", "can_external_metadata_by_name", "can_read", + "can_get_drill_info", } ALPHA_ONLY_PERMISSIONS = { @@ -570,6 +571,73 @@ def can_access_datasource(self, datasource: "BaseDatasource") -> bool: return True + def can_drill_dataset_via_dashboard_access( + self, dataset: "BaseDatasource", dashboard: "Dashboard" + ) -> bool: + """ + Return True if an embedded user or DASHBOARD_RBAC user can drill a dataset. + """ + from superset import is_feature_enabled + + if ( + ( + is_feature_enabled("EMBEDDED_SUPERSET") + and self.is_guest_user() + and self.has_guest_access(dashboard) + ) + or ( + is_feature_enabled("DASHBOARD_RBAC") + and dashboard.roles + and dashboard.published + and {role.id for role in dashboard.roles} + & {role.id for role in self.get_user_roles()} + ) + ) and dataset.id in {dataset.id for dataset in dashboard.datasources}: + return True + + return False + + def has_drill_by_access( + self, + form_data: dict[str, Any], + dashboard: "Dashboard", + datasource: "BaseDatasource", + ) -> bool: + """ + Return True if the form_data is performing a supported drill by operation, + False otherwise. + + :param form_data: The form_data included in the request. + :param dashboard: The dashboard the user is drilling from. + :returns: Whether the user has drill byaccess. + """ + + from superset.connectors.sqla.models import TableColumn + + return bool( + form_data.get("type") != "NATIVE_FILTER" + and form_data.get("slice_id") == 0 + and (chart_id := form_data.get("chart_id")) + and ( + slc := self.get_session.query(Slice) + .filter(Slice.id == chart_id) + .one_or_none() + ) + and slc in dashboard.slices + and slc.datasource == datasource + and (dimensions := form_data.get("groupby")) + and ( + drillable_columns := { + row[0] + for row in self.get_session.query(TableColumn.column_name) + .filter(TableColumn.table_id == datasource.id) + .filter(TableColumn.groupby) + .all() + } + ) + and set(dimensions).issubset(drillable_columns) + ) + def can_access_dashboard(self, dashboard: "Dashboard") -> bool: """ Return True if the user can access the specified dashboard, False otherwise. @@ -2401,6 +2469,7 @@ def raise_for_access( # noqa: C901 and slc in dashboard_.slices and slc.datasource == datasource ) + or self.has_drill_by_access(form_data, dashboard_, datasource) ) and self.can_access_dashboard(dashboard_) ) diff --git a/superset/views/datasource/schemas.py b/superset/views/datasource/schemas.py index d66f33471248..b3da21581233 100644 --- a/superset/views/datasource/schemas.py +++ b/superset/views/datasource/schemas.py @@ -103,6 +103,7 @@ class SamplesRequestSchema(Schema): validate=validate.Range(min=1, max=1000), load_default=None, ) + dashboard_id = fields.Integer(required=False, allow_none=True, load_default=None) @pre_load def set_default_per_page( diff --git a/superset/views/datasource/views.py b/superset/views/datasource/views.py index c06080c6fe5c..d0a4b6851c53 100644 --- a/superset/views/datasource/views.py +++ b/superset/views/datasource/views.py @@ -33,6 +33,8 @@ from superset.commands.utils import populate_owner_list from superset.connectors.sqla.models import SqlaTable from superset.connectors.sqla.utils import get_physical_table_metadata +from superset.daos.dashboard import DashboardDAO +from superset.daos.dataset import DatasetDAO from superset.daos.datasource import DatasourceDAO from superset.exceptions import SupersetException, SupersetSecurityException from superset.models.core import Database @@ -200,6 +202,23 @@ def samples(self) -> FlaskResponse: except ValidationError as err: return json_error_response(err.messages, status=400) + if security_manager.is_guest_user(): + if not params["dashboard_id"]: + return json_error_response(_("Forbidden"), status=403) + dataset = DatasetDAO.find_by_id( + params["datasource_id"], skip_base_filter=True + ) + dashboard = DashboardDAO.find_by_id( + params["dashboard_id"], skip_base_filter=True + ) + if not (dashboard and dataset): + return self.response_404() + if not security_manager.can_drill_dataset_via_dashboard_access( + dataset, + dashboard, + ): + return json_error_response(_("Forbidden"), status=403) + rv = get_samples( datasource_type=params["datasource_type"], datasource_id=params["datasource_id"], diff --git a/tests/integration_tests/base_tests.py b/tests/integration_tests/base_tests.py index 208a1e72c391..07278637cbfc 100644 --- a/tests/integration_tests/base_tests.py +++ b/tests/integration_tests/base_tests.py @@ -160,12 +160,13 @@ def create_user_with_roles( return user_to_create @contextmanager - def temporary_user( + def temporary_user( # noqa: C901 self, clone_user=None, username=None, extra_roles=None, extra_pvms=None, + pvms_to_remove=None, login=False, ): """ @@ -180,33 +181,39 @@ def temporary_user( temp_user = ab_models.User( username=username, email=f"{username}@temp.com", active=True ) + pvms = [] + if clone_user: - temp_user.roles = clone_user.roles temp_user.first_name = clone_user.first_name temp_user.last_name = clone_user.last_name temp_user.password = clone_user.password + if clone_user.roles: + for role in clone_user.roles: + pvms.extend(role.permissions) else: temp_user.first_name = temp_user.last_name = username - if clone_user: - temp_user.roles = clone_user.roles - if extra_roles: - temp_user.roles.extend(extra_roles) + for role in extra_roles: + pvms.extend(role.permissions) - pvms = [] - temp_role = None - if extra_pvms: - temp_role = ab_models.Role(name=f"tmp_role_{shortid()}") - for pvm in extra_pvms: - if isinstance(pvm, (tuple, list)): - pvms.append(security_manager.find_permission_view_menu(*pvm)) - else: - pvms.append(pvm) - temp_role.permissions = pvms - temp_user.roles.append(temp_role) - db.session.add(temp_role) - db.session.commit() + for pvm in extra_pvms or []: + if isinstance(pvm, (tuple, list)): + pvms.append(security_manager.find_permission_view_menu(*pvm)) + else: + pvms.append(pvm) + + for pvm in pvms_to_remove or []: + if isinstance(pvm, (tuple, list)): + pvm = security_manager.find_permission_view_menu(*pvm) + if pvm in pvms: + pvms.remove(pvm) + + temp_role = ab_models.Role(name=f"tmp_role_{shortid()}") + temp_role.permissions = pvms + temp_user.roles.append(temp_role) + db.session.add(temp_role) + db.session.commit() # Add the temp user to the session and commit to apply changes for the test db.session.add(temp_user) diff --git a/tests/integration_tests/charts/data/api_tests.py b/tests/integration_tests/charts/data/api_tests.py index ac184245f3ba..bccd093c6e02 100644 --- a/tests/integration_tests/charts/data/api_tests.py +++ b/tests/integration_tests/charts/data/api_tests.py @@ -18,6 +18,7 @@ import copy import time import unittest +from contextlib import contextmanager from datetime import datetime from io import BytesIO from typing import Any, Optional @@ -135,6 +136,32 @@ def quote_name(self, name: str): ) return name + @contextmanager + def set_column_groupby_false(self, column_name: str): + """ + Context manager to temporarily set a column's groupby property to false. + """ + birth_names_table = self.get_birth_names_dataset() + target_column = None + original_groupby_value = None + + for col in birth_names_table.columns: + if col.column_name == column_name: + target_column = col + original_groupby_value = col.groupby + break + + if target_column: + target_column.groupby = False + db.session.commit() + + try: + yield target_column + finally: + if target_column and original_groupby_value is not False: + target_column.groupby = original_groupby_value + db.session.commit() + @pytest.mark.chart_data_flow class TestPostChartDataApi(BaseTestChartDataApi): @@ -972,6 +999,73 @@ def test_with_adhoc_column_without_metrics(self): assert "name" in result["query"] assert list(result["data"][0].keys()) == ["name", "num divide by 10"] + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_drill_by_allowed_column(self): + """ + Chart data API: Test that user can drill by column with + isDimension set to True + """ + request_payload = self.query_context_payload + request_payload["queries"][0]["columns"] = ["name"] + rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data") + assert rv.status_code == 200 + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_drill_by_disallowed_column_regular_user(self): + """ + Chart data API: Test that user can still drill by column with + isDimension set to False (given the dataset access) + """ + with self.set_column_groupby_false("num_girls"): + self.query_context_payload["queries"][0]["columns"] = ["num_girls"] + rv = self.post_assert_metric( + CHART_DATA_URI, self.query_context_payload, "data" + ) + assert rv.status_code == 200 + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @mock.patch("superset.security.manager.SupersetSecurityManager.has_guest_access") + @mock.patch("superset.security.manager.SupersetSecurityManager.is_guest_user") + @with_feature_flags(EMBEDDED_SUPERSET=True) + def test_embedded_user_drill_by_allowed_column( + self, mock_is_guest_user, mock_has_guest_access + ): + """ + Chart data API: Test that embedded user can drill by column with + isDimension set to True. + """ + g.user.rls = [] + mock_has_guest_access.return_value = True + mock_is_guest_user.return_value = True + request_payload = self.query_context_payload + request_payload["queries"][0]["columns"] = ["name"] + rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data") + assert rv.status_code == 200 + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @mock.patch("superset.security.manager.SupersetSecurityManager.has_guest_access") + @mock.patch("superset.security.manager.SupersetSecurityManager.is_guest_user") + @with_feature_flags(EMBEDDED_SUPERSET=True) + def test_embedded_user_drill_by_disallowed_column( + self, mock_is_guest_user, mock_has_guest_access + ): + """ + Chart data API: Test that embedded user can't drill by column with + isDimension set to False. + """ + self.logout() + self.login(GAMMA_USERNAME) + + with self.set_column_groupby_false("num_girls"): + g.user.rls = [] + mock_has_guest_access.return_value = True + mock_is_guest_user.return_value = True + self.query_context_payload["queries"][0]["columns"] = ["num_girls"] + rv = self.post_assert_metric( + CHART_DATA_URI, self.query_context_payload, "data" + ) + assert rv.status_code == 403 + @pytest.mark.chart_data_flow class TestGetChartDataApi(BaseTestChartDataApi): diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index 63c6b11a4dc2..91bada22759f 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -84,16 +84,21 @@ def tearDown(self): def insert_dataset( table_name: str, owners: list[int], - database: Database, + database: Database | None = None, sql: str | None = None, schema: str | None = None, catalog: str | None = None, fetch_metadata: bool = True, + columns: list[TableColumn] | None = None, + metrics: list[SqlMetric] | None = None, + extra: str | None = None, ) -> SqlaTable: obj_owners = list() # noqa: C408 for owner in owners: user = db.session.query(security_manager.user_model).get(owner) obj_owners.append(user) + database = database or get_example_database() + schema = schema or get_example_default_schema() table = SqlaTable( table_name=table_name, schema=schema, @@ -101,13 +106,36 @@ def insert_dataset( database=database, sql=sql, catalog=catalog, + extra=extra, ) + if columns: + table.columns = columns + if metrics: + table.metrics = metrics db.session.add(table) db.session.commit() if fetch_metadata: table.fetch_metadata() return table + @staticmethod + def insert_chart( + chart_title: str, + dataset_id: int, + viz_type: str = "bar", + params: str = "{}", + ) -> Slice: + chart = Slice( + slice_name=chart_title, + datasource_id=dataset_id, + datasource_type="table", + viz_type=viz_type, + params=params, + ) + db.session.add(chart) + db.session.commit() + return chart + def insert_default_dataset(self): return self.insert_dataset( "ab_permission", [self.get_user("admin").id], get_main_database() @@ -421,12 +449,11 @@ def test_get_dataset_render_jinja(self): """ Dataset API: Test get dataset with the render parameter. """ - database = get_example_database() - dataset = SqlaTable( + dataset = self.insert_dataset( table_name="test_sql_table_with_jinja", - database=database, - schema=get_example_default_schema(), - main_dttm_col="default_dttm", + owners=[], + sql="SELECT {{ current_user_id() }} as my_user_id", + fetch_metadata=False, columns=[ TableColumn( column_name="my_user_id", @@ -446,10 +473,7 @@ def test_get_dataset_render_jinja(self): expression="{{ url_param('multiplier') }} * 1.4", ) ], - sql="SELECT {{ current_user_id() }} as my_user_id", ) - db.session.add(dataset) - db.session.commit() self.login(ADMIN_USERNAME) admin = self.get_user(ADMIN_USERNAME) @@ -493,12 +517,11 @@ def test_get_dataset_render_jinja_exceptions(self): Dataset API: Test get dataset with the render parameter when rendering raises an exception. """ - database = get_example_database() - dataset = SqlaTable( + dataset = self.insert_dataset( table_name="test_sql_table_with_incorrect_jinja", - database=database, - schema=get_example_default_schema(), - main_dttm_col="default_dttm", + owners=[], + sql="SELECT {{ current_user_id() } as my_user_id", + fetch_metadata=False, columns=[ TableColumn( column_name="my_user_id", @@ -518,10 +541,7 @@ def test_get_dataset_render_jinja_exceptions(self): expression="{{ url_param('multiplier') } * 1.4", ) ], - sql="SELECT {{ current_user_id() } as my_user_id", ) - db.session.add(dataset) - db.session.commit() self.login(ADMIN_USERNAME) @@ -679,6 +699,7 @@ def test_info_security_dataset(self): "can_duplicate", "can_get_or_create_dataset", "can_warm_up_cache", + "can_get_drill_info", } def test_create_dataset_item(self): @@ -2716,17 +2737,12 @@ def test_get_datasets_is_certified_filter(self): """ Dataset API: Test custom dataset_is_certified filter """ - - table_w_certification = SqlaTable( + table_w_certification = self.insert_dataset( table_name="foo", - schema=None, owners=[], - database=get_main_database(), - sql=None, + fetch_metadata=False, extra='{"certification": 1}', ) - db.session.add(table_w_certification) - db.session.commit() arguments = { "filters": [{"col": "id", "opr": "dataset_is_certified", "value": True}] @@ -2999,3 +3015,478 @@ def test_warm_up_cache_table_not_found(self): assert data == { "message": "The provided table was not found in the provided database" } + + def test_get_drill_info_admin_user(self): + """ + Dataset API: Test drill_info endpoint returns metadata for admin users, even + without a dashboard param. + """ + self.login(ADMIN_USERNAME) + dataset = self.insert_dataset( + table_name="test_drill_dataset", + owners=[], + columns=[ + TableColumn( + column_name="category", + type="VARCHAR(255)", + verbose_name="Category Column", + groupby=True, + ), + TableColumn( + column_name="region", + type="VARCHAR(255)", + groupby=True, + ), + TableColumn( + column_name="value", + type="VARCHAR(255)", + groupby=False, + ), + TableColumn( + column_name="description", + type="VARCHAR(255)", + groupby=False, + ), + ], + fetch_metadata=False, + ) + + # Test the drill_info endpoint + uri = f"api/v1/dataset/{dataset.id}/drill_info/" + rv = self.get_assert_metric(uri, "get_drill_info") + assert rv.status_code == 200 + + data = json.loads(rv.data.decode("utf-8")) + result = data["result"] + + # Verify admin gets full dataset metadata + assert "created_by" in result + assert "created_on_humanized" in result + assert "changed_by" in result + assert "changed_on_humanized" in result + assert result["id"] == dataset.id + assert result["table_name"] == "test_drill_dataset" + assert result["owners"] == [] + assert len(result["columns"]) == 2 + assert result["columns"] == [ + {"column_name": "category", "verbose_name": "Category Column"}, + {"column_name": "region", "verbose_name": None}, + ] + + self.items_to_delete = [dataset] + + def test_get_drill_info_admin_user_dataset_not_found(self): + """ + Dataset API: Test drill_info endpoint returns 404 for non-existent dataset. + """ + self.login(ADMIN_USERNAME) + uri = "api/v1/dataset/99999/drill_info/" + rv = self.client.get(uri) + + assert rv.status_code == 404 + + def test_get_drill_info_no_perm_to_drill(self): + """ + Dataset API: Test drill_info endpoint returns 403 for users without permission + to access the API. + """ + dataset = self.insert_dataset(table_name="foo", owners=[], fetch_metadata=False) + + # Log in as alpha for dataset access but remove pvm access + with self.temporary_user( + clone_user=security_manager.find_user(username=ALPHA_USERNAME), + pvms_to_remove=[("can_get_drill_info", "Dataset")], + login=True, + ): + uri = f"api/v1/dataset/{dataset.id}/drill_info/" + rv = self.client.get(uri) + + assert rv.status_code == 403 + + self.items_to_delete = [dataset] + + @patch("superset.security.manager.SupersetSecurityManager.has_guest_access") + @patch("superset.security.manager.SupersetSecurityManager.is_guest_user") + @with_feature_flags(EMBEDDED_SUPERSET=True) + def test_get_drill_info_embedded_user_no_perm_to_drill( + self, mock_is_guest_user, mock_has_guest_access + ): + """ + Dataset API: Test drill_info endpoint returns 403 for embedded users when + the role does not have permission. + """ + dataset = self.insert_dataset( + table_name="test_embedded_dataset", + owners=[], + columns=[ + TableColumn( + column_name="category", + type="VARCHAR(255)", + verbose_name="Category Column", + groupby=True, + ), + TableColumn( + column_name="region", + type="VARCHAR(255)", + groupby=True, + ), + ], + fetch_metadata=False, + ) + chart = self.insert_chart("Test Embedded Chart", dataset.id) + dash = self.insert_dashboard( + "Embedded Test Dashboard", "embedded-test-dashboard", [], slices=[chart] + ) + + # Log in to role without `can_get_drill_info` permission, and mock guest checks + with self.temporary_user( + clone_user=security_manager.find_user(username=GAMMA_USERNAME), + pvms_to_remove=[("can_get_drill_info", "Dataset")], + login=True, + ): + mock_is_guest_user.return_value = True + mock_has_guest_access.return_value = True + + uri = f"api/v1/dataset/{dataset.id}/drill_info/?q=(dashboard_id:{dash.id})" + rv = self.client.get(uri) + + assert rv.status_code == 403 + + self.items_to_delete = [dash, chart, dataset] + + @patch("superset.security.manager.SupersetSecurityManager.has_guest_access") + @patch("superset.security.manager.SupersetSecurityManager.is_guest_user") + @with_feature_flags(EMBEDDED_SUPERSET=True) + def test_get_drill_info_embedded_user_with_dashboard_id( + self, mock_is_guest_user, mock_has_guest_access + ): + """ + Dataset API: Test drill_info endpoint with dashboard ID parameter for + embedded users. + """ + dataset = self.insert_dataset( + table_name="test_embedded_dataset", + owners=[], + columns=[ + TableColumn( + column_name="category", + type="VARCHAR(255)", + verbose_name="Category Column", + groupby=True, + ), + TableColumn( + column_name="region", + type="VARCHAR(255)", + groupby=True, + ), + ], + fetch_metadata=False, + ) + chart = self.insert_chart("Test Embedded Chart", dataset.id) + dash = self.insert_dashboard( + "Embedded Test Dashboard", "embedded-test-dashboard", [], slices=[chart] + ) + + with self.temporary_user( + clone_user=security_manager.find_user(username=GAMMA_USERNAME), + login=True, + ): + mock_is_guest_user.return_value = True + mock_has_guest_access.return_value = True + + uri = f"api/v1/dataset/{dataset.id}/drill_info/?q=(dashboard_id:{dash.id})" + rv = self.client.get(uri) + + assert rv.status_code == 200 + data = json.loads(rv.data.decode("utf-8")) + result = data["result"] + assert result == { + "id": dataset.id, + "columns": [ + {"column_name": "category", "verbose_name": "Category Column"}, + {"column_name": "region", "verbose_name": None}, + ], + } + + self.items_to_delete = [dash, chart, dataset] + + @patch("superset.security.manager.SupersetSecurityManager.has_guest_access") + @patch("superset.security.manager.SupersetSecurityManager.is_guest_user") + @with_feature_flags(EMBEDDED_SUPERSET=True) + def test_get_drill_info_embedded_user_without_dashboard_parameter( + self, mock_is_guest_user, mock_has_guest_access + ): + """ + Dataset API: Test drill_info endpoint without dashboard ID parameter + for embedded users. + """ + dataset = self.insert_dataset( + table_name="test_embedded_dataset", + owners=[], + columns=[ + TableColumn( + column_name="category", + type="VARCHAR(255)", + verbose_name="Category Column", + groupby=True, + ), + TableColumn( + column_name="region", + type="VARCHAR(255)", + groupby=True, + ), + ], + fetch_metadata=False, + ) + chart = self.insert_chart("Test Embedded Chart", dataset.id) + dashboard = self.insert_dashboard( + "Embedded Test Dashboard", "embedded-test-dashboard", [], slices=[chart] + ) + + with self.temporary_user( + clone_user=security_manager.find_user(username=GAMMA_USERNAME), + login=True, + ): + mock_is_guest_user.return_value = True + mock_has_guest_access.return_value = True + + uri = f"api/v1/dataset/{dataset.id}/drill_info/" + rv = self.client.get(uri) + + assert rv.status_code == 403 + + self.items_to_delete = [dashboard, chart, dataset] + + @patch("superset.security.manager.SupersetSecurityManager.has_guest_access") + @patch("superset.security.manager.SupersetSecurityManager.is_guest_user") + @with_feature_flags(EMBEDDED_SUPERSET=True) + def test_get_drill_info_embedded_user_dashboard_without_dataset( + self, mock_is_guest_user, mock_has_guest_access + ): + """ + Dataset API: Test drill_info with dashboard ID that user has access to but + does not contain the dataset. + """ + dataset = self.insert_dataset( + table_name="test_d2d_table", + owners=[], + columns=[ + TableColumn( + column_name="category", + type="VARCHAR(255)", + groupby=True, + ), + ], + fetch_metadata=False, + ) + dashboard_dataset = self.insert_dataset( + table_name="test_dashboard_dataset", + owners=[], + fetch_metadata=False, + ) + chart = self.insert_chart("Dashboard Chart", dashboard_dataset.id) + dash = self.insert_dashboard( + "Dashboard Without Test Dataset", + "dashboard-without-test-dataset", + [], + slices=[chart], + ) + + with self.temporary_user( + clone_user=security_manager.find_user(username=GAMMA_USERNAME), + login=True, + ): + mock_is_guest_user.return_value = True + mock_has_guest_access.return_value = True + + uri = f"api/v1/dataset/{dataset.id}/drill_info/?q=(dashboard_id:{dash.id})" + rv = self.client.get(uri) + + assert rv.status_code == 403 + + self.items_to_delete = [dash, chart, dataset, dashboard_dataset] + + @with_feature_flags(DASHBOARD_RBAC=True) + def test_get_drill_info_dashboard_rbac_access_granted(self): + """ + Dataset API: Test drill_info with dashboard parameter when user has access + via the DASHBOARD_RBAC FF. + """ + with self.temporary_user( + clone_user=security_manager.find_user(username=GAMMA_USERNAME) + ) as test_user: + user_role_ids = [role.id for role in test_user.roles] + # Login as admin to avoid FK issues during temp account deletion + self.login(ADMIN_USERNAME) + dataset = self.insert_dataset( + table_name="test_rbac_dataset", + owners=[], + columns=[ + TableColumn( + column_name="restricted_column", + type="VARCHAR(255)", + verbose_name="Restricted Column", + groupby=True, + ), + ], + fetch_metadata=False, + ) + chart = self.insert_chart("Test RBAC Chart", dataset.id) + dash = self.insert_dashboard( + "RBAC Test Dashboard", + "rbac-test-dashboard", + [], + roles=user_role_ids, + slices=[chart], + published=True, + ) + + self.logout() + self.login(test_user.username) + + uri = f"api/v1/dataset/{dataset.id}/drill_info/?q=(dashboard_id:{dash.id})" + rv = self.client.get(uri) + + assert rv.status_code == 200 + data = json.loads(rv.data.decode("utf-8")) + result = data["result"] + + assert "created_by" in result + assert "created_on_humanized" in result + assert "changed_by" in result + assert "changed_on_humanized" in result + assert result["id"] == dataset.id + assert result["table_name"] == "test_rbac_dataset" + assert len(result["columns"]) == 1 + assert result["columns"][0]["column_name"] == "restricted_column" + + self.items_to_delete = [dash, chart, dataset] + + @with_feature_flags(DASHBOARD_RBAC=True) + def test_get_drill_info_dashboard_rbac_no_perm_to_drill(self): + """ + Dataset API: Test drill_info with dashboard parameter when user has + no permission to access the API. + """ + with self.temporary_user( + clone_user=security_manager.find_user(username=GAMMA_USERNAME), + pvms_to_remove=[("can_get_drill_info", "Dataset")], + ) as test_user: + user_role_ids = [role.id for role in test_user.roles] + self.login(ADMIN_USERNAME) + + dataset = self.insert_dataset( + table_name="test_rbac_dataset_denied", + owners=[], + columns=[ + TableColumn( + column_name="restricted_column", + type="VARCHAR(255)", + groupby=True, + ), + ], + fetch_metadata=False, + ) + chart = self.insert_chart("Test RBAC Chart second", dataset.id) + dash = self.insert_dashboard( + "RBAC Test Dashboard 2", + "rbac-test-dashboard-2", + [], + slices=[chart], + roles=user_role_ids, + published=True, + ) + + self.logout() + self.login(test_user.username) + + uri = f"api/v1/dataset/{dataset.id}/drill_info/?q=(dashboard_id:{dash.id})" + rv = self.client.get(uri) + + assert rv.status_code == 403 + + self.items_to_delete = [dash, chart, dataset] + + @with_feature_flags(DASHBOARD_RBAC=True) + def test_get_drill_info_dashboard_rbac_no_access_on_dashboard(self): + """ + Dataset API: Test drill_info with dashboard parameter when user has + no access to the dashboard. + """ + dataset = self.insert_dataset( + table_name="test_rbac_dataset_denied", + owners=[], + columns=[ + TableColumn( + column_name="restricted_column", + type="VARCHAR(255)", + groupby=True, + ), + ], + fetch_metadata=False, + ) + chart = self.insert_chart("Test RBAC Chart second", dataset.id) + dash = self.insert_dashboard( + "RBAC Test Dashboard 2", + "rbac-test-dashboard-2", + [], + slices=[chart], + roles=[], + published=True, + ) + + with self.temporary_user( + clone_user=security_manager.find_user(username=GAMMA_USERNAME), + login=True, + username="test_new_account", + ): + uri = f"api/v1/dataset/{dataset.id}/drill_info/?q=(dashboard_id:{dash.id})" + rv = self.client.get(uri) + + assert rv.status_code == 403 + + self.items_to_delete = [dash, chart, dataset] + + @with_feature_flags(DASHBOARD_RBAC=True) + def test_get_drill_info_dashboard_rbac_no_dashboard_id(self): + """ + Dataset API: Test drill_info without dashboard ID parameter falls back + to regular access control. + """ + with self.temporary_user( + clone_user=security_manager.find_user(username=GAMMA_USERNAME), + ) as test_user: + self.login(ADMIN_USERNAME) + user_role_ids = [role.id for role in test_user.roles] + + dataset = self.insert_dataset( + table_name="test_no_dashboard_id", + owners=[], + columns=[ + TableColumn( + column_name="restricted_column", + type="VARCHAR(255)", + groupby=True, + ), + ], + fetch_metadata=False, + ) + chart = self.insert_chart("Test RBAC Chart second", dataset.id) + dashboard = self.insert_dashboard( + "RBAC Test Dashboard 2", + "rbac-test-dashboard-2", + [], + slices=[chart], + roles=user_role_ids, + published=True, + ) + + self.logout() + self.login(test_user.username) + + uri = f"api/v1/dataset/{dataset.id}/drill_info/" + rv = self.client.get(uri) + + assert rv.status_code == 404 + + self.items_to_delete = [dashboard, chart, dataset] diff --git a/tests/integration_tests/datasource_tests.py b/tests/integration_tests/datasource_tests.py index 0053374a3568..65c063c5811d 100644 --- a/tests/integration_tests/datasource_tests.py +++ b/tests/integration_tests/datasource_tests.py @@ -44,12 +44,16 @@ ) from tests.integration_tests.base_tests import db_insert_temp_object, SupersetTestCase from tests.integration_tests.conftest import with_feature_flags -from tests.integration_tests.constants import ADMIN_USERNAME +from tests.integration_tests.constants import ADMIN_USERNAME, GAMMA_USERNAME from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, # noqa: F401 load_birth_names_data, # noqa: F401 ) from tests.integration_tests.fixtures.datasource import get_datasource_post +from tests.integration_tests.fixtures.world_bank_dashboard import ( + load_world_bank_dashboard_with_slices, # noqa: F401 + load_world_bank_data, # noqa: F401 +) @contextmanager @@ -516,6 +520,72 @@ def test_get_datasource_invalid_datasource_failed(self): resp = self.get_json_resp("/datasource/get/druid/500000/", raise_on_error=False) assert resp.get("error") == "'druid' is not a valid DatasourceType" + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @mock.patch( + "superset.security.manager.SupersetSecurityManager.get_guest_rls_filters" + ) + @mock.patch("superset.security.manager.SupersetSecurityManager.is_guest_user") + @mock.patch("superset.security.manager.SupersetSecurityManager.has_guest_access") + @with_feature_flags(EMBEDDED_SUPERSET=True) + def test_get_samples_embedded_user( + self, mock_has_guest_access, mock_is_guest_user, mock_rls + ): + """ + Embedded user can access the /samples view. + """ + self.login(ADMIN_USERNAME) + mock_is_guest_user.return_value = True + mock_has_guest_access.return_value = True + mock_rls.return_value = [] + tbl = self.get_table(name="birth_names") + dash = self.get_dash_by_slug("births") + uri = f"/datasource/samples?datasource_id={tbl.id}&datasource_type=table&dashboard_id={dash.id}" # noqa: E501 + resp = self.client.post(uri, json={}) + assert resp.status_code == 200 + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @mock.patch( + "superset.security.manager.SupersetSecurityManager.get_guest_rls_filters" + ) + @mock.patch("superset.security.manager.SupersetSecurityManager.is_guest_user") + @with_feature_flags(EMBEDDED_SUPERSET=True) + def test_get_samples_embedded_user_without_dash_id( + self, mock_is_guest_user, mock_rls + ): + """ + Embedded user can't access the /samples view if not providing a dashboard ID. + """ + self.login(GAMMA_USERNAME) + mock_is_guest_user.return_value = True + mock_rls.return_value = [] + tbl = self.get_table(name="birth_names") + uri = f"/datasource/samples?datasource_id={tbl.id}&datasource_type=table" + resp = self.client.post(uri, json={}) + assert resp.status_code == 403 + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + @mock.patch( + "superset.security.manager.SupersetSecurityManager.get_guest_rls_filters" + ) + @mock.patch("superset.security.manager.SupersetSecurityManager.is_guest_user") + @with_feature_flags(EMBEDDED_SUPERSET=True) + def test_get_samples_embedded_user_dashboard_without_dataset( + self, mock_is_guest_user, mock_rls + ): + """ + Embedded user can't access the /samples view when providing a dashboard ID that + does not include the target dataset. + """ + self.login(GAMMA_USERNAME) + mock_is_guest_user.return_value = True + mock_rls.return_value = [] + tbl = self.get_table(name="birth_names") + dash = self.get_dash_by_slug("world_health") + uri = f"/datasource/samples?datasource_id={tbl.id}&datasource_type=table&dashboard_id={dash.id}" # noqa: E501 + resp = self.client.post(uri, json={}) + assert resp.status_code == 403 + def test_get_samples(test_client, login_as_admin, virtual_dataset): """