Allow GET from non-admins on data source resource #4992
Conversation
gabrieldutra
changed the title
There was a problem hiding this comment.
See my comment about the issue with having this API call without permission.
To address the UI issue: let's address the case when you can't call this API and if the data source details aren't available just not show the data source label and assume user has view only access.
| @@ -34,12 +34,15 @@ def get(self): | |||
|
|
|||
|
|
|||
| class DataSourceResource(BaseResource): | |||
| @require_admin | |||
| def get(self, data_source_id): | |||
There was a problem hiding this comment.
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.
I suggested the same but late a bit 😅
There was a problem hiding this comment.
Having "list_data_sources" doesn't fix the problem :(
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
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.
4958ec4 with "returning the { view_only: true/false } when the user doesn't have the list permission"
There was a problem hiding this comment.
I had that in mind, that's why I put the "all" only when the user is an admin.
By details I meant even the name or type.
redash/handlers/data_sources.py
Outdated
| data_source.to_dict(all=self.current_user.has_permission("admin")) | ||
| if self.current_user.has_permission("list_data_sources") | ||
| else {} | ||
| ) |
There was a problem hiding this comment.
Please correct me if I misunderstand something:
- for admins it will serialize all fields +
view_onlyflags - for those who can
list_data_sourcesit will serialize limited set of fields +view_onlyflags - for all others it will return just
view_onlyflags
If so - it looks good. Maybe just add some comment to explain this logic, or split this one-liner to make it a bit easier to understand.
There was a problem hiding this comment.
@kravets-levko correct, sorry if the code is a bit weird, for now I only made de simplest changes to what was there 😅
If @arikfr agrees with this flow I'll happily try to simplify the code (or add the explaining comment) + add tests to make sure it keeps working as it should.
There was a problem hiding this comment.
I guess this flow is fine, but we still need the relevant UI changes (hide data source if we don't have its name/type).
There was a problem hiding this comment.
I've added tests and updated the code.
One extra thing to @kravets-levko flow that was not mentioned: The user has to either be an admin or have at least view permissions to follow to that flow, otherwise a 403 is returned.
What type of PR is this? (check all applicable)
Description
This allows requests from non-admin users that have at least "view_only" permissions on the DS. Also adds the
view_onlyinformation and limit the result information when the user is non-admin.Reverts #4927 as with this the specific route can be used.
Reasoning: it removes the need for the "list_data_sources" permission when accessing the View Query page.
Related Tickets & Documents
--
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
--