apache · mistercrunch · Sep 10, 2019 · Sep 9, 2019 · Sep 9, 2019 · Sep 9, 2019
diff --git a/UPDATING.md b/UPDATING.md
@@ -23,6 +23,8 @@ assists people when migrating to a new version.
 
 ## Next Version
 
+* We're deprecating the concept of "restricted metric", this feature
+  was not fully working anyhow.
 * [8117](https://github.com/apache/incubator-superset/pull/8117): If you are
 using `ENABLE_PROXY_FIX = True`, review the newly-introducted variable,
 `PROXY_FIX_CONFIG`, which changes the proxy behavior in accordance with
@@ -34,7 +36,7 @@ using `ENABLE_PROXY_FIX = True`, review the newly-introducted variable,
 backend serialization. To disable set `RESULTS_BACKEND_USE_MSGPACK = False`
 in your configuration.
 
-* [7848](https://github.com/apache/incubator-superset/pull/7848): If you are 
+* [7848](https://github.com/apache/incubator-superset/pull/7848): If you are
 running redis with celery, celery bump to 4.3.0 requires redis-py upgrade to
 3.2.0 or later.
 

diff --git a/docs/security.rst b/docs/security.rst
@@ -20,7 +20,7 @@ Security
 Security in Superset is handled by Flask AppBuilder (FAB). FAB is a
 "Simple and rapid application development framework, built on top of Flask.".
 FAB provides authentication, user management, permissions and roles.
-Please read its `Security documentation 
+Please read its `Security documentation
 <https://flask-appbuilder.readthedocs.io/en/latest/security.html>`_.
 
 Provided Roles
@@ -153,26 +153,3 @@ a set of data sources that power dashboards only made available to executives.
 When looking at its dashboard list, this user will only see the
 list of dashboards it has access to, based on the roles and
 permissions that were attributed.
-
-
-Restricting the access to some metrics
-""""""""""""""""""""""""""""""""""""""
-
-Sometimes some metrics are relatively sensitive (e.g. revenue).
-We may want to restrict those metrics to only a few roles.
-For example, assumed there is a metric ``[cluster1].[datasource1].[revenue]``
-and only Admin users are allowed to see it. Here’s how to restrict the access.
-
-1. Edit the datasource (``Menu -> Source -> Druid datasources -> edit the
-   record "datasource1"``) and go to the tab ``List Druid Metric``. Check
-   the checkbox ``Is Restricted`` in the row of the metric ``revenue``.
-
-2. Edit the role (``Menu -> Security -> List Roles -> edit the record
-   “Admin”``), in the permissions field, type-and-search the permission
-   ``metric access on [cluster1].[datasource1].[revenue] (id: 1)``, then
-   click the Save button on the bottom of the page.
-
-Any users without the permission will see the error message
-*Access to the metrics denied: revenue (Status: 500)* in the slices.
-It also happens when the user wants to access a post-aggregation metric that
-is dependent on revenue.
diff --git a/superset/assets/package.json b/superset/assets/package.json
@@ -10,7 +10,7 @@
   "scripts": {
     "tdd": "jest --watch",
     "test": "jest",
-    "cover": "jest --maxWorkers=8 --coverage",
+    "cover": "jest --coverage",
     "dev": "webpack --mode=development --colors --progress --debug --watch",
     "dev-server": "webpack-dev-server --mode=development --progress",
     "prod": "node --max_old_space_size=4096 webpack --mode=production --colors --progress",

diff --git a/superset/connectors/base/models.py b/superset/connectors/base/models.py
@@ -417,7 +417,6 @@ class BaseMetric(AuditMixinNullable, ImportMixin):
     verbose_name = Column(String(1024))
     metric_type = Column(String(32))
     description = Column(Text)
-    is_restricted = Column(Boolean, default=False, nullable=True)
     d3format = Column(String(128))
     warning_text = Column(Text)
 

diff --git a/superset/connectors/druid/models.py b/superset/connectors/druid/models.py
@@ -66,7 +66,7 @@
 
 from superset import conf, db, security_manager
 from superset.connectors.base.models import BaseColumn, BaseDatasource, BaseMetric
-from superset.exceptions import MetricPermException, SupersetException
+from superset.exceptions import SupersetException
 from superset.models.helpers import AuditMixinNullable, ImportMixin, QueryResult
 from superset.utils import core as utils, import_datasource
 
@@ -389,7 +389,6 @@ class DruidMetric(Model, BaseMetric):
         "datasource_id",
         "json",
         "description",
-        "is_restricted",
         "d3format",
         "warning_text",
     )
@@ -1044,19 +1043,6 @@ def get_aggregations(metrics_dict, saved_metrics, adhoc_metrics=[]):
             }
         return aggregations
 
-    def check_restricted_metrics(self, aggregations):
-        rejected_metrics = [
-            m.metric_name
-            for m in self.metrics
-            if m.is_restricted
-            and m.metric_name in aggregations.keys()
-            and not security_manager.has_access("metric_access", m.perm)
-        ]
-        if rejected_metrics:
-            raise MetricPermException(
-                "Access to the metrics denied: " + ", ".join(rejected_metrics)
-            )
-
     def get_dimensions(self, groupby, columns_dict):
         dimensions = []
         groupby = [gb for gb in groupby if gb in columns_dict]
@@ -1164,8 +1150,6 @@ def run_query(  # noqa / druid
             metrics, metrics_dict
         )
 
-        self.check_restricted_metrics(aggregations)
-
         # the dimensions list with dimensionSpecs expanded
         dimensions = self.get_dimensions(groupby, columns_dict)
         extras = extras or {}

diff --git a/superset/connectors/druid/views.py b/superset/connectors/druid/views.py
@@ -151,7 +151,6 @@ class DruidMetricInlineView(CompactCRUDMixin, SupersetModelView):  # noqa
         "json",
         "datasource",
         "d3format",
-        "is_restricted",
         "warning_text",
     ]
     add_columns = edit_columns
@@ -163,13 +162,7 @@ class DruidMetricInlineView(CompactCRUDMixin, SupersetModelView):  # noqa
             "[Druid Post Aggregation]"
             "(http://druid.io/docs/latest/querying/post-aggregations.html)",
             True,
-        ),
-        "is_restricted": _(
-            "Whether access to this metric is restricted "
-            "to certain roles. Only roles with the permission "
-            "'metric access on XXX (the name of this metric)' "
-            "are allowed to access this metric"
-        ),
+        )
     }
     label_columns = {
         "metric_name": _("Metric"),
@@ -179,7 +172,6 @@ class DruidMetricInlineView(CompactCRUDMixin, SupersetModelView):  # noqa
         "json": _("JSON"),
         "datasource": _("Druid Datasource"),
         "warning_text": _("Warning Message"),
-        "is_restricted": _("Is Restricted"),
     }
 
     add_form_extra_fields = {
@@ -193,18 +185,6 @@ class DruidMetricInlineView(CompactCRUDMixin, SupersetModelView):  # noqa
 
     edit_form_extra_fields = add_form_extra_fields
 
-    def post_add(self, metric):
-        if metric.is_restricted:
-            security_manager.add_permission_view_menu(
-                "metric_access", metric.get_perm()
-            )
-
-    def post_update(self, metric):
-        if metric.is_restricted:
-            security_manager.add_permission_view_menu(
-                "metric_access", metric.get_perm()
-            )
-
 
 appbuilder.add_view_no_menu(DruidMetricInlineView)
 

diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py
@@ -241,7 +241,6 @@ class SqlMetric(Model, BaseMetric):
         "table_id",
         "expression",
         "description",
-        "is_restricted",
         "d3format",
         "warning_text",
     )

