Be more permissive when parameters are safe

client/app/pages/queries/view.js

@@ -157,7 +157,7 @@ function QueryViewCtrl(
   $scope.canEdit = currentUser.canEdit($scope.query) || $scope.query.can_edit;
   $scope.canViewSource = currentUser.hasPermission('view_source');
 
-  $scope.canExecuteQuery = () => currentUser.hasPermission('execute_query') && !$scope.dataSource.view_only;
+  $scope.canExecuteQuery = () => $scope.query.is_safe || (currentUser.hasPermission('execute_query') && !$scope.dataSource.view_only);
 
   $scope.canForkQuery = () => currentUser.hasPermission('edit_query') && !$scope.dataSource.view_only;
 

redash/handlers/queries.py

@@ -16,6 +16,7 @@
                                 require_permission, view_only)
 from redash.utils import collect_parameters_from_request
 from redash.serializers import QuerySerializer
+from redash.utils.parameterized_query import ParameterizedQuery
 
 
 # Ordering map for relationships
@@ -411,8 +412,9 @@ def post(self, query_id):
         require_access(query.groups, self.current_user, not_view_only)
 
         parameter_values = collect_parameters_from_request(request.args)
+        parameterized_query = ParameterizedQuery(query.query_text)
 
-        return run_query(query.data_source, parameter_values, query.query_text, query.id)
+        return run_query(parameterized_query, parameter_values, query.data_source, query.id)
 
 
 class QueryTagsResource(BaseResource):

redash/handlers/query_results.py

@@ -11,7 +11,7 @@
 from redash.tasks import QueryTask
 from redash.tasks.queries import enqueue_query
 from redash.utils import (collect_parameters_from_request, gen_query_hash, json_dumps, utcnow)
-from redash.utils.parameterized_query import ParameterizedQuery, dropdown_values
+from redash.utils.parameterized_query import ParameterizedQuery, InvalidParameterError, dropdown_values
 
 
 def error_response(message):
@@ -64,7 +64,7 @@ def run_query_sync(data_source, parameter_values, query_text, max_age=0):
         return None
 
 
-def run_query(data_source, parameter_values, query_text, query_id, max_age=0, parameter_schema=None):
+def run_query(query, parameters, data_source, query_id, max_age=0):
     if data_source.paused:
         if data_source.pause_reason:
             message = '{} is paused ({}). Please try later.'.format(data_source.name, data_source.pause_reason)
@@ -73,7 +73,10 @@ def run_query(data_source, parameter_values, query_text, query_id, max_age=0, pa
 
         return error_response(message)
 
-    query = ParameterizedQuery(query_text, parameter_schema).apply(parameter_values)
+    try:
+        query.apply(parameters)
+    except InvalidParameterError as e:
+        abort(400, message=e.message)
 
     if query.missing_params:
         return error_response(u'Missing parameter value for: {}'.format(u", ".join(query.missing_params)))
@@ -86,7 +89,10 @@ def run_query(data_source, parameter_values, query_text, query_id, max_age=0, pa
     if query_result:
         return {'query_result': query_result.to_dict()}
     else:
-        job = enqueue_query(query.text, data_source, current_user.id, metadata={"Username": current_user.email, "Query ID": query_id})
+        job = enqueue_query(query.text, data_source, current_user.id, metadata={
+            "Username": current_user.email,
+            "Query ID": query_id
+        })
         return {'job': job.to_dict()}
 
 
@@ -112,6 +118,8 @@ def post(self):
         query_id = params.get('query_id', 'adhoc')
         parameters = params.get('parameters', collect_parameters_from_request(request.args))
 
+        parameterized_query = ParameterizedQuery(query)
+
         data_source = models.DataSource.get_by_id_and_org(params.get('data_source_id'), self.current_org)
 
         if not has_access(data_source.groups, self.current_user, not_view_only):
@@ -125,7 +133,7 @@ def post(self):
             'query_id': query_id,
             'parameters': parameters
         })
-        return run_query(data_source, parameters, query, query_id, max_age)
+        return run_query(parameterized_query, parameters, data_source, query_id, max_age)
 
 
 ONE_YEAR = 60 * 60 * 24 * 365.25
@@ -159,23 +167,6 @@ def options(self, query_id=None, query_result_id=None, filetype='json'):
 
         return make_response("", 200, headers)
 
