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.

Sign up for GitHub

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

Jump to bottom

[security] Refactor security code into SupersetSecurityManager #4565

Merged
merged 7 commits into from
Mar 27, 2018
Merged

[security] Refactor security code into SupersetSecurityManager #4565

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1cd6a9e
move access permissions methods to security manager
timifasubaa Feb 26, 2018
434a146
consolidate all security methods into SupersetSecurityManager
timifasubaa Mar 25, 2018
f3d9fd7
update security method calls
timifasubaa Mar 25, 2018
7c32934
update calls from tests
timifasubaa Mar 25, 2018
a385444
move get_or_create_main_db to utils
timifasubaa Mar 27, 2018
4124ac0
raise if supersetsecuritymanager is not extended
timifasubaa Mar 27, 2018
c766500
rename sm to security_manager
timifasubaa Mar 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions superset/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@
from flask_wtf.csrf import CSRFProtect
from werkzeug.contrib.fixers import ProxyFix

from superset import config, utils
from superset.connectors.connector_registry import ConnectorRegistry
from superset import utils, config # noqa
from superset.security import SupersetSecurityManager

APP_DIR = os.path.dirname(__file__)
CONFIG_MODULE = os.environ.get('SUPERSET_CONFIG', 'superset.config')
Expand Down Expand Up @@ -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.

This can be shortened to be:

sm = app.config.get('CUSTOM_SECURITY_MANAGER', SupersetSecurityManager)

Copy link
Contributor Author

@timifasubaa timifasubaa Mar 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found out that this only works when the variable doesn't exist. In this case, it is set to be None and so it returns None.

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

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)

appbuilder = AppBuilder(
app,
db.session,
base_template='superset/base.html',
indexview=MyIndexView,
security_manager_class=app.config.get('CUSTOM_SECURITY_MANAGER'),
security_manager_class=custom_sm,
update_perms=utils.get_update_perms_flag(),
)

Expand Down
6 changes: 3 additions & 3 deletions superset/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).


config = app.config
celery_app = utils.get_celery_app(config)
Expand All @@ -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.

sm.sync_role_definitions()


@manager.option(
Expand Down Expand Up @@ -108,7 +109,6 @@ def version(verbose):
help='Load additional test data')
def load_examples(load_test_data):
"""Loads a set of Slices and Dashboards and a supporting dataset """
from superset import data
print('Loading examples into {}'.format(db))

data.load_css_templates()
Expand Down
2 changes: 1 addition & 1 deletion superset/connectors/druid/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ def schema(self):
@property
def schema_perm(self):
"""Returns schema permission if present, cluster one otherwise."""
return utils.get_schema_perm(self.cluster, self.schema)
return sm.get_schema_perm(self.cluster, self.schema)

def get_perm(self):
return (
Expand Down
12 changes: 6 additions & 6 deletions superset/connectors/druid/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from flask_babel import gettext as __
from flask_babel import lazy_gettext as _

from superset import appbuilder, db, security, sm, utils
from superset import appbuilder, db, sm, utils
from superset.connectors.base.views import DatasourceModelView
from superset.connectors.connector_registry import ConnectorRegistry
from superset.utils import has_access
Expand Down Expand Up @@ -140,11 +140,11 @@ class DruidMetricInlineView(CompactCRUDMixin, SupersetModelView): # noqa

def post_add(self, metric):
if metric.is_restricted:
security.merge_perm(sm, 'metric_access', metric.get_perm())
sm.merge_perm('metric_access', metric.get_perm())

def post_update(self, metric):
if metric.is_restricted:
security.merge_perm(sm, 'metric_access', metric.get_perm())
sm.merge_perm('metric_access', metric.get_perm())


appbuilder.add_view_no_menu(DruidMetricInlineView)
Expand Down Expand Up @@ -177,7 +177,7 @@ class DruidClusterModelView(SupersetModelView, DeleteMixin, YamlExportMixin): #
}

def pre_add(self, cluster):
security.merge_perm(sm, 'database_access', cluster.perm)
sm.merge_perm('database_access', cluster.perm)

def pre_update(self, cluster):
self.pre_add(cluster)
Expand Down Expand Up @@ -278,9 +278,9 @@ def pre_add(self, datasource):

def post_add(self, datasource):
datasource.refresh_metrics()
security.merge_perm(sm, 'datasource_access', datasource.get_perm())
sm.merge_perm('datasource_access', datasource.get_perm())
if datasource.schema:
security.merge_perm(sm, 'schema_access', datasource.schema_perm)
sm.merge_perm('schema_access', datasource.schema_perm)

def post_update(self, datasource):
self.post_add(datasource)
Expand Down
2 changes: 1 addition & 1 deletion superset/connectors/sqla/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ def link(self):
@property
def schema_perm(self):
"""Returns schema permission if present, database one otherwise."""
return utils.get_schema_perm(self.database, self.schema)
return sm.get_schema_perm(self.database, self.schema)

