Support DB perms #8816
Support DB perms #8816
Conversation
|
@bkyryliuk I think you have a fair amount of context here ^^^ :) |
Codecov Report
@@ Coverage Diff @@
## master #8816 +/- ##
==========================================
+ Coverage 65.84% 65.89% +0.04%
==========================================
Files 483 483
Lines 24177 24180 +3
Branches 2777 2777
==========================================
+ Hits 15920 15933 +13
+ Misses 8079 8069 -10
Partials 178 178
Continue to review full report at Codecov.
|
|
Totally a side note and outside the scope of this PR, but I think eventually we might want to not use FAB's permission model for data access information. Instead we'd have a many to many from roles to schemas/tables/database. I'm not sure whether we'd overload the current |
|
@betodealmeida - definitely add unit tests, example is here: https://github.com/apache/incubator-superset/blob/master/tests/security_tests.py#L349 |
| class SliceFilter(BaseFilter): | ||
| def apply(self, query, func): # noqa | ||
| if security_manager.all_datasource_access(): | ||
| return query | ||
| perms = security_manager.user_view_menu_names("datasource_access") | ||
| database_perms = security_manager.user_view_menu_names("database_access") |
There was a problem hiding this comment.
what do you think about materializing db permissions in the tables and slices?
this would simplify the checks significantly.
Right, we should definitely rethink it. Today at Lyft somebody requested setting the ownership of a dashboard to a team (which would be a role?), not only users. |
I'm curious on how we would overload |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue |
|
Please don't let the stale bot close this issue... |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue |
|
Please don't let the stale bot close this issue... |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue |
|
Please don't let the stale bot close this issue... |
CATEGORY
Choose one
SUMMARY
Currently, if a user has permissions to a given DB, they can't see charts or dashboards built on top of that DB, since
SliceFilterandDashboardFilterignore the DB permissions.@mistercrunch I tried to fix this in the past (#6933), but I used the wrong permissions and we reverted the PR. This one uses the actual database permissions.
TEST PLAN
Created users that have access to only a specific DB, and verified that the charts and dashboards showed up.
ADDITIONAL INFORMATION
REVIEWERS
@mistercrunch @dpgaspar