diff --git a/superset-frontend/src/explore/components/SaveModal.test.jsx b/superset-frontend/src/explore/components/SaveModal.test.jsx index 74d1c1199..d9d6f2983 100644 --- a/superset-frontend/src/explore/components/SaveModal.test.jsx +++ b/superset-frontend/src/explore/components/SaveModal.test.jsx @@ -164,7 +164,7 @@ test('disables overwrite option for new slice', () => { test('disables overwrite option for non-owner', () => { const wrapperForNonOwner = getWrapper(); - wrapperForNonOwner.setProps({ userId: 2 }); + wrapperForNonOwner.setProps({ user: { userId: 2 } }); const overwriteRadio = wrapperForNonOwner.find('#overwrite-radio'); expect(overwriteRadio).toHaveLength(1); expect(overwriteRadio.prop('disabled')).toBe(true); diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 1de97b926..592bab2f9 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -41,8 +41,11 @@ import { Radio } from 'src/components/Radio'; import Button from 'src/components/Button'; import { AsyncSelect } from 'src/components'; import Loading from 'src/components/Loading'; +import { canUserEditDashboard } from 'src/dashboard/util/permissionUtils'; import { setSaveChartModalVisibility } from 'src/explore/actions/saveModalActions'; import { SaveActionType } from 'src/explore/types'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { Dashboard } from 'src/types/Dashboard'; // Session storage key for recent dashboard const SK_DASHBOARD_ID = 'save_chart_recent_dashboard'; @@ -51,7 +54,7 @@ interface SaveModalProps extends RouteComponentProps { addDangerToast: (msg: string) => void; actions: Record; form_data?: Record; - userId: number; + user: UserWithPermissionsAndRoles; alert?: string; sliceName?: string; slice?: Record; @@ -111,7 +114,7 @@ class SaveModal extends React.Component { canOverwriteSlice(): boolean { return ( - this.props.slice?.owners?.includes(this.props.userId) && + this.props.slice?.owners?.includes(this.props.user.userId) && !this.props.slice?.is_managed_externally ); } @@ -124,8 +127,8 @@ class SaveModal extends React.Component { } if (dashboardId) { try { - const result = await this.loadDashboard(dashboardId); - if (result) { + const result = (await this.loadDashboard(dashboardId)) as Dashboard; + if (canUserEditDashboard(result, this.props.user)) { this.setState({ dashboard: { label: result.dashboard_title, value: result.id }, }); @@ -298,7 +301,7 @@ class SaveModal extends React.Component { { col: 'owners', opr: 'rel_m_m', - value: this.props.userId, + value: this.props.user.userId, }, ], page, @@ -484,7 +487,7 @@ class SaveModal extends React.Component { interface StateProps { datasource: any; slice: any; - userId: any; + user: UserWithPermissionsAndRoles; dashboards: any; alert: any; isVisible: boolean; @@ -498,7 +501,7 @@ function mapStateToProps({ return { datasource: explore.datasource, slice: explore.slice, - userId: user?.userId, + user, dashboards: saveModal.dashboards, alert: saveModal.saveModalAlert, isVisible: saveModal.isVisible, diff --git a/superset/charts/api.py b/superset/charts/api.py index e8ccfa0ff..8cfe9fa4c 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -43,6 +43,7 @@ from superset.charts.commands.exceptions import ( ChartInvalidError, ChartNotFoundError, ChartUpdateFailedError, + DashboardsForbiddenError, ) from superset.charts.commands.export import ExportChartsCommand from superset.charts.commands.importers.dispatcher import ImportChartsCommand @@ -314,6 +315,8 @@ class ChartRestApi(BaseSupersetModelRestApi): $ref: '#/components/responses/400' 401: $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' 422: $ref: '#/components/responses/422' 500: @@ -327,6 +330,8 @@ class ChartRestApi(BaseSupersetModelRestApi): try: new_model = CreateChartCommand(item).run() return self.response(201, id=new_model.id, result=item) + except DashboardsForbiddenError as ex: + return self.response(ex.status, message=ex.message) except ChartInvalidError as ex: return self.response_422(message=ex.normalized_messages()) except ChartCreateFailedError as ex: diff --git a/superset/charts/commands/create.py b/superset/charts/commands/create.py index 3eb0001bb..397e174d7 100644 --- a/superset/charts/commands/create.py +++ b/superset/charts/commands/create.py @@ -22,9 +22,11 @@ from flask import g from flask_appbuilder.models.sqla import Model from marshmallow import ValidationError +from superset import security_manager from superset.charts.commands.exceptions import ( ChartCreateFailedError, ChartInvalidError, + DashboardsForbiddenError, DashboardsNotFoundValidationError, ) from superset.commands.base import BaseCommand, CreateMixin @@ -69,6 +71,9 @@ class CreateChartCommand(CreateMixin, BaseCommand): dashboards = DashboardDAO.find_by_ids(dashboard_ids) if len(dashboards) != len(dashboard_ids): exceptions.append(DashboardsNotFoundValidationError()) + for dash in dashboards: + if not security_manager.is_owner(dash): + raise DashboardsForbiddenError() self._properties["dashboards"] = dashboards try: diff --git a/superset/charts/commands/exceptions.py b/superset/charts/commands/exceptions.py index 1079cdca8..83792ae25 100644 --- a/superset/charts/commands/exceptions.py +++ b/superset/charts/commands/exceptions.py @@ -155,6 +155,10 @@ class ChartImportError(ImportFailedError): message = _("Import chart failed for an unknown reason") +class DashboardsForbiddenError(ForbiddenError): + message = _("Changing one or more of these dashboards is forbidden") + + class WarmUpCacheChartNotFoundError(CommandException): status = 404 message = _("Chart not found") diff --git a/tests/integration_tests/charts/api_tests.py b/tests/integration_tests/charts/api_tests.py index 68b2b5580..6648dc374 100644 --- a/tests/integration_tests/charts/api_tests.py +++ b/tests/integration_tests/charts/api_tests.py @@ -466,7 +466,7 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin): "certification_details": "Sample certification", } self.login(username="admin") - uri = f"api/v1/chart/" + uri = "api/v1/chart/" rv = self.post_assert_metric(uri, chart_data, "post") self.assertEqual(rv.status_code, 201) data = json.loads(rv.data.decode("utf-8")) @@ -484,7 +484,7 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin): "datasource_type": "table", } self.login(username="admin") - uri = f"api/v1/chart/" + uri = "api/v1/chart/" rv = self.post_assert_metric(uri, chart_data, "post") self.assertEqual(rv.status_code, 201) data = json.loads(rv.data.decode("utf-8")) @@ -503,7 +503,7 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin): "owners": [1000], } self.login(username="admin") - uri = f"api/v1/chart/" + uri = "api/v1/chart/" rv = self.post_assert_metric(uri, chart_data, "post") self.assertEqual(rv.status_code, 422) response = json.loads(rv.data.decode("utf-8")) @@ -521,7 +521,7 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin): "params": '{"A:"a"}', } self.login(username="admin") - uri = f"api/v1/chart/" + uri = "api/v1/chart/" rv = self.post_assert_metric(uri, chart_data, "post") self.assertEqual(rv.status_code, 400) @@ -560,6 +560,31 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin): response, {"message": {"datasource_id": ["Datasource does not exist"]}} ) + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + def test_create_chart_validate_user_is_dashboard_owner(self): + """ + Chart API: Test create validate user is dashboard owner + """ + dash = db.session.query(Dashboard).filter_by(slug="world_health").first() + # Must be published so that alpha user has read access to dash + dash.published = True + db.session.commit() + chart_data = { + "slice_name": "title1", + "datasource_id": 1, + "datasource_type": "table", + "dashboards": [dash.id], + } + self.login(username="alpha") + uri = "api/v1/chart/" + rv = self.post_assert_metric(uri, chart_data, "post") + self.assertEqual(rv.status_code, 403) + response = json.loads(rv.data.decode("utf-8")) + self.assertEqual( + response, + {"message": "Changing one or more of these dashboards is forbidden"}, + ) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_update_chart(self): """