-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
[security] New, deprecate merge_perm, FAB method is fixed #7355
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7355 +/- ##
==========================================
+ Coverage 64.89% 65.18% +0.28%
==========================================
Files 428 433 +5
Lines 20937 21428 +491
Branches 2330 2360 +30
==========================================
+ Hits 13588 13968 +380
- Misses 7225 7340 +115
+ Partials 124 120 -4
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.
Will merge after the FAB 2.0 PR is merged
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.
Minor comment, otherwise LGTM.
The SupersetSecurityManager
API is fairly public given that in most settings people are expected to create their own security manager. Let's make sure upgrading is easy.
superset/security.py
Outdated
@@ -262,27 +262,17 @@ def accessible_by_user(self, database, datasource_names, schema=None): | |||
full_names = {d.full_name for d in user_datasources} | |||
return [d for d in datasource_names if d in full_names] | |||
|
|||
def merge_perm(self, permission_name, view_menu_name): |
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.
For backwards compatibility, we should just call self.add_permission_view_menu
here and maybe add a deprecation warning.
CATEGORY
SUMMARY
Since FAB version 2.0.0 that safe guards made on merge_perms on Superset were included on FAB.
This makes it possible to simplify Superset's custom SecurityManager. And remove
merge_perms
.Note: Only after PR #7323
ADDITIONAL INFORMATION
REVIEWERS