Skip to content

fix: SQLLab role permissions #14372

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.

Sign up for GitHub

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

Merged
dpgaspar merged 6 commits into from
Apr 29, 2021
Merged

fix: SQLLab role permissions #14372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2c203fb
fix: SQLLab role permissions
dpgaspar Apr 27, 2021
7d5217a
add missing perm
dpgaspar Apr 27, 2021
68293ac
fix tests
dpgaspar Apr 27, 2021
2ab520b
fix security test
dpgaspar Apr 27, 2021
f131047
fix security test
dpgaspar Apr 27, 2021
bf9c45d
fix tests
dpgaspar Apr 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions superset/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ class RouteMethod: # pylint: disable=too-few-public-methods
"data_from_cache": "read",
"get_charts": "read",
"get_datasets": "read",
"function_names": "read",
"available": "read",
}

EXTRA_FORM_DATA_APPEND_KEYS = {
Expand Down
33 changes: 15 additions & 18 deletions superset/security/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,20 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods

ACCESSIBLE_PERMS = {"can_userinfo", "resetmypassword"}

SQLLAB_PERMISSION_VIEWS = {
("can_csv", "Superset"),
("can_read", "SavedQuery"),
("can_read", "Database"),
("can_sql_json", "Superset"),
("can_sqllab_viz", "Superset"),
("can_sqllab_table_viz", "Superset"),
("can_sqllab", "Superset"),
("menu_access", "SQL Lab"),
("menu_access", "SQL Editor"),
("menu_access", "Saved Queries"),
("menu_access", "Query Search"),
}

data_access_permissions = (
"database_access",
"schema_access",
Expand Down Expand Up @@ -820,24 +834,7 @@ def _is_sql_lab_pvm(self, pvm: PermissionView) -> bool:
:param pvm: The FAB permission/view
:returns: Whether the FAB object is SQL Lab related
"""

return (
pvm.view_menu.name
in {"SQL Lab", "SQL Editor", "Query Search", "Saved Queries"}
or pvm.permission.name
in {
"can_sql_json",
"can_csv",
"can_search_queries",
"can_sqllab_viz",
"can_sqllab_table_viz",
"can_sqllab",
}
or (
pvm.view_menu.name in self.USER_MODEL_VIEWS
and pvm.permission.name == "can_list"
)
)
return (pvm.permission.name, pvm.view_menu.name) in self.SQLLAB_PERMISSION_VIEWS
Copy link
Member

@willbarrett willbarrett Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that this removes self.USER_MODEL_VIEWS can_list permission - am I misreading?

Copy link
Member Author

@dpgaspar dpgaspar Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not, I could not find any valid reason for this permission. My guess is that it's a left over from the old Query History view, that view was populating a user dropdown list.

We now use: /api/v1/query/related/user


def _is_granter_pvm( # pylint: disable=no-self-use
self, pvm: PermissionView
Expand Down
4 changes: 1 addition & 3 deletions tests/databases/api_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,9 +614,7 @@ def test_info_security_database(self):
assert rv.status_code == 200
assert "can_read" in data["permissions"]
assert "can_write" in data["permissions"]
assert "can_function_names" in data["permissions"]
assert "can_available" in data["permissions"]
assert len(data["permissions"]) == 4
assert len(data["permissions"]) == 2

def test_get_invalid_database_table_metadata(self):
"""
Expand Down
13 changes: 11 additions & 2 deletions tests/security_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -832,9 +832,18 @@ def test_admin_permissions(self):

def test_sql_lab_permissions(self):
sql_lab_set = get_perm_tuples("sql_lab")
self.assertIn(("can_sql_json", "Superset"), sql_lab_set)
self.assertIn(("can_csv", "Superset"), sql_lab_set)
self.assertIn(("can_search_queries", "Superset"), sql_lab_set)
self.assertIn(("can_read", "Database"), sql_lab_set)
self.assertIn(("can_read", "SavedQuery"), sql_lab_set)
self.assertIn(("can_sql_json", "Superset"), sql_lab_set)
self.assertIn(("can_sqllab_viz", "Superset"), sql_lab_set)
self.assertIn(("can_sqllab_table_viz", "Superset"), sql_lab_set)
self.assertIn(("can_sqllab", "Superset"), sql_lab_set)

self.assertIn(("menu_access", "SQL Lab"), sql_lab_set)
self.assertIn(("menu_access", "SQL Editor"), sql_lab_set)
self.assertIn(("menu_access", "Saved Queries"), sql_lab_set)
self.assertIn(("menu_access", "Query Search"), sql_lab_set)

self.assert_cannot_alpha(sql_lab_set)

Expand Down