-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
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
4a8a377
extract property
00a1bfc
split to 2 different dropdown endpoints and implement the second
87d959f
make access control optional for dropdowns (assuming it is verified at a
a09d151
add test cases for /api/queries/:id/dropdowns/:id
f1a585c
use new /dropdowns endpoint in frontend
d5b809b
require access to dropdown queries when creating or updating parent
43366cb
rename Query resource dropdown endpoints
527dc91
check access to dropdown query associations in one fugly query
4649e6d
move ParameterizedQuery to models folder
b5638af
add dropdown association tests to query creation
91655ae
move group by query ids query into models.Query
b55d1c6
use bound parameters for groups query
d2c3629
format groups query
5ac0e13
use new associatedDropdowns endpoint in dashboards
2d82996
pass down parameter and let it return dropdown options. Go Levko!
d52f87f
change API to /api/queries/:id/dropdowns/:dropdown_id
83e911c
split to 2 different dropdown endpoints and implement the second
20f9b51
use new /dropdowns endpoint in frontend
1f39d04
pass down parameter and let it return dropdown options. Go Levko!
6ad4424
fix bad rebase
d2b95a7
add comment to clarify the purpose of checking the queryId
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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') | ||
|
@@ -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) | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 withawait
this fragment looks linear, and this condition looks confusing (but it's really important). That's my opinion. @arikfr, @ranbena - WDYT?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 embracingawait
s elsewhere in the code?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to leave things for future refactors 😎
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.
There was a problem hiding this comment.
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 transpileasync
/await
- since we don't need to support IE, we can use that syntax natively.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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