jfrag1 commented on code in PR #24630:
URL: https://github.com/apache/superset/pull/24630#discussion_r1258637142
##########
superset/charts/commands/create.py:
##########
@@ -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()
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
