fix: Chart can be added to dashboard by non-owner via save as option #24630
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24630 +/- ##
==========================================
+ Coverage 68.97% 69.08% +0.10%
==========================================
Files 1907 1907
Lines 74153 74156 +3
Branches 8182 8180 -2
==========================================
+ Hits 51148 51230 +82
+ Misses 20882 20803 -79
Partials 2123 2123
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -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() | |||
There was a problem hiding this comment.
I wonder if we should return DashboardsNotFoundValidationError as otherwise it can be exploit to validate this dashboard id does exist
There was a problem hiding this comment.
also should we append the error to the exceptions array to keep it consistent
There was a problem hiding this 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.
jfrag1
requested review from
zephyring,
hughhhh,
michael-s-molina and
Antonio-RiveroMartnez
There was a problem hiding this comment.
LGTM. Thank you for the fix @jfrag1 and for addressing all comments.
Antonio-RiveroMartnez
deleted the
jack/dashboard-non-owner-can-add-chart-via-save-as
branch
michael-s-molina
added
the
v3.0
mistercrunch
added
🍒 3.0.0
🍒 3.0.1
🍒 3.0.2
🍒 3.0.3
🍒 3.0.4
🏷️ bot
SUMMARY
This PR fixes a bug that allowed a user to add a chart to a dashboard they had access to view, but did not own. This was possible because the
POST /api/v1/chartendpoint used to create new charts allows a list of dashboard id's to be passed that the new chart should be added to. However, the endpoint did not validate whether the user was an owner of these dashboards, only if they could view the dashboard (via the filter on theDashboardDAO).This PR patches this endpoint so that it now verifies the user is an owner of each dashboard they are trying to add the new chart to. It also includes a fix to the explore frontend code so that the save modal dashboard dropdown only pre-populates if the user is an owner of the dashboard they came from.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before (logged in as alpha role but not owner of the dashboard):
After (same user):
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION