You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2023/08/10 16:16:40 UTC
[superset] 06/10: fix: Dashboard aware RBAC "Save as" menu item (#24806)
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 309582516d7e1dfe70c1618d7c91e73a197d8a3a
Author: John Bodley <45...@users.noreply.github.com>
AuthorDate: Wed Aug 9 13:37:52 2023 -0700
fix: Dashboard aware RBAC "Save as" menu item (#24806)
(cherry picked from commit f6c3f0cbbb820b26ac9dc2f24832d59092a22f53)
---
_update-notifier-last-checked | 0
superset-frontend/src/dashboard/actions/hydrate.js | 7 +-
.../HeaderActionsDropdown.test.tsx | 4 +-
.../src/dashboard/components/SaveModal.tsx | 2 +-
.../src/dashboard/util/permissionUtils.test.ts | 91 ++++++++++++++++++----
.../src/dashboard/util/permissionUtils.ts | 12 +++
superset/daos/dashboard.py | 7 ++
superset/dashboards/api.py | 6 +-
.../dashboards/security/security_rbac_tests.py | 90 ++++++++++++++++++++-
9 files changed, 197 insertions(+), 22 deletions(-)
diff --git a/_update-notifier-last-checked b/_update-notifier-last-checked
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/superset-frontend/src/dashboard/actions/hydrate.js b/superset-frontend/src/dashboard/actions/hydrate.js
index 68a5e91613..6fd06c8f12 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 e4affa887f..a89bccb480 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 d024ffc914..fc41798847 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<SaveModalProps, SaveModalState> {
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 7ebf0362d2..e5e958013d 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<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);
+ });
+});
diff --git a/superset-frontend/src/dashboard/util/permissionUtils.ts b/superset-frontend/src/dashboard/util/permissionUtils.ts
index c07f0bb0f8..b01484a4e7 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 51f90709da..f01d610448 100644
--- a/superset/daos/dashboard.py
+++ b/superset/daos/dashboard.py
@@ -26,10 +26,12 @@ from flask_appbuilder.models.sqla import Model
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 @@ class DashboardDAO(BaseDAO[Dashboard]):
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 8464c87444..07f5ed6fbc 100644
--- a/superset/dashboards/api.py
+++ b/superset/dashboards/api.py
@@ -1420,7 +1420,11 @@ class DashboardRestApi(BaseSupersetModelRestApi):
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 ba464064c3..8b7f2ad1ef 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.birth_names_dashboard import (
)
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 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
# 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()