-
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
[security] Refactor security code into SupersetSecurityManager #4565
[security] Refactor security code into SupersetSecurityManager #4565
Conversation
69ab53d
to
071a823
Compare
fd9a753
to
ba3009f
Compare
Codecov Report
@@ Coverage Diff @@
## master #4565 +/- ##
=========================================
- Coverage 71.52% 71.5% -0.03%
=========================================
Files 190 190
Lines 14927 14935 +8
Branches 1102 1102
=========================================
+ Hits 10677 10679 +2
- Misses 4247 4253 +6
Partials 3 3
Continue to review full report at Codecov.
|
dda246b
to
56d0116
Compare
ping |
Why 2 security managers? To me the inheritance scheme should be I don't think it makes sense to grow in the direction of having many security managers. What's next? The scheme I described is a [necessary] breaking change since I'd assume in most environment have a custom SecurityManager that currently derives FAB's and will need to change to derive Superset's. An [perhaps compatible] alternative would be a |
@mistercrunch the thinking was that the default security manager is still tightly coupled with FAB concepts (views/menus) which correspond to security related to the app UI and API. These concepts are irrelevant from a data security perspective and thus the reason for two security managers. |
4222e46
to
2ff5790
Compare
c2e7996
to
940e71b
Compare
940e71b
to
a385444
Compare
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 looks great.
After reading the PR I think it should be in scope to rename sm
to security_manager
everywhere. Rename all the things!
Curious whether you ran into circular deps issues?
superset/__init__.py
Outdated
custom_sm = app.config.get('CUSTOM_SECURITY_MANAGER') | ||
if not custom_sm: | ||
custom_sm = SupersetSecurityManager | ||
|
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.
We should absolutely raise with an explicit message with perhaps a link to the UPDATE notes if not issubclass(custom_sm, SupersetSecurityManager)
superset/cli.py
Outdated
@@ -16,7 +16,7 @@ | |||
from pathlib2 import Path | |||
import yaml | |||
|
|||
from superset import app, db, dict_import_export_util, security, utils | |||
from superset import app, data, db, dict_import_export_util, sm, utils |
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 could be out-of-scope for this PR, but it would be nice to always be explicit about sm
and always call it security_manager
(my bad!).
It may be reasonable to take it on as part of this PR.
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 don’t mind the shortened sm (inline with db and other variables) as it’s used frequently and is considerably shorter than security_manager (2 characters vs. 16).
superset/connectors/sqla/views.py
Outdated
@@ -13,7 +13,7 @@ | |||
from flask_babel import lazy_gettext as _ | |||
from past.builtins import basestring | |||
|
|||
from superset import appbuilder, db, security, sm, utils | |||
from superset import appbuilder, db, sm, utils |
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.
Nice! Having both security
and sm
was confusing, we should have a single entry point for all security checks and operations.
superset/__init__.py
Outdated
@@ -149,12 +150,16 @@ def index(self): | |||
return redirect('/superset/welcome') | |||
|
|||
|
|||
custom_sm = app.config.get('CUSTOM_SECURITY_MANAGER') |
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.
Then you could sm = app.config.get('CUSTOM_SECURITY_MANAGER') or SupersetSecurityManager
:)
89053da
to
6903ba1
Compare
@mistercrunch @john-bodley Done! |
superset/__init__.py
Outdated
raise Exception( | ||
"""Your CUSTOM_SECURITY_MANAGER must now extend SupersetSecurityManager, | ||
not FAB's security manager. | ||
See https://github.com/apache/incubator-superset/pull/4565""") |
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'm not certain whether we should be referencing other PRs in the code. Given this is a breaking PR you may want to follow this pattern: #4587.
@@ -28,7 +28,8 @@ | |||
@manager.command | |||
def init(): | |||
"""Inits the Superset application""" | |||
security.sync_role_definitions() | |||
utils.get_or_create_main_db() |
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.
Why is this now needed?
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.
Previously it was called inside of sync_role_definitions and that led to weird cyclical dependencies after moving the method to utils.py.
This also make it clear what steps it takes to initialize rather than bundle them together inside one method.
6903ba1
to
88d3582
Compare
88d3582
to
c766500
Compare
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 added an updating.md as suggested.
@@ -28,7 +28,8 @@ | |||
@manager.command | |||
def init(): | |||
"""Inits the Superset application""" | |||
security.sync_role_definitions() | |||
utils.get_or_create_main_db() |
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.
Previously it was called inside of sync_role_definitions and that led to weird cyclical dependencies after moving the method to utils.py.
This also make it clear what steps it takes to initialize rather than bundle them together inside one method.
…e#4565) * move access permissions methods to security manager * consolidate all security methods into SupersetSecurityManager * update security method calls * update calls from tests * move get_or_create_main_db to utils * raise if supersetsecuritymanager is not extended * rename sm to security_manager
def merge_pv(view_menu, perm): | ||
"""Create permission view menu only if it doesn't exist""" | ||
if view_menu and perm and (view_menu, perm) not in all_pvs: | ||
self.merge_perm(view_menu, perm) |
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.
should it be merge_perm(perm, view_menu) as the definition is " def merge_perm(self, permission_name, view_menu_name):" ? @timifasubaa @mistercrunch (#4745)
…e#4565) * move access permissions methods to security manager * consolidate all security methods into SupersetSecurityManager * update security method calls * update calls from tests * move get_or_create_main_db to utils * raise if supersetsecuritymanager is not extended * rename sm to security_manager
…e#4565) * move access permissions methods to security manager * consolidate all security methods into SupersetSecurityManager * update security method calls * update calls from tests * move get_or_create_main_db to utils * raise if supersetsecuritymanager is not extended * rename sm to security_manager
This PR refactors all the security methods into the extensible SupersetSecurityManager. I left has_access in utils.py for now as it is defined differently from the method with the same name in fab's security manager. I will make that change in another PR.
Historically, superset defined its security methods in different files. All over the code base, there are calls to security methods in security.py, utils.py, fab's security manager and the base superset view. By consolidating all the logic in one place it creates one abstraction for all security in superset and makes the code more understandable.
One example of a change resulting from this consolidation of the security logic is
security.merge_perm(sm, permission_name, view_menu_name)
becoming
sm.merge_perm(permission_name, view_menu_name)
This is much cleaner in several ways as a security object is not receiving another security object as a parameter. There are other examples of methods in utils directly accessing fab security manager's private methods.
@john-bodley @mistercrunch