Nest query ACL to dropdowns #3544

rauchy · 2019-03-07T08:56:52Z

What type of PR is this? (check all applicable)

Refactor

Description

Dropdown queries are currently available through /api/queries/:id/dropdown. This works fine for users who create queries themselves and share it within the same group, but once such a dropdown query is to be accessed by someone else (for example, anonymous embed users), then the api_key used for the share wouldn't provide access to the dropdown query.

This PR adds another dropdown endpoint (/api/queries/:parent_id/dropdowns/:id) which will check access to the parent query (the parent query is the query which uses values from the dropdown as parameters), and if the user has access to the parent query, he will have access to all associated dropdown queries. The reasoning behind creating a second endpoint for this scenario is that it requires a parent query, but when you are in the process of creating a query, it is not yet persisted so there is no parent query to inherit access from.

Related Tickets & Documents

Addresses #3467

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

arikfr · 2019-03-11T14:41:01Z

client/app/components/QueryBasedParameterInput.jsx

-      });
+      };
+
+      if (this.props.parentQueryId) {


It doesn't feel right to have this logic here and pass around the parentQueryId. How about we have a dropdownValues function on the Query instance. When it's a new query it will use the unsafe API and when it's not, it won't?

(and pass this function into this component)

Gosh, at first it felt like a "d'oh! how didn't I do this before?", but digging into this, I kinda feel that while putting this function on Query feels like a more natural place for this logic, it will probably look uglier because of the multiple layers of function signature that will have to carry this function across in order to get to QueryBasedParameterInput. (that is - QueryBasedParameterInput props, ParameterValueInput props, parameter-value-input directive, parameters directive and query page)

Let me know which you prefer.

Isn't it the same amount of effort to pass around the Query object as passing around the parentQueryId? What's the difference?

Also, what happens on dashboard level parameters? We will pick one of the queries to serve as a parent? 🤔

I wonder if there is another way to approach this.

client/app/components/QueryBasedParameterInput.jsx

arikfr · 2019-03-11T14:50:09Z

redash/handlers/queries.py

@@ -171,6 +171,12 @@ def get(self):

        return response

+def require_access_to_dropdown_queries(user, query_def):
+    parameters = query_def.get('options', {}).get('parameters', [])
+    dropdown_queries = [models.Query.get_by_id(p["queryId"]) for p in parameters if p["type"] == "query"]


This will execute N queries. Better to rewrite in a way that will execute a single query.

It was actually using query.in_ before (which is a single query), but it didn't fail on queries outside the range (and that led to a situation in which you could have specified a non-existing query id, which may exist later on). Given that most queries have a very small number dropdowns, I felt its best not to handle it and keep the code clean.

I guess I could find another way of writing it in a single query, I just didn't see it as that important.

Good catch. We can verify that all the ids passed in were returned in the database response?

While this is generally not a big deal, these things tend to accumulate. Considering this will run on every query save, it can be an annoyance. If the fix isn't that complex, worth handling.

Also, please add a test case that verifies you can't pass in non existing query ids.

b4fca74 does the trick. Let me know if you find it better or worse.

arikfr · 2019-03-11T14:52:18Z

redash/handlers/queries.py

+    dropdown_queries = [models.Query.get_by_id(p["queryId"]) for p in parameters if p["type"] == "query"]
+
+    for query in dropdown_queries:
+        require_access(query.data_source.groups, user, view_only)


This will trigger N+1 queries for data sources and groups. :| Not sure if there is a pretty way around it.

Also handled in b4fca74

arikfr · 2019-03-11T14:52:49Z

redash/handlers/queries.py

@@ -310,6 +317,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)


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

Also handled in b4fca74

arikfr · 2019-03-11T15:06:06Z

redash/utils/parameterized_query.py

@@ -16,19 +16,22 @@ def _pluck_name_and_value(default_column, row):
    return {"name": row[name_column], "value": row[value_column]}


(unrelated to this line)

I think this class should move into redash.models.

arikfr · 2019-03-11T15:07:36Z

tests/handlers/test_queries.py

+            }]
+        }
+
+        rv = self.make_request('post', '/api/queries/{0}'.format(my_query.id), data={'options': options}, user=self.factory.user)


Worth adding similar test for creation of a query, because it's a different code path.

arikfr · 2019-03-14T08:25:38Z

redash/handlers/queries.py

+    dropdown_query_ids = [str(p['queryId']) for p in parameters if p['type'] == 'query']
+
+    if dropdown_query_ids:
+        groups = models.db.session.execute('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 ({})'.format(','.join(dropdown_query_ids))).fetchall()


The concept is definitely better than previously: we run a single query and don't have to do N+1 queries twice. 👌

But the implementation has a few issues --

All direct queries should be in redash.models. Outside of redash.models we should use an API exposed by the models.

By building a query on your own, you open this to SQL injections. I'm pretty sure session.execute can take a parameterized query and if not there is a similar API that can do this.

Why not use SQLAlchemy's models to build this query? I know it's more effort at first due to the learning curve, but it's more consistent with the rest of the code and has other benefits.

Would you feel better with this hanging off a Query class method?

You're right, should have used params.

I saw this coming, but decided to push a quicker fix to hear your thoughts, specifically because

If the fix isn't that complex, worth handling

Yes. Something like Query.get_groups_for_query_ids?

