-
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
enable view_source permission #3782
enable view_source permission #3782
Conversation
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.
This is indeed something we should address, but your change breaks visualization embeds. I think a better solution would be to allow using this endpoint without view_source
permission, but if you don't have it filter out the query text from the result. Wdyt?
6e8911f
to
a80308b
Compare
@arikfr Thank you for your comment. I changed to show This commit enables to display embed visualization and to hide query source. |
@arikfr Hi I fixed that the chart is not displayed in embedded environment. |
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.
See comments below.
Also, this behavior change needs tests.
@@ -372,6 +372,12 @@ def get(self, query_id): | |||
'object_type': 'query', | |||
}) | |||
|
|||
if not self.current_user.has_permission('view_source'): | |||
result['query'] = '-- Query Source requires \'view_source\' permission.' |
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.
Why not just remove this field?
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.
Also this logic belongs in the QuerySerializer
.
if 'parameters' in result['options']: | ||
result['query'] += '\n' + '\n'.join( | ||
[] + list(map(lambda p: '-- {{{{{}}}}}'.format(p['name']), result['options']['parameters'])) | ||
) |
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.
What is this?
allow_executing_with_view_only_permissions = query.parameterized.is_safe | ||
|
||
if has_access(query, self.current_user, allow_executing_with_view_only_permissions): | ||
return run_query(query.parameterized, parameter_values, query.data_source, query_id, max_age) | ||
query_result = run_query(query.parameterized, parameter_values, query.data_source, query_id, max_age) | ||
if isinstance(query_result, tuple) and query_result[1] != 200: |
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.
As this is not the only place we return a QueryResult
instance, this behavior needs to be in a serializer. I don't think we created one for a QueryResult
yet and we still use a to_dict
method, but we can as part of this change.
Seems like this is abandoned. I'm closing this now, but you're welcome to re-open after addressing the comments. Thanks! |
Permission for viewing query source
Description
Now users who do not have
view_source
permission can view query source, so this permission does not work for the intent that is.In this commit, the users are not able to view query sources.
Related Tickets & Documents
#3212
Mobile & Desktop Screenshots/Recordings (if there are UI changes)