Secure unsecured views and prevent regressions (#6553)

* Secure views and prevent regressions

* Force POST on shortner

* Fix tests

superset/views/core.py

@@ -621,6 +621,8 @@ class DashboardModelView(SupersetModelView, DeleteMixin):  # noqa
         return redirect(
             '/dashboard/export_dashboards_form?{}'.format(ids[1:]))
 
+    @log_this
+    @has_access
     @expose('/export_dashboards_form')
     def download_dashboards(self):
         if request.args.get('action') == 'go':
@@ -721,6 +723,7 @@ class KV(BaseSupersetView):
     """Used for storing and retrieving key value pairs"""
 
     @log_this
+    @has_access_api
     @expose('/store/', methods=['POST'])
     def store(self):
         try:
@@ -735,6 +738,7 @@ class KV(BaseSupersetView):
             status=200)
 
     @log_this
+    @has_access_api
     @expose('/<key_id>/', methods=['GET'])
     def get_value(self, key_id):
         kv = None
@@ -763,7 +767,8 @@ class R(BaseSupersetView):
             return redirect('/')
 
     @log_this
-    @expose('/shortner/', methods=['POST', 'GET'])
+    @has_access_api
+    @expose('/shortner/', methods=['POST'])
     def shortner(self):
         url = request.form.get('data')
         obj = models.Url(url=url)
@@ -774,12 +779,6 @@ class R(BaseSupersetView):
                 scheme=request.scheme, request=request, obj=obj),
             mimetype='text/plain')
 
-    @expose('/msg/')
-    def msg(self):
-        """Redirects to specified url while flash a message"""
-        flash(Markup(request.args.get('msg')), 'info')
-        return redirect(request.args.get('url'))
-
 
 appbuilder.add_view_no_menu(R)
 
@@ -2063,6 +2062,7 @@ class Superset(BaseSupersetView):
             [{'slice_id': slc.id, 'slice_name': slc.slice_name}
              for slc in slices]))
 
+    @has_access_api
     @expose('/favstar/<class_name>/<obj_id>/<action>/')
     def favstar(self, class_name, obj_id, action):
         """Toggle favorite stars on Slices and Dashboard"""
@@ -2631,6 +2631,7 @@ class Superset(BaseSupersetView):
         security_manager.assert_datasource_permission(datasource)
         return json_success(json.dumps(datasource.data))
 
+    @has_access_api
     @expose('/queries/<last_updated_ms>')
     def queries(self, last_updated_ms):
         """Get the updated queries."""

superset/views/datasource.py

@@ -35,6 +35,7 @@ class Datasource(BaseSupersetView):
         return self.json_response(data)
 
     @expose('/external_metadata/<datasource_type>/<datasource_id>/')
