[security] Refactor security code into SupersetSecurityManager (#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

UPDATING.MD

@@ -0,0 +1,8 @@
+# Updating Superset
+
+This file documents any backwards-incompatible changes in Superset and
+assists people when migrating to a new version.
+
+## Superset 0.23.0
+
+* [4565](https://github.com/apache/incubator-superset/pull/4565)

superset/__init__.py

@@ -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')
@@ -149,16 +150,23 @@ def index(self):
         return redirect('/superset/welcome')
 
 
+custom_sm = app.config.get('CUSTOM_SECURITY_MANAGER') or SupersetSecurityManager
+if not issubclass(custom_sm, SupersetSecurityManager):
+    raise Exception(
+        """Your CUSTOM_SECURITY_MANAGER must now extend SupersetSecurityManager,
+         not FAB's security manager.
+         See [4565] in UPDATING.md""")
+
 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(),
 )
 
-sm = appbuilder.sm
+security_manager = appbuilder.sm
 
 results_backend = app.config.get('RESULTS_BACKEND')
 

superset/cli.py

@@ -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, security_manager, utils
 
 config = app.config
 celery_app = utils.get_celery_app(config)
@@ -28,7 +28,8 @@
 @manager.command
 def init():
     """Inits the Superset application"""
-    security.sync_role_definitions()
+    utils.get_or_create_main_db()
+    security_manager.sync_role_definitions()
 
 
 @manager.option(
@@ -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()

superset/connectors/druid/models.py

@@ -33,7 +33,7 @@
 )
 from sqlalchemy.orm import backref, relationship
 
-from superset import conf, db, import_util, sm, utils
+from superset import conf, db, import_util, security_manager, utils
 from superset.connectors.base.models import BaseColumn, BaseDatasource, BaseMetric
 from superset.exceptions import MetricPermException
 from superset.models.helpers import (
@@ -465,7 +465,7 @@ class DruidDatasource(Model, BaseDatasource):
         'DruidCluster', backref='datasources', foreign_keys=[cluster_name])
     user_id = Column(Integer, ForeignKey('ab_user.id'))
     owner = relationship(
-        sm.user_model,
+        security_manager.user_model,
         backref=backref('datasources', cascade='all, delete-orphan'),
         foreign_keys=[user_id])
     UniqueConstraint('cluster_name', 'datasource_name')
@@ -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 security_manager.get_schema_perm(self.cluster, self.schema)
 
     def get_perm(self):
         return (
@@ -980,7 +980,7 @@ def check_restricted_metrics(self, aggregations):
             m.metric_name for m in self.metrics
             if m.is_restricted and
             m.metric_name in aggregations.keys() and
-            not sm.has_access('metric_access', m.perm)
+            not security_manager.has_access('metric_access', m.perm)
         ]
         if rejected_metrics:
             raise MetricPermException(

superset/connectors/druid/views.py

@@ -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, security_manager, utils
 from superset.connectors.base.views import DatasourceModelView
 from superset.connectors.connector_registry import ConnectorRegistry
 from superset.utils import has_access
@@ -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())
+            security_manager.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())
+            security_manager.merge_perm('metric_access', metric.get_perm())
 
 
 appbuilder.add_view_no_menu(DruidMetricInlineView)
@@ -177,7 +177,7 @@ class DruidClusterModelView(SupersetModelView, DeleteMixin, YamlExportMixin):  #
     }
 
     def pre_add(self, cluster):
-        security.merge_perm(sm, 'database_access', cluster.perm)
+        security_manager.merge_perm('database_access', cluster.perm)
 
     def pre_update(self, cluster):
         self.pre_add(cluster)
@@ -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())
+        security_manager.merge_perm('datasource_access', datasource.get_perm())
         if datasource.schema:
-            security.merge_perm(sm, 'schema_access', datasource.schema_perm)
+            security_manager.merge_perm('schema_access', datasource.schema_perm)
 
     def post_update(self, datasource):
         self.post_add(datasource)

superset/connectors/sqla/models.py

@@ -24,7 +24,7 @@
 from sqlalchemy.sql.expression import TextAsFrom
 import sqlparse
 
-from superset import db, import_util, sm, utils
+from superset import db, import_util, security_manager, utils
 from superset.connectors.base.models import BaseColumn, BaseDatasource, BaseMetric
 from superset.jinja_context import get_template_processor
 from superset.models.annotations import Annotation
