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

Nest query ACL to dropdowns #3544

Merged
merged 22 commits into from
Mar 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
70e86a5
change API to /api/queries/:id/dropdowns/:dropdown_id
Mar 6, 2019
4a8a377
extract property
Mar 6, 2019
00a1bfc
split to 2 different dropdown endpoints and implement the second
Mar 6, 2019
87d959f
make access control optional for dropdowns (assuming it is verified at a
Mar 6, 2019
a09d151
add test cases for /api/queries/:id/dropdowns/:id
Mar 6, 2019
f1a585c
use new /dropdowns endpoint in frontend
Mar 7, 2019
d5b809b
require access to dropdown queries when creating or updating parent
Mar 10, 2019
43366cb
rename Query resource dropdown endpoints
Mar 12, 2019
527dc91
check access to dropdown query associations in one fugly query
Mar 12, 2019
4649e6d
move ParameterizedQuery to models folder
Mar 14, 2019
b5638af
add dropdown association tests to query creation
Mar 14, 2019
91655ae
move group by query ids query into models.Query
Mar 14, 2019
b55d1c6
use bound parameters for groups query
Mar 14, 2019
d2c3629
format groups query
Mar 14, 2019
5ac0e13
use new associatedDropdowns endpoint in dashboards
Mar 18, 2019
2d82996
pass down parameter and let it return dropdown options. Go Levko!
Mar 18, 2019
d52f87f
change API to /api/queries/:id/dropdowns/:dropdown_id
Mar 6, 2019
83e911c
split to 2 different dropdown endpoints and implement the second
Mar 6, 2019
20f9b51
use new /dropdowns endpoint in frontend
Mar 7, 2019
1f39d04
pass down parameter and let it return dropdown options. Go Levko!
Mar 18, 2019
6ad4424
fix bad rebase
Mar 19, 2019
d2b95a7
add comment to clarify the purpose of checking the queryId
Mar 19, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion client/app/components/ParameterValueInput.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand All @@ -27,6 +28,7 @@ export class ParameterValueInput extends React.Component {
value: null,
enumOptions: '',
queryId: null,
parameter: null,
onSelect: () => {},
className: '',
};
Expand Down Expand Up @@ -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 (
<QueryBasedParameterInput
className={this.props.className}
parameter={parameter}
value={value}
queryId={queryId}
onSelect={onSelect}
Expand Down Expand Up @@ -173,8 +176,10 @@ export default function init(ngModule) {
<parameter-value-input-impl
type="$ctrl.param.type"
value="$ctrl.param.normalizedValue"
parameter="$ctrl.param"
enum-options="$ctrl.param.enumOptions"
query-id="$ctrl.param.queryId"
parent-query-id="$ctrl.param.parentQueryId"
on-select="$ctrl.setValue"
></parameter-value-input-impl>
`,
Expand Down
22 changes: 12 additions & 10 deletions client/app/components/QueryBasedParameterInput.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -17,6 +17,7 @@ export class QueryBasedParameterInput extends React.Component {

static defaultProps = {
value: null,
parameter: null,
queryId: null,
onSelect: () => {},
className: '',
Expand All @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

While I think that async/await are amazing, in this case they're hiding very important thing. This condition is needed because while we load dropdown values, component may receive new queryId - so we should drop outdated results. With .then(...) it's very clear that we have async operation; but with await this fragment looks linear, and this condition looks confusing (but it's really important). That's my opinion. @arikfr, @ranbena - WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think just adding a comment // stale queryId check would make it clear for both patterns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that's the case, I'd favor await over Promises. Not sure why we haven't begun embracing awaits elsewhere in the code?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure why we haven't begun embracing awaits elsewhere in the code?

We try to leave things for future refactors 😎

but with await this fragment looks linear, and this condition looks confusing

But await itself signals that this is async operation. Maybe it's just confusing because we're less used to using it?

Let's give it a try and see how goes.

Copy link
Collaborator

@kravets-levko kravets-levko Mar 19, 2019

Choose a reason for hiding this comment

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

@arikfr I'm not against of using async/await - just wanted to say that we need somehow indicate non-obvious/confusing code (like in this case). Comment is 100% enough. Also, @rauchy - can you please check that babel does not transpile async/await - since we don't need to support IE, we can use that syntax natively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we care if the transpiled version is using async/await natively? Feels meaningless to me but I may be missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Took care of it #3609

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);
}
});
}
}
}

Expand Down
23 changes: 19 additions & 4 deletions client/app/services/query.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,15 @@ function isDateRangeParameter(paramType) {
}

export class Parameter {
constructor(parameter) {
constructor(parameter, parentQueryId) {
kravets-levko marked this conversation as resolved.
Show resolved Hide resolved
this.title = parameter.title;
this.name = parameter.name;
this.type = parameter.type;
this.useCurrentDateTime = parameter.useCurrentDateTime;
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 = [];
Expand All @@ -75,7 +76,7 @@ export class Parameter {
}

clone() {
return new Parameter(this);
return new Parameter(this, this.parentQueryId);
}

get isEmpty() {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions redash/handlers/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
QueryTagsResource)
from redash.handlers.query_results import (JobResource,
QueryResultDropdownResource,
QueryDropdownsResource,
QueryResultListResource,
QueryResultResource)
from redash.handlers.query_snippets import (QuerySnippetListResource,
Expand Down Expand Up @@ -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/<query_id>/dropdown', endpoint='query_result_dropdown')
api.add_org_resource(QueryDropdownsResource, '/api/queries/<query_id>/dropdowns/<dropdown_query_id>', endpoint='query_result_dropdowns')
api.add_org_resource(QueryResultResource,
'/api/query_results/<query_result_id>.<filetype>',
'/api/query_results/<query_result_id>',
Expand Down
15 changes: 14 additions & 1 deletion redash/handlers/queries.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should do this only if there was a change to the dropdown queries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also handled in b4fca74


for field in ['id', 'created_at', 'api_key', 'visualizations', 'latest_query_data', 'user', 'last_modified_by', 'org']:
query_def.pop(field, None)
Expand Down
13 changes: 11 additions & 2 deletions redash/handlers/query_results.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand Down
17 changes: 15 additions & 2 deletions redash/models/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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']
Expand Down Expand Up @@ -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')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
Expand Down
2 changes: 1 addition & 1 deletion redash/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
Loading