-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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): check edit permissions correctly on frontend #14626
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14626 +/- ##
==========================================
+ Coverage 77.47% 77.51% +0.03%
==========================================
Files 959 959
Lines 48492 48853 +361
Branches 5681 5830 +149
==========================================
+ Hits 37569 37867 +298
- Misses 10723 10782 +59
- Partials 200 204 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
189e22e
to
0ba22d3
Compare
0ba22d3
to
784b128
Compare
@suddjian hi Aaron, can you add manual test plan for our QA team to help test? thanks 😊 |
Done @junlincc thank you for the heads up |
@@ -25,8 +25,9 @@ export const useDashboard = (idOrSlug: string | number) => | |||
useApiV1Resource<Dashboard>(`/api/v1/dashboard/${idOrSlug}`), | |||
dashboard => ({ | |||
...dashboard, | |||
metadata: JSON.parse(dashboard.json_metadata), | |||
position_data: JSON.parse(dashboard.position_json), | |||
metadata: dashboard.json_metadata && JSON.parse(dashboard.json_metadata), |
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.
should this be initialized as a empty object if dashboard comes back as undefined.
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.
I assume you mean if the metadata is undefined (if the dashboard is undefined it will be a 404). I don't think it should be initialized like that, null is different from an empty object.
This is not actually new behavior, I just had to write this guard to appease the type system since I corrected the types of some dashboard fields to be nullable.
fixes #14442 |
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!
…14626) * fix(dashboard): check edit permissions correctly on frontend * fix types, appease linter * handle nulls better
…14626) * fix(dashboard): check edit permissions correctly on frontend * fix types, appease linter * handle nulls better
…14626) * fix(dashboard): check edit permissions correctly on frontend * fix types, appease linter * handle nulls better
SUMMARY
This fixes a case where users who were not owners of a dashboard were incorrectly seeing the "Edit" button. This is not a security hole as the users cannot successfully submit their edits, but it was incorrect UI that needed to be fixed.
The frontend was incorrectly inferring dashboard edit permissions. This change fixes the issue by copying logic from the backend's check_ownership function.
The code of the frontend version implements the same check, while being much simpler than the backend version due to being a pure function, dashboard-specific, and not having parameterized function behavior.
I did this instead of changing endpoints, because the caching of the existing dashboard endpoints depends on returning the same data to all users, and because implementing a new endpoint to just return a boolean felt wrong.
TEST PLAN
ADDITIONAL INFORMATION