@@ -259,7 +259,7 @@ class SqlaTable(Model, BaseDatasource):
     fetch_values_predicate = Column(String(1000))
     user_id = Column(Integer, ForeignKey('ab_user.id'))
     owner = relationship(
-        sm.user_model,
+        security_manager.user_model,
         backref='tables',
         foreign_keys=[user_id])
     database = relationship(
@@ -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 security_manager.get_schema_perm(self.database, self.schema)
 
     def get_perm(self):
         return (

superset/connectors/sqla/views.py

@@ -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, security_manager, utils
 from superset.connectors.base.views import DatasourceModelView
 from superset.utils import has_access
 from superset.views.base import (
@@ -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())
+            security_manager.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())
+            security_manager.merge_perm('metric_access', metric.get_perm())
 
 
 appbuilder.add_view_no_menu(SqlMetricInlineView)
@@ -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())
+        security_manager.merge_perm('datasource_access', table.get_perm())
         if table.schema:
-            security.merge_perm(sm, 'schema_access', table.schema_perm)
+            security_manager.merge_perm('schema_access', table.schema_perm)
 
         if flash_message:
             flash(_(

superset/data/__init__.py

@@ -16,10 +16,9 @@
 import geohash
 import polyline
 
-from superset import app, db, utils
+from superset import app, db, security_manager, 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
@@ -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()
@@ -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()
@@ -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()
@@ -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()
@@ -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()
@@ -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()
@@ -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()
@@ -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],
@@ -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()
@@ -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()
@@ -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()
@@ -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()

superset/models/core.py

@@ -33,7 +33,7 @@
 from sqlalchemy.sql.expression import TextAsFrom
 from sqlalchemy_utils import EncryptedType
 
-from superset import app, db, db_engine_specs, sm, utils
+from superset import app, db, db_engine_specs, security_manager, utils
 from superset.connectors.connector_registry import ConnectorRegistry
 from superset.models.helpers import AuditMixinNullable, ImportMixin, set_perm
 from superset.viz import viz_types
@@ -104,7 +104,7 @@ class Slice(Model, AuditMixinNullable, ImportMixin):
     description = Column(Text)
     cache_timeout = Column(Integer)
     perm = Column(String(1000))
-    owners = relationship(sm.user_model, secondary=slice_user)
+    owners = relationship(security_manager.user_model, secondary=slice_user)
 
     export_fields = ('slice_name', 'datasource_type', 'datasource_name',
                      'viz_type', 'params', 'cache_timeout')
@@ -322,7 +322,7 @@ class Dashboard(Model, AuditMixinNullable, ImportMixin):
     slug = Column(String(255), unique=True)
     slices = relationship(
         'Slice', secondary=dashboard_slices, backref='dashboards')
-    owners = relationship(sm.user_model, secondary=dashboard_user)
+    owners = relationship(security_manager.user_model, secondary=dashboard_user)
 
     export_fields = ('dashboard_title', 'position_json', 'json_metadata',
                      'description', 'css', 'slug')
@@ -681,7 +681,7 @@ def get_sqla_engine(self, schema=None, nullpool=True, user_name=None):
         DB_CONNECTION_MUTATOR = config.get('DB_CONNECTION_MUTATOR')
         if DB_CONNECTION_MUTATOR:
             url, params = DB_CONNECTION_MUTATOR(
-                url, params, effective_username, sm)
+                url, params, effective_username, security_manager)
         return create_engine(url, **params)
 
     def get_reserved_words(self):
@@ -862,7 +862,8 @@ class Log(Model):
     dashboard_id = Column(Integer)
     slice_id = Column(Integer)
     json = Column(Text)
-    user = relationship(sm.user_model, backref='logs', foreign_keys=[user_id])
+    user = relationship(
+        security_manager.user_model, backref='logs', foreign_keys=[user_id])
     dttm = Column(DateTime, default=datetime.utcnow)
     dt = Column(Date, default=date.today())
     duration_ms = Column(Integer)
@@ -961,7 +962,7 @@ def datasource_link(self):
     def roles_with_datasource(self):
         action_list = ''
         perm = self.datasource.perm  # pylint: disable=no-member
-        pv = sm.find_permission_view_menu('datasource_access', perm)
+        pv = security_manager.find_permission_view_menu('datasource_access', perm)
         for r in pv.role:
             if r.name in self.ROLES_BLACKLIST:
                 continue