chore: Add granular permissions for actions in Dashboard (#27029)

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 =

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();
 });

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']] },
       },
     },
   );

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();

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.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();
 });

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')}>

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

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:
         """

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

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"""