mistercrunch closed pull request #6553: Secure unsecured views and prevent
regressions
URL: https://github.com/apache/incubator-superset/pull/6553
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/superset/views/core.py b/superset/views/core.py
index 08aa7ac137..a18ce783cc 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -621,6 +621,8 @@ def mulexport(self, items):
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 @@ def store(self):
status=200)
@log_this
+ @has_access_api
@expose('/<key_id>/', methods=['GET'])
def get_value(self, key_id):
kv = None
@@ -763,7 +767,8 @@ def index(self, url_id):
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 @@ def shortner(self):
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 @@ def warm_up_cache(self):
[{'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 @@ def fetch_datasource_metadata(self):
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 db37ce7037..5df20a2977 100644
--- a/superset/views/datasource.py
+++ b/superset/views/datasource.py
@@ -35,6 +35,7 @@ def save(self):
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 d60fd3d6d8..3c2fab3d7d 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 @@ class SavedQueryViewApi(SavedQueryView):
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 fc26385341..d461658779 100644
--- a/tests/import_export_tests.py
+++ b/tests/import_export_tests.py
@@ -129,7 +129,8 @@ def get_dash(self, dash_id):
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 @@ def assert_slice_equals(self, expected_slc, actual_slc):
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 @@ def test_export_1_dashboard(self):
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 @@ def test_export_1_dashboard(self):
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 @@ def test_export_2_dashboards(self):
)['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 edeb2b7bd4..6f046ba7a1 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 @@ def assert_can_all(view_menu):
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}')
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
