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/07/03 08:36:52 UTC
[GitHub] [incubator-superset] dpgaspar opened a new pull request #10241: fix(permissions): alpha role is inconsistent
dpgaspar opened a new pull request #10241:
URL: https://github.com/apache/incubator-superset/pull/10241
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 pull request #10241: fix(permissions): alpha role is inconsistent
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on pull request #10241:
URL: https://github.com/apache/incubator-superset/pull/10241#issuecomment-663631744
Good points, I'll add both. I think we don't have a doc describing what roles are, I'll create one on the docs if it does not exist
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar merged pull request #10241: fix(permissions): alpha role has all full features
Posted by GitBox <gi...@apache.org>.
dpgaspar merged pull request #10241:
URL: https://github.com/apache/incubator-superset/pull/10241
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 pull request #10241: fix(permissions): alpha role is inconsistent
Posted by GitBox <gi...@apache.org>.
dpgaspar edited a comment on pull request #10241:
URL: https://github.com/apache/incubator-superset/pull/10241#issuecomment-653643629
That was my objective, since by removing the "Manage" menu item, it's kind of implied the user does not have access to the `AnnotationsLayerModelView`, `CssTemplateModelView`, `QueryView` but he does.
But seems like we are using the old API for the annotations. Would be awesome to instead of removing we would add the "Manager" menu item to Alpha, that would make it more consistent, and inline with the idea that an Alpha user has access to all features except security
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on pull request #10241: fix(permissions): alpha role is inconsistent
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10241:
URL: https://github.com/apache/incubator-superset/pull/10241#issuecomment-653683575
I think users still access the API in some other ways though (dashboard / explore / SQL Lab), we'd have to check/confirm whether they hit the modelview endpoints or not.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on pull request #10241: fix(permissions): alpha role is inconsistent
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10241:
URL: https://github.com/apache/incubator-superset/pull/10241#issuecomment-653584272
By adding the MVs in `ADMIN_ONLY_VIEW_MENUS` you're removing access to `Alpha` and `Gamma`. I don't think that's what we want
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 pull request #10241: fix(permissions): alpha role is inconsistent
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on pull request #10241:
URL: https://github.com/apache/incubator-superset/pull/10241#issuecomment-653643629
That was my objective, since by removing the "Manage" menu item, it's kind of implied the user does not have access to the `AnnotationsLayerModelView, `CssTemplateModelView`, `QueryView` but he does.
But seems like we are using the old API for the annotations. Would be awesome to instead of removing we would add the "Manager" menu item to Alpha, that would make it more consistent, and inline with the idea that an Alpha user has access to all features except security
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on pull request #10241: fix(permissions): alpha role is inconsistent
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10241:
URL: https://github.com/apache/incubator-superset/pull/10241#issuecomment-663630691
+1 on a unit test, do we have a doc somewhere that give an idea of what `Gamma` is? Also should have a notice pointing to this PR in UPDATING.md
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull request #10241: fix(permissions): alpha role is inconsistent
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10241:
URL: https://github.com/apache/incubator-superset/pull/10241#discussion_r456632603
##########
File path: superset/security/manager.py
##########
@@ -139,7 +138,13 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
"RowLevelSecurityFiltersModelView",
} | USER_MODEL_VIEWS
- ALPHA_ONLY_VIEW_MENUS = {"Upload a CSV"}
+ ALPHA_ONLY_VIEW_MENUS = {
Review comment:
we do the same in the permission sync script.
1 suggestion, maybe add a unit test
example:
```
def test_sync_creator_role(setup):
# type: (Any) -> None
session = db.create_scoped_session()
sync_creator_role(session)
creator_role = session.query(ab_models.Role).filter_by(name="Creator").one()
assert set([p.view_menu.name for p in creator_role.permissions]) == {
"TableModelView",
"TableColumnInlineView",
"SqlMetricInlineView",
# TODO(bogdankyryliuk): share the same config with production.
# "DashboardEmailScheduleView",
# "SliceEmailScheduleView",
"Manage",
"AnnotationModelView",
"AnnotationLayerModelView",
"all_query_access",
"CsvToDatabaseView",
"Datasource",
"Upload a CSV",
}
assert set([p.permission.name for p in creator_role.permissions]) == {
"all_query_access",
"can_add",
"can_save",
"can_delete",
"can_edit",
"can_external_metadata",
"can_get",
"can_list",
"can_mulexport",
"can_show",
"can_this_form_get",
"can_this_form_post",
"muldelete",
"refresh",
"yaml_export",
"menu_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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull request #10241: fix(permissions): alpha role is inconsistent
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10241:
URL: https://github.com/apache/incubator-superset/pull/10241#discussion_r456632603
##########
File path: superset/security/manager.py
##########
@@ -139,7 +138,13 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
"RowLevelSecurityFiltersModelView",
} | USER_MODEL_VIEWS
- ALPHA_ONLY_VIEW_MENUS = {"Upload a CSV"}
+ ALPHA_ONLY_VIEW_MENUS = {
Review comment:
we do the same in the permission sync script.
1 suggestion, maybe add a unit test
example:
```
def test_sync_creator_role(setup):
# type: (Any) -> None
session = db.create_scoped_session()
sync_creator_role(session)
creator_role = session.query(ab_models.Role).filter_by(name="Creator").one()
assert set([p.view_menu.name for p in creator_role.permissions]) == {
"TableModelView",
"TableColumnInlineView",
"SqlMetricInlineView",
# TODO(bogdankyryliuk): share the same config with production.
# "DashboardEmailScheduleView",
# "SliceEmailScheduleView",
"Manage",
"AnnotationModelView",
"AnnotationLayerModelView",
"all_query_access",
"CsvToDatabaseView",
"Datasource",
"Upload a CSV",
}
assert set([p.permission.name for p in creator_role.permissions]) == {
"all_query_access",
"can_add",
"can_save",
"can_delete",
"can_edit",
"can_external_metadata",
"can_get",
"can_list",
"can_mulexport",
"can_show",
"can_this_form_get",
"can_this_form_post",
"muldelete",
"refresh",
"yaml_export",
"menu_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
---------------------------------------------------------------------
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 pull request #10241: fix(permissions): alpha role is inconsistent
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on pull request #10241:
URL: https://github.com/apache/incubator-superset/pull/10241#issuecomment-660136664
Max, I've reverted the direction here. I'm adding the Manage menu and it's subitems to the alpha role. What do you think?
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 pull request #10241: fix(permissions): alpha role is inconsistent
Posted by GitBox <gi...@apache.org>.
dpgaspar edited a comment on pull request #10241:
URL: https://github.com/apache/incubator-superset/pull/10241#issuecomment-660136664
@mistercrunch , I've reverted the direction here. I'm adding the Manage menu and it's subitems to the alpha role. What do you think?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org