This is an automated email from the ASF dual-hosted git repository. diegopucci pushed a commit to branch diego/ch77449/dashboard-granular-permissions in repository https://gitbox.apache.org/repos/asf/superset.git
commit aa55625e2488bb23d7171ec3612ff491e6d3b3ca Author: geido <[email protected]> AuthorDate: Tue Feb 6 13:11:30 2024 +0200 Add granular permissions --- .../Chart/ChartContextMenu/ChartContextMenu.tsx | 13 ++-- .../Chart/ChartContextMenu/useContextMenu.test.tsx | 34 +++++++--- .../components/Chart/DrillBy/DrillByModal.test.tsx | 2 +- .../Chart/DrillDetail/DrillDetailModal.test.tsx | 2 +- .../SliceHeaderControls.test.tsx | 78 ++++++++++++++++++---- .../components/SliceHeaderControls/index.tsx | 23 ++++--- superset/security/manager.py | 4 +- tests/integration_tests/security_tests.py | 6 +- 8 files changed, 120 insertions(+), 42 deletions(-) diff --git a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx index c03b1a1301..1e7fcf3d7f 100644 --- a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx +++ b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx @@ -90,8 +90,11 @@ 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 canDrillBy = useSelector((state: RootState) => + findPermission('can_write', 'ExploreFormDataRestAPI', state.user?.roles), + ); + const canDrillToDetail = useSelector((state: RootState) => + findPermission('can_drill_to_detail', 'Dashboard', state.user?.roles), ); const canDatasourceSamples = useSelector((state: RootState) => findPermission('can_samples', 'Datasource', state.user?.roles), @@ -111,17 +114,15 @@ const ChartContextMenu = ( }>({ clientX: 0, clientY: 0 }); const menuItems = []; - const canExploreOrView = canExplore || canViewDrill; const showDrillToDetail = isFeatureEnabled(FeatureFlag.DrillToDetail) && - canExploreOrView && - canDatasourceSamples && + ((canExplore && canDatasourceSamples) || canDrillToDetail) && isDisplayed(ContextMenuItem.DrillToDetail); const showDrillBy = isFeatureEnabled(FeatureFlag.DrillBy) && - canExploreOrView && + (canExplore || canDrillBy) && 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 2e7937ffbd..a78d35dece 100644 --- a/superset-frontend/src/components/Chart/ChartContextMenu/useContextMenu.test.tsx +++ b/superset-frontend/src/components/Chart/ChartContextMenu/useContextMenu.test.tsx @@ -92,27 +92,45 @@ test('Context menu contains all displayed items only', () => { expect(screen.getByText('Drill by')).toBeInTheDocument(); }); -test('Context menu shows all items tied to can_view_and_drill permission', () => { +test('Context menu shows "Drill by"', () => { + const result = setup({ + roles: { + Admin: [['can_write', 'ExploreFormDataRestAPI']], + }, + }); + result.current.onContextMenu(0, 0, {}); + expect(screen.getByText('Drill by')).toBeInTheDocument(); +}); + +test('Context menu does not show "Drill by"', () => { + const result = setup({ + roles: { + Admin: [['invalid_permission', 'Dashboard']], + }, + }); + result.current.onContextMenu(0, 0, {}); + expect(screen.queryByText('Drill by')).not.toBeInTheDocument(); +}); + +test('Context menu shows "Drill to detail"', () => { const result = setup({ roles: { Admin: [ - ['can_view_and_drill', 'Dashboard'], ['can_samples', 'Datasource'], + ['can_explore', 'Superset'], ], }, }); 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', () => { +test('Context menu does not show "Drill to detail"', () => { const result = setup({ - roles: { Admin: [['can_view_and_drill', 'Dashboard']] }, + roles: { + Admin: [['can_explore', 'Superset']], + }, }); 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 fb43fbfcb3..898c033927 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByModal.test.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByModal.test.tsx @@ -246,7 +246,7 @@ test('should render "Edit chart" as disabled without can_explore permission', as { user: { ...drillByModalState.user, - roles: { Admin: [['test_invalid_role', 'Superset']] }, + roles: { Admin: [['invalid_permission', 'Superset']] }, }, }, ); diff --git a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx index 5b6cc71241..f5185972cc 100644 --- a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx +++ b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx @@ -108,7 +108,7 @@ test('should render "Edit chart" as disabled without can_explore permission', as await renderModal({ user: { ...drillToDetailModalState.user, - roles: { Admin: [['test_invalid_role', 'Superset']] }, + roles: { Admin: [['invalid_permission', 'Superset']] }, }, }); expect(screen.getByRole('button', { name: 'Edit chart' })).toBeDisabled(); diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx index 31a3cad9f6..0970904843 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx @@ -19,9 +19,9 @@ import userEvent from '@testing-library/user-event'; import React from 'react'; -import { getMockStore } from 'spec/fixtures/mockStore'; import { render, screen } from 'spec/helpers/testing-library'; import { FeatureFlag } from '@superset-ui/core'; +import mockState from 'spec/fixtures/mockState'; import SliceHeaderControls, { SliceHeaderControlsProps } from '.'; jest.mock('src/components/Dropdown', () => { @@ -104,8 +104,6 @@ const renderWrapper = ( roles?: Record<string, string[][]>, ) => { const props = overrideProps || createProps(); - const store = getMockStore(); - const mockState = store.getState(); return render(<SliceHeaderControls {...props} />, { useRedux: true, useRouter: true, @@ -113,7 +111,9 @@ const renderWrapper = ( ...mockState, user: { ...mockState.user, - roles: roles ?? mockState.user.roles, + roles: roles ?? { + Admin: [['can_samples', 'Datasource']], + }, }, }, }); @@ -310,18 +310,18 @@ test('Should show "Drill to detail"', () => { global.featureFlags = { [FeatureFlag.DrillToDetail]: true, }; - const props = createProps(); + const props = { + ...createProps(), + supersetCanExplore: true, + }; props.slice.slice_id = 18; renderWrapper(props, { - Admin: [ - ['can_view_and_drill', 'Dashboard'], - ['can_samples', 'Datasource'], - ], + Admin: [['can_samples', 'Datasource']], }); expect(screen.getByText('Drill to detail')).toBeInTheDocument(); }); -test('Should show menu items tied to can_view_and_drill permission', () => { +test('Should not show "Drill to detail"', () => { // @ts-ignore global.featureFlags = { [FeatureFlag.DrillToDetail]: true, @@ -332,21 +332,71 @@ test('Should show menu items tied to can_view_and_drill permission', () => { }; props.slice.slice_id = 18; renderWrapper(props, { - Admin: [['can_view_and_drill', 'Dashboard']], + Admin: [['invalid_permission', 'Dashboard']], + }); + expect(screen.queryByText('Drill to detail')).not.toBeInTheDocument(); +}); + +test('Should show "View query"', () => { + const props = { + ...createProps(), + supersetCanExplore: false, + }; + props.slice.slice_id = 18; + renderWrapper(props, { + Admin: [['can_view_query', 'Dashboard']], }); expect(screen.getByText('View query')).toBeInTheDocument(); +}); + +test('Should not show "View query"', () => { + const props = { + ...createProps(), + supersetCanExplore: false, + }; + props.slice.slice_id = 18; + renderWrapper(props, { + Admin: [['invalid_permission', 'Dashboard']], + }); + expect(screen.queryByText('View query')).not.toBeInTheDocument(); +}); + +test('Should show "View as table"', () => { + const props = { + ...createProps(), + supersetCanExplore: false, + }; + props.slice.slice_id = 18; + renderWrapper(props, { + Admin: [['can_view_table', 'Dashboard']], + }); 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', () => { +test('Should not show "View as table"', () => { const props = { ...createProps(), supersetCanExplore: false, }; props.slice.slice_id = 18; renderWrapper(props, { - Admin: [['can_view_and_drill', 'Dashboard']], + Admin: [['invalid_permission', 'Dashboard']], + }); + expect(screen.queryByText('View as table')).not.toBeInTheDocument(); +}); + +test('Should not show the "Edit chart" button', () => { + const props = { + ...createProps(), + supersetCanExplore: false, + }; + props.slice.slice_id = 18; + renderWrapper(props, { + Admin: [ + ['can_samples', 'Datasource'], + ['can_view_query', 'Dashboard'], + ['can_view_table', '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 d4b9a1a0b9..5043ab26a3 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx @@ -273,13 +273,19 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => { getChartMetadataRegistry() .get(props.slice.viz_type) ?.behaviors?.includes(Behavior.InteractiveChart); - const canViewDrill = useSelector((state: RootState) => - findPermission('can_view_and_drill', 'Dashboard', state.user?.roles), + const canExplore = props.supersetCanExplore; + const canDrillToDetail = useSelector((state: RootState) => + findPermission('can_drill_to_detail', 'Dashboard', state.user?.roles), ); - const canExploreOrView = props.supersetCanExplore || canViewDrill; const canDatasourceSamples = useSelector((state: RootState) => findPermission('can_samples', 'Datasource', state.user?.roles), ); + const canViewQuery = useSelector((state: RootState) => + findPermission('can_view_query', 'Dashboard', state.user?.roles), + ); + const canViewTable = useSelector((state: RootState) => + findPermission('can_view_table', 'Dashboard', state.user?.roles), + ); const refreshChart = () => { if (props.updatedDttm) { props.forceRefresh(props.slice.slice_id, props.dashboardId); @@ -429,7 +435,7 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => { </Menu.Item> )} - {props.supersetCanExplore && ( + {canExplore && ( <Menu.Item key={MENU_KEYS.EXPLORE_CHART}> <Link to={props.exploreUrl}> <Tooltip title={getSliceHeaderTooltip(props.slice.slice_name)}> @@ -448,7 +454,7 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => { </> )} - {canExploreOrView && ( + {(canExplore || canViewQuery) && ( <Menu.Item key={MENU_KEYS.VIEW_QUERY}> <ModalTrigger triggerNode={ @@ -463,7 +469,7 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => { </Menu.Item> )} - {canExploreOrView && ( + {(canExplore || canViewTable) && ( <Menu.Item key={MENU_KEYS.VIEW_RESULTS}> <ViewResultsModalTrigger canExplore={props.supersetCanExplore} @@ -486,15 +492,14 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => { )} {isFeatureEnabled(FeatureFlag.DrillToDetail) && - canExploreOrView && - canDatasourceSamples && ( + ((canExplore && canDatasourceSamples) || canDrillToDetail) && ( <DrillDetailMenuItems chartId={slice.slice_id} formData={props.formData} /> )} - {(slice.description || canExploreOrView) && <Menu.Divider />} + {(slice.description || canExplore) && <Menu.Divider />} {supersetCanShare && ( <Menu.SubMenu title={t('Share')}> diff --git a/superset/security/manager.py b/superset/security/manager.py index 8268b19411..ceaa99f8a2 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -726,7 +726,9 @@ 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") + self.add_permission_view_menu("can_view_query", "Dashboard") + self.add_permission_view_menu("can_view_table", "Dashboard") + self.add_permission_view_menu("can_drill_to_detail", "Dashboard") def create_missing_perms(self) -> None: """ diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index d9c98aa253..fb64f06330 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -1337,7 +1337,8 @@ 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.assertIn(("can_view_table", "Dashboard"), perm_set) + self.assertIn(("can_view_query", "Dashboard"), perm_set) self.assert_can_menu("Databases", perm_set) self.assert_can_menu("Datasets", perm_set) self.assert_can_menu("Data", perm_set) @@ -1505,7 +1506,8 @@ 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) + self.assertIn(("can_view_table", "Dashboard"), gamma_perm_set) + self.assertIn(("can_view_query", "Dashboard"), gamma_perm_set) def test_views_are_secured(self): """Preventing the addition of unsecured views without has_access decorator"""