-    def _fetch_rows(self, query_id):
-        query = models.Query.get_by_id_and_org(query_id, self.current_org)
-        require_access(query.data_source.groups, self.current_user, view_only)
-
-        query_result = models.QueryResult.get_by_id_and_org(query.latest_query_data_id, self.current_org)
-
-        return json_loads(query_result.data)["rows"]
-
-    def _convert_queries_to_enums(self, definition):
-        if definition["type"] == "query":
-            definition["type"] = "enum"
-
-            rows = self._fetch_rows(definition.pop("queryId"))
-            definition["enumOptions"] = [row.get('value', row.get(row.keys()[0])) for row in rows if row]
-
-        return definition
-
     @require_permission('execute_query')
     def post(self, query_id):
         """
@@ -195,10 +186,13 @@ def post(self, query_id):
         query = get_object_or_404(models.Query.get_by_id_and_org, query_id, self.current_org)
         parameter_schema = query.options.get("parameters", [])
 
-        if not has_access(query.data_source.groups, self.current_user, not_view_only):
-            return {'job': {'status': 4, 'error': 'You do not have permission to run queries with this data source.'}}, 403
+        parameterized_query = ParameterizedQuery(query.query_text, parameter_schema)
+        allow_executing_with_view_only_permissions = parameterized_query.is_safe
+
+        if has_access(query.data_source.groups, self.current_user, allow_executing_with_view_only_permissions):
+            return run_query(parameterized_query, parameters, query.data_source, query_id, max_age)
         else:
-            return run_query(query.data_source, parameters, query.query_text, query_id, max_age, parameter_schema=parameter_schema)
+            return {'job': {'status': 4, 'error': 'You do not have permission to run queries with this data source.'}}, 403
 
     @require_permission('view_query')
     def get(self, query_id=None, query_result_id=None, filetype='json'):

redash/serializers.py

@@ -10,6 +10,7 @@
 from redash import models
 from redash.permissions import has_access, view_only
 from redash.utils import json_loads
+from redash.utils.parameterized_query import ParameterizedQuery
 
 
 def public_widget(widget):
@@ -100,6 +101,7 @@ def serialize_query(query, with_stats=False, with_visualizations=False, with_use
         'options': query.options,
         'version': query.version,
         'tags': query.tags or [],
+        'is_safe': ParameterizedQuery(query.query_text, query.options.get("parameters", [])).is_safe,
     }
 
     if with_user:

redash/utils/parameterized_query.py

@@ -1,7 +1,6 @@
 import pystache
 from functools import partial
 from flask_login import current_user
-from redash.authentication.org_resolving import current_org
 from numbers import Number
 from redash import models
 from redash.utils import mustache_render, json_loads
@@ -19,6 +18,7 @@ def _pluck_name_and_value(default_column, row):
 
 
 def _load_result(query_id):
+    from redash.authentication.org_resolving import current_org
     query = models.Query.get_by_id_and_org(query_id, current_org)
     require_access(query.data_source.groups, current_user, view_only)
     query_result = models.QueryResult.get_by_id_and_org(query.latest_query_data_id, current_org)
@@ -118,6 +118,11 @@ def _valid(self, name, value):
 
         return validate(value)
 
+    @property
+    def is_safe(self):
+        text_parameters = filter(lambda p: p["type"] == "text", self.schema)
+        return not any(text_parameters)
+
     @property
     def missing_params(self):
         query_parameters = set(_collect_query_parameters(self.template))

tests/handlers/test_query_results.py

@@ -136,6 +136,20 @@ def test_execute_new_query(self):
         self.assertEquals(rv.status_code, 200)
         self.assertIn('job', rv.json)
 
+    def test_prevents_execution_of_unsafe_queries_on_view_only_data_sources(self):
+        ds = self.factory.create_data_source(group=self.factory.org.default_group, view_only=True)
+        query = self.factory.create_query(data_source=ds, options={"parameters": [{"name": "foo", "type": "text"}]})
+
+        rv = self.make_request('post', '/api/queries/{}/results'.format(query.id), data={"parameters": {}})
+        self.assertEquals(rv.status_code, 403)
+
+    def test_allows_execution_of_safe_queries_on_view_only_data_sources(self):
+        ds = self.factory.create_data_source(group=self.factory.org.default_group, view_only=True)
+        query = self.factory.create_query(data_source=ds, options={"parameters": [{"name": "foo", "type": "number"}]})
+
+        rv = self.make_request('post', '/api/queries/{}/results'.format(query.id), data={"parameters": {}})
+        self.assertEquals(rv.status_code, 200)
+
     def test_access_with_query_api_key(self):
         ds = self.factory.create_data_source(group=self.factory.org.default_group, view_only=False)
         query = self.factory.create_query()

tests/utils/test_parameterized_query.py

@@ -159,6 +159,18 @@ def test_raises_on_unexpected_param_types(self):
         with pytest.raises(InvalidParameterError):
             query.apply({"bar": "baz"})
 
+    def test_is_not_safe_if_expecting_text_parameter(self):
+        schema = [{"name": "bar", "type": "text"}]
+        query = ParameterizedQuery("foo", schema)
+
+        self.assertFalse(query.is_safe)
+
+    def test_is_safe_if_not_expecting_text_parameter(self):
+        schema = [{"name": "bar", "type": "number"}]
+        query = ParameterizedQuery("foo", schema)
+
+        self.assertTrue(query.is_safe)
+
     @patch('redash.utils.parameterized_query._load_result', return_value={
         "columns": [{"name": "id"}, {"name": "Name"}, {"name": "Value"}],
         "rows": [{"id": 5, "Name": "John", "Value": "John Doe"}]})