fix(frontend): fix edit/delete permissions for charts, dashboards, and datasets (#18870) (#32981)#32995
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Suppressed issues based on your team's Korbit activity
| This issue | Is similar to | Because |
|---|---|---|
lines 412:414:
|
Similar issues were not addressed in the past |
When you react to issues (for example, an upvote or downvote) or you fix them, Korbit will tune future reviews based on these signals.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/pages/DashboardList/index.tsx | ✅ |
| superset-frontend/src/pages/ChartList/index.tsx | ✅ |
| superset-frontend/src/pages/DatasetList/index.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Documentation ✅ Logging ✅ Error Handling ✅ Readability ✅ Design ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This might be a backend problem and not a front end problem. We used to have a |
@sadpandajoe Permissions are obtained through the |
|
Not to confuse this issue further, but @geido brought up a good point that perhaps the dimming/disabling shouldn't be the business of the Icon component, but perhaps the link it's wrapped in? In this case it's a with and onClick handler, but maybe we should be using a (modified?) |
|
Here's another use case along the same lines: #30124 |
|
This needs a rebase at this point, if you're able to do so. We'd love to see it get across the finish line and close out the linked issue(s). |
c1ca5aa to
8dc9fbf
Compare
|
@rusackas I rebased the changes to master and implemented the disabled logic into the |
0751441 to
66dd716
Compare
|
Approving CI to run 🤞 |
66dd716 to
1ebed37
Compare
|
Rebased and fixed lint issues in IconButton.tsx |
|
We haven't forgotten about this! The logic looks basically good to me, but there are a handful of style/theme related changes that I think we should have someone look at who's closer to the Theming project, so I've tagged some additional reviewers here. In general, we just want to be as "vanilla" as possible with AntD components, and minimize CSS overrides wherever possible. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #1bbbaaActionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Thanks for coming back to this and knocking out the earlier notes. EditableTitle's out, the editors check is DRY'd up into Looks like the latest push went red, though. There's a type error in A couple of unrelated bits are still hanging around too, the SliceHeader max-width bump and the blank line in DashboardWrapper. Left suggestions to revert both. Thanks for sticking with this one! |
Code Review Agent Run #0d6d7aActionable Suggestions - 0Additional Suggestions - 3
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #fb7cefActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@rusackas I applied the suggestions, fixed the lint issues, and updated the tests. CI is now passing. Please check again, I think we're near! |
|
I'll see about rebasing this, since it's my fault you're stuck with more conflicts ;) |
|
Taking a review pass while I'm lookin' at the rebase, @fnardin-maystreet, it's really close. Earlier notes are all knocked out, thanks. The edit/delete gate keys off The fix should be small, since the full user is already right there. In Worth a test for it too. I noticed the DatasetList spec swapped Holler if you want a hand wiring the admin user through, happy to push a commit. It also needs a rebase on current |
…ts and dashboards
Co-authored-by: Evan Rusackas <evan@preset.io>
…shboardWrapper.tsx Co-authored-by: Evan Rusackas <evan@preset.io>
|
@rusackas The issue you're raising is real but a bit overstated as it's the type of the This is actually a pre-existing issue: DatasetList on master already uses the user prop to check admin privileges with the same narrow type. The bug is only in the declared type of user. The actual value passed in from App.tsx is the full user object with roles. I think the better fix is widening those types ( On the DatasetList spec: the test would behave the same with mockAdminUser, since that fixture lacks username/permissions and therefore fails isUserWithPermissionsAndRoles in permissionUtils.ts. isUserAdmin never actually resolves true for it. I swapped to mockEditorUser to mainly to keep the bot quiet. Good catch though. I'll add a proper fixture (with username/permissions/roles.Admin) that isn't listed as an editor, and a test asserting Edit/Delete stay enabled for it, to actually lock in the admin-bypass behavior. |
…omponents and tests
da4a08a to
db96e2f
Compare
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks for staying with this through all the rebases, @fnardin-maystreet. LGTM, and the IconButton/permissionUtils refactor ended up in a nicer shape than where this started. Approving, merging soon, hopefully.
4 participants
SUMMARY
In both
Dashboards,Datasets,Chartspages the delete and edit buttons are shown on the only condition that the user hascan_writepermission even if not the owner. Attempting to delete or edit an object results in aForbiddenerror.This PR fixes the button disabled status in the frontend preventing the user from attempting to delete or edit a resource it has no permission to.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
All three pages will show the delete and edit buttons as disabled if the user is not an owner or an admin:
fixes: #18870
fixes: #32981
TESTING INSTRUCTIONS
Create a dashboard, a chart, and a dataset with an user and try to edit / delete them with a different user.
ADDITIONAL INFORMATION