From 595c6ce3e6a02f6086d5987e50032ac3090c3122 Mon Sep 17 00:00:00 2001 From: Geido <60598000+geido@users.noreply.github.com> Date: Fri, 9 Feb 2024 16:25:32 +0200 Subject: [PATCH] chore: Add granular permissions for actions in Dashboard (#27029) --- .../ChartContextMenu/ChartContextMenu.tsx | 12 +-- .../ChartContextMenu/useContextMenu.test.tsx | 38 +++++++-- .../Chart/DrillBy/DrillByModal.test.tsx | 2 +- .../DrillDetail/DrillDetailModal.test.tsx | 2 +- .../SliceHeaderControls.test.tsx | 80 +++++++++++++---- .../components/SliceHeaderControls/index.tsx | 34 ++++---- ...8_migrate_can_view_and_drill_permission.py | 85 +++++++++++++++++++ superset/security/manager.py | 3 +- ...te_can_view_and_drill_permission__tests.py | 61 +++++++++++++ tests/integration_tests/security_tests.py | 6 +- 10 files changed, 273 insertions(+), 50 deletions(-) create mode 100644 superset/migrations/versions/2024-02-07_17-13_87d38ad83218_migrate_can_view_and_drill_permission.py create mode 100644 tests/integration_tests/migrations/87d38ad83218_migrate_can_view_and_drill_permission__tests.py 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( ({ 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, ) => { const props = overrideProps || createProps(); - const store = getMockStore(); - const mockState = store.getState(); return render(, { 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.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', () => { +test('Should show "View query"', () => { const props = { ...createProps(), supersetCanExplore: false, }; props.slice.slice_id = 18; renderWrapper(props, { - Admin: [['can_view_and_drill', 'Dashboard']], + 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(); +}); + +test('Should not show "View as table"', () => { + const props = { + ...createProps(), + supersetCanExplore: false, + }; + props.slice.slice_id = 18; + renderWrapper(props, { + 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) => { )} - {props.supersetCanExplore && ( + {canExplore && ( @@ -448,7 +452,7 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => { )} - {canExploreOrView && ( + {(canExplore || canViewQuery) && ( { )} - {canExploreOrView && ( + {(canExplore || canViewTable) && ( { )} - {isFeatureEnabled(FeatureFlag.DrillToDetail) && - canExploreOrView && - canDatasourceSamples && ( - - )} + {isFeatureEnabled(FeatureFlag.DrillToDetail) && canDrillToDetail && ( + + )} - {(slice.description || canExploreOrView) && } + {(slice.description || canExplore) && } {supersetCanShare && ( 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"""