You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by vi...@apache.org on 2020/09/11 12:07:17 UTC
[incubator-superset] 19/34: fix: change public role like gamma
procedure (#10674)
This is an automated email from the ASF dual-hosted git repository.
villebro pushed a commit to branch 0.38
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
commit 4cb9cfdc81d0a7d541e2cbd22282540d25866130
Author: Daniel Vaz Gaspar <da...@gmail.com>
AuthorDate: Fri Aug 28 10:49:10 2020 +0100
fix: change public role like gamma procedure (#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 <33...@users.noreply.github.com>
* add simple public welcome page
Co-authored-by: Ville Brofeldt <33...@users.noreply.github.com>
---
UPDATING.md | 2 +
superset/config.py | 4 +-
superset/connectors/sqla/models.py | 2 +-
superset/security/manager.py | 82 +++++++++++++++++++++++--
superset/templates/superset/public_welcome.html | 23 +++++++
superset/views/core.py | 2 +
tests/dashboard_tests.py | 8 +++
tests/security_tests.py | 56 ++++++++++++++++-
tests/superset_test_config.py | 5 +-
tests/superset_test_config_thumbnails.py | 2 +-
10 files changed, 173 insertions(+), 13 deletions(-)
diff --git a/UPDATING.md b/UPDATING.md
index 054afaf..47a5dd3 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 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
index 0dfde99..ff136e4 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -260,10 +260,10 @@ AUTH_TYPE = AUTH_DB
# ---------------------------------------------------
# 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
index e4065ae..601a69d 100644
--- a/superset/connectors/sqla/models.py
+++ b/superset/connectors/sqla/models.py
@@ -585,7 +585,7 @@ class SqlaTable( # pylint: disable=too-many-public-methods,too-many-instance-at
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 f939d6d..f5376d8 100644
--- 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]:
@@ -609,8 +619,15 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
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()
@@ -618,6 +635,58 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
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 +698,15 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
"""
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
new file mode 100644
index 0000000..0d82151
--- /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 %}
+<h2><center>Welcome to Apache Superset</center></h2>
+{% endblock %}
diff --git a/superset/views/core.py b/superset/views/core.py
index d6ee690..243b9c6 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -2523,6 +2523,8 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods
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
index f05c980..27926e4 100644
--- a/tests/dashboard_tests.py
+++ b/tests/dashboard_tests.py
@@ -320,6 +320,9 @@ class TestDashboard(SupersetTestCase):
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 @@ class TestDashboard(SupersetTestCase):
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 @@ class TestDashboard(SupersetTestCase):
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
index 60d20fd..fa87822 100644
--- a/tests/security_tests.py
+++ b/tests/security_tests.py
@@ -20,7 +20,7 @@ import unittest
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 @@ class TestRolePermission(SupersetTestCase):
) # 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 @@ class TestRolePermission(SupersetTestCase):
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 @@ class TestRolePermission(SupersetTestCase):
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 c88c584..b62f678 100644
--- 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
index 3b97604..af30af4 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