This is an automated email from the ASF dual-hosted git repository.
diegopucci pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 6c029ce2e8 chore: Add permission to view and drill on Dashboard
context (#26798)
6c029ce2e8 is described below
commit 6c029ce2e81bfd46adb36f15dc7ab039fe625ac7
Author: Geido <[email protected]>
AuthorDate: Mon Jan 29 18:28:10 2024 +0100
chore: Add permission to view and drill on Dashboard context (#26798)
---
.../Chart/ChartContextMenu/ChartContextMenu.tsx | 12 ++++-
.../Chart/ChartContextMenu/useContextMenu.test.tsx | 36 ++++++++++++-
.../components/Chart/DrillBy/DrillByModal.test.tsx | 36 ++++++++++++-
.../src/components/Chart/DrillBy/DrillByModal.tsx | 17 +++++-
.../Chart/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 6ab80aad9e..ce8bb9d7d7 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<RootState, boolean>(
({ 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 ebab36b14f..0d65bb6038 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<string, any>;
+ roles?: Record<string, string[][]>;
} = {}) => {
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 95e2f6027f..fb43fbfcb3 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<DrillByModalProps> = {}) => {
+const renderModal = async (
+ modalProps: Partial<DrillByModalProps> = {},
+ overrideState: Record<string, any> = {},
+) => {
const DrillByModalWrapper = () => {
const [showModal, setShowModal] = useState(false);
@@ -93,7 +96,10 @@ const renderModal = async (modalProps:
Partial<DrillByModalProps> = {}) => {
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 d1e8b39e0c..8662b7582f 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 (
<>
<Button
buttonStyle="secondary"
buttonSize="small"
onClick={onEditChartClick}
- disabled={!url}
+ disabled={isEditDisabled}
+ tooltip={
+ isEditDisabled
+ ? t('You do not have sufficient permissions to edit the chart')
+ : undefined
+ }
>
<Link
css={css`
@@ -128,6 +140,9 @@ const ModalFooter = ({ formData, closeModal }:
ModalFooterProps) => {
buttonSize="small"
onClick={closeModal}
data-test="close-drill-by-modal"
+ css={css`
+ margin-left: ${theme.gridUnit * 2}px;
+ `}
>
{t('Close')}
</Button>
diff --git
a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx
b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx
index 038541d390..5b6cc71241 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<string, any> = {}) => {
const DrillDetailModalWrapper = () => {
const [showModal, setShowModal] = useState(false);
return (
@@ -59,7 +66,10 @@ const renderModal = async () => {
render(<DrillDetailModalWrapper />, {
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 e1207f8aac..26c31b5846 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) => (
- <>
- <Button buttonStyle="secondary" buttonSize="small" onClick={exploreChart}>
- {t('Edit chart')}
- </Button>
- <Button
- buttonStyle="primary"
- buttonSize="small"
- onClick={closeModal}
- data-test="close-drilltodetail-modal"
- >
- {t('Close')}
- </Button>
- </>
-);
+const ModalFooter = ({
+ canExplore,
+ closeModal,
+ exploreChart,
+}: ModalFooterProps) => {
+ const theme = useTheme();
+
+ return (
+ <>
+ <Button
+ 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')}
+ </Button>
+ <Button
+ buttonStyle="primary"
+ buttonSize="small"
+ onClick={closeModal}
+ data-test="close-drilltodetail-modal"
+ css={css`
+ margin-left: ${theme.gridUnit * 2}px;
+ `}
+ >
+ {t('Close')}
+ </Button>
+ </>
+ );
+};
interface DrillDetailModalProps {
chartId: number;
@@ -76,6 +100,9 @@ export default function DrillDetailModal({
(state: { sliceEntities: { slices: Record<number, Slice> } }) =>
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={<ModalFooter exploreChart={exploreChart} />}
+ footer={
+ <ModalFooter exploreChart={exploreChart} canExplore={canExplore} />
+ }
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 83099e5490..dc94e77cac 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<string, string[][]>,
+) => {
const props = overrideProps || createProps();
const store = getMockStore();
+ const mockState = store.getState();
return render(<SliceHeaderControls {...props} />, {
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 305bc38434..b1196e7c11 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')}
</Button>
@@ -221,6 +232,9 @@ const ViewResultsModalTrigger = ({
buttonStyle="primary"
buttonSize="small"
onClick={closeModal}
+ css={css`
+ margin-left: ${theme.gridUnit * 2}px;
+ `}
>
{t('Close')}
</Button>
@@ -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 && (
<Menu.Item key={MENU_KEYS.VIEW_QUERY}>
<ModalTrigger
triggerNode={
@@ -443,9 +463,10 @@ const SliceHeaderControls = (props:
SliceHeaderControlsPropsWithRouter) => {
</Menu.Item>
)}
- {props.supersetCanExplore && (
+ {canExploreOrView && (
<Menu.Item key={MENU_KEYS.VIEW_RESULTS}>
<ViewResultsModalTrigger
+ canExplore={props.supersetCanExplore}
exploreUrl={props.exploreUrl}
triggerNode={
<span data-test="view-query-menu-item">{t('View as
table')}</span>
@@ -465,14 +486,15 @@ const SliceHeaderControls = (props:
SliceHeaderControlsPropsWithRouter) => {
)}
{isFeatureEnabled(FeatureFlag.DRILL_TO_DETAIL) &&
- props.supersetCanExplore && (
+ canExploreOrView &&
+ canDatasourceSamples && (
<DrillDetailMenuItems
chartId={slice.slice_id}
formData={props.formData}
/>
)}
- {(slice.description || props.supersetCanExplore) && <Menu.Divider />}
+ {(slice.description || canExploreOrView) && <Menu.Divider />}
{supersetCanShare && (
<Menu.SubMenu title={t('Share')}>
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 7e1b697840..b29529edb2 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 395aebf29c..d9c98aa253 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"""
