From 63f0caa1906efa05403433195ca2e5776a398848 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 25 Aug 2020 18:07:35 +0100 Subject: [PATCH 1/9] fix: change public role like gamma procedure --- superset/config.py | 4 +- superset/connectors/sqla/models.py | 2 +- superset/security/manager.py | 78 +++++++++++++++++++++--- tests/security_tests.py | 35 ++++++++++- tests/superset_test_config.py | 2 +- tests/superset_test_config_thumbnails.py | 2 +- 6 files changed, 110 insertions(+), 13 deletions(-) diff --git a/superset/config.py b/superset/config.py index db5aa6e08549a..40844855c263d 100644 --- a/superset/config.py +++ b/superset/config.py @@ -260,10 +260,10 @@ def _try_json_readsha( # pylint: disable=unused-argument # --------------------------------------------------- # Roles config # --------------------------------------------------- -# Grant public role the same set of permissions as for the GAMMA role. +# Grant public role the same set of permissions as for a selected builtin role. # This is useful if one wants to enable anonymous users to view # dashboards. Explicit grant on specific datasets is still required. -PUBLIC_ROLE_LIKE_GAMMA = False +PUBLIC_ROLE_LIKE = "Gamma" # --------------------------------------------------- # Babel config for translations diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index e4065aeeea15d..601a69ddb63dd 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -585,7 +585,7 @@ def get_schema_perm(self) -> Optional[str]: return security_manager.get_schema_perm(self.database, self.schema) def get_perm(self) -> str: - return ("[{obj.database}].[{obj.table_name}]" "(id:{obj.id})").format(obj=self) + return f"[{self.database}].[{self.table_name}](id:{self.id})" @property def name(self) -> str: diff --git a/superset/security/manager.py b/superset/security/manager.py index f939d6d258eb8..b105574cde66b 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -18,6 +18,8 @@ """A set of constants and methods to manage permissions and security""" import logging from typing import Any, Callable, cast, List, Optional, Set, Tuple, TYPE_CHECKING, Union +import re + from flask import current_app, g from flask_appbuilder import Model @@ -172,6 +174,15 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods ACCESSIBLE_PERMS = {"can_userinfo"} + data_access_permissions = ( + "database_access", + "schema_access", + "datasource_access", + "all_datasource_access", + "all_database_access", + "all_query_access", + ) + def get_schema_perm( # pylint: disable=no-self-use self, database: Union["Database", str], schema: Optional[str] = None ) -> Optional[str]: @@ -609,8 +620,8 @@ def sync_role_definitions(self) -> None: self.set_role("granter", self._is_granter_pvm) self.set_role("sql_lab", self._is_sql_lab_pvm) - if conf.get("PUBLIC_ROLE_LIKE_GAMMA", False): - self.set_role("Public", self._is_gamma_pvm) + if conf["PUBLIC_ROLE_LIKE"]: + self.copy_role(conf["PUBLIC_ROLE_LIKE"], self.auth_role_public, merge=True) self.create_missing_perms() @@ -618,6 +629,58 @@ def sync_role_definitions(self) -> None: self.get_session.commit() self.clean_perms() + def _get_pvms_from_builtin_role(self, role_name: str) -> List[PermissionView]: + """ + Gets a list of model PermissionView permissions infered from a builtin role + definition + """ + role_from_permissions_names = self.builtin_roles.get(role_name, []) + all_pvms = self.get_session.query(PermissionView).all() + role_from_permissions = [] + for pvm_regex in role_from_permissions_names: + view_name_regex = pvm_regex[0] + permission_name_regex = pvm_regex[1] + for pvm in all_pvms: + if re.match(view_name_regex, pvm.view_menu.name) and re.match( + permission_name_regex, pvm.permission.name + ): + if pvm not in role_from_permissions: + role_from_permissions.append(pvm) + return role_from_permissions + + def copy_role( + self, role_from_name: str, role_to_name: str, merge: bool = True + ) -> None: + """ + Copies permissions from a role to another. + + Note: Supports regex defined builtin roles + + :param role_from_name: The FAB role name from where the permissions are taken + :param role_to_name: The FAB role name from where the permissions are copied to + :param merge: If merge is true, keep data access permissions + if they already exist on the target role + """ + + logger.info("Copy/Merge %s to %s", role_from_name, role_to_name) + # If it's a builtin role extract permissions from it + if role_from_name in self.builtin_roles: + role_from_permissions = self._get_pvms_from_builtin_role(role_from_name) + else: + role_from_permissions = list(self.find_role(role_from_name).permissions) + role_to = self.add_role(role_to_name) + # If merge, recover existing data access permissions + if merge: + for permission_view in role_to.permissions: + if ( + permission_view not in role_from_permissions + and permission_view.permission.name in self.data_access_permissions + ): + role_from_permissions.append(permission_view) + role_to.permissions = role_from_permissions + self.get_session.merge(role_to) + self.get_session.commit() + def set_role( self, role_name: str, pvm_check: Callable[[PermissionView], bool] ) -> None: @@ -629,14 +692,15 @@ def set_role( """ logger.info("Syncing %s perms", role_name) - sesh = self.get_session - pvms = sesh.query(PermissionView).all() + pvms = self.get_session.query(PermissionView).all() pvms = [p for p in pvms if p.permission and p.view_menu] role = self.add_role(role_name) - role_pvms = [p for p in pvms if pvm_check(p)] + role_pvms = [ + permission_view for permission_view in pvms if pvm_check(permission_view) + ] role.permissions = role_pvms - sesh.merge(role) - sesh.commit() + self.get_session.merge(role) + self.get_session.commit() def _is_admin_only(self, pvm: Model) -> bool: """ diff --git a/tests/security_tests.py b/tests/security_tests.py index 60d20fde09dc7..876eef4237f44 100644 --- a/tests/security_tests.py +++ b/tests/security_tests.py @@ -531,6 +531,33 @@ def test_gamma_user_schema_access_to_charts(self): ) # wb_health_population slice, has access self.assertNotIn("Girl Name Cloud", data) # birth_names slice, no access + def test_public_sync_role_data_perms(self): + """ + Security: Tests if the sync role method preserves data access permissions + if they already exist on a public role. + Also check that non data access permissions are removed + """ + table = db.session.query(SqlaTable).filter_by(table_name="birth_names").one() + self.grant_public_access_to_table(table) + public_role = security_manager.get_public_role() + unwanted_pvm = security_manager.find_permission_view_menu( + "menu_access", "Security" + ) + public_role.permissions.append(unwanted_pvm) + db.session.commit() + + security_manager.sync_role_definitions() + public_role = security_manager.get_public_role() + public_role_resource_names = [ + permission.view_menu.name for permission in public_role.permissions + ] + + assert table.get_perm() in public_role_resource_names + assert "Security" not in public_role_resource_names + + # Cleanup + self.revoke_public_access_to_table(table) + def test_sqllab_gamma_user_schema_access_to_sqllab(self): session = db.session @@ -724,7 +751,10 @@ def test_is_gamma_pvm(self): def test_gamma_permissions_basic(self): self.assert_can_gamma(get_perm_tuples("Gamma")) - self.assert_cannot_alpha(get_perm_tuples("Alpha")) + self.assert_cannot_alpha(get_perm_tuples("Gamma")) + + def test_public_permissions_basic(self): + self.assert_can_gamma(get_perm_tuples("Public")) @unittest.skipUnless( SupersetTestCase.is_module_installed("pydruid"), "pydruid not installed" @@ -788,6 +818,9 @@ def assert_can_all(view_menu): assert_can_all("SliceModelView") assert_can_all("DashboardModelView") + assert_cannot_write("UserDBModelView") + assert_cannot_write("RoleModelView") + self.assertIn(("can_add_slices", "Superset"), gamma_perm_set) self.assertIn(("can_copy_dash", "Superset"), gamma_perm_set) self.assertIn(("can_created_dashboards", "Superset"), gamma_perm_set) diff --git a/tests/superset_test_config.py b/tests/superset_test_config.py index c88c5847bbb75..6390ea90a891f 100644 --- a/tests/superset_test_config.py +++ b/tests/superset_test_config.py @@ -53,7 +53,7 @@ def GET_FEATURE_FLAGS_FUNC(ff): TESTING = True WTF_CSRF_ENABLED = False -PUBLIC_ROLE_LIKE_GAMMA = True +PUBLIC_ROLE_LIKE = "Gamma" AUTH_ROLE_PUBLIC = "Public" EMAIL_NOTIFICATIONS = False ENABLE_ROW_LEVEL_SECURITY = True diff --git a/tests/superset_test_config_thumbnails.py b/tests/superset_test_config_thumbnails.py index 3b97604e9ae03..af30af4e4d61f 100644 --- a/tests/superset_test_config_thumbnails.py +++ b/tests/superset_test_config_thumbnails.py @@ -50,7 +50,7 @@ def GET_FEATURE_FLAGS_FUNC(ff): TESTING = True WTF_CSRF_ENABLED = False -PUBLIC_ROLE_LIKE_GAMMA = True +PUBLIC_ROLE_LIKE = "Gamma" AUTH_ROLE_PUBLIC = "Public" EMAIL_NOTIFICATIONS = False From d4667e486ac23b2b628055fe58be46a57e5e680e Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 25 Aug 2020 18:40:06 +0100 Subject: [PATCH 2/9] lint and updating UPDATING with breaking change --- UPDATING.md | 2 ++ superset/security/manager.py | 3 +-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 054afafe2b890..4cc4c5a62ded4 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -23,6 +23,8 @@ assists people when migrating to a new version. ## Next +* [10674](https://github.com/apache/incubator-superset/pull/10674): Breaking change: PUBLIC_ROLE_LIKE_GAMMA was removed is favour of the new PUBLIC_ROLE_LIKE so it can set it to whatever role you want. + * [10590](https://github.com/apache/incubator-superset/pull/10590): Breaking change: this PR will convert iframe chart into dashboard markdown component, and remove all `iframe`, `separator`, and `markup` slices (and support) from Superset. If you have important data in those slices, please backup manually. * [10562](https://github.com/apache/incubator-superset/pull/10562): EMAIL_REPORTS_WEBDRIVER is deprecated use WEBDRIVER_TYPE instead. diff --git a/superset/security/manager.py b/superset/security/manager.py index b105574cde66b..fd15c04020e4d 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -17,9 +17,8 @@ # pylint: disable=too-few-public-methods """A set of constants and methods to manage permissions and security""" import logging -from typing import Any, Callable, cast, List, Optional, Set, Tuple, TYPE_CHECKING, Union import re - +from typing import Any, Callable, cast, List, Optional, Set, Tuple, TYPE_CHECKING, Union from flask import current_app, g from flask_appbuilder import Model From b3a78a382c170987f02d73f922a4ed7ef2da1f9f Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 25 Aug 2020 18:41:11 +0100 Subject: [PATCH 3/9] fix updating text --- UPDATING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/UPDATING.md b/UPDATING.md index 4cc4c5a62ded4..47a5dd396e1ec 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -23,7 +23,7 @@ assists people when migrating to a new version. ## Next -* [10674](https://github.com/apache/incubator-superset/pull/10674): Breaking change: PUBLIC_ROLE_LIKE_GAMMA was removed is favour of the new PUBLIC_ROLE_LIKE so it can set it to whatever role you want. +* [10674](https://github.com/apache/incubator-superset/pull/10674): Breaking change: PUBLIC_ROLE_LIKE_GAMMA was removed is favour of the new PUBLIC_ROLE_LIKE so it can be set it whatever role you want. * [10590](https://github.com/apache/incubator-superset/pull/10590): Breaking change: this PR will convert iframe chart into dashboard markdown component, and remove all `iframe`, `separator`, and `markup` slices (and support) from Superset. If you have important data in those slices, please backup manually. From b5cbe5926e5bd3ad872df14188b008f504ca5948 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 26 Aug 2020 11:48:41 +0100 Subject: [PATCH 4/9] add test and support PUBLIC_ROLE_LIKE_GAMMA --- superset/security/manager.py | 7 +++++++ tests/security_tests.py | 20 +++++++++++++++++++- tests/superset_test_config.py | 3 +++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index fd15c04020e4d..f5376d8f6d215 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -619,8 +619,15 @@ def sync_role_definitions(self) -> None: self.set_role("granter", self._is_granter_pvm) self.set_role("sql_lab", self._is_sql_lab_pvm) + # Configure public role if conf["PUBLIC_ROLE_LIKE"]: self.copy_role(conf["PUBLIC_ROLE_LIKE"], self.auth_role_public, merge=True) + if conf.get("PUBLIC_ROLE_LIKE_GAMMA", False): + logger.warning( + "The config `PUBLIC_ROLE_LIKE_GAMMA` is deprecated and will be removed " + "in Superset 1.0. Please use `PUBLIC_ROLE_LIKE ` instead." + ) + self.copy_role("Gamma", self.auth_role_public, merge=True) self.create_missing_perms() diff --git a/tests/security_tests.py b/tests/security_tests.py index 876eef4237f44..c98b12aadf4d9 100644 --- a/tests/security_tests.py +++ b/tests/security_tests.py @@ -20,7 +20,7 @@ from unittest.mock import Mock, patch import prison -from flask import g +from flask import current_app, g import tests.test_app from superset import app, appbuilder, db, security_manager, viz @@ -558,6 +558,24 @@ def test_public_sync_role_data_perms(self): # Cleanup self.revoke_public_access_to_table(table) + def test_public_sync_role_builtin_perms(self): + """ + Security: Tests public role creation based on a builtin role + """ + current_app.config["PUBLIC_ROLE_LIKE"] = "TestRole" + + security_manager.sync_role_definitions() + public_role = security_manager.get_public_role() + public_role_resource_names = [ + [permission.view_menu.name, permission.permission.name] + for permission in public_role.permissions + ] + assert current_app.config["FAB_ROLES"]["TestRole"] == public_role_resource_names + + # Cleanup + current_app.config["PUBLIC_ROLE_LIKE"] = "Gamma" + security_manager.sync_role_definitions() + def test_sqllab_gamma_user_schema_access_to_sqllab(self): session = db.session diff --git a/tests/superset_test_config.py b/tests/superset_test_config.py index 6390ea90a891f..b62f67822fe6c 100644 --- a/tests/superset_test_config.py +++ b/tests/superset_test_config.py @@ -53,6 +53,9 @@ def GET_FEATURE_FLAGS_FUNC(ff): TESTING = True WTF_CSRF_ENABLED = False + +FAB_ROLES = {"TestRole": [["Security", "menu_access"], ["List Users", "menu_access"]]} + PUBLIC_ROLE_LIKE = "Gamma" AUTH_ROLE_PUBLIC = "Public" EMAIL_NOTIFICATIONS = False From 569f2ec2e136181447f8e3c82015c57e0a62e1c1 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 26 Aug 2020 13:30:41 +0100 Subject: [PATCH 5/9] fix, cleanup tests --- tests/dashboard_tests.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/dashboard_tests.py b/tests/dashboard_tests.py index f05c98082bb67..27926e4f6a350 100644 --- a/tests/dashboard_tests.py +++ b/tests/dashboard_tests.py @@ -320,6 +320,9 @@ def test_public_user_dashboard_access(self): resp = self.get_resp("/api/v1/dashboard/") self.assertNotIn("/superset/dashboard/world_health/", resp) + # Cleanup + self.revoke_public_access_to_table(table) + def test_dashboard_with_created_by_can_be_accessed_by_public_users(self): self.logout() table = db.session.query(SqlaTable).filter_by(table_name="birth_names").one() @@ -332,6 +335,8 @@ def test_dashboard_with_created_by_can_be_accessed_by_public_users(self): db.session.commit() assert "Births" in self.get_resp("/superset/dashboard/births/") + # Cleanup + self.revoke_public_access_to_table(table) def test_only_owners_can_save(self): dash = db.session.query(Dashboard).filter_by(slug="births").first() @@ -400,6 +405,9 @@ def test_users_can_view_published_dashboard(self): self.assertNotIn(f"/superset/dashboard/{hidden_dash_slug}/", resp) self.assertIn(f"/superset/dashboard/{published_dash_slug}/", resp) + # Cleanup + self.revoke_public_access_to_table(table) + def test_users_can_view_own_dashboard(self): user = security_manager.find_user("gamma") my_dash_slug = f"my_dash_{random()}" From ad11aaa5582dd7fc62a3da6fcb65ada4990630e4 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 26 Aug 2020 13:42:48 +0100 Subject: [PATCH 6/9] fix, new test --- tests/security_tests.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/security_tests.py b/tests/security_tests.py index c98b12aadf4d9..fa8782295d98d 100644 --- a/tests/security_tests.py +++ b/tests/security_tests.py @@ -570,7 +570,8 @@ def test_public_sync_role_builtin_perms(self): [permission.view_menu.name, permission.permission.name] for permission in public_role.permissions ] - assert current_app.config["FAB_ROLES"]["TestRole"] == public_role_resource_names + for pvm in current_app.config["FAB_ROLES"]["TestRole"]: + assert pvm in public_role_resource_names # Cleanup current_app.config["PUBLIC_ROLE_LIKE"] = "Gamma" From f507cc34826c2a322f654ae8f9bdf1780ae42073 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 27 Aug 2020 10:22:27 +0100 Subject: [PATCH 7/9] fix, public default --- superset/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/config.py b/superset/config.py index 40844855c263d..8587616faaabb 100644 --- a/superset/config.py +++ b/superset/config.py @@ -263,7 +263,7 @@ def _try_json_readsha( # pylint: disable=unused-argument # Grant public role the same set of permissions as for a selected builtin role. # This is useful if one wants to enable anonymous users to view # dashboards. Explicit grant on specific datasets is still required. -PUBLIC_ROLE_LIKE = "Gamma" +PUBLIC_ROLE_LIKE = None # --------------------------------------------------- # Babel config for translations From 462aa95c3a02b5f67269694c26556c389f8be16d Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Thu, 27 Aug 2020 10:24:20 +0100 Subject: [PATCH 8/9] Update superset/config.py Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> --- superset/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/config.py b/superset/config.py index 8587616faaabb..11b95c91999f9 100644 --- a/superset/config.py +++ b/superset/config.py @@ -263,7 +263,7 @@ def _try_json_readsha( # pylint: disable=unused-argument # Grant public role the same set of permissions as for a selected builtin role. # This is useful if one wants to enable anonymous users to view # dashboards. Explicit grant on specific datasets is still required. -PUBLIC_ROLE_LIKE = None +PUBLIC_ROLE_LIKE: Optional[str] = None # --------------------------------------------------- # Babel config for translations From e238b527d78a25c8f77e41eb66f0ce6ec11c6d69 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 27 Aug 2020 17:09:00 +0100 Subject: [PATCH 9/9] add simple public welcome page --- .../templates/superset/public_welcome.html | 23 +++++++++++++++++++ superset/views/core.py | 2 ++ 2 files changed, 25 insertions(+) create mode 100644 superset/templates/superset/public_welcome.html diff --git a/superset/templates/superset/public_welcome.html b/superset/templates/superset/public_welcome.html new file mode 100644 index 0000000000000..0d821512eb782 --- /dev/null +++ b/superset/templates/superset/public_welcome.html @@ -0,0 +1,23 @@ +{# +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +#} +{% extends "appbuilder/base.html" %} + +{% block content %} +

Welcome to Apache Superset

+{% endblock %} diff --git a/superset/views/core.py b/superset/views/core.py index d6ee6907fe7c7..243b9c63ef37f 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -2523,6 +2523,8 @@ def show_traceback(self) -> FlaskResponse: # pylint: disable=no-self-use def welcome(self) -> FlaskResponse: """Personalized welcome page""" if not g.user or not g.user.get_id(): + if conf.get("PUBLIC_ROLE_LIKE_GAMMA", False) or conf["PUBLIC_ROLE_LIKE"]: + return self.render_template("superset/public_welcome.html") return redirect(appbuilder.get_url_for_login) welcome_dashboard_id = (