From 6c029ce2e81bfd46adb36f15dc7ab039fe625ac7 Mon Sep 17 00:00:00 2001 From: Geido <60598000+geido@users.noreply.github.com> Date: Mon, 29 Jan 2024 18:28:10 +0100 Subject: [PATCH] chore: Add permission to view and drill on Dashboard context (#26798) --- .../ChartContextMenu/ChartContextMenu.tsx | 12 +++- .../ChartContextMenu/useContextMenu.test.tsx | 36 ++++++++++- .../Chart/DrillBy/DrillByModal.test.tsx | 36 ++++++++++- .../components/Chart/DrillBy/DrillByModal.tsx | 17 ++++- .../DrillDetail/DrillDetailModal.test.tsx | 26 +++++++- .../Chart/DrillDetail/DrillDetailModal.tsx | 63 ++++++++++++++----- .../SliceHeaderControls.test.tsx | 53 ++++++++++++++-- .../components/SliceHeaderControls/index.tsx | 32 ++++++++-- superset/security/manager.py | 1 + tests/integration_tests/security_tests.py | 2 + 10 files changed, 242 insertions(+), 36 deletions(-) diff --git a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx index 6ab80aad9..ce8bb9d7d 100644 --- a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx +++ b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx @@ -90,6 +90,12 @@ const ChartContextMenu = ( const canExplore = useSelector((state: RootState) => findPermission('can_explore', 'Superset', state.user?.roles), ); + const canViewDrill = useSelector((state: RootState) => + findPermission('can_view_and_drill', 'Dashboard', state.user?.roles), + ); + const canDatasourceSamples = useSelector((state: RootState) => + findPermission('can_samples', 'Datasource', state.user?.roles), + ); const crossFiltersEnabled = useSelector( ({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled, ); @@ -105,15 +111,17 @@ const ChartContextMenu = ( }>({ clientX: 0, clientY: 0 }); const menuItems = []; + const canExploreOrView = canExplore || canViewDrill; const showDrillToDetail = isFeatureEnabled(FeatureFlag.DRILL_TO_DETAIL) && - canExplore && + canExploreOrView && + canDatasourceSamples && isDisplayed(ContextMenuItem.DrillToDetail); const showDrillBy = isFeatureEnabled(FeatureFlag.DRILL_BY) && - canExplore && + canExploreOrView && isDisplayed(ContextMenuItem.DrillBy); const showCrossFilters = diff --git a/superset-frontend/src/components/Chart/ChartContextMenu/useContextMenu.test.tsx b/superset-frontend/src/components/Chart/ChartContextMenu/useContextMenu.test.tsx index ebab36b14..0d65bb603 100644 --- a/superset-frontend/src/components/Chart/ChartContextMenu/useContextMenu.test.tsx +++ b/superset-frontend/src/components/Chart/ChartContextMenu/useContextMenu.test.tsx @@ -38,10 +38,12 @@ const setup = ({ onSelection = noOp, displayedItems = ContextMenuItem.All, additionalConfig = {}, + roles = undefined, }: { onSelection?: () => void; displayedItems?: ContextMenuItem | ContextMenuItem[]; additionalConfig?: Record; + roles?: Record; } = {}) => { const { result } = renderHook(() => useContextMenu( @@ -58,7 +60,12 @@ const setup = ({ ...mockState, user: { ...mockState.user, - roles: { Admin: [['can_explore', 'Superset']] }, + roles: roles ?? { + Admin: [ + ['can_explore', 'Superset'], + ['can_samples', 'Datasource'], + ], + }, }, }, }); @@ -75,7 +82,7 @@ test('Context menu renders', () => { expect(screen.getByText('Drill by')).toBeInTheDocument(); }); -test('Context menu contains all items only', () => { +test('Context menu contains all displayed items only', () => { const result = setup({ displayedItems: [ContextMenuItem.DrillToDetail, ContextMenuItem.DrillBy], }); @@ -84,3 +91,28 @@ test('Context menu contains all items only', () => { expect(screen.getByText('Drill to detail')).toBeInTheDocument(); expect(screen.getByText('Drill by')).toBeInTheDocument(); }); + +test('Context menu shows all items tied to can_view_and_drill permission', () => { + const result = setup({ + roles: { + Admin: [ + ['can_view_and_drill', 'Dashboard'], + ['can_samples', 'Datasource'], + ], + }, + }); + result.current.onContextMenu(0, 0, {}); + expect(screen.getByText('Drill to detail')).toBeInTheDocument(); + expect(screen.getByText('Drill by')).toBeInTheDocument(); + expect(screen.getByText('Add cross-filter')).toBeInTheDocument(); +}); + +test('Context menu does not show "Drill to detail" without proper permissions', () => { + const result = setup({ + roles: { Admin: [['can_view_and_drill', 'Dashboard']] }, + }); + result.current.onContextMenu(0, 0, {}); + expect(screen.queryByText('Drill to detail')).not.toBeInTheDocument(); + expect(screen.getByText('Drill by')).toBeInTheDocument(); + expect(screen.getByText('Add cross-filter')).toBeInTheDocument(); +}); diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByModal.test.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByModal.test.tsx index 95e2f6027..fb43fbfcb 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByModal.test.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByModal.test.tsx @@ -68,7 +68,10 @@ const dataset = { ], }; -const renderModal = async (modalProps: Partial = {}) => { +const renderModal = async ( + modalProps: Partial = {}, + overrideState: Record = {}, +) => { const DrillByModalWrapper = () => { const [showModal, setShowModal] = useState(false); @@ -93,7 +96,10 @@ const renderModal = async (modalProps: Partial = {}) => { useDnd: true, useRedux: true, useRouter: true, - initialState: drillByModalState, + initialState: { + ...drillByModalState, + ...overrideState, + }, }); userEvent.click(screen.getByRole('button', { name: 'Show modal' })); @@ -233,3 +239,29 @@ test('render breadcrumbs', async () => { expect(newBreadcrumbItems).toHaveLength(1); expect(within(breadcrumbItems[0]).getByText('gender')).toBeInTheDocument(); }); + +test('should render "Edit chart" as disabled without can_explore permission', async () => { + await renderModal( + {}, + { + user: { + ...drillByModalState.user, + roles: { Admin: [['test_invalid_role', 'Superset']] }, + }, + }, + ); + expect(screen.getByRole('button', { name: 'Edit chart' })).toBeDisabled(); +}); + +test('should render "Edit chart" enabled with can_explore permission', async () => { + await renderModal( + {}, + { + user: { + ...drillByModalState.user, + roles: { Admin: [['can_explore', 'Superset']] }, + }, + }, + ); + expect(screen.getByRole('button', { name: 'Edit chart' })).toBeEnabled(); +}); diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx index d1e8b39e0..8662b7582 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx @@ -54,6 +54,7 @@ import { LOG_ACTIONS_DRILL_BY_MODAL_OPENED, LOG_ACTIONS_FURTHER_DRILL_BY, } from 'src/logger/LogUtils'; +import { findPermission } from 'src/utils/findPermission'; import { Dataset, DrillByType } from '../types'; import DrillByChart from './DrillByChart'; import { ContextMenuItem } from '../ChartContextMenu/ChartContextMenu'; @@ -75,6 +76,7 @@ interface ModalFooterProps { const ModalFooter = ({ formData, closeModal }: ModalFooterProps) => { const dispatch = useDispatch(); const { addDangerToast } = useToasts(); + const theme = useTheme(); const [url, setUrl] = useState(''); const dashboardPageId = useContext(DashboardPageIdContext); const onEditChartClick = useCallback(() => { @@ -84,6 +86,9 @@ const ModalFooter = ({ formData, closeModal }: ModalFooterProps) => { }), ); }, [dispatch, formData.slice_id]); + const canExplore = useSelector((state: RootState) => + findPermission('can_explore', 'Superset', state.user?.roles), + ); const [datasource_id, datasource_type] = formData.datasource.split('__'); useEffect(() => { @@ -103,13 +108,20 @@ const ModalFooter = ({ formData, closeModal }: ModalFooterProps) => { datasource_type, formData, ]); + const isEditDisabled = !url || !canExplore; + return ( <> diff --git a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx index 038541d39..5b6cc7124 100644 --- a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx +++ b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx @@ -35,9 +35,16 @@ jest.mock('react-router-dom', () => ({ const { id: chartId, form_data: formData } = chartQueries[sliceId]; const { slice_name: chartName } = formData; +const store = getMockStoreWithNativeFilters(); +const drillToDetailModalState = { + ...store.getState(), + user: { + ...store.getState().user, + roles: { Admin: [['can_explore', 'Superset']] }, + }, +}; -const renderModal = async () => { - const store = getMockStoreWithNativeFilters(); +const renderModal = async (overrideState: Record = {}) => { const DrillDetailModalWrapper = () => { const [showModal, setShowModal] = useState(false); return ( @@ -59,7 +66,10 @@ const renderModal = async () => { render(, { useRouter: true, useRedux: true, - store, + initialState: { + ...drillToDetailModalState, + ...overrideState, + }, }); userEvent.click(screen.getByRole('button', { name: 'Show modal' })); @@ -93,3 +103,13 @@ test('should forward to Explore', async () => { `/explore/?dashboard_page_id=&slice_id=${sliceId}`, ); }); + +test('should render "Edit chart" as disabled without can_explore permission', async () => { + await renderModal({ + user: { + ...drillToDetailModalState.user, + roles: { Admin: [['test_invalid_role', 'Superset']] }, + }, + }); + expect(screen.getByRole('button', { name: 'Edit chart' })).toBeDisabled(); +}); diff --git a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx index e1207f8aa..26c31b584 100644 --- a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx +++ b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx @@ -31,28 +31,52 @@ import Button from 'src/components/Button'; import { useSelector } from 'react-redux'; import { DashboardPageIdContext } from 'src/dashboard/containers/DashboardPage'; import { Slice } from 'src/types/Chart'; +import { RootState } from 'src/dashboard/types'; +import { findPermission } from 'src/utils/findPermission'; import DrillDetailPane from './DrillDetailPane'; interface ModalFooterProps { - exploreChart: () => void; + canExplore: boolean; closeModal?: () => void; + exploreChart: () => void; } -const ModalFooter = ({ exploreChart, closeModal }: ModalFooterProps) => ( - <> - - - -); +const ModalFooter = ({ + canExplore, + closeModal, + exploreChart, +}: ModalFooterProps) => { + const theme = useTheme(); + + return ( + <> + + + + ); +}; interface DrillDetailModalProps { chartId: number; @@ -76,6 +100,9 @@ export default function DrillDetailModal({ (state: { sliceEntities: { slices: Record } }) => state.sliceEntities.slices[chartId], ); + const canExplore = useSelector((state: RootState) => + findPermission('can_explore', 'Superset', state.user?.roles), + ); const exploreUrl = useMemo( () => `/explore/?dashboard_page_id=${dashboardPageId}&slice_id=${chartId}`, @@ -97,7 +124,9 @@ export default function DrillDetailModal({ } `} title={t('Drill to detail: %s', chartName)} - footer={} + footer={ + + } responsive resizable resizableConfig={{ diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx index 83099e549..dc94e77ca 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx @@ -99,13 +99,23 @@ const createProps = (viz_type = 'sunburst_v2') => exploreUrl: '/explore', } as SliceHeaderControlsProps); -const renderWrapper = (overrideProps?: SliceHeaderControlsProps) => { +const renderWrapper = ( + overrideProps?: SliceHeaderControlsProps, + roles?: Record, +) => { const props = overrideProps || createProps(); const store = getMockStore(); + const mockState = store.getState(); return render(, { useRedux: true, useRouter: true, - store, + initialState: { + ...mockState, + user: { + ...mockState.user, + roles: roles ?? mockState.user.roles, + }, + }, }); }; @@ -295,13 +305,48 @@ test('Drill to detail modal is under featureflag', () => { expect(screen.queryByText('Drill to detail')).not.toBeInTheDocument(); }); -test('Should show the "Drill to detail"', () => { +test('Should show "Drill to detail"', () => { // @ts-ignore global.featureFlags = { [FeatureFlag.DRILL_TO_DETAIL]: true, }; const props = createProps(); props.slice.slice_id = 18; - renderWrapper(props); + renderWrapper(props, { + Admin: [ + ['can_view_and_drill', 'Dashboard'], + ['can_samples', 'Datasource'], + ], + }); expect(screen.getByText('Drill to detail')).toBeInTheDocument(); }); + +test('Should show menu items tied to can_view_and_drill permission', () => { + // @ts-ignore + global.featureFlags = { + [FeatureFlag.DRILL_TO_DETAIL]: true, + }; + const props = { + ...createProps(), + supersetCanExplore: false, + }; + props.slice.slice_id = 18; + renderWrapper(props, { + Admin: [['can_view_and_drill', 'Dashboard']], + }); + expect(screen.getByText('View query')).toBeInTheDocument(); + expect(screen.getByText('View as table')).toBeInTheDocument(); + expect(screen.queryByText('Drill to detail')).not.toBeInTheDocument(); +}); + +test('Should not show the "Edit chart" without proper permissions', () => { + const props = { + ...createProps(), + supersetCanExplore: false, + }; + props.slice.slice_id = 18; + renderWrapper(props, { + Admin: [['can_view_and_drill', 'Dashboard']], + }); + expect(screen.queryByText('Edit chart')).not.toBeInTheDocument(); +}); diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx index 305bc3843..b1196e7c1 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx @@ -57,6 +57,7 @@ import Modal from 'src/components/Modal'; import { DrillDetailMenuItems } from 'src/components/Chart/DrillDetail'; import { LOG_ACTIONS_CHART_DOWNLOAD_AS_IMAGE } from 'src/logger/LogUtils'; import { RootState } from 'src/dashboard/types'; +import { findPermission } from 'src/utils/findPermission'; import { useCrossFiltersScopingModal } from '../nativeFilters/FilterBar/CrossFilters/ScopingModal/useCrossFiltersScopingModal'; const MENU_KEYS = { @@ -170,11 +171,13 @@ const dropdownIconsStyles = css` `; const ViewResultsModalTrigger = ({ + canExplore, exploreUrl, triggerNode, modalTitle, modalBody, }: { + canExplore?: boolean; exploreUrl: string; triggerNode: ReactChild; modalTitle: ReactChild; @@ -214,6 +217,14 @@ const ViewResultsModalTrigger = ({ buttonStyle="secondary" buttonSize="small" onClick={exploreChart} + disabled={!canExplore} + tooltip={ + !canExplore + ? t( + 'You do not have sufficient permissions to edit the chart', + ) + : undefined + } > {t('Edit chart')} @@ -221,6 +232,9 @@ const ViewResultsModalTrigger = ({ buttonStyle="primary" buttonSize="small" onClick={closeModal} + css={css` + margin-left: ${theme.gridUnit * 2}px; + `} > {t('Close')} @@ -259,7 +273,13 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => { getChartMetadataRegistry() .get(props.slice.viz_type) ?.behaviors?.includes(Behavior.INTERACTIVE_CHART); - + const canViewDrill = useSelector((state: RootState) => + findPermission('can_view_and_drill', 'Dashboard', state.user?.roles), + ); + const canExploreOrView = props.supersetCanExplore || canViewDrill; + const canDatasourceSamples = useSelector((state: RootState) => + findPermission('can_samples', 'Datasource', state.user?.roles), + ); const refreshChart = () => { if (props.updatedDttm) { props.forceRefresh(props.slice.slice_id, props.dashboardId); @@ -428,7 +448,7 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => { )} - {props.supersetCanExplore && ( + {canExploreOrView && ( { )} - {props.supersetCanExplore && ( + {canExploreOrView && ( {t('View as table')} @@ -465,14 +486,15 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => { )} {isFeatureEnabled(FeatureFlag.DRILL_TO_DETAIL) && - props.supersetCanExplore && ( + canExploreOrView && + canDatasourceSamples && ( )} - {(slice.description || props.supersetCanExplore) && } + {(slice.description || canExploreOrView) && } {supersetCanShare && ( diff --git a/superset/security/manager.py b/superset/security/manager.py index 7e1b69784..b29529edb 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -726,6 +726,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods self.add_permission_view_menu("can_csv", "Superset") self.add_permission_view_menu("can_share_dashboard", "Superset") self.add_permission_view_menu("can_share_chart", "Superset") + self.add_permission_view_menu("can_view_and_drill", "Dashboard") def create_missing_perms(self) -> None: """ diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 395aebf29..d9c98aa25 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -1337,6 +1337,7 @@ class TestRolePermission(SupersetTestCase): self.assertIn(("can_explore_json", "Superset"), perm_set) self.assertIn(("can_explore_json", "Superset"), perm_set) self.assertIn(("can_userinfo", "UserDBModelView"), perm_set) + self.assertIn(("can_view_and_drill", "Dashboard"), perm_set) self.assert_can_menu("Databases", perm_set) self.assert_can_menu("Datasets", perm_set) self.assert_can_menu("Data", perm_set) @@ -1504,6 +1505,7 @@ class TestRolePermission(SupersetTestCase): self.assertIn(("can_share_dashboard", "Superset"), gamma_perm_set) self.assertIn(("can_explore_json", "Superset"), gamma_perm_set) self.assertIn(("can_userinfo", "UserDBModelView"), gamma_perm_set) + self.assertIn(("can_view_and_drill", "Dashboard"), gamma_perm_set) def test_views_are_secured(self): """Preventing the addition of unsecured views without has_access decorator"""