From bcb8281b4db1255807db9c333ff186cc63e7c257 Mon Sep 17 00:00:00 2001 From: Arik Fraimovich Date: Sun, 1 Sep 2019 11:22:59 +0300 Subject: [PATCH 1/4] Fix: allow users with view only acces to access the queries --- redash/query_runner/query_results.py | 47 ++++++++++++++++------------ 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/redash/query_runner/query_results.py b/redash/query_runner/query_results.py index 910df7c9c1..2b21a5493c 100644 --- a/redash/query_runner/query_results.py +++ b/redash/query_runner/query_results.py @@ -3,7 +3,7 @@ import sqlite3 from redash import models -from redash.permissions import has_access, not_view_only +from redash.permissions import has_access, view_only from redash.query_runner import BaseQueryRunner, TYPE_STRING, guess_type, register from redash.utils import json_dumps, json_loads @@ -24,7 +24,8 @@ def extract_query_ids(query): def extract_cached_query_ids(query): - queries = re.findall(r'(?:join|from)\s+cached_query_(\d+)', query, re.IGNORECASE) + queries = re.findall(r'(?:join|from)\s+cached_query_(\d+)', query, + re.IGNORECASE) return [int(q) for q in queries] @@ -34,9 +35,12 @@ def _load_query(user, query_id): if user.org_id != query.org_id: raise PermissionError("Query id {} not found.".format(query.id)) - if not has_access(query.data_source, user, not_view_only): - raise PermissionError(u"You are not allowed to execute queries on {} data source (used for query id {}).".format( - query.data_source.name, query.id)) + # TODO: this duplicates some of the logic we already have in the redash.handlers.query_results. + # We should merge it so it's consistent. + if not has_access(query.data_source, user, view_only): + raise PermissionError( + u"You are not allowed to execute queries on {} data source (used for query id {})." + .format(query.data_source.name, query.id)) return query @@ -47,16 +51,22 @@ def get_query_results(user, query_id, bring_from_cache): if query.latest_query_data_id is not None: results = query.latest_query_data.data else: - raise Exception("No cached result available for query {}.".format(query.id)) + raise Exception("No cached result available for query {}.".format( + query.id)) else: - results, error = query.data_source.query_runner.run_query(query.query_text, user) + results, error = query.data_source.query_runner.run_query( + query.query_text, user) if error: - raise Exception("Failed loading results for query id {}.".format(query.id)) + raise Exception("Failed loading results for query id {}.".format( + query.id)) return json_loads(results) -def create_tables_from_query_ids(user, connection, query_ids, cached_query_ids=[]): +def create_tables_from_query_ids(user, + connection, + query_ids, + cached_query_ids=[]): for query_id in set(cached_query_ids): results = get_query_results(user, query_id, True) table_name = 'cached_query_{query_id}'.format(query_id=query_id) @@ -81,8 +91,7 @@ def flatten(value): def create_table(connection, table_name, query_results): try: - columns = [column['name'] - for column in query_results['columns']] + columns = [column['name'] for column in query_results['columns']] safe_columns = [fix_column_name(column) for column in columns] column_list = ", ".join(safe_columns) @@ -91,7 +100,8 @@ def create_table(connection, table_name, query_results): logger.debug("CREATE TABLE query: %s", create_table) connection.execute(create_table) except sqlite3.OperationalError as exc: - raise CreateTableError(u"Error creating table {}: {}".format(table_name, exc.message)) + raise CreateTableError(u"Error creating table {}: {}".format( + table_name, exc.message)) insert_template = u"insert into {table_name} ({column_list}) values ({place_holders})".format( table_name=table_name, @@ -108,11 +118,7 @@ class Results(BaseQueryRunner): @classmethod def configuration_schema(cls): - return { - "type": "object", - "properties": { - } - } + return {"type": "object", "properties": {}} @classmethod def annotate_query(cls): @@ -127,7 +133,8 @@ def run_query(self, query, user): query_ids = extract_query_ids(query) cached_query_ids = extract_cached_query_ids(query) - create_tables_from_query_ids(user, connection, query_ids, cached_query_ids) + create_tables_from_query_ids(user, connection, query_ids, + cached_query_ids) cursor = connection.cursor() @@ -135,8 +142,8 @@ def run_query(self, query, user): cursor.execute(query) if cursor.description is not None: - columns = self.fetch_columns( - [(i[0], None) for i in cursor.description]) + columns = self.fetch_columns([(i[0], None) + for i in cursor.description]) rows = [] column_names = [c['name'] for c in columns] From 99c5d4406909d8b3903780ed3f9535e3bca2c716 Mon Sep 17 00:00:00 2001 From: Arik Fraimovich Date: Sun, 1 Sep 2019 11:30:01 +0300 Subject: [PATCH 2/4] Add tests --- tests/query_runner/test_query_results.py | 125 +++++++++++++++++++---- 1 file changed, 105 insertions(+), 20 deletions(-) diff --git a/tests/query_runner/test_query_results.py b/tests/query_runner/test_query_results.py index db047e587f..8c3da3e387 100644 --- a/tests/query_runner/test_query_results.py +++ b/tests/query_runner/test_query_results.py @@ -3,7 +3,9 @@ import pytest -from redash.query_runner.query_results import (CreateTableError, PermissionError, _load_query, create_table, extract_cached_query_ids, extract_query_ids, fix_column_name) +from redash.query_runner.query_results import ( + CreateTableError, PermissionError, _load_query, create_table, + extract_cached_query_ids, extract_query_ids, fix_column_name) from tests import BaseTestCase @@ -28,40 +30,86 @@ def test_finds_queries_with_whitespace_characters(self): class TestCreateTable(TestCase): def test_creates_table_with_colons_in_column_name(self): connection = sqlite3.connect(':memory:') - results = {'columns': [{'name': 'ga:newUsers'}, { - 'name': 'test2'}], 'rows': [{'ga:newUsers': 123, 'test2': 2}]} + results = { + 'columns': [{ + 'name': 'ga:newUsers' + }, { + 'name': 'test2' + }], + 'rows': [{ + 'ga:newUsers': 123, + 'test2': 2 + }] + } table_name = 'query_123' create_table(connection, table_name, results) connection.execute('SELECT 1 FROM query_123') def test_creates_table_with_double_quotes_in_column_name(self): connection = sqlite3.connect(':memory:') - results = {'columns': [{'name': 'ga:newUsers'}, { - 'name': '"test2"'}], 'rows': [{'ga:newUsers': 123, '"test2"': 2}]} + results = { + 'columns': [{ + 'name': 'ga:newUsers' + }, { + 'name': '"test2"' + }], + 'rows': [{ + 'ga:newUsers': 123, + '"test2"': 2 + }] + } table_name = 'query_123' create_table(connection, table_name, results) connection.execute('SELECT 1 FROM query_123') def test_creates_table(self): connection = sqlite3.connect(':memory:') - results = {'columns': [{'name': 'test1'}, - {'name': 'test2'}], 'rows': []} + results = { + 'columns': [{ + 'name': 'test1' + }, { + 'name': 'test2' + }], + 'rows': [] + } table_name = 'query_123' create_table(connection, table_name, results) connection.execute('SELECT 1 FROM query_123') def test_creates_table_with_missing_columns(self): connection = sqlite3.connect(':memory:') - results = {'columns': [{'name': 'test1'}, {'name': 'test2'}], 'rows': [ - {'test1': 1, 'test2': 2}, {'test1': 3}]} + results = { + 'columns': [{ + 'name': 'test1' + }, { + 'name': 'test2' + }], + 'rows': [{ + 'test1': 1, + 'test2': 2 + }, { + 'test1': 3 + }] + } table_name = 'query_123' create_table(connection, table_name, results) connection.execute('SELECT 1 FROM query_123') def test_creates_table_with_spaces_in_column_name(self): connection = sqlite3.connect(':memory:') - results = {'columns': [{'name': 'two words'}, {'name': 'test2'}], 'rows': [ - {'two words': 1, 'test2': 2}, {'test1': 3}]} + results = { + 'columns': [{ + 'name': 'two words' + }, { + 'name': 'test2' + }], + 'rows': [{ + 'two words': 1, + 'test2': 2 + }, { + 'test1': 3 + }] + } table_name = 'query_123' create_table(connection, table_name, results) connection.execute('SELECT 1 FROM query_123') @@ -69,8 +117,15 @@ def test_creates_table_with_spaces_in_column_name(self): def test_creates_table_with_dashes_in_column_name(self): connection = sqlite3.connect(':memory:') results = { - 'columns': [{'name': 'two-words'}, {'name': 'test2'}], - 'rows': [{'two-words': 1, 'test2': 2}] + 'columns': [{ + 'name': 'two-words' + }, { + 'name': 'test2' + }], + 'rows': [{ + 'two-words': 1, + 'test2': 2 + }] } table_name = 'query_123' create_table(connection, table_name, results) @@ -79,8 +134,17 @@ def test_creates_table_with_dashes_in_column_name(self): def test_creates_table_with_non_ascii_in_column_name(self): connection = sqlite3.connect(':memory:') - results = {'columns': [{'name': u'\xe4'}, {'name': 'test2'}], 'rows': [ - {u'\xe4': 1, 'test2': 2}]} + results = { + 'columns': [{ + 'name': u'\xe4' + }, { + 'name': 'test2' + }], + 'rows': [{ + u'\xe4': 1, + 'test2': 2 + }] + } table_name = 'query_123' create_table(connection, table_name, results) connection.execute('SELECT 1 FROM query_123') @@ -95,8 +159,14 @@ def test_shows_meaningful_error_on_failure_to_create_table(self): def test_loads_results(self): connection = sqlite3.connect(':memory:') rows = [{'test1': 1, 'test2': 'test'}, {'test1': 2, 'test2': 'test2'}] - results = {'columns': [{'name': 'test1'}, - {'name': 'test2'}], 'rows': rows} + results = { + 'columns': [{ + 'name': 'test1' + }, { + 'name': 'test2' + }], + 'rows': rows + } table_name = 'query_123' create_table(connection, table_name, results) self.assertEquals( @@ -104,9 +174,15 @@ def test_loads_results(self): def test_loads_list_and_dict_results(self): connection = sqlite3.connect(':memory:') - rows = [{'test1': [1,2,3]}, {'test2': {'a': 'b'}}] - results = {'columns': [{'name': 'test1'}, - {'name': 'test2'}], 'rows': rows} + rows = [{'test1': [1, 2, 3]}, {'test2': {'a': 'b'}}] + results = { + 'columns': [{ + 'name': 'test1' + }, { + 'name': 'test2' + }], + 'rows': rows + } table_name = 'query_123' create_table(connection, table_name, results) self.assertEquals( @@ -135,6 +211,15 @@ def test_returns_query(self): loaded = _load_query(user, query.id) self.assertEquals(query, loaded) + def test_returns_query_when_user_has_view_only_access(self): + ds = self.factory.create_data_source( + group=self.factory.org.default_group, view_only=True) + query = self.factory.create_query(data_source=ds) + user = self.factory.create_user() + + loaded = _load_query(user, query.id) + self.assertEquals(query, loaded) + class TestExtractCachedQueryIds(TestCase): def test_works_with_simple_query(self): From 32ba5d76790871175934db016a4f34c08d192a4e Mon Sep 17 00:00:00 2001 From: Arik Fraimovich Date: Sun, 1 Sep 2019 12:41:54 +0300 Subject: [PATCH 3/4] Update error message --- redash/query_runner/query_results.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/redash/query_runner/query_results.py b/redash/query_runner/query_results.py index 2b21a5493c..4e9032d4ae 100644 --- a/redash/query_runner/query_results.py +++ b/redash/query_runner/query_results.py @@ -38,9 +38,7 @@ def _load_query(user, query_id): # TODO: this duplicates some of the logic we already have in the redash.handlers.query_results. # We should merge it so it's consistent. if not has_access(query.data_source, user, view_only): - raise PermissionError( - u"You are not allowed to execute queries on {} data source (used for query id {})." - .format(query.data_source.name, query.id)) + raise PermissionError("Query id {} not found.".format(query.id)) return query From bb4f76808e206549d5397bc7e14fa852583255a9 Mon Sep 17 00:00:00 2001 From: Arik Fraimovich Date: Sun, 1 Sep 2019 12:50:35 +0300 Subject: [PATCH 4/4] Update error message. Take 2 --- redash/query_runner/query_results.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/redash/query_runner/query_results.py b/redash/query_runner/query_results.py index 4e9032d4ae..3183fc8ca8 100644 --- a/redash/query_runner/query_results.py +++ b/redash/query_runner/query_results.py @@ -38,7 +38,8 @@ def _load_query(user, query_id): # TODO: this duplicates some of the logic we already have in the redash.handlers.query_results. # We should merge it so it's consistent. if not has_access(query.data_source, user, view_only): - raise PermissionError("Query id {} not found.".format(query.id)) + raise PermissionError(u"You do not have access to query id {}.".format( + query.id)) return query