diff --git a/_update-notifier-last-checked b/_update-notifier-last-checked new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/superset-frontend/src/dashboard/actions/hydrate.js b/superset-frontend/src/dashboard/actions/hydrate.js index 68a5e91613b70..6fd06c8f12cda 100644 --- a/superset-frontend/src/dashboard/actions/hydrate.js +++ b/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', diff --git a/superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/HeaderActionsDropdown.test.tsx b/superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/HeaderActionsDropdown.test.tsx index e4affa887f8c4..a89bccb4802a5 100644 --- a/superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/HeaderActionsDropdown.test.tsx +++ b/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(); diff --git a/superset-frontend/src/dashboard/components/SaveModal.tsx b/superset-frontend/src/dashboard/components/SaveModal.tsx index d024ffc914666..fc41798847287 100644 --- a/superset-frontend/src/dashboard/components/SaveModal.tsx +++ b/superset-frontend/src/dashboard/components/SaveModal.tsx @@ -81,7 +81,7 @@ class SaveModal extends React.PureComponent { super(props); this.state = { saveType: props.saveType, - newDashName: props.dashboardTitle + t('[copy]'), + newDashName: `${props.dashboardTitle} ${t('[copy]')}`, duplicateSlices: false, }; diff --git a/superset-frontend/src/dashboard/util/permissionUtils.test.ts b/superset-frontend/src/dashboard/util/permissionUtils.test.ts index 7ebf0362d2220..e5e958013d6bb 100644 --- a/superset-frontend/src/dashboard/util/permissionUtils.test.ts +++ b/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; + +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); + }); +}); diff --git a/superset-frontend/src/dashboard/util/permissionUtils.ts b/superset-frontend/src/dashboard/util/permissionUtils.ts index c07f0bb0f8df7..b01484a4e78e2 100644 --- a/superset-frontend/src/dashboard/util/permissionUtils.ts +++ b/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)); diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py index 51f90709dafc9..f01d61044882b 100644 --- a/superset/daos/dashboard.py +++ b/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"] diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 60dd7949d266c..1602c8e2f961c 100644 --- a/superset/dashboards/api.py +++ b/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={ diff --git a/tests/integration_tests/dashboards/security/security_rbac_tests.py b/tests/integration_tests/dashboards/security/security_rbac_tests.py index ba464064c3745..8b7f2ad1ef055 100644 --- a/tests/integration_tests/dashboards/security/security_rbac_tests.py +++ b/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": "", + "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": "", + "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()