This is an automated email from the ASF dual-hosted git repository.
maximebeauchemin pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
The following commit(s) were added to refs/heads/master by this push:
new 3f29a1d Secure unsecured views and prevent regressions (#6553)
3f29a1d is described below
commit 3f29a1dd70ce2ecd435ef6dd18b808c3e0046cb2
Author: Maxime Beauchemin <maximebeauche...@gmail.com>
AuthorDate: Tue Dec 18 11:57:13 2018 -0800
Secure unsecured views and prevent regressions (#6553)
* Secure views and prevent regressions
* Force POST on shortner
* Fix tests
---
superset/views/core.py | 15 ++++++++-------
superset/views/datasource.py | 1 +
superset/views/sql_lab.py | 2 ++
tests/import_export_tests.py | 9 ++++++++-
tests/security_tests.py | 38 ++++++++++++++++++++++++++++++++++----
5 files changed, 53 insertions(+), 12 deletions(-)
diff --git a/superset/views/core.py b/superset/views/core.py
index 08aa7ac..a18ce78 100755
--- a/superset/views/core.py
+++ b/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."""
diff --git a/superset/views/datasource.py b/superset/views/datasource.py
index db37ce7..5df20a2 100644
--- a/superset/views/datasource.py
+++ b/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(
diff --git a/superset/views/sql_lab.py b/superset/views/sql_lab.py
index d60fd3d..3c2fab3 100644
--- a/superset/views/sql_lab.py
+++ b/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(
diff --git a/tests/import_export_tests.py b/tests/import_export_tests.py
index fc26385..d461658 100644
--- a/tests/import_export_tests.py
+++ b/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,
diff --git a/tests/security_tests.py b/tests/security_tests.py
index edeb2b7..6f046ba 100644
--- a/tests/security_tests.py
+++ b/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}')