👍

ranbena

I wish there wasn't so much nesting but at least it's just a primitive value.
@kravets-levko maybe we can utilize global state here?

arikfr · 2019-03-17T14:47:18Z

@ranbena I doubt global state is the solution here as we’re not dealing with a global thing.

There are a few possible approaches here, but after review with @rauchy to go with the current implementation until we remove Angular from the upper layers (Parameters component and the query and dashboard pages). Then we can revisit with a better approach.

client/app/services/query.js

kravets-levko · 2019-03-18T17:55:17Z

client/app/components/QueryBasedParameterInput.jsx

-        if (this.props.queryId === queryId) {
-          this.setState({ options, loading: false });
+      const options = await this.props.parameter.loadDropdownValues();
+      if (this.props.queryId === queryId) {


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?

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

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

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.

@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.

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

Took care of it #3609

client/app/services/query.js

different level)

queries

* 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

rauchy requested a review from arikfr March 7, 2019 08:56

ghost assigned rauchy Mar 7, 2019

ghost added the in progress label Mar 7, 2019

rauchy requested review from kravets-levko and ranbena March 7, 2019 13:13

weekly-digest bot mentioned this pull request Mar 10, 2019

Weekly Digest (3 March, 2019 - 10 March, 2019) #3560

Closed

arikfr requested changes Mar 11, 2019

View reviewed changes

arikfr requested changes Mar 14, 2019

View reviewed changes

ranbena reviewed Mar 17, 2019

View reviewed changes

kravets-levko reviewed Mar 18, 2019

View reviewed changes

client/app/services/query.js Show resolved Hide resolved

kravets-levko reviewed Mar 18, 2019

View reviewed changes

client/app/services/query.js Outdated Show resolved Hide resolved

Omer Lachish added 17 commits March 19, 2019 11:33

change API to /api/queries/:id/dropdowns/:dropdown_id

70e86a5

extract property

4a8a377

split to 2 different dropdown endpoints and implement the second

00a1bfc

make access control optional for dropdowns (assuming it is verified at a

87d959f

different level)

add test cases for /api/queries/:id/dropdowns/:id

a09d151

use new /dropdowns endpoint in frontend

f1a585c

require access to dropdown queries when creating or updating parent

d5b809b

queries

rename Query resource dropdown endpoints

43366cb

check access to dropdown query associations in one fugly query

527dc91

move ParameterizedQuery to models folder

4649e6d

add dropdown association tests to query creation

b5638af

move group by query ids query into models.Query

91655ae

use bound parameters for groups query

b55d1c6

format groups query

d2c3629

use new associatedDropdowns endpoint in dashboards

5ac0e13

pass down parameter and let it return dropdown options. Go Levko!

2d82996

change API to /api/queries/:id/dropdowns/:dropdown_id

d52f87f

Omer Lachish added 4 commits March 19, 2019 11:36

split to 2 different dropdown endpoints and implement the second

83e911c

use new /dropdowns endpoint in frontend

20f9b51

pass down parameter and let it return dropdown options. Go Levko!

1f39d04

fix bad rebase

6ad4424

rauchy force-pushed the nest-query-acl-to-dropdowns branch from ba7859d to 6ad4424 Compare March 19, 2019 09:42

add comment to clarify the purpose of checking the queryId

d2b95a7

arikfr approved these changes Mar 19, 2019

View reviewed changes

rauchy merged commit c47dd05 into master Mar 20, 2019

ghost removed the in progress label Mar 20, 2019

guidopetri deleted the nest-query-acl-to-dropdowns branch July 22, 2023 03:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nest query ACL to dropdowns #3544

Nest query ACL to dropdowns #3544

rauchy commented Mar 7, 2019 •

edited by arikfr

Loading

arikfr Mar 11, 2019

arikfr Mar 11, 2019

rauchy Mar 14, 2019

arikfr Mar 14, 2019

arikfr Mar 11, 2019

rauchy Mar 11, 2019

arikfr Mar 12, 2019

rauchy Mar 12, 2019

arikfr Mar 11, 2019

rauchy Mar 12, 2019

arikfr Mar 11, 2019

rauchy Mar 12, 2019

arikfr Mar 11, 2019

rauchy Mar 14, 2019

arikfr Mar 11, 2019

rauchy Mar 14, 2019

arikfr Mar 14, 2019

rauchy Mar 14, 2019

arikfr Mar 14, 2019

rauchy Mar 14, 2019

ranbena left a comment

arikfr commented Mar 17, 2019

kravets-levko Mar 18, 2019

ranbena Mar 19, 2019

rauchy Mar 19, 2019

arikfr Mar 19, 2019

kravets-levko Mar 19, 2019 •

edited

Loading

rauchy Mar 20, 2019

ranbena Mar 20, 2019

		@@ -16,19 +16,22 @@ def _pluck_name_and_value(default_column, row):
		return {"name": row[name_column], "value": row[value_column]}

Nest query ACL to dropdowns #3544

Nest query ACL to dropdowns #3544

Conversation

rauchy commented Mar 7, 2019 • edited by arikfr Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranbena left a comment

Choose a reason for hiding this comment

arikfr commented Mar 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kravets-levko Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauchy commented Mar 7, 2019 •

edited by arikfr

Loading

kravets-levko Mar 19, 2019 •

edited

Loading