-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
feat(dashboard_rbac): dashboard lists #12680
feat(dashboard_rbac): dashboard lists #12680
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12680 +/- ##
==========================================
+ Coverage 63.13% 66.98% +3.85%
==========================================
Files 1022 1022
Lines 50032 50186 +154
Branches 4915 5204 +289
==========================================
+ Hits 31587 33618 +2031
+ Misses 18245 16433 -1812
+ Partials 200 135 -65
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Could you add a test plan and maybe some test cases?
I think we should at least test visiting the dashboards list page as each of the following:
- an anonymous user
- a non-admin (test user: alpha/general)
- admin
This comment has been minimized.
This comment has been minimized.
superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py
Outdated
Show resolved
Hide resolved
superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py
Outdated
Show resolved
Hide resolved
superset/migrations/versions/11ccdd12658_add_roles_relationship_to_dashboard.py
Outdated
Show resolved
Hide resolved
Just saw it has passed. :D Didn't notice you've updated the SIP summary. Thanks for iterating on your proposal and creating this POC! @dpgaspar's diagram makes perfect sense to me. Looks like things can be even simpler than I original thought. Maybe include the diagram in the SIP as a reference, too? |
* master: (52 commits) docs: Updates to Superset Site for 1.0 (apache#12626) test(native-filters): scoping tree in native filters modal (apache#12655) Fix tests errors and warnings - iteration 3 (apache#12212) (apache#12219) Fix tests errors and warnings - iteration 5 (apache#12212) (apache#12224) Fix tests errors and warnings - iteration 6 (apache#12212) (apache#12227) feat(native-filters): apply scoping of native filters to dashboard (apache#12716) Fix tests errors and warnings - iteration 4 (apache#12212) (apache#12223) Fix tests errors and warnings - iteration 7 (apache#12212) (apache#12245) fix: missing select menu background (apache#12759) fix(explore): incorrect missing datasource condition (apache#12758) feat: default timepicker to last week when dataset is changed (apache#12609) feat(explore): allow opening charts with missing dataset (apache#12705) chore: upgrade Cypress to 6.2.1 (apache#12605) refactor(explore): Enhance Dataset and Control panel Collapse components (apache#12218) feat: Adding option to set_database_uri CLI command (apache#12740) docs: Fixed typo on line 348 (apache#12739) Fix tests errors and warnings - iteration 2 (apache#12212) (apache#12214) docs: Remove gatsby-plugin-offline (apache#12693) test: oracle engine spec (apache#12615) test: hive db engine spec (apache#12520) ...
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.
Small proposal to improve how the feature flag is handled
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.
Code LGTM, tested the following:
- FF enabled: no roles for dashboard works like before
- FF enabled: role specified - only shows up for the roles listed on the dashboard
- FF disabled: roles don't affect dashboard visibility
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.
Looking great! Thanks for the iterations.
Screen.Recording.2021-01-30.at.10.41.59.PM.movtested~ for some reason, getting "unexpected errors" on all charts in the dashboard from gamma account. is it something I did wrong? @amitmiran137 |
thanks for checking this. |
Got it, thanks for adding test plan, and clarifying your plan going forward. we should expect a follow up PR to address both issues mentioned above. |
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.
Approving as product signoff. thank you so much for your contribution. 🙏
SUMMARY
Role based dashboard level access with focus on dashboard list APIs
credits also goes to @Ofeknielsen who took part in this months ago
resolves #10408
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2021-01-23.at.14.12.29.mov
Design slide;
https://user-images.githubusercontent.com/4025227/105494222-6302e400-5cb2-11eb-8f0d-6a609d73cc6c.png
TEST PLAN
ff:
DASHBOARD_RBAC: True
ENABLE_REACT_CRUD_VIEWS: False
happy path:
backward compatibility:
check backward compatibility by DASHBOARD_RBAC: False
access should be managed by the data level
code test coverage plan
dashboards list page for the following scenarios both for dataset and RBAC levels
To make sure we keep backward compatibility and new feature alignment on capabilities
Test need to be written in a decoupled approch from existing users/roles/tables/dashboards so other tests suites won't break
ADDITIONAL INFORMATION