Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query Result API response shouldn't include query information for non authenticated users #3985

Merged
merged 9 commits into from
Jul 18, 2019
4 changes: 2 additions & 2 deletions redash/handlers/query_results.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
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.models.parameterized_query import ParameterizedQuery, InvalidParameterError, dropdown_values
from redash.serializers import serialize_query_result_to_csv, serialize_query_result_to_xlsx
from redash.serializers import serialize_query_result, serialize_query_result_to_csv, serialize_query_result_to_xlsx


def error_response(message):
Expand Down Expand Up @@ -42,7 +42,7 @@ def run_query(query, parameters, data_source, query_id, max_age=0):
query_result = models.QueryResult.get_latest(data_source, query.text, max_age)

if query_result:
return {'query_result': query_result.to_dict()}
return {'query_result': serialize_query_result(query_result, current_user.is_api_user())}
else:
job = enqueue_query(query.text, data_source, current_user.id, current_user.is_api_user(), metadata={
"Username": repr(current_user) if current_user.is_api_user() else current_user.email,
Expand Down
2 changes: 1 addition & 1 deletion redash/serializers/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from redash.utils import json_loads
from redash.models.parameterized_query import ParameterizedQuery

from .query_result import serialize_query_result_to_csv, serialize_query_result_to_xlsx
from .query_result import serialize_query_result, serialize_query_result_to_csv, serialize_query_result_to_xlsx


def public_widget(widget):
Expand Down
10 changes: 9 additions & 1 deletion redash/serializers/query_result.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import cStringIO
import csv
import xlsxwriter
from funcy import rpartial
from funcy import rpartial, project
from dateutil.parser import isoparse as parse_date
from redash.utils import json_loads, UnicodeWriter
from redash.query_runner import (TYPE_BOOLEAN, TYPE_DATE, TYPE_DATETIME)
Expand Down Expand Up @@ -57,6 +57,14 @@ def _get_column_lists(columns):
return fieldnames, special_columns


def serialize_query_result(query_result, is_api_user):
if is_api_user:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's safe to use current_user in the serialization code instead of passing is_api_user. It's not a big deal for this PR, but #3782 will extend this method to apply other checks anyway.

publicly_needed_keys = ['data', 'retrieved_at']
return project(query_result.to_dict(), publicly_needed_keys)
else:
return query_result.to_dict()


def serialize_query_result_to_csv(query_result):
s = cStringIO.StringIO()

Expand Down
15 changes: 14 additions & 1 deletion tests/serializers/test_query_results.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

from redash import models
from redash.utils import utcnow, json_dumps
from redash.serializers import serialize_query_result_to_csv
from redash.serializers import serialize_query_result, serialize_query_result_to_csv


data = {
Expand All @@ -24,6 +24,19 @@
]
}

class QueryResultSerializationTest(BaseTestCase):
def test_serializes_all_keys_for_authenticated_users(self):
query_result = self.factory.create_query_result(data=json_dumps({}))
serialized = serialize_query_result(query_result, False)
self.assertSetEqual(set(query_result.to_dict().keys()),
set(serialized.keys()))

def test_doesnt_serialize_sensitive_keys_for_unauthenticated_users(self):
query_result = self.factory.create_query_result(data=json_dumps({}))
serialized = serialize_query_result(query_result, True)
self.assertSetEqual(set(['data', 'retrieved_at']),
set(serialized.keys()))

class CsvSerializationTest(BaseTestCase):
def get_csv_content(self):
query_result = self.factory.create_query_result(data=json_dumps(data))
Expand Down