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.
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
Allow GET from non-admins on data source resource #4992
Allow GET from non-admins on data source resource #4992
Changes from all commits
8fb5bc7
76ca540
4958ec4
f4331f2
b2dcbb4
90248f2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It should still require some permission to view the data source details, because otherwise this API will be available when using the query API key...
Also, when the user doesn't have a
list_data_sources
permission it's very likely the admin wouldn't want them to see the details of the data source anyway.Let's keep this new implementation, but still require
list_data_sources
permission here for now (ideally we would use a new permission, but this is a bit of a hassle to add).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 suggested the same but late a bit 😅
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.
Having "list_data_sources" doesn't fix the problem :(
I had that in mind, that's why I put the "all" only when the user is an admin.
redash/redash/handlers/data_sources.py
Line 42 in 8fb5bc7
Basically we show exactly the same information we would show in the list resource if the user is not an admin.
I see your point anyway, just that if felt ok as the user would actually be "listing data sources". WDYT about only returning the
{ view_only: true/false }
when the user doesn't have the list 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.
4958ec4 with "returning the
{ view_only: true/false }
when the user doesn't have the list 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.
By details I meant even the name or type.