You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by di...@apache.org on 2024/02/09 14:25:37 UTC
(superset) branch master updated: chore: Add granular permissions for actions in Dashboard (#27029)
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 595c6ce3e6 chore: Add granular permissions for actions in Dashboard (#27029)
595c6ce3e6 is described below
commit 595c6ce3e6a02f6086d5987e50032ac3090c3122
Author: Geido <60...@users.noreply.github.com>
AuthorDate: Fri Feb 9 16:25:32 2024 +0200
chore: Add granular permissions for actions in Dashboard (#27029)
---
.../Chart/ChartContextMenu/ChartContextMenu.tsx | 12 +--
.../Chart/ChartContextMenu/useContextMenu.test.tsx | 38 ++++++++--
.../components/Chart/DrillBy/DrillByModal.test.tsx | 2 +-
.../Chart/DrillDetail/DrillDetailModal.test.tsx | 2 +-
.../SliceHeaderControls.test.tsx | 78 ++++++++++++++++----
.../components/SliceHeaderControls/index.tsx | 34 +++++----
...d83218_migrate_can_view_and_drill_permission.py | 85 ++++++++++++++++++++++
superset/security/manager.py | 3 +-
...migrate_can_view_and_drill_permission__tests.py | 61 ++++++++++++++++
tests/integration_tests/security_tests.py | 6 +-
10 files changed, 272 insertions(+), 49 deletions(-)
diff --git a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx
index c03b1a1301..337047c67d 100644
--- a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx
+++ b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx
@@ -90,12 +90,14 @@ 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 canWriteExploreFormData = useSelector((state: RootState) =>
+ findPermission('can_write', 'ExploreFormDataRestApi', state.user?.roles),
);
const canDatasourceSamples = useSelector((state: RootState) =>
findPermission('can_samples', 'Datasource', state.user?.roles),
);
+ const canDrillBy = canExplore && canWriteExploreFormData;
+ const canDrillToDetail = canExplore && canDatasourceSamples;
const crossFiltersEnabled = useSelector<RootState, boolean>(
({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled,
);
@@ -111,17 +113,15 @@ const ChartContextMenu = (
}>({ clientX: 0, clientY: 0 });
const menuItems = [];
- const canExploreOrView = canExplore || canViewDrill;
const showDrillToDetail =
isFeatureEnabled(FeatureFlag.DrillToDetail) &&
- canExploreOrView &&
- canDatasourceSamples &&
+ canDrillToDetail &&
isDisplayed(ContextMenuItem.DrillToDetail);
const showDrillBy =
isFeatureEnabled(FeatureFlag.DrillBy) &&
- canExploreOrView &&
+ 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..f0157e8073 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 = ({
Admin: [
['can_explore', 'Superset'],
['can_samples', 'Datasource'],
+ ['can_write', 'ExploreFormDataRestApi'],
],
},
},
@@ -92,27 +93,48 @@ 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'],
+ ['can_explore', 'Superset'],
+ ],
+ },
+ });
+ 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..6006489a9d 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_chart_as_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_chart_as_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..e2480d0431 100644
--- a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
+++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
@@ -273,13 +273,17 @@ 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 canExploreOrView = props.supersetCanExplore || canViewDrill;
+ const canExplore = props.supersetCanExplore;
const canDatasourceSamples = useSelector((state: RootState) =>
findPermission('can_samples', 'Datasource', state.user?.roles),
);
+ const canDrillToDetail = canExplore && canDatasourceSamples;
+ const canViewQuery = useSelector((state: RootState) =>
+ findPermission('can_view_query', 'Dashboard', state.user?.roles),
+ );
+ const canViewTable = useSelector((state: RootState) =>
+ findPermission('can_view_chart_as_table', 'Dashboard', state.user?.roles),
+ );
const refreshChart = () => {
if (props.updatedDttm) {
props.forceRefresh(props.slice.slice_id, props.dashboardId);
@@ -429,7 +433,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 +452,7 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => {
</>
)}
- {canExploreOrView && (
+ {(canExplore || canViewQuery) && (
<Menu.Item key={MENU_KEYS.VIEW_QUERY}>
<ModalTrigger
triggerNode={
@@ -463,7 +467,7 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => {
</Menu.Item>
)}
- {canExploreOrView && (
+ {(canExplore || canViewTable) && (
<Menu.Item key={MENU_KEYS.VIEW_RESULTS}>
<ViewResultsModalTrigger
canExplore={props.supersetCanExplore}
@@ -485,16 +489,14 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => {
</Menu.Item>
)}
- {isFeatureEnabled(FeatureFlag.DrillToDetail) &&
- canExploreOrView &&
- canDatasourceSamples && (
- <DrillDetailMenuItems
- chartId={slice.slice_id}
- formData={props.formData}
- />
- )}
+ {isFeatureEnabled(FeatureFlag.DrillToDetail) && 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/migrations/versions/2024-02-07_17-13_87d38ad83218_migrate_can_view_and_drill_permission.py b/superset/migrations/versions/2024-02-07_17-13_87d38ad83218_migrate_can_view_and_drill_permission.py
new file mode 100644
index 0000000000..abc7adc660
--- /dev/null
+++ b/superset/migrations/versions/2024-02-07_17-13_87d38ad83218_migrate_can_view_and_drill_permission.py
@@ -0,0 +1,85 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Migrate can_view_and_drill permission
+
+Revision ID: 87d38ad83218
+Revises: 1cf8e4344e2b
+Create Date: 2024-02-07 17:13:20.937186
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "87d38ad83218"
+down_revision = "1cf8e4344e2b"
+
+from alembic import op
+from sqlalchemy.exc import SQLAlchemyError
+from sqlalchemy.orm import Session
+
+from superset.migrations.shared.security_converge import (
+ add_pvms,
+ get_reversed_new_pvms,
+ get_reversed_pvm_map,
+ migrate_roles,
+ Pvm,
+)
+
+NEW_PVMS = {"Dashboard": ("can_view_chart_as_table",)}
+
+PVM_MAP = {
+ Pvm("Dashboard", "can_view_and_drill"): (
+ Pvm("Dashboard", "can_view_chart_as_table"),
+ Pvm("Dashboard", "can_view_query"),
+ ),
+}
+
+
+def do_upgrade(session: Session) -> None:
+ add_pvms(session, NEW_PVMS)
+ migrate_roles(session, PVM_MAP)
+
+
+def do_downgrade(session: Session) -> None:
+ add_pvms(session, get_reversed_new_pvms(PVM_MAP))
+ migrate_roles(session, get_reversed_pvm_map(PVM_MAP))
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = Session(bind=bind)
+
+ do_upgrade(session)
+
+ try:
+ session.commit()
+ except SQLAlchemyError as ex:
+ session.rollback()
+ raise Exception(f"An error occurred while upgrading permissions: {ex}")
+
+
+def downgrade():
+ bind = op.get_bind()
+ session = Session(bind=bind)
+
+ do_downgrade(session)
+
+ try:
+ session.commit()
+ except SQLAlchemyError as ex:
+ print(f"An error occurred while downgrading permissions: {ex}")
+ session.rollback()
+ pass
diff --git a/superset/security/manager.py b/superset/security/manager.py
index f1b1b2bcec..ffc4da250f 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -757,7 +757,8 @@ 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_chart_as_table", "Dashboard")
def create_missing_perms(self) -> None:
"""
diff --git a/tests/integration_tests/migrations/87d38ad83218_migrate_can_view_and_drill_permission__tests.py b/tests/integration_tests/migrations/87d38ad83218_migrate_can_view_and_drill_permission__tests.py
new file mode 100644
index 0000000000..e8b825a3e4
--- /dev/null
+++ b/tests/integration_tests/migrations/87d38ad83218_migrate_can_view_and_drill_permission__tests.py
@@ -0,0 +1,61 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from importlib import import_module
+
+from superset import db
+from superset.migrations.shared.security_converge import (
+ _find_pvm,
+ Permission,
+ PermissionView,
+ ViewMenu,
+)
+from tests.integration_tests.test_app import app
+
+migration_module = import_module(
+ "superset.migrations.versions."
+ "2024-02-07_17-13_87d38ad83218_migrate_can_view_and_drill_permission"
+)
+
+upgrade = migration_module.do_upgrade
+downgrade = migration_module.do_downgrade
+
+
+def test_migration_upgrade():
+ with app.app_context():
+ pre_perm = PermissionView(
+ permission=Permission(name="can_view_and_drill"),
+ view_menu=db.session.query(ViewMenu).filter_by(name="Dashboard").one(),
+ )
+ db.session.add(pre_perm)
+ db.session.commit()
+
+ assert _find_pvm(db.session, "Dashboard", "can_view_and_drill") is not None
+
+ upgrade(db.session)
+
+ assert _find_pvm(db.session, "Dashboard", "can_view_chart_as_table") is not None
+ assert _find_pvm(db.session, "Dashboard", "can_view_query") is not None
+ assert _find_pvm(db.session, "Dashboard", "can_view_and_drill") is None
+
+
+def test_migration_downgrade():
+ with app.app_context():
+ downgrade(db.session)
+
+ assert _find_pvm(db.session, "Dashboard", "can_view_chart_as_table") is None
+ assert _find_pvm(db.session, "Dashboard", "can_view_query") is None
+ assert _find_pvm(db.session, "Dashboard", "can_view_and_drill") is not None
diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py
index d9c98aa253..ece9afcccb 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_chart_as_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_chart_as_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"""