Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 6 commits into from
Jul 11, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3ab1f75
Backend
jfrag1 Jul 8, 2023
94d8ada
Frontend
jfrag1 Jul 8, 2023
679fb97
Fix lint
jfrag1 Jul 8, 2023
dbf9f6b
Refactor to re-use code
jfrag1 Jul 10, 2023
2274e3f
Fix lint
jfrag1 Jul 10, 2023
b5fe9c4
Fix test
jfrag1 Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion superset-frontend/src/explore/components/SaveModal.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
17 changes: 10 additions & 7 deletions superset-frontend/src/explore/components/SaveModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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>;
Expand Down Expand Up @@ -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
);
}
Expand All @@ -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 },
});
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand All @@ -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,
Expand Down
5 changes: 5 additions & 0 deletions superset/charts/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
ChartInvalidError,
ChartNotFoundError,
ChartUpdateFailedError,
DashboardsForbiddenError,
)
from superset.charts.commands.export import ExportChartsCommand
from superset.charts.commands.importers.dispatcher import ImportChartsCommand
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down
5 changes: 5 additions & 0 deletions superset/charts/commands/create.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should return DashboardsNotFoundValidationError as otherwise it can be exploit to validate this dashboard id does exist

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also should we append the error to the exceptions array to keep it consistent

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should return DashboardsNotFoundValidationError as otherwise it can be exploit to validate this dashboard id does exist

I don't believe this is necessary here since DashboardDAO.find_by_ids will only return dashboards the user can view (via the DashboardAccessFilter).

also should we append the error to the exceptions array to keep it consistent

I considered this, but the ChartInvalidError thrown at the end with all the exceptions will always return a 422 error response, and I thought it was more appropriate to return a 403 here. Other commands, for example UpdateChartCommand have some exceptions which get appended and others which are thrown immediately, so it's not a new pattern.

self._properties["dashboards"] = dashboards

try:
Expand Down
4 changes: 4 additions & 0 deletions superset/charts/commands/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 changes: 29 additions & 4 deletions tests/integration_tests/charts/api_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand All @@ -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"))
Expand All @@ -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"))
Expand All @@ -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)

Expand Down Expand Up @@ -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
michael-s-molina marked this conversation as resolved.
Show resolved Hide resolved
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):
"""
Expand Down