chore: Refactor dashboard security access (apache#24804)

superset-frontend/src/dashboard/actions/hydrate.js

@@ -24,7 +24,10 @@ import { getInitialState as getInitialNativeFilterState } from 'src/dashboard/re
 import { applyDefaultFormData } from 'src/explore/store';
 import { buildActiveFilters } from 'src/dashboard/util/activeDashboardFilters';
 import { findPermission } from 'src/utils/findPermission';
-import { canUserEditDashboard } from 'src/dashboard/util/permissionUtils';
+import {
+  canUserEditDashboard,
+  canUserSaveAsDashboard,
+} from 'src/dashboard/util/permissionUtils';
 import {
   getCrossFiltersConfiguration,
   isCrossFiltersEnabled,
@@ -336,7 +339,7 @@ export const hydrateDashboard =
           metadata,
           userId: user.userId ? String(user.userId) : null, // legacy, please use state.user instead
           dash_edit_perm: canEdit,
-          dash_save_perm: findPermission('can_write', 'Dashboard', roles),
+          dash_save_perm: canUserSaveAsDashboard(dashboard, user),
           dash_share_perm: findPermission(
             'can_share_dashboard',
             'Superset',

superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/HeaderActionsDropdown.test.tsx

@@ -196,7 +196,7 @@ test('should show the share actions', async () => {
   expect(screen.getByText('Share')).toBeInTheDocument();
 });
 
-test('should render the "Save Modal" when user can save', async () => {
+test('should render the "Save as" menu item when user can save', async () => {
   const mockedProps = createProps();
   const canSaveProps = {
     ...mockedProps,
@@ -206,7 +206,7 @@ test('should render the "Save Modal" when user can save', async () => {
   expect(screen.getByText('Save as')).toBeInTheDocument();
 });
 
-test('should NOT render the "Save Modal" menu item when user cannot save', async () => {
+test('should NOT render the "Save as" menu item when user cannot save', async () => {
   const mockedProps = createProps();
   setup(mockedProps);
   expect(screen.queryByText('Save as')).not.toBeInTheDocument();

superset-frontend/src/dashboard/components/SaveModal.tsx

@@ -81,7 +81,7 @@ class SaveModal extends React.PureComponent<SaveModalProps, SaveModalState> {
     super(props);
     this.state = {
       saveType: props.saveType,
-      newDashName: props.dashboardTitle + t('[copy]'),
+      newDashName: `${props.dashboardTitle} ${t('[copy]')}`,
       duplicateSlices: false,
     };
 

superset-frontend/src/dashboard/util/permissionUtils.test.ts

@@ -16,6 +16,8 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import { FeatureFlag } from '@superset-ui/core';
+import * as featureFlags from 'src/featureFlags';
 import {
   UndefinedUser,
   UserWithPermissionsAndRoles,
@@ -25,6 +27,7 @@ import Owner from 'src/types/Owner';
 import {
   canUserAccessSqlLab,
   canUserEditDashboard,
+  canUserSaveAsDashboard,
   isUserAdmin,
 } from './permissionUtils';
 
@@ -73,22 +76,24 @@ const sqlLabUser: UserWithPermissionsAndRoles = {
 
 const undefinedUser: UndefinedUser = {};
 
-describe('canUserEditDashboard', () => {
-  const dashboard: Dashboard = {
-    id: 1,
-    dashboard_title: 'Test Dash',
-    url: 'https://dashboard.example.com/1',
-    thumbnail_url: 'https://dashboard.example.com/1/thumbnail.png',
-    published: true,
-    css: null,
-    changed_by_name: 'Test User',
-    changed_by: owner,
-    changed_on: '2021-05-12T16:56:22.116839',
-    charts: [],
-    owners: [owner],
-    roles: [],
-  };
+const dashboard: Dashboard = {
+  id: 1,
+  dashboard_title: 'Test Dash',
+  url: 'https://dashboard.example.com/1',
+  thumbnail_url: 'https://dashboard.example.com/1/thumbnail.png',
+  published: true,
+  css: null,
+  changed_by_name: 'Test User',
+  changed_by: owner,
+  changed_on: '2021-05-12T16:56:22.116839',
+  charts: [],
+  owners: [owner],
+  roles: [],
+};
 
+let isFeatureEnabledMock: jest.MockInstance<boolean, [feature: FeatureFlag]>;
+
+describe('canUserEditDashboard', () => {
   it('allows owners to edit', () => {
     expect(canUserEditDashboard(dashboard, ownerUser)).toEqual(true);
   });
@@ -151,3 +156,59 @@ test('canUserAccessSqlLab returns false for non-sqllab role', () => {
 test('canUserAccessSqlLab returns true for sqllab role', () => {
   expect(canUserAccessSqlLab(sqlLabUser)).toEqual(true);
 });
+
+describe('canUserSaveAsDashboard with RBAC feature flag disabled', () => {
+  beforeAll(() => {
+    isFeatureEnabledMock = jest
+      .spyOn(featureFlags, 'isFeatureEnabled')
+      .mockImplementation(
+        (featureFlag: FeatureFlag) =>
+          featureFlag !== FeatureFlag.DASHBOARD_RBAC,
+      );
+  });
+
+  afterAll(() => {
+    // @ts-ignore
+    isFeatureEnabledMock.restore();
+  });
+
+  it('allows owners', () => {
+    expect(canUserSaveAsDashboard(dashboard, ownerUser)).toEqual(true);
+  });
+
+  it('allows admin users', () => {
+    expect(canUserSaveAsDashboard(dashboard, adminUser)).toEqual(true);
+  });
+
+  it('allows non-owners', () => {
+    expect(canUserSaveAsDashboard(dashboard, outsiderUser)).toEqual(true);
+  });
+});
+
+describe('canUserSaveAsDashboard with RBAC feature flag enabled', () => {
+  beforeAll(() => {
+    isFeatureEnabledMock = jest
+      .spyOn(featureFlags, 'isFeatureEnabled')
+      .mockImplementation(
+        (featureFlag: FeatureFlag) =>
+          featureFlag === FeatureFlag.DASHBOARD_RBAC,
+      );
+  });
+
+  afterAll(() => {
+    // @ts-ignore
+    isFeatureEnabledMock.restore();
+  });
+
+  it('allows owners', () => {
+    expect(canUserSaveAsDashboard(dashboard, ownerUser)).toEqual(true);
+  });
+
+  it('allows admin users', () => {
+    expect(canUserSaveAsDashboard(dashboard, adminUser)).toEqual(true);
+  });
+
+  it('reject non-owners', () => {
+    expect(canUserSaveAsDashboard(dashboard, outsiderUser)).toEqual(false);
+  });
+});

superset-frontend/src/dashboard/util/permissionUtils.ts

@@ -16,6 +16,8 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import { FeatureFlag } from '@superset-ui/core';
+import { isFeatureEnabled } from 'src/featureFlags';
 import {
   isUserWithPermissionsAndRoles,
   UndefinedUser,
@@ -63,3 +65,13 @@ export function canUserAccessSqlLab(
       ))
   );
 }
+
+export const canUserSaveAsDashboard = (
+  dashboard: Dashboard,
+  user?: UserWithPermissionsAndRoles | UndefinedUser | null,
+) =>
+  isUserWithPermissionsAndRoles(user) &&
+  findPermission('can_write', 'Dashboard', user?.roles) &&
+  (!isFeatureEnabled(FeatureFlag.DASHBOARD_RBAC) ||
+    isUserAdmin(user) ||
+    isUserDashboardOwner(dashboard, user));

superset/daos/dashboard.py

@@ -26,10 +26,12 @@
 from flask_appbuilder.models.sqla.interface import SQLAInterface
 from sqlalchemy.exc import SQLAlchemyError
 
+from superset import is_feature_enabled, security_manager
 from superset.daos.base import BaseDAO
 from superset.daos.exceptions import DAOConfigError, DAOCreateFailedError
 from superset.dashboards.commands.exceptions import (
     DashboardAccessDeniedError,
+    DashboardForbiddenError,
     DashboardNotFoundError,
 )
 from superset.dashboards.filter_sets.consts import (
@@ -321,6 +323,11 @@ def favorited_ids(dashboards: list[Dashboard]) -> list[FavStar]:
     def copy_dashboard(
         cls, original_dash: Dashboard, data: dict[str, Any]
     ) -> Dashboard:
+        if is_feature_enabled("DASHBOARD_RBAC") and not security_manager.is_owner(
+            original_dash
+        ):
+            raise DashboardForbiddenError()
+
         dash = Dashboard()
         dash.owners = [g.user] if g.user else []
         dash.dashboard_title = data["dashboard_title"]

superset/dashboards/api.py

@@ -1408,7 +1408,11 @@ def copy_dash(self, original_dash: Dashboard) -> Response:
         except ValidationError as error:
             return self.response_400(message=error.messages)
 
-        dash = DashboardDAO.copy_dashboard(original_dash, data)
+        try:
+            dash = DashboardDAO.copy_dashboard(original_dash, data)
+        except DashboardForbiddenError:
+            return self.response_403()
+
         return self.response(
             200,
             result={

tests/integration_tests/dashboards/security/security_rbac_tests.py

@@ -15,11 +15,16 @@
 # specific language governing permissions and limitations
 # under the License.
 """Unit tests for Superset"""
+import json
 from unittest import mock
+from unittest.mock import patch
 
 import pytest
 
-from superset.utils.core import backend
+from superset.daos.dashboard import DashboardDAO
+from superset.dashboards.commands.exceptions import DashboardForbiddenError
+from superset.utils.core import backend, override_user
+from tests.integration_tests.conftest import with_feature_flags
 from tests.integration_tests.dashboards.dashboard_test_utils import *
 from tests.integration_tests.dashboards.security.base_case import (
     BaseTestDashboardSecurity,
@@ -36,6 +41,10 @@
 )
 from tests.integration_tests.fixtures.public_role import public_role_like_gamma
 from tests.integration_tests.fixtures.query_context import get_query_context
+from tests.integration_tests.fixtures.world_bank_dashboard import (
+    load_world_bank_dashboard_with_slices,
+    load_world_bank_data,
+)
 
 CHART_DATA_URI = "api/v1/chart/data"
 
@@ -431,3 +440,82 @@ def test_cannot_get_draft_dashboard_with_roles_by_uuid(self):
         # rollback changes
         db.session.delete(dashboard)
         db.session.commit()
+
+    @with_feature_flags(DASHBOARD_RBAC=True)
+    @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+    def test_copy_dashboard_via_api(self):
+        source = db.session.query(Dashboard).filter_by(slug="world_health").first()
+        source.roles = [self.get_role("Gamma")]
+
+        if not (published := source.published):
+            source.published = True  # Required per the DashboardAccessFilter for RBAC.
+
+        db.session.commit()
+
+        uri = f"api/v1/dashboard/{source.id}/copy/"
+
+        data = {
+            "dashboard_title": "copied dash",
+            "css": "<css>",
+            "duplicate_slices": False,
+            "json_metadata": json.dumps(
+                {
+                    "positions": source.position,
+                    "color_namespace": "Color Namespace Test",
+                    "color_scheme": "Color Scheme Test",
+                }
+            ),
+        }
+
+        self.login(username="gamma")
+        rv = self.client.post(uri, json=data)
+        self.assertEqual(rv.status_code, 403)
+        self.logout()
+
+        self.login(username="admin")
+        rv = self.client.post(uri, json=data)
+        self.assertEqual(rv.status_code, 200)
+        self.logout()
+        response = json.loads(rv.data.decode("utf-8"))
+
+        target = (
+            db.session.query(Dashboard)
+            .filter(Dashboard.id == response["result"]["id"])
+            .one()
+        )
+
+        db.session.delete(target)
+        source.roles = []
+
+        if not published:
+            source.published = False
+
+        db.session.commit()
+
+    @with_feature_flags(DASHBOARD_RBAC=True)
+    @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+    def test_copy_dashboard_via_dao(self):
+        source = db.session.query(Dashboard).filter_by(slug="world_health").first()
+
+        data = {
+            "dashboard_title": "copied dash",
+            "css": "<css>",
+            "duplicate_slices": False,
+            "json_metadata": json.dumps(
+                {
+                    "positions": source.position,
+                    "color_namespace": "Color Namespace Test",
+                    "color_scheme": "Color Scheme Test",
+                }
+            ),
+        }
+
+        with override_user(security_manager.find_user("gamma")):
+            with pytest.raises(DashboardForbiddenError):
+                DashboardDAO.copy_dashboard(source, data)
+
+        with override_user(security_manager.find_user("admin")):
+            target = DashboardDAO.copy_dashboard(source, data)
+            db.session.delete(target)
+
+        db.session.commit()