+    @has_access_api
     def external_metadata(self, datasource_type=None, datasource_id=None):
         """Gets column info from the source system"""
         orm_datasource = ConnectorRegistry.get_datasource(

superset/views/sql_lab.py

@@ -2,6 +2,7 @@
 from flask import g, redirect
 from flask_appbuilder import expose
 from flask_appbuilder.models.sqla.interface import SQLAInterface
+from flask_appbuilder.security.decorators import has_access
 from flask_babel import gettext as __
 from flask_babel import lazy_gettext as _
 
@@ -92,6 +93,7 @@ appbuilder.add_link(
 class SqlLab(BaseSupersetView):
     """The base views for Superset!"""
     @expose('/my_queries/')
+    @has_access
     def my_queries(self):
         """Assigns a list of found users to the given role."""
         return redirect(

tests/import_export_tests.py

@@ -129,7 +129,8 @@ class ImportExportTests(SupersetTestCase):
             id=dash_id).first()
 
     def get_dash_by_slug(self, dash_slug):
-        return db.session.query(models.Dashboard).filter_by(
+        sesh = db.session()
+        return sesh.query(models.Dashboard).filter_by(
             slug=dash_slug).first()
 
     def get_datasource(self, datasource_id):
@@ -195,6 +196,7 @@ class ImportExportTests(SupersetTestCase):
             json.loads(expected_slc.params), json.loads(actual_slc.params))
 
     def test_export_1_dashboard(self):
+        self.login('admin')
         birth_dash = self.get_dash_by_slug('births')
         export_dash_url = (
             '/dashboard/export_dashboards_form?id={}&action=go'
@@ -206,6 +208,7 @@ class ImportExportTests(SupersetTestCase):
             object_hook=utils.decode_dashboards,
         )['dashboards']
 
+        birth_dash = self.get_dash_by_slug('births')
         self.assert_dash_equals(birth_dash, exported_dashboards[0])
         self.assertEquals(
             birth_dash.id,
@@ -223,6 +226,7 @@ class ImportExportTests(SupersetTestCase):
             self.get_table_by_name('birth_names'), exported_tables[0])
 
     def test_export_2_dashboards(self):
+        self.login('admin')
         birth_dash = self.get_dash_by_slug('births')
         world_health_dash = self.get_dash_by_slug('world_health')
         export_dash_url = (
@@ -236,12 +240,15 @@ class ImportExportTests(SupersetTestCase):
             )['dashboards'],
             key=lambda d: d.dashboard_title)
         self.assertEquals(2, len(exported_dashboards))
+
+        birth_dash = self.get_dash_by_slug('births')
         self.assert_dash_equals(birth_dash, exported_dashboards[0])
         self.assertEquals(
             birth_dash.id,
             json.loads(exported_dashboards[0].json_metadata)['remote_id'],
         )
 
+        world_health_dash = self.get_dash_by_slug('world_health')
         self.assert_dash_equals(world_health_dash, exported_dashboards[1])
         self.assertEquals(
             world_health_dash.id,

tests/security_tests.py

@@ -1,4 +1,6 @@
-from superset import app, security_manager
+import inspect
+
+from superset import app, appbuilder, security_manager
 from .base_tests import SupersetTestCase
 
 
@@ -12,9 +14,6 @@ def get_perm_tuples(role_name):
 class RolePermissionTests(SupersetTestCase):
     """Testing export import functionality for dashboards"""
 
-    def __init__(self, *args, **kwargs):
-        super(RolePermissionTests, self).__init__(*args, **kwargs)
-
     def assert_can_read(self, view_menu, permissions_set):
         self.assertIn(('can_show', view_menu), permissions_set)
         self.assertIn(('can_list', view_menu), permissions_set)
@@ -216,3 +215,34 @@ class RolePermissionTests(SupersetTestCase):
         self.assertIn(('can_fave_slices', 'Superset'), gamma_perm_set)
         self.assertIn(('can_save_dash', 'Superset'), gamma_perm_set)
         self.assertIn(('can_slice', 'Superset'), gamma_perm_set)
+
+    def test_views_are_secured(self):
+        """Preventing the addition of unsecured views without has_access decorator"""
+        # These FAB views are secured in their body as opposed to by decorators
+        method_whitelist = ('action', 'action_post')
+        # List of redirect & other benign views
+        views_whitelist = [
+            ['MyIndexView', 'index'],
+            ['UtilView', 'back'],
+            ['LocaleView', 'index'],
+            ['AuthDBView', 'login'],
+            ['AuthDBView', 'logout'],
+            ['R', 'index'],
+            ['Superset', 'log'],
+            ['Superset', 'theme'],
+            ['Superset', 'welcome'],
+        ]
+        unsecured_views = []
+        for view_class in appbuilder.baseviews:
+            class_name = view_class.__class__.__name__
+            for name, value in inspect.getmembers(view_class, predicate=inspect.ismethod):
+                if (
+                        name not in method_whitelist and
+                        [class_name, name] not in views_whitelist and
+                        hasattr(value, '_urls') and
+                        not hasattr(value, '_permission_name')
+                ):
+                    unsecured_views.append((class_name, name))
+        if unsecured_views:
+            view_str = '\n'.join([str(v) for v in unsecured_views])
+            raise Exception(f'Some views are not secured:\n{view_str}')