Skip to content

Commit

Permalink
fix: change public role like gamma procedure (apache#10674)
Browse files Browse the repository at this point in the history
* 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 <33317356+villebro@users.noreply.github.com>

* add simple public welcome page

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
  • Loading branch information
dpgaspar and villebro committed Sep 10, 2020
1 parent 2b03928 commit c1aecf7
Show file tree
Hide file tree
Showing 10 changed files with 173 additions and 13 deletions.
2 changes: 2 additions & 0 deletions UPDATING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions superset/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion superset/connectors/sqla/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
82 changes: 76 additions & 6 deletions superset/security/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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]:
Expand Down Expand Up @@ -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:
Expand All @@ -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:
"""
Expand Down
23 changes: 23 additions & 0 deletions superset/templates/superset/public_welcome.html
Original file line number Diff line number Diff line change
@@ -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 %}
2 changes: 2 additions & 0 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
Expand Down
8 changes: 8 additions & 0 deletions tests/dashboard_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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()}"
Expand Down
56 changes: 54 additions & 2 deletions tests/security_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 4 additions & 1 deletion tests/superset_test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/superset_test_config_thumbnails.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit c1aecf7

Please sign in to comment.