Only creating perms for restricted metrics (#655)

* Only creating perms for restricted metrics

* Adding post_update hooks for is_restricted

caravel/models.py

@@ -1196,19 +1196,19 @@ def recursive_get_fields(_conf):
             m.metric_name: m.json_obj
             for m in self.metrics
             if m.metric_name in all_metrics
-            }
+        }
 
         rejected_metrics = [
             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)
-            ]
+        ]
 
         if rejected_metrics:
             raise MetricPermException(
                 "Access to the metrics denied: " + ', '.join(rejected_metrics)
-            )
+        )
 
         granularity = granularity or "all"
         if granularity != "all":

caravel/utils.py

@@ -34,6 +34,15 @@ class MetricPermException(Exception):
     pass
 
 
+def can_access(security_manager, permission_name, view_name):
+    """Protecting from has_access failing from missing perms/view"""
+    try:
+        return security_manager.has_access(permission_name, view_name)
+    except:
+        pass
+    return False
+
+
 def flasher(msg, severity=None):
     """Flask's flash if available, logging call if not"""
     try:
@@ -232,18 +241,24 @@ def init(caravel):
 
 
 def init_metrics_perm(caravel, metrics=None):
+    """Create permissions for restricted metrics
+
+    :param metrics: a list of metrics to be processed, if not specified,
+        all metrics are processed
+    :type metrics: models.SqlMetric or models.DruidMetric
+    """
     db = caravel.db
     models = caravel.models
     sm = caravel.appbuilder.sm
 
-    if metrics is None:
+    if not metrics:
         metrics = []
         for model in [models.SqlMetric, models.DruidMetric]:
             metrics += list(db.session.query(model).all())
 
-    metric_perms = filter(None, [metric.perm for metric in metrics])
-    for metric_perm in metric_perms:
-        merge_perm(sm, 'metric_access', metric_perm)
+    for metric in metrics:
+        if metric.is_restricted and metric.perm:
+            merge_perm(sm, 'metric_access', metric.perm)
 
 
 def datetime_f(dttm):

caravel/views.py

@@ -36,6 +36,12 @@
 
 config = app.config
 log_this = models.Log.log_this
+can_access = utils.can_access
+
+
+class BaseCaravelView(BaseView):
+    def can_access(self, permission_name, view_name):
+        return utils.can_access(appbuilder.sm, permission_name, view_name)
 
 
 def get_error_msg():
@@ -244,8 +250,7 @@ def post_update(self, col):
 
 class SqlMetricInlineView(CompactCRUDMixin, CaravelModelView):  # noqa
     datamodel = SQLAInterface(models.SqlMetric)
-    list_columns = ['metric_name', 'verbose_name', 'metric_type',
-                    'is_restricted']
+    list_columns = ['metric_name', 'verbose_name', 'metric_type']
     edit_columns = [
         'metric_name', 'description', 'verbose_name', 'metric_type',
         'expression', 'table', 'is_restricted']
@@ -269,16 +274,18 @@ class SqlMetricInlineView(CompactCRUDMixin, CaravelModelView):  # noqa
         'table': _("Table"),
     }
 
-    def post_add(self, new_item):
-        utils.init_metrics_perm(caravel, [new_item])
+    def post_add(self, metric):
+        utils.init_metrics_perm(caravel, [metric])
+
+    def post_update(self, metric):
+        utils.init_metrics_perm(caravel, [metric])
 
 appbuilder.add_view_no_menu(SqlMetricInlineView)
 
 
 class DruidMetricInlineView(CompactCRUDMixin, CaravelModelView):  # noqa
     datamodel = SQLAInterface(models.DruidMetric)
-    list_columns = ['metric_name', 'verbose_name', 'metric_type',
-                    'is_restricted']
+    list_columns = ['metric_name', 'verbose_name', 'metric_type']
     edit_columns = [
         'metric_name', 'description', 'verbose_name', 'metric_type', 'json',
         'datasource', 'is_restricted']
@@ -307,8 +314,11 @@ class DruidMetricInlineView(CompactCRUDMixin, CaravelModelView):  # noqa
         'datasource': _("Druid Datasource"),
     }
 
-    def post_add(self, new_item):
-        utils.init_metrics_perm(caravel, [new_item])
+    def post_add(self, metric):
+        utils.init_metrics_perm(caravel, [metric])
+
+    def post_update(self, metric):
+        utils.init_metrics_perm(caravel, [metric])
 
 
 appbuilder.add_view_no_menu(DruidMetricInlineView)
@@ -685,7 +695,7 @@ def ping():
     return "OK"
 
 
-class R(BaseView):
+class R(BaseCaravelView):
 
     """used for short urls"""
 
@@ -718,16 +728,7 @@ def msg(self):
 appbuilder.add_view_no_menu(R)
 
 
-def caravel_has_access(permission_name, view_name):
-    """Protecting from has_access failing from missing perms/view"""
-    try:
-        return appbuilder.sm.has_access(permission_name, view_name)
-    except:
-        pass
-    return False
-
-
-class Caravel(BaseView):
+class Caravel(BaseCaravelView):
 
     """The base views for Caravel!"""
 
@@ -749,9 +750,9 @@ def explore(self, datasource_type, datasource_id):
         datasource = datasource[0] if datasource else None
         slice_id = request.args.get("slice_id")
         slc = None
-        slice_add_perm = caravel_has_access('can_add', 'SliceModelView')
-        slice_edit_perm = caravel_has_access('can_edit', 'SliceModelView')
-        slice_download_perm = caravel_has_access('can_download', 'SliceModelView')
+        slice_add_perm = self.can_access('can_add', 'SliceModelView')
+        slice_edit_perm = self.can_access('can_edit', 'SliceModelView')
+        slice_download_perm = self.can_access('can_download', 'SliceModelView')
 
         if slice_id:
             slc = (
@@ -763,9 +764,9 @@ def explore(self, datasource_type, datasource_id):
             flash(__("The datasource seems to have been deleted"), "alert")
             return redirect(error_redirect)
 
-        all_datasource_access = caravel_has_access(
+        all_datasource_access = self.can_access(
             'all_datasource_access', 'all_datasource_access')
-        datasource_access = caravel_has_access(
+        datasource_access = self.can_access(
             'datasource_access', datasource.perm)
         if not (all_datasource_access or datasource_access):
             flash(__("You don't seem to have access to this datasource"), "danger")
@@ -1029,15 +1030,15 @@ def dashboard(**kwargs):  # noqa
         return self.render_template(
             "caravel/dashboard.html", dashboard=dash,
             templates=templates,
-            dash_save_perm=appbuilder.sm.has_access('can_save_dash', 'Caravel'),
-            dash_edit_perm=appbuilder.sm.has_access('can_edit', 'DashboardModelView'))
+            dash_save_perm=self.can_access('can_save_dash', 'Caravel'),
+            dash_edit_perm=self.can_access('can_edit', 'DashboardModelView'))
 
     @has_access
     @expose("/sql/<database_id>/")
     @log_this
     def sql(self, database_id):
         if (
-                not self.appbuilder.sm.has_access(
+                not self.can_access(
                     'all_datasource_access', 'all_datasource_access')):
             flash(
                 "This view requires the `all_datasource_access` "
@@ -1102,7 +1103,7 @@ def runsql(self):
         mydb = session.query(models.Database).filter_by(id=database_id).first()
 
         if (
-                not self.appbuilder.sm.has_access(
+                not self.can_access(
                     'all_datasource_access', 'all_datasource_access')):
             raise utils.CaravelSecurityException(_(
                 "This view requires the `all_datasource_access` permission"))