Skip to content

Commit

Permalink
Fix: allow users with view only acces to use the queries in Query Res…
Browse files Browse the repository at this point in the history 
…ults (#4112)

* Fix: allow users with view only acces to access the queries

* Add tests

* Update error message

* Update error message. Take 2
  • Loading branch information
@arikfr
arikfr committed Oct 27, 2019
1 parent 7e9db06 commit 8af099b
Show file tree
Hide file tree
Showing 2 changed files with 131 additions and 40 deletions.
46 changes: 26 additions & 20 deletions redash/query_runner/query_results.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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]


Expand All @@ -34,9 +35,11 @@ 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 do not have access to query id {}.".format(
query.id))

return query

Expand All @@ -47,16 +50,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)
Expand All @@ -81,8 +90,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)
Expand All @@ -91,7 +99,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,
Expand All @@ -108,11 +117,7 @@ class Results(BaseQueryRunner):

@classmethod
def configuration_schema(cls):
return {
"type": "object",
"properties": {
}
}
return {"type": "object", "properties": {}}

@classmethod
def annotate_query(cls):
Expand All @@ -127,16 +132,17 @@ 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()

try:
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]
Expand Down
125 changes: 105 additions & 20 deletions tests/query_runner/test_query_results.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -28,49 +30,102 @@ 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')

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)
Expand All @@ -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')
Expand All @@ -95,18 +159,30 @@ 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(
len(list(connection.execute('SELECT * FROM query_123'))), 2)

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(
Expand Down Expand Up @@ -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):
Expand Down

0 comments on commit 8af099b

Please sign in to comment.