From c47dd05095b449fe6c7648bf9d0625aef1be9ac6 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Wed, 20 Mar 2019 09:16:10 +0200 Subject: [PATCH] Nest query ACL to dropdowns (#3544) * change API to /api/queries/:id/dropdowns/:dropdown_id * extract property * split to 2 different dropdown endpoints and implement the second * make access control optional for dropdowns (assuming it is verified at a different level) * add test cases for /api/queries/:id/dropdowns/:id * use new /dropdowns endpoint in frontend * require access to dropdown queries when creating or updating parent queries * rename Query resource dropdown endpoints * check access to dropdown query associations in one fugly query * move ParameterizedQuery to models folder * add dropdown association tests to query creation * move group by query ids query into models.Query * use bound parameters for groups query * format groups query * use new associatedDropdowns endpoint in dashboards * pass down parameter and let it return dropdown options. Go Levko! * change API to /api/queries/:id/dropdowns/:dropdown_id * split to 2 different dropdown endpoints and implement the second * use new /dropdowns endpoint in frontend * pass down parameter and let it return dropdown options. Go Levko! * fix bad rebase * add comment to clarify the purpose of checking the queryId --- client/app/components/ParameterValueInput.jsx | 7 +- .../components/QueryBasedParameterInput.jsx | 22 ++-- client/app/services/query.js | 23 +++- redash/handlers/api.py | 2 + redash/handlers/queries.py | 15 ++- redash/handlers/query_results.py | 13 +- redash/models/__init__.py | 17 ++- .../{utils => models}/parameterized_query.py | 11 +- redash/serializers.py | 2 +- tests/handlers/test_queries.py | 115 ++++++++++++++++++ tests/handlers/test_query_results.py | 44 +++++++ .../test_parameterized_query.py | 16 +-- 12 files changed, 254 insertions(+), 33 deletions(-) rename redash/{utils => models}/parameterized_query.py (94%) rename tests/{utils => models}/test_parameterized_query.py (91%) diff --git a/client/app/components/ParameterValueInput.jsx b/client/app/components/ParameterValueInput.jsx index 01f0fdbec8..c6648d3687 100644 --- a/client/app/components/ParameterValueInput.jsx +++ b/client/app/components/ParameterValueInput.jsx @@ -18,6 +18,7 @@ export class ParameterValueInput extends React.Component { value: PropTypes.any, // eslint-disable-line react/forbid-prop-types enumOptions: PropTypes.string, queryId: PropTypes.number, + parameter: PropTypes.any, // eslint-disable-line react/forbid-prop-types onSelect: PropTypes.func, className: PropTypes.string, }; @@ -27,6 +28,7 @@ export class ParameterValueInput extends React.Component { value: null, enumOptions: '', queryId: null, + parameter: null, onSelect: () => {}, className: '', }; @@ -117,10 +119,11 @@ export class ParameterValueInput extends React.Component { } renderQueryBasedInput() { - const { value, onSelect, queryId } = this.props; + const { value, onSelect, queryId, parameter } = this.props; return ( `, diff --git a/client/app/components/QueryBasedParameterInput.jsx b/client/app/components/QueryBasedParameterInput.jsx index 668cacab6e..e2e731cd03 100644 --- a/client/app/components/QueryBasedParameterInput.jsx +++ b/client/app/components/QueryBasedParameterInput.jsx @@ -3,12 +3,12 @@ import React from 'react'; import PropTypes from 'prop-types'; import { react2angular } from 'react2angular'; import Select from 'antd/lib/select'; -import { Query } from '@/services/query'; const { Option } = Select; export class QueryBasedParameterInput extends React.Component { static propTypes = { + parameter: PropTypes.any, // eslint-disable-line react/forbid-prop-types value: PropTypes.any, // eslint-disable-line react/forbid-prop-types queryId: PropTypes.number, onSelect: PropTypes.func, @@ -17,6 +17,7 @@ export class QueryBasedParameterInput extends React.Component { static defaultProps = { value: null, + parameter: null, queryId: null, onSelect: () => {}, className: '', @@ -41,19 +42,20 @@ export class QueryBasedParameterInput extends React.Component { } } - _loadOptions(queryId) { + async _loadOptions(queryId) { if (queryId && (queryId !== this.state.queryId)) { this.setState({ loading: true }); - Query.dropdownOptions({ id: queryId }, (options) => { - if (this.props.queryId === queryId) { - this.setState({ options, loading: false }); + const options = await this.props.parameter.loadDropdownValues(); - const found = find(options, option => option.value === this.props.value) !== undefined; - if (!found && isFunction(this.props.onSelect)) { - this.props.onSelect(options[0].value); - } + // stale queryId check + if (this.props.queryId === queryId) { + this.setState({ options, loading: false }); + + const found = find(options, option => option.value === this.props.value) !== undefined; + if (!found && isFunction(this.props.onSelect)) { + this.props.onSelect(options[0].value); } - }); + } } } diff --git a/client/app/services/query.js b/client/app/services/query.js index 593026375c..c206a24c25 100644 --- a/client/app/services/query.js +++ b/client/app/services/query.js @@ -50,7 +50,7 @@ function isDateRangeParameter(paramType) { } export class Parameter { - constructor(parameter) { + constructor(parameter, parentQueryId) { this.title = parameter.title; this.name = parameter.name; this.type = parameter.type; @@ -58,6 +58,7 @@ export class Parameter { this.global = parameter.global; // backward compatibility in Widget service this.enumOptions = parameter.enumOptions; this.queryId = parameter.queryId; + this.parentQueryId = parentQueryId; // Used for meta-parameters (i.e. dashboard-level params) this.locals = []; @@ -75,7 +76,7 @@ export class Parameter { } clone() { - return new Parameter(this); + return new Parameter(this, this.parentQueryId); } get isEmpty() { @@ -200,6 +201,14 @@ export class Parameter { } return `{{ ${this.name} }}`; } + + loadDropdownValues() { + if (this.parentQueryId) { + return Query.associatedDropdown({ queryId: this.parentQueryId, dropdownQueryId: this.queryId }).$promise; + } + + return Query.asDropdown({ id: this.queryId }).$promise; + } } class Parameters { @@ -250,7 +259,8 @@ class Parameters { }); const parameterExists = p => includes(parameterNames, p.name); - this.query.options.parameters = this.query.options.parameters.filter(parameterExists).map(p => new Parameter(p)); + const parameters = this.query.options.parameters; + this.query.options.parameters = parameters.filter(parameterExists).map(p => new Parameter(p, this.query.id)); } initFromQueryString(query) { @@ -371,11 +381,16 @@ function QueryResource( isArray: false, url: 'api/queries/:id/results.json', }, - dropdownOptions: { + asDropdown: { method: 'get', isArray: true, url: 'api/queries/:id/dropdown', }, + associatedDropdown: { + method: 'get', + isArray: true, + url: 'api/queries/:queryId/dropdowns/:dropdownQueryId', + }, favorites: { method: 'get', isArray: false, diff --git a/redash/handlers/api.py b/redash/handlers/api.py index 94da6aa859..6de8802c57 100644 --- a/redash/handlers/api.py +++ b/redash/handlers/api.py @@ -38,6 +38,7 @@ QueryTagsResource) from redash.handlers.query_results import (JobResource, QueryResultDropdownResource, + QueryDropdownsResource, QueryResultListResource, QueryResultResource) from redash.handlers.query_snippets import (QuerySnippetListResource, @@ -120,6 +121,7 @@ def json_representation(data, code, headers=None): api.add_org_resource(QueryResultListResource, '/api/query_results', endpoint='query_results') api.add_org_resource(QueryResultDropdownResource, '/api/queries//dropdown', endpoint='query_result_dropdown') +api.add_org_resource(QueryDropdownsResource, '/api/queries//dropdowns/', endpoint='query_result_dropdowns') api.add_org_resource(QueryResultResource, '/api/query_results/.', '/api/query_results/', diff --git a/redash/handlers/queries.py b/redash/handlers/queries.py index 33e35d9e48..b18acc1bf4 100644 --- a/redash/handlers/queries.py +++ b/redash/handlers/queries.py @@ -16,7 +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 +from redash.models.parameterized_query import ParameterizedQuery # Ordering map for relationships @@ -171,6 +171,17 @@ def get(self): return response +def require_access_to_dropdown_queries(user, query_def): + parameters = query_def.get('options', {}).get('parameters', []) + dropdown_query_ids = [str(p['queryId']) for p in parameters if p['type'] == 'query'] + + if dropdown_query_ids: + groups = models.Query.all_groups_for_query_ids(dropdown_query_ids) + + if len(groups) < len(dropdown_query_ids): + abort(400, message='You are trying to associate a dropdown query that does not have a matching group. Please verify the dropdown query id you are trying to associate with this query.') + + require_access(dict(groups), user, view_only) class QueryListResource(BaseQueryListResource): @require_permission('create_query') @@ -210,6 +221,7 @@ def post(self): query_def = request.get_json(force=True) data_source = models.DataSource.get_by_id_and_org(query_def.pop('data_source_id'), self.current_org) require_access(data_source.groups, self.current_user, not_view_only) + require_access_to_dropdown_queries(self.current_user, query_def) for field in ['id', 'created_at', 'api_key', 'visualizations', 'latest_query_data', 'last_modified_by']: query_def.pop(field, None) @@ -310,6 +322,7 @@ def post(self, query_id): query_def = request.get_json(force=True) require_object_modify_permission(query, self.current_user) + require_access_to_dropdown_queries(self.current_user, query_def) for field in ['id', 'created_at', 'api_key', 'visualizations', 'latest_query_data', 'user', 'last_modified_by', 'org']: query_def.pop(field, None) diff --git a/redash/handlers/query_results.py b/redash/handlers/query_results.py index 548b41173a..436a3edfb7 100644 --- a/redash/handlers/query_results.py +++ b/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, to_filename) -from redash.utils.parameterized_query import ParameterizedQuery, InvalidParameterError, dropdown_values +from redash.models.parameterized_query import ParameterizedQuery, InvalidParameterError, dropdown_values def error_response(message): @@ -151,11 +151,20 @@ def post(self): ONE_YEAR = 60 * 60 * 24 * 365.25 - class QueryResultDropdownResource(BaseResource): def get(self, query_id): return dropdown_values(query_id) +class QueryDropdownsResource(BaseResource): + def get(self, query_id, dropdown_query_id): + query = get_object_or_404(models.Query.get_by_id_and_org, query_id, self.current_org) + + related_queries_ids = [p['queryId'] for p in query.parameters if p['type'] == 'query'] + if int(dropdown_query_id) not in related_queries_ids: + abort(403) + + return dropdown_values(dropdown_query_id, should_require_access=False) + class QueryResultResource(BaseResource): @staticmethod diff --git a/redash/models/__init__.py b/redash/models/__init__.py index 1d9b5b11a2..ef9d3f01ef 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -28,7 +28,7 @@ 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 redash.models.parameterized_query import ParameterizedQuery from .base import db, gfk_type, Column, GFKBase, SearchBaseQuery from .changes import ChangeTrackingMixin, Change # noqa @@ -627,6 +627,15 @@ def recent(cls, group_ids, user_id=None, limit=20): def get_by_id(cls, _id): return cls.query.filter(cls.id == _id).one() + @classmethod + def all_groups_for_query_ids(cls, query_ids): + query = """SELECT group_id, view_only + FROM queries + JOIN data_source_groups ON queries.data_source_id = data_source_groups.data_source_id + WHERE queries.id in :ids""" + + return db.session.execute(query, {'ids': tuple(query_ids)}).fetchall() + def fork(self, user): forked_list = ['org', 'data_source', 'latest_query_data', 'description', 'query_text', 'query_hash', 'options'] @@ -669,9 +678,13 @@ def lowercase_name(cls): "The SQLAlchemy expression for the property above." return func.lower(cls.name) + @property + def parameters(self): + return self.options.get("parameters", []) + @property def parameterized(self): - return ParameterizedQuery(self.query_text, self.options.get("parameters", [])) + return ParameterizedQuery(self.query_text, self.parameters) @listens_for(Query.query_text, 'set') diff --git a/redash/utils/parameterized_query.py b/redash/models/parameterized_query.py similarity index 94% rename from redash/utils/parameterized_query.py rename to redash/models/parameterized_query.py index 84073057e6..7dc955025f 100644 --- a/redash/utils/parameterized_query.py +++ b/redash/models/parameterized_query.py @@ -16,19 +16,22 @@ def _pluck_name_and_value(default_column, row): return {"name": row[name_column], "value": unicode(row[value_column])} -def _load_result(query_id): +def _load_result(query_id, should_require_access): 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) + + if should_require_access: + 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) return json_loads(query_result.data) -def dropdown_values(query_id): - data = _load_result(query_id) +def dropdown_values(query_id, should_require_access=True): + data = _load_result(query_id, should_require_access) first_column = data["columns"][0]["name"] pluck = partial(_pluck_name_and_value, first_column) return map(pluck, data["rows"]) diff --git a/redash/serializers.py b/redash/serializers.py index 4306461dee..f1f01f3313 100644 --- a/redash/serializers.py +++ b/redash/serializers.py @@ -10,7 +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 +from redash.models.parameterized_query import ParameterizedQuery def public_widget(widget): diff --git a/tests/handlers/test_queries.py b/tests/handlers/test_queries.py index de587d6ed4..bda5b423e9 100644 --- a/tests/handlers/test_queries.py +++ b/tests/handlers/test_queries.py @@ -112,6 +112,59 @@ def test_raises_error_in_case_of_conflict(self): rv = self.make_request('post', '/api/queries/{0}'.format(q.id), data={'name': 'Testing', 'version': q.version - 1}, user=self.factory.user) self.assertEqual(rv.status_code, 409) + def test_allows_association_with_authorized_dropdown_queries(self): + data_source = self.factory.create_data_source(group=self.factory.default_group) + + other_query = self.factory.create_query(data_source=data_source) + db.session.add(other_query) + + my_query = self.factory.create_query(data_source=data_source) + db.session.add(my_query) + + options = { + 'parameters': [{ + 'type': 'query', + 'queryId': other_query.id + }] + } + + rv = self.make_request('post', '/api/queries/{0}'.format(my_query.id), data={'options': options}, user=self.factory.user) + self.assertEqual(rv.status_code, 200) + + def test_prevents_association_with_unauthorized_dropdown_queries(self): + other_data_source = self.factory.create_data_source(group=self.factory.create_group()) + other_query = self.factory.create_query(data_source=other_data_source) + db.session.add(other_query) + + my_data_source = self.factory.create_data_source(group=self.factory.create_group()) + my_query = self.factory.create_query(data_source=my_data_source) + db.session.add(my_query) + + options = { + 'parameters': [{ + 'type': 'query', + 'queryId': other_query.id + }] + } + + rv = self.make_request('post', '/api/queries/{0}'.format(my_query.id), data={'options': options}, user=self.factory.user) + self.assertEqual(rv.status_code, 403) + + def test_prevents_association_with_non_existing_dropdown_queries(self): + my_data_source = self.factory.create_data_source(group=self.factory.create_group()) + my_query = self.factory.create_query(data_source=my_data_source) + db.session.add(my_query) + + options = { + 'parameters': [{ + 'type': 'query', + 'queryId': 100000 + }] + } + + rv = self.make_request('post', '/api/queries/{0}'.format(my_query.id), data={'options': options}, user=self.factory.user) + self.assertEqual(rv.status_code, 400) + def test_overrides_existing_if_no_version_specified(self): q = self.factory.create_query() q.name = "Another Name" @@ -186,6 +239,68 @@ def test_create_query(self): self.assertEquals(len(list(query.visualizations)), 1) self.assertTrue(query.is_draft) + def test_allows_association_with_authorized_dropdown_queries(self): + data_source = self.factory.create_data_source(group=self.factory.default_group) + + other_query = self.factory.create_query(data_source=data_source) + db.session.add(other_query) + + query_data = { + 'name': 'Testing', + 'query': 'SELECT 1', + 'schedule': {"interval": "3600"}, + 'data_source_id': self.factory.data_source.id, + 'options': { + 'parameters': [{ + 'type': 'query', + 'queryId': other_query.id + }] + } + } + + rv = self.make_request('post', '/api/queries', data=query_data) + self.assertEqual(rv.status_code, 200) + + def test_prevents_association_with_unauthorized_dropdown_queries(self): + other_data_source = self.factory.create_data_source(group=self.factory.create_group()) + other_query = self.factory.create_query(data_source=other_data_source) + db.session.add(other_query) + + my_data_source = self.factory.create_data_source(group=self.factory.create_group()) + + query_data = { + 'name': 'Testing', + 'query': 'SELECT 1', + 'schedule': {"interval": "3600"}, + 'data_source_id': my_data_source.id, + 'options': { + 'parameters': [{ + 'type': 'query', + 'queryId': other_query.id + }] + } + } + + rv = self.make_request('post', '/api/queries', data=query_data) + self.assertEqual(rv.status_code, 403) + + def test_prevents_association_with_non_existing_dropdown_queries(self): + query_data = { + 'name': 'Testing', + 'query': 'SELECT 1', + 'schedule': {"interval": "3600"}, + 'data_source_id': self.factory.data_source.id, + 'options': { + 'parameters': [{ + 'type': 'query', + 'queryId': 100000 + }] + } + } + + rv = self.make_request('post', '/api/queries', data=query_data) + self.assertEqual(rv.status_code, 400) + class TestQueryArchiveResourceGet(BaseTestCase): def test_returns_queries(self): diff --git a/tests/handlers/test_query_results.py b/tests/handlers/test_query_results.py index 455cb81704..97d16fcd54 100644 --- a/tests/handlers/test_query_results.py +++ b/tests/handlers/test_query_results.py @@ -193,6 +193,50 @@ def test_checks_for_access_to_the_query(self): self.assertEquals(rv.status_code, 403) +class TestQueryDropdownsResource(BaseTestCase): + def test_prevents_access_if_query_isnt_associated_with_parent(self): + query = self.factory.create_query() + unrelated_dropdown_query = self.factory.create_query() + + rv = self.make_request('get', '/api/queries/{}/dropdowns/{}'.format(query.id, unrelated_dropdown_query.id)) + + self.assertEquals(rv.status_code, 403) + + def test_allows_access_if_user_has_access_to_parent_query(self): + query_result = self.factory.create_query_result() + data = { + 'rows': [], + 'columns': [{'name': 'whatever'}] + } + query_result = self.factory.create_query_result(data=json_dumps(data)) + dropdown_query = self.factory.create_query(latest_query_data=query_result) + + options = { + 'parameters': [{ + 'type': 'query', + 'queryId': dropdown_query.id + }] + } + query = self.factory.create_query(options=options) + + rv = self.make_request('get', '/api/queries/{}/dropdowns/{}'.format(query.id, dropdown_query.id)) + + self.assertEquals(rv.status_code, 200) + + def test_prevents_access_if_user_doesnt_have_access_to_parent_query(self): + related_dropdown_query = self.factory.create_query() + unrelated_dropdown_query = self.factory.create_query() + options = { + 'parameters': [{ + 'type': 'query', + 'queryId': related_dropdown_query.id + }] + } + query = self.factory.create_query(options=options) + + rv = self.make_request('get', '/api/queries/{}/dropdowns/{}'.format(query.id, unrelated_dropdown_query.id)) + + self.assertEquals(rv.status_code, 403) class TestQueryResultExcelResponse(BaseTestCase): def test_renders_excel_file(self): diff --git a/tests/utils/test_parameterized_query.py b/tests/models/test_parameterized_query.py similarity index 91% rename from tests/utils/test_parameterized_query.py rename to tests/models/test_parameterized_query.py index 9af0667d48..2ff445020c 100644 --- a/tests/utils/test_parameterized_query.py +++ b/tests/models/test_parameterized_query.py @@ -3,7 +3,7 @@ from collections import namedtuple import pytest -from redash.utils.parameterized_query import ParameterizedQuery, InvalidParameterError, dropdown_values +from redash.models.parameterized_query import ParameterizedQuery, InvalidParameterError, dropdown_values class TestParameterizedQuery(TestCase): @@ -119,7 +119,7 @@ def test_validates_enum_parameters(self): self.assertEquals("foo baz", query.text) - @patch('redash.utils.parameterized_query.dropdown_values', return_value=[{"value": "1"}]) + @patch('redash.models.parameterized_query.dropdown_values', return_value=[{"value": "1"}]) def test_validation_accepts_integer_values_for_dropdowns(self, _): schema = [{"name": "bar", "type": "query", "queryId": 1}] query = ParameterizedQuery("foo {{bar}}", schema) @@ -128,7 +128,7 @@ def test_validation_accepts_integer_values_for_dropdowns(self, _): self.assertEquals("foo 1", query.text) - @patch('redash.utils.parameterized_query.dropdown_values') + @patch('redash.models.parameterized_query.dropdown_values') def test_raises_on_invalid_query_parameters(self, _): schema = [{"name": "bar", "type": "query", "queryId": 1}] query = ParameterizedQuery("foo", schema) @@ -136,7 +136,7 @@ def test_raises_on_invalid_query_parameters(self, _): with pytest.raises(InvalidParameterError): query.apply({"bar": 7}) - @patch('redash.utils.parameterized_query.dropdown_values', return_value=[{"value": "baz"}]) + @patch('redash.models.parameterized_query.dropdown_values', return_value=[{"value": "baz"}]) def test_raises_on_unlisted_query_value_parameters(self, _): schema = [{"name": "bar", "type": "query", "queryId": 1}] query = ParameterizedQuery("foo", schema) @@ -144,7 +144,7 @@ def test_raises_on_unlisted_query_value_parameters(self, _): with pytest.raises(InvalidParameterError): query.apply({"bar": "shlomo"}) - @patch('redash.utils.parameterized_query.dropdown_values', return_value=[{"value": "baz"}]) + @patch('redash.models.parameterized_query.dropdown_values', return_value=[{"value": "baz"}]) def test_validates_query_parameters(self, _): schema = [{"name": "bar", "type": "query", "queryId": 1}] query = ParameterizedQuery("foo {{bar}}", schema) @@ -193,21 +193,21 @@ def test_is_safe_if_not_expecting_any_parameters(self): self.assertTrue(query.is_safe) - @patch('redash.utils.parameterized_query._load_result', return_value={ + @patch('redash.models.parameterized_query._load_result', return_value={ "columns": [{"name": "id"}, {"name": "Name"}, {"name": "Value"}], "rows": [{"id": 5, "Name": "John", "Value": "John Doe"}]}) def test_dropdown_values_prefers_name_and_value_columns(self, _): values = dropdown_values(1) self.assertEquals(values, [{"name": "John", "value": "John Doe"}]) - @patch('redash.utils.parameterized_query._load_result', return_value={ + @patch('redash.models.parameterized_query._load_result', return_value={ "columns": [{"name": "id"}, {"name": "fish"}, {"name": "poultry"}], "rows": [{"fish": "Clown", "id": 5, "poultry": "Hen"}]}) def test_dropdown_values_compromises_for_first_column(self, _): values = dropdown_values(1) self.assertEquals(values, [{"name": 5, "value": "5"}]) - @patch('redash.utils.parameterized_query._load_result', return_value={ + @patch('redash.models.parameterized_query._load_result', return_value={ "columns": [{"name": "ID"}, {"name": "fish"}, {"name": "poultry"}], "rows": [{"fish": "Clown", "ID": 5, "poultry": "Hen"}]}) def test_dropdown_supports_upper_cased_columns(self, _):