def get_perm(self):
return (
Expand Down
10 changes: 5 additions & 5 deletions superset/connectors/sqla/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

from superset.connectors.base.views import DatasourceModelView
from superset.utils import has_access
from superset.views.base import (
Expand Down Expand Up @@ -144,11 +144,11 @@ class SqlMetricInlineView(CompactCRUDMixin, SupersetModelView): # noqa

def post_add(self, metric):
if metric.is_restricted:
security.merge_perm(sm, 'metric_access', metric.get_perm())
sm.merge_perm('metric_access', metric.get_perm())

def post_update(self, metric):
if metric.is_restricted:
security.merge_perm(sm, 'metric_access', metric.get_perm())
sm.merge_perm('metric_access', metric.get_perm())


appbuilder.add_view_no_menu(SqlMetricInlineView)
Expand Down Expand Up @@ -253,9 +253,9 @@ def pre_add(self, table):

def post_add(self, table, flash_message=True):
table.fetch_metadata()
security.merge_perm(sm, 'datasource_access', table.get_perm())
sm.merge_perm('datasource_access', table.get_perm())
if table.schema:
security.merge_perm(sm, 'schema_access', table.schema_perm)
sm.merge_perm('schema_access', table.schema_perm)

if flash_message:
flash(_(
Expand Down
27 changes: 13 additions & 14 deletions superset/data/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,9 @@
import geohash
import polyline

from superset import app, db, utils
from superset import app, db, sm, utils
from superset.connectors.connector_registry import ConnectorRegistry
from superset.models import core as models
from superset.security import get_or_create_main_db

# Shortcuts
DB = models.Database
Expand Down Expand Up @@ -71,7 +70,7 @@ def load_energy():
if not tbl:
tbl = TBL(table_name=tbl_name)
tbl.description = "Energy consumption"
tbl.database = get_or_create_main_db()
tbl.database = utils.get_or_create_main_db()
db.session.merge(tbl)
db.session.commit()
tbl.fetch_metadata()
Expand Down Expand Up @@ -179,7 +178,7 @@ def load_world_bank_health_n_pop():
tbl = TBL(table_name=tbl_name)
tbl.description = utils.readfile(os.path.join(DATA_FOLDER, 'countries.md'))
tbl.main_dttm_col = 'year'
tbl.database = get_or_create_main_db()
tbl.database = utils.get_or_create_main_db()
tbl.filter_select_enabled = True
db.session.merge(tbl)
db.session.commit()
Expand Down Expand Up @@ -583,7 +582,7 @@ def load_birth_names():
if not obj:
obj = TBL(table_name='birth_names')
obj.main_dttm_col = 'ds'
obj.database = get_or_create_main_db()
obj.database = utils.get_or_create_main_db()
obj.filter_select_enabled = True
db.session.merge(obj)
db.session.commit()
Expand Down Expand Up @@ -870,7 +869,7 @@ def load_unicode_test_data():
if not obj:
obj = TBL(table_name='unicode_test')
obj.main_dttm_col = 'dttm'
obj.database = get_or_create_main_db()
obj.database = utils.get_or_create_main_db()
db.session.merge(obj)
db.session.commit()
obj.fetch_metadata()
Expand Down Expand Up @@ -948,7 +947,7 @@ def load_random_time_series_data():
if not obj:
obj = TBL(table_name='random_time_series')
obj.main_dttm_col = 'ds'
obj.database = get_or_create_main_db()
obj.database = utils.get_or_create_main_db()
db.session.merge(obj)
db.session.commit()
obj.fetch_metadata()
Expand Down Expand Up @@ -1011,7 +1010,7 @@ def load_country_map_data():
if not obj:
obj = TBL(table_name='birth_france_by_region')
obj.main_dttm_col = 'dttm'
obj.database = get_or_create_main_db()
obj.database = utils.get_or_create_main_db()
db.session.merge(obj)
db.session.commit()
obj.fetch_metadata()
Expand Down Expand Up @@ -1086,7 +1085,7 @@ def load_long_lat_data():
if not obj:
obj = TBL(table_name='long_lat')
obj.main_dttm_col = 'datetime'
obj.database = get_or_create_main_db()
obj.database = utils.get_or_create_main_db()
db.session.merge(obj)
db.session.commit()
obj.fetch_metadata()
Expand Down Expand Up @@ -1147,7 +1146,7 @@ def load_multiformat_time_series_data():
if not obj:
obj = TBL(table_name='multiformat_time_series')
obj.main_dttm_col = 'ds'
obj.database = get_or_create_main_db()
obj.database = utils.get_or_create_main_db()
dttm_and_expr_dict = {
'ds': [None, None],
'ds2': [None, None],
Expand Down Expand Up @@ -1769,7 +1768,7 @@ def load_flights():
if not tbl:
tbl = TBL(table_name=tbl_name)
tbl.description = "Random set of flights in the US"
tbl.database = get_or_create_main_db()
tbl.database = utils.get_or_create_main_db()
db.session.merge(tbl)
db.session.commit()
tbl.fetch_metadata()
Expand Down Expand Up @@ -1800,7 +1799,7 @@ def load_paris_iris_geojson():
if not tbl:
tbl = TBL(table_name=tbl_name)
tbl.description = "Map of Paris"
tbl.database = get_or_create_main_db()
tbl.database = utils.get_or_create_main_db()
db.session.merge(tbl)
db.session.commit()
tbl.fetch_metadata()
Expand Down Expand Up @@ -1830,7 +1829,7 @@ def load_sf_population_polygons():
if not tbl:
tbl = TBL(table_name=tbl_name)
tbl.description = "Population density of San Francisco"
tbl.database = get_or_create_main_db()
tbl.database = utils.get_or_create_main_db()
db.session.merge(tbl)
db.session.commit()
tbl.fetch_metadata()
Expand Down Expand Up @@ -1860,7 +1859,7 @@ def load_bart_lines():
if not tbl:
tbl = TBL(table_name=tbl_name)
tbl.description = "BART lines"
tbl.database = get_or_create_main_db()
tbl.database = utils.get_or_create_main_db()
db.session.merge(tbl)
db.session.commit()
tbl.fetch_metadata()
Loading