-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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.
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
fix: Dashboard aware RBAC "Save as" menu item #24806
fix: Dashboard aware RBAC "Save as" menu item #24806
Conversation
9a7403c
to
c04f3f6
Compare
86e1352
to
310efc7
Compare
Codecov Report
@@ Coverage Diff @@
## master #24806 +/- ##
=======================================
Coverage 69.00% 69.00%
=======================================
Files 1906 1906
Lines 74138 74147 +9
Branches 8208 8209 +1
=======================================
+ Hits 51158 51166 +8
- Misses 20857 20858 +1
Partials 2123 2123
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@john-bodley can you please look into failing tests. |
67ad0c3
to
3690a8c
Compare
@betodealmeida @villebro @michael-s-molina Please review this. |
I think you mistakenly committed _update-notifier-last-checked. Could you delete it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but please remove _update-notifier-last-checked before merging it.
0c98ff9
to
c6eff95
Compare
c6eff95
to
fe44656
Compare
(cherry picked from commit f6c3f0c)
SUMMARY
This PR (alongside #24789) fixes #24782.
Specifically (per #24782 (comment)) this PR ensures that the
Save as
menu item is not present when theDASHBOARD_RBAC
feature is enabled and the user is neither an admin nor an owner. Note it's unclear whether a non-owner (irrespective of the feature flag setting) should ever be able toSave as
a dashboard, though this PR simply preserves said behavior.Regarding the backend logic he DashboardRestApi.copy_dash method invokes the @with_dashboard decorator which in turn calls DashboardDAO.get_by_id_or_slug which has the necessary security checks to ensure ensure said user has access. The conundrum is, if you ask where the user has access to the dashboard the answer is "yes", even though we're stating they can't copy it. The solution to this seems to be to add logic within the dashboard DAO.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE
AFTER
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
cc: @mdeshmu @sfirke