fix: change public role like gamma procedure (apache#10674)

* fix: change public role like gamma procedure * lint and updating UPDATING with breaking change * fix updating text * add test and support PUBLIC_ROLE_LIKE_GAMMA * fix, cleanup tests * fix, new test * fix, public default * Update superset/config.py Co-authored-by: Ville Brofeldt <[email protected]> * add simple public welcome page Co-authored-by: Ville Brofeldt <[email protected]>
preset-io · Sep 10, 2020 · 45d4da2 · 45d4da2
1 parent 75e9c49
commit 45d4da2
Show file tree

Hide file tree

Showing 10 changed files with 173 additions and 13 deletions.
diff --git 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 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.
 
 * [10562](https://github.com/apache/incubator-superset/pull/10562): EMAIL_REPORTS_WEBDRIVER is deprecated use WEBDRIVER_TYPE instead.

diff --git 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: Optional[str] = None
 
 # ---------------------------------------------------
 # Babel config for translations

diff --git 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
@@ -17,6 +17,7 @@
 # pylint: disable=too-few-public-methods
 """A set of constants and methods to manage permissions and security"""
 import logging
+import re
 from typing import Any, Callable, cast, List, Optional, Set, Tuple, TYPE_CHECKING, Union
 
 from flask import current_app, g
@@ -172,6 +173,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]:
@@ -621,15 +631,74 @@ 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):
-            self.set_role("Public", self._is_gamma_pvm)
+            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()
 
         # commit role and view menu updates
         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:
@@ -641,14 +710,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/superset/templates/superset/public_welcome.html 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 %}
+<h2><center>Welcome to Apache Superset</center></h2>
+{% endblock %}
diff --git 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 = (

diff --git 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()}"

diff --git 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
@@ -531,6 +531,52 @@ 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_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
+        ]
+        for pvm in current_app.config["FAB_ROLES"]["TestRole"]:
+            assert pvm in 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
 
@@ -724,7 +770,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 +837,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
@@ -53,7 +53,10 @@ def GET_FEATURE_FLAGS_FUNC(ff):
 
 TESTING = True
 WTF_CSRF_ENABLED = False
-PUBLIC_ROLE_LIKE_GAMMA = True
+
+FAB_ROLES = {"TestRole": [["Security", "menu_access"], ["List Users", "menu_access"]]}
+
+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
@@ -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