Be more permissive when parameters are safe (#3383)

* use the textless endpoint (/api/queries/:id/results) for pristine
queriest

* reverse conditional. not not is making me the headaches.

* add ParameterizedQuery#is_safe with an inital naive implementation which
treats any query with a text parameter as not safe. This will be
remedied later when DB drivers will handle these parameters.

* allow getting new query results even if user has only view permissions
to the data source (given that the query is safe)

* fix lint error - getDerivedStateFromProps should be placed after state

* Revert "use the textless endpoint (/api/queries/:id/results) for pristine"

This reverts commit cd2cee77385ecf79efd2f1aa21fab0dd43943264.

* move execution preparation to a different function, which will be soon
reused

* go to textless /api/queries/:id/results by default

* let the query view decide if text or textless endpoint is needed

* allow safe queries to be executed in the UI even if the user has no
permission to execute and create new query results

* change `run_query`'s signature to accept a ParameterizedQuery instead of
constructing it inside

* use dict#get instead of a None guard

* use ParameterizedQuery in queries handler as well

* test that /queries/:id/results allows execution of safe queries even if
user has view_only permissions

* lint

* raise HTTP 400 when receiving invalid parameter values. Fixes #3394

* remove unused methods

* avoid cyclic imports by importing only when needed

* verify that a ParameterizedQuery without any parameters is considered
safe

* introduce query.parameter_schema

* encapsulate ParameterizedQuery creation inside Query

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 @@ from redash.permissions import (can_modify, not_view_only, require_access,
                                 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 @@ class QueryRefreshResource(BaseResource):
         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.permissions import (has_access, not_view_only, require_access,
 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()}
 
 
@@ -116,6 +122,8 @@ class QueryResultListResource(BaseResource):
         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):
@@ -129,7 +137,7 @@ class QueryResultListResource(BaseResource):
             '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
@@ -163,23 +171,6 @@ class QueryResultResource(BaseResource):
 
         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):
         """
@@ -201,12 +192,13 @@ class QueryResultResource(BaseResource):
         max_age = int(max_age)
 
         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
+        allow_executing_with_view_only_permissions = query.parameterized.is_safe
+
+        if has_access(query.data_source.groups, self.current_user, allow_executing_with_view_only_permissions):
+            return run_query(query.parameterized, 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/models/__init__.py

@@ -28,6 +28,7 @@ from redash.query_runner import (get_configuration_schema_for_query_runner_type,
                                  get_query_runner)
 from redash.utils import generate_token, json_dumps, json_loads
 from redash.utils.configuration import ConfigurationContainer
+from redash.utils.parameterized_query import ParameterizedQuery
 
 from .base import db, gfk_type, Column, GFKBase, SearchBaseQuery
 from .changes import ChangeTrackingMixin, Change  # noqa
@@ -667,6 +668,10 @@ class Query(ChangeTrackingMixin, TimestampMixin, BelongsToOrgMixin, db.Model):
         "The SQLAlchemy expression for the property above."
         return func.lower(cls.name)
 
+    @property
+    def parameterized(self):
+        return ParameterizedQuery(self.query_text, self.options.get("parameters", []))
+
 
 @listens_for(Query.query_text, 'set')
 def gen_query_hash(target, val, oldval, initiator):

redash/serializers.py

@@ -10,6 +10,7 @@ from flask_login import current_user
 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': query.parameterized.is_safe,
     }
 
     if with_user:

redash/utils/parameterized_query.py

@@ -1,9 +1,7 @@
 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
 from redash.permissions import require_access, view_only
 from funcy import distinct
@@ -19,6 +17,9 @@ def _pluck_name_and_value(default_column, row):
 
 
 def _load_result(query_id):
+    from redash.authentication.org_resolving import current_org
+    from redash import models
+
     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)
@@ -121,6 +122,11 @@ class ParameterizedQuery(object):
 
         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 @@ class TestQueryResultAPI(BaseTestCase):
         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

@@ -166,6 +166,24 @@ class TestParameterizedQuery(TestCase):
         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)
+
+    def test_is_safe_if_not_expecting_any_parameters(self):
+        schema = []
+        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"}]})