-
Notifications
You must be signed in to change notification settings - Fork 16.6k
Fix issues around Database permissions #7009
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
Conversation
eaf6301 to
b072dee
Compare
Codecov Report
@@ Coverage Diff @@
## master #7009 +/- ##
==========================================
+ Coverage 64.46% 64.47% +<.01%
==========================================
Files 421 421
Lines 20521 20534 +13
Branches 2245 2245
==========================================
+ Hits 13229 13239 +10
- Misses 7165 7168 +3
Partials 127 127
Continue to review full report at Codecov.
|
| # add to view menu if not already exists | ||
| permission_name = 'datasource_access' | ||
| from superset.models.core import Database | ||
| if mapper.class_ == Database: |
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.
FYI , superset init would create the proper database_access (one for each database), but the code here (executed on update/insert) would create/update a datasource_access entry for the database
| .outerjoin(models.Database, models.Database.id == SQLTable.c.database_id) | ||
| .filter(or_( | ||
| models.Database.perm.in_(datasource_perms), | ||
| models.Database.perm.in_(database_perms), |
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.
This appears to be using the wrong entry.
graceguo-supercat
left a comment
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
| 'allow_multi_schema_metadata_fetch': _('Allow Multi Schema Metadata Fetch'), | ||
| 'backend': _('Backend'), | ||
| } | ||
| base_filters = [['id', DatabaseFilter, lambda: []]] |
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 think this broke stuff on our end, users can't see DBs in SQL Lab. I commented it out and permissions work correctly. (edited to say "can't")
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.
Do you mean "can't see" (you wrote "can see"). This may require adding all_database_access to alpha
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.
This reverts commit f5274a9.
No description provided.