diff --git a/superset/connectors/sqla/views.py b/superset/connectors/sqla/views.py
@@ -154,7 +154,6 @@ class SqlMetricInlineView(CompactCRUDMixin, SupersetModelView):  # noqa
         "expression",
         "table",
         "d3format",
-        "is_restricted",
         "warning_text",
     ]
     description_columns = {
@@ -163,12 +162,6 @@ class SqlMetricInlineView(CompactCRUDMixin, SupersetModelView):  # noqa
             "underlying backend. Example: `count(DISTINCT userid)`",
             True,
         ),
-        "is_restricted": _(
-            "Whether access to this metric is restricted "
-            "to certain roles. Only roles with the permission "
-            "'metric access on XXX (the name of this metric)' "
-            "are allowed to access this metric"
-        ),
         "d3format": utils.markdown(
             "d3 formatting string as defined [here]"
             "(https://github.com/d3/d3-format/blob/master/README.md#format). "
@@ -188,7 +181,6 @@ class SqlMetricInlineView(CompactCRUDMixin, SupersetModelView):  # noqa
         "expression": _("SQL Expression"),
         "table": _("Table"),
         "d3format": _("D3 Format"),
-        "is_restricted": _("Is Restricted"),
         "warning_text": _("Warning Message"),
     }
 
@@ -203,18 +195,6 @@ class SqlMetricInlineView(CompactCRUDMixin, SupersetModelView):  # noqa
 
     edit_form_extra_fields = add_form_extra_fields
 
-    def post_add(self, metric):
-        if metric.is_restricted:
-            security_manager.add_permission_view_menu(
-                "metric_access", metric.get_perm()
-            )
-
-    def post_update(self, metric):
-        if metric.is_restricted:
-            security_manager.add_permission_view_menu(
-                "metric_access", metric.get_perm()
-            )
-
 
 appbuilder.add_view_no_menu(SqlMetricInlineView)
 

diff --git a/superset/exceptions.py b/superset/exceptions.py
@@ -36,10 +36,6 @@ def __init__(self, msg, link=None):
         self.link = link
 
 
-class MetricPermException(SupersetException):
-    pass
-
-
 class NoDataException(SupersetException):
     status = 400
 

@@ -0,0 +1,44 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""deprecate_restricted_metrics
+
+Revision ID: 11c737c17cc6
+Revises: def97f26fdfb
+Create Date: 2019-09-08 21:50:58.200229
+
+"""
+from alembic import op
+import sqlalchemy as sa
+
+
+# revision identifiers, used by Alembic.
+revision = "11c737c17cc6"
+down_revision = "def97f26fdfb"
+
+
+def upgrade():
+    with op.batch_alter_table("metrics") as batch_op:
+        batch_op.drop_column("is_restricted")
+    with op.batch_alter_table("sql_metrics") as batch_op:
+        batch_op.drop_column("is_restricted")
+
+
+def downgrade():
+    op.add_column(
+        "sql_metrics", sa.Column("is_restricted", sa.BOOLEAN(), nullable=True)
+    )
+    op.add_column("metrics", sa.Column("is_restricted", sa.BOOLEAN(), nullable=True))
diff --git a/superset/security.py b/superset/security.py
@@ -535,10 +535,6 @@ def merge_pv(view_menu, perm):
         for datasource_class in ConnectorRegistry.sources.values():
             metrics += list(db.session.query(datasource_class.metric_class).all())
 
-        for metric in metrics:
-            if metric.is_restricted:
-                merge_pv("metric_access", metric.perm)
-
     def clean_perms(self) -> None:
         """
         Clean up the FAB faulty permissions.