You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ar...@apache.org on 2023/07/11 15:58:37 UTC
[superset] branch master updated: fix: Chart can be added to dashboard by non-owner via save as option (#24630)
This is an automated email from the ASF dual-hosted git repository.
arivero pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 4caf33b41d fix: Chart can be added to dashboard by non-owner via save as option (#24630)
4caf33b41d is described below
commit 4caf33b41d38beed718887d7866baeafc8b15181
Author: Jack Fragassi <jf...@gmail.com>
AuthorDate: Tue Jul 11 08:58:29 2023 -0700
fix: Chart can be added to dashboard by non-owner via save as option (#24630)
---
.../src/explore/components/SaveModal.test.jsx | 2 +-
.../src/explore/components/SaveModal.tsx | 17 ++++++-----
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 +++++++++++++++++++---
6 files changed, 54 insertions(+), 12 deletions(-)
diff --git a/superset-frontend/src/explore/components/SaveModal.test.jsx b/superset-frontend/src/explore/components/SaveModal.test.jsx
index 74d1c1199c..d9d6f2983d 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 1de97b926f..592bab2f9f 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<string, any>;
form_data?: Record<string, any>;
- userId: number;
+ user: UserWithPermissionsAndRoles;
alert?: string;
sliceName?: string;
slice?: Record<string, any>;
@@ -111,7 +114,7 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
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<SaveModalProps, SaveModalState> {
}
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<SaveModalProps, SaveModalState> {
{
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<SaveModalProps, SaveModalState> {
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 e8ccfa0fff..8cfe9fa4c2 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 3eb0001bb9..397e174d73 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 1079cdca81..83792ae252 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 68b2b55809..6648dc3745 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):
"""