Skip to content
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

Conversation

timifasubaa
Copy link
Contributor

@timifasubaa timifasubaa commented Mar 7, 2018

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

@timifasubaa timifasubaa force-pushed the move_access_permissions_to_security_manager branch 13 times, most recently from 69ab53d to 071a823 Compare March 12, 2018 08:42
@timifasubaa timifasubaa force-pushed the move_access_permissions_to_security_manager branch 5 times, most recently from fd9a753 to ba3009f Compare March 15, 2018 18:07
@codecov-io
Copy link

codecov-io commented Mar 15, 2018

Codecov Report

Merging #4565 into master will decrease coverage by 0.02%.
The diff coverage is 70.95%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
superset/connectors/druid/models.py 78.85% <100%> (ø) ⬆️
superset/connectors/sqla/models.py 77.91% <100%> (ø) ⬆️
superset/utils.py 88.12% <100%> (+0.15%) ⬆️
superset/data/__init__.py 100% <100%> (ø) ⬆️
superset/models/helpers.py 87.5% <100%> (ø) ⬆️
superset/models/sql_lab.py 98.59% <100%> (ø) ⬆️
superset/connectors/druid/views.py 68.02% <16.66%> (ø) ⬆️
superset/views/base.py 67.12% <20%> (+8.86%) ⬆️
superset/connectors/sqla/views.py 71.56% <20%> (ø) ⬆️
superset/cli.py 47.74% <33.33%> (-0.65%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9d85bd...c766500. Read the comment docs.

@timifasubaa timifasubaa force-pushed the move_access_permissions_to_security_manager branch 4 times, most recently from dda246b to 56d0116 Compare March 16, 2018 06:12
@timifasubaa timifasubaa changed the title [WIP] Move access permissions methods to security manager Move access permissions methods to security manager Mar 16, 2018
@timifasubaa
Copy link
Contributor Author

ping

@mistercrunch
Copy link
Member

Why 2 security managers? To me the inheritance scheme should be SupersetSecurityManager (new abstraction) inherits from FAB's SecurityManager, and environment-specific say AirbnbSupersetSecurityManager should be forced to derived SupersetSecurityManager.

I don't think it makes sense to grow in the direction of having many security managers. What's next? DatabaseSecurityManager, DashboardSecurityManager?

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 SupersetSecurityManagerMixin that we'd somehow inject into FAB's or the enviornment-specific SecurityManger. Not sure if runtime injection of mixin is an acceptable pattern though.

@john-bodley
Copy link
Member

@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.

@timifasubaa timifasubaa force-pushed the move_access_permissions_to_security_manager branch 3 times, most recently from 4222e46 to 2ff5790 Compare March 25, 2018 07:32
@timifasubaa timifasubaa force-pushed the move_access_permissions_to_security_manager branch from c2e7996 to 940e71b Compare March 27, 2018 02:37
@timifasubaa timifasubaa force-pushed the move_access_permissions_to_security_manager branch from 940e71b to a385444 Compare March 27, 2018 02:40
Copy link
Member

@mistercrunch mistercrunch left a 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?

custom_sm = app.config.get('CUSTOM_SECURITY_MANAGER')
if not custom_sm:
custom_sm = SupersetSecurityManager

Copy link
Member

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
Copy link
Member

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.

Copy link
Member

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).

@@ -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
Copy link
Member

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.

@@ -149,12 +150,16 @@ def index(self):
return redirect('/superset/welcome')


custom_sm = app.config.get('CUSTOM_SECURITY_MANAGER')
Copy link
Member

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 :)

@timifasubaa timifasubaa force-pushed the move_access_permissions_to_security_manager branch 5 times, most recently from 89053da to 6903ba1 Compare March 27, 2018 18:01
@timifasubaa
Copy link
Contributor Author

@mistercrunch @john-bodley Done!

raise Exception(
"""Your CUSTOM_SECURITY_MANAGER must now extend SupersetSecurityManager,
not FAB's security manager.
See https://github.com/apache/incubator-superset/pull/4565""")
Copy link
Member

@john-bodley john-bodley Mar 27, 2018

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()
Copy link
Member

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?

Copy link
Contributor Author

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.

@timifasubaa timifasubaa force-pushed the move_access_permissions_to_security_manager branch from 6903ba1 to 88d3582 Compare March 27, 2018 22:05
@timifasubaa timifasubaa force-pushed the move_access_permissions_to_security_manager branch from 88d3582 to c766500 Compare March 27, 2018 22:08
Copy link
Contributor Author

@timifasubaa timifasubaa left a 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()
Copy link
Contributor Author

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.

@john-bodley john-bodley merged commit 8dd052d into apache:master Mar 27, 2018
timifasubaa added a commit to timifasubaa/incubator-superset that referenced this pull request Apr 2, 2018
…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)
Copy link
Member

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)

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
…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
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
…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
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants