iop
/
superset
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix: Chart can be added to dashboard by non-owner via save as option (#24630)

This commit is contained in:
Jack Fragassi 2023-07-11 08:58:29 -07:00 committed by GitHub
parent 2e4d9f2e2a
commit 4caf33b41d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 54 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
superset-frontend/src/explore/components/SaveModal.test.jsx
View File

@ -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);

17
superset-frontend/src/explore/components/SaveModal.tsx
View File

@ -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,

5
superset/charts/api.py
View File

@ -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:

5
superset/charts/commands/create.py
View File

@ -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:

4
superset/charts/commands/exceptions.py
View File

@ -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")

33
tests/integration_tests/charts/api_tests.py
View File

@ -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):
"""