From 3ab1f752c454e4218aae9e424d16b8e269b1a293 Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Fri, 7 Jul 2023 21:17:05 -0700 Subject: [PATCH 1/6] Backend --- superset/charts/api.py | 5 ++++ superset/charts/commands/create.py | 5 ++++ superset/charts/commands/exceptions.py | 4 +++ tests/integration_tests/charts/api_tests.py | 33 ++++++++++++++++++--- 4 files changed, 43 insertions(+), 4 deletions(-) diff --git a/superset/charts/api.py b/superset/charts/api.py index e8ccfa0fffd60..8cfe9fa4c2c43 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -43,6 +43,7 @@ ChartInvalidError, ChartNotFoundError, ChartUpdateFailedError, + DashboardsForbiddenError, ) from superset.charts.commands.export import ExportChartsCommand from superset.charts.commands.importers.dispatcher import ImportChartsCommand @@ -314,6 +315,8 @@ def post(self) -> Response: $ref: '#/components/responses/400' 401: $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' 422: $ref: '#/components/responses/422' 500: @@ -327,6 +330,8 @@ def post(self) -> Response: 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 3eb0001bb9315..397e174d7397f 100644 --- a/superset/charts/commands/create.py +++ b/superset/charts/commands/create.py @@ -22,9 +22,11 @@ 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 @@ def validate(self) -> None: 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 1079cdca81c90..83792ae2527fe 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 68b2b55809bac..6648dc3745143 100644 --- a/tests/integration_tests/charts/api_tests.py +++ b/tests/integration_tests/charts/api_tests.py @@ -466,7 +466,7 @@ def test_create_chart(self): "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 @@ def test_create_simple_chart(self): "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 @@ def test_create_chart_validate_owners(self): "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 @@ def test_create_chart_validate_params(self): "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 @@ def test_create_chart_validate_datasource(self): 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): """ From 94d8ada27864b6297fa3b2859e90f6c71655966b Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Fri, 7 Jul 2023 21:53:53 -0700 Subject: [PATCH 2/6] Frontend --- superset-frontend/src/explore/components/SaveModal.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 1de97b926f859..49500e3a45f54 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -125,7 +125,10 @@ class SaveModal extends React.Component { if (dashboardId) { try { const result = await this.loadDashboard(dashboardId); - if (result) { + if ( + result && + result.owners.some((owner: any) => owner.id === this.props.userId) + ) { this.setState({ dashboard: { label: result.dashboard_title, value: result.id }, }); From 679fb974e6bf3c353cfbf5b7e12da92c9375830d Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Fri, 7 Jul 2023 22:11:27 -0700 Subject: [PATCH 3/6] Fix lint --- superset-frontend/src/explore/components/SaveModal.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 49500e3a45f54..6ca3a0f20ba4d 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -126,8 +126,7 @@ class SaveModal extends React.Component { try { const result = await this.loadDashboard(dashboardId); if ( - result && - result.owners.some((owner: any) => owner.id === this.props.userId) + result?.owners.some((owner: any) => owner.id === this.props.userId) ) { this.setState({ dashboard: { label: result.dashboard_title, value: result.id }, From dbf9f6b5b9bc311b1bf6468da39516af2d8ca59e Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Mon, 10 Jul 2023 11:23:07 -0700 Subject: [PATCH 4/6] Refactor to re-use code --- .../src/explore/components/SaveModal.tsx | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 6ca3a0f20ba4d..7ed85d4560f2e 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,10 +127,8 @@ class SaveModal extends React.Component { } if (dashboardId) { try { - const result = await this.loadDashboard(dashboardId); - if ( - result?.owners.some((owner: any) => owner.id === this.props.userId) - ) { + const result = (await this.loadDashboard(dashboardId)) as Dashboard; + if (canUserEditDashboard(result, this.props.user)) { this.setState({ dashboard: { label: result.dashboard_title, value: result.id }, }); @@ -300,7 +301,7 @@ class SaveModal extends React.Component { { col: 'owners', opr: 'rel_m_m', - value: this.props.userId, + value: this.props.user.userId, }, ], page, @@ -486,7 +487,7 @@ class SaveModal extends React.Component { interface StateProps { datasource: any; slice: any; - userId: any; + user: UserWithPermissionsAndRoles; dashboards: any; alert: any; isVisible: boolean; @@ -500,7 +501,7 @@ function mapStateToProps({ return { datasource: explore.datasource, slice: explore.slice, - userId: user?.userId, + user: user, dashboards: saveModal.dashboards, alert: saveModal.saveModalAlert, isVisible: saveModal.isVisible, From 2274e3f987ddaca17ce3faa4c3d20281e7c7398c Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Mon, 10 Jul 2023 11:34:57 -0700 Subject: [PATCH 5/6] Fix lint --- superset-frontend/src/explore/components/SaveModal.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 7ed85d4560f2e..592bab2f9f964 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -501,7 +501,7 @@ function mapStateToProps({ return { datasource: explore.datasource, slice: explore.slice, - user: user, + user, dashboards: saveModal.dashboards, alert: saveModal.saveModalAlert, isVisible: saveModal.isVisible, From b5fe9c447c701e5a2d264c980be85b78498f0e12 Mon Sep 17 00:00:00 2001 From: Jack Fragassi Date: Mon, 10 Jul 2023 14:48:14 -0700 Subject: [PATCH 6/6] Fix test --- superset-frontend/src/explore/components/SaveModal.test.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/explore/components/SaveModal.test.jsx b/superset-frontend/src/explore/components/SaveModal.test.jsx index 74d1c1199c75e..d9d6f2983d67b 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);