You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/04/01 17:03:56 UTC
[GitHub] [incubator-superset] graceguo-supercat opened a new pull request
#9438: revert #9329
graceguo-supercat opened a new pull request #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438
### CATEGORY
Choose one
- [x] Revert
### SUMMARY
Airbnb users can not use chart edit modal because of following error:
<img width="1300" alt="Screen Shot 2020-04-01 at 9 32 28 AM" src="https://user-images.githubusercontent.com/27990562/78164864-4f268280-73ff-11ea-8cc1-b3d110362b0c.png">
We reported this issue last week, but it is still in master branch so we have to revert it again in our release branch.
@dpgaspar If you have suggested fix, I can test it in airbnb environment.
@mistercrunch @robdiciuccio
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: #9329
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar edited a comment on issue #9438:
revert #9329
Posted by GitBox <gi...@apache.org>.
dpgaspar edited a comment on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607413835
I'm able to reproduce it the following way:
- Give users access to dashboards that contain charts they are able to access by having database access permission, and datasource permission.
- Low priv user Favorites the dashboard
- Remove the datasource access permission from the user/role.
- Users are not able to view these charts on the charts list view, but they can viz them on the dashboard
- Users use "Explore Chart" on the dashboard, and then click "Edit properties" and get HTTP 404.
This is caused by user's having access to viz charts by having the database access permission, but the chart filter does not include the database access filter, just schema, datasource or all datasources:
here: https://github.com/apache/incubator-superset/blob/master/superset/charts/filters.py
and here: https://github.com/apache/incubator-superset/blob/master/superset/views/chart/filters.py
Can you confirm this reasoning?
Possible path forward could be to add the database access to this filter, and assume this is the new security filter in-place for charts
Other path: could be the front end disables the edit properties if the backend returns 404
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on issue #9438: revert #9329
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607474929
I don't mean the dashboard edit, I mean accessing the chart from the dashboard and then access "Edit chart property". Can you confirm?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat closed pull request #9438:
revert #9329
Posted by GitBox <gi...@apache.org>.
graceguo-supercat closed pull request #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar edited a comment on issue #9438:
revert #9329
Posted by GitBox <gi...@apache.org>.
dpgaspar edited a comment on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607413835
I'm able to reproduce it the following way:
- Give users access to dashboards that contain charts they are able to access by having database access permission, and datasource permission.
- Low priv user Favorites the dashboard
- Remove the datasource access permission to the user.
- Users are not able to view these charts on the charts list view, but they can viz them on the dashboard
- Users use "Explore Chart" on the dashboard, and then click "Edit properties" and get HTTP 404.
This is caused by user's having access to viz charts by having the database access permission, but the chart filter does not include the database access filter, just schema, datasource or all datasources:
here: https://github.com/apache/incubator-superset/blob/master/superset/charts/filters.py
and here: https://github.com/apache/incubator-superset/blob/master/superset/views/chart/filters.py
Can you confirm this reasoning?
Possible path forward could be to add the database access to this filter, and assume this is the new security filter in-place for charts
Other path: could be the front end disable the edit properties if the backend returns 404
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat edited a comment on issue
#9438: revert #9329
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607461604
The error is from chart view: open `edit chart property` modal will see the Error message. (edit dashboard property works good no 404).
This error message is showing just for all charts.
@dpgaspar I feel your investigate is right, all users have `datasource_access` permission, but no `schema_access`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar edited a comment on issue #9438:
revert #9329
Posted by GitBox <gi...@apache.org>.
dpgaspar edited a comment on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607384313
Having an hard time to reproduce the issue. Are user's getting this when coming from dashboards? can user's view the charts on the chart list?
Note: HTTP 404, means that the chart does not exist or it's being filtered by the already in-place filter.
Following logic, so can't be sure, are you certain that the revert solves the issue?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on issue #9438: revert #9329
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607384313
Having an hard time to reproduce the issue. Are user's getting this when coming from dashboards? can user's view the charts on the chart list?
Note: HTTP 404, means that the chart does not exist or it's being filtered by the already in-place filter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on issue #9438:
revert #9329
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607461604
The error is from chart view: open edit chart property modal will see the Error message. (edit dashboard property works good no 404).
This error message is showing just for all charts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on issue #9438: revert #9329
Posted by GitBox <gi...@apache.org>.
suddjian commented on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607380879
Can you point to where this issue was reported, for context?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar edited a comment on issue #9438:
revert #9329
Posted by GitBox <gi...@apache.org>.
dpgaspar edited a comment on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607413835
I'm able to reproduce it the following way:
- Give users access to dashboards that contain charts they are able to access by having database access permission, published or favorite.
- Users are not able to view these charts on the charts list view, but they can viz them on the dashboard
- Users use "Explore Chart" on the dashboard, and then click "Edit properties" and get HTTP 404.
This is caused by user's having access to viz charts by having the database access permission, but the chart filter does not include the database access filter, just schema, datasource or all datasources:
here: https://github.com/apache/incubator-superset/blob/master/superset/charts/filters.py
and here: https://github.com/apache/incubator-superset/blob/master/superset/views/chart/filters.py
Can you confirm this reasoning?
Possible path forward could be to add the database access to this filter, and assume this is the new security filter in-place for charts
Other path: could be the front end disable the edit properties if the backend returns 404
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on issue #9438: revert #9329
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607505700
@graceguo-supercat sounds good, I'll be around for any further help on this you my need. Thks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on issue #9438:
revert #9329
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607498412
@dpgaspar I feel this issue may only happens in airbnb environment. So we will add configuration internally, but keep open source codebase as is.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on issue #9438: revert #9329
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607413835
I'm able to reproduce it the following way:
- Give users access to dashboards that contain charts they are able to access by having database access permission, published or favorite.
- Users are not able to view these charts on the charts list view, but they can viz them on the dashboard
- Users use "Explore Chart" on the dashboard, and then click "Edit properties" and get HTTP 404.
This is caused by user's having access to viz charts by having the database access permission, but the chart filter does not include the database access filter, just schema, datasource or all datasources:
here: https://github.com/apache/incubator-superset/blob/master/superset/charts/filters.py
and here: https://github.com/apache/incubator-superset/blob/master/superset/views/chart/filters.py
Can you confirm this reasoning?
Possible path forward could be to add the database access to this filter, and assume this is the new security filter in-place for charts
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar edited a comment on issue #9438:
revert #9329
Posted by GitBox <gi...@apache.org>.
dpgaspar edited a comment on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607474929
I don't mean the dashboard edit, I mean accessing the chart from the dashboard and then access "Edit chart property". Can you confirm?
`datasource_access` shoud be enough, but to be practical can you confirm the revert solves the issue?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat edited a comment on issue
#9438: revert #9329
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607461604
The error is from chart view: open `edit chart property` modal will see the Error message. (edit dashboard property works good no 404).
This error message is showing just for all charts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar edited a comment on issue #9438:
revert #9329
Posted by GitBox <gi...@apache.org>.
dpgaspar edited a comment on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607413835
I'm able to reproduce it the following way:
- Give users access to dashboards that contain charts they are able to access by having database access permission, and datasource permission.
- Low priv user Favorites the dashboard
- Remove the datasource access permission from the user/role.
- Users are not able to view these charts on the charts list view, but they can viz them on the dashboard
- Users use "Explore Chart" on the dashboard, and then click "Edit properties" and get HTTP 404.
This is caused by user's having access to viz charts by having the database access permission, but the chart filter does not include the database access filter, just schema, datasource or all datasources:
here: https://github.com/apache/incubator-superset/blob/master/superset/charts/filters.py
and here: https://github.com/apache/incubator-superset/blob/master/superset/views/chart/filters.py
Can you confirm this reasoning?
Possible path forward could be to add the database access to this filter, and assume this is the new security filter in-place for charts
Other path: could be the front end disable the edit properties if the backend returns 404
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar edited a comment on issue #9438:
revert #9329
Posted by GitBox <gi...@apache.org>.
dpgaspar edited a comment on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607384313
Having an hard time to reproduce the issue. Are user's getting this when coming from dashboards? can user's view the charts on the chart list?
Note: HTTP 404, means that the chart does not exist or it's being filtered by the already in-place filter.
Do user's have datasource access or schema access or all datasource access to the datasource related to the chart?
Following logic, can't be sure, are you certain that the revert solves the issue?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian edited a comment on issue #9438:
revert #9329
Posted by GitBox <gi...@apache.org>.
suddjian edited a comment on issue #9438: revert #9329
URL: https://github.com/apache/incubator-superset/pull/9438#issuecomment-607380879
Can you point to where this issue was reported, for context?
edit: nevermind I see, the discussion was on the original PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org