From a21a4175c3fc1ff5c4189bd782be747e0de5a108 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Tue, 7 Dec 2021 14:13:44 -0800 Subject: [PATCH 01/40] helper methods and dashboard access --- superset/common/request_contexed_based.py | 16 +-------------- superset/config.py | 2 +- superset/dashboards/filters.py | 24 ++++++++++++++++++----- superset/security/api.py | 2 +- superset/security/guest_token.py | 14 ++++++++++--- superset/security/manager.py | 23 ++++++++++++++++------ superset/views/base.py | 9 +-------- superset/views/core.py | 5 +++-- 8 files changed, 54 insertions(+), 41 deletions(-) diff --git a/superset/common/request_contexed_based.py b/superset/common/request_contexed_based.py index 0b06a0ccbe1d5..5d8405e36cf05 100644 --- a/superset/common/request_contexed_based.py +++ b/superset/common/request_contexed_based.py @@ -16,24 +16,10 @@ # under the License. from __future__ import annotations -from typing import List, TYPE_CHECKING - -from flask import g - from superset import conf, security_manager -if TYPE_CHECKING: - from flask_appbuilder.security.sqla.models import Role - - -def get_user_roles() -> List[Role]: - if g.user.is_anonymous: - public_role = conf.get("AUTH_ROLE_PUBLIC") - return [security_manager.get_public_role()] if public_role else [] - return g.user.roles - def is_user_admin() -> bool: - user_roles = [role.name.lower() for role in get_user_roles()] + user_roles = [role.name.lower() for role in security_manager.get_user_roles()] admin_role = conf.get("AUTH_ROLE_ADMIN").lower() return admin_role in user_roles diff --git a/superset/config.py b/superset/config.py index f9b734340dbc5..2ff2f1a2389d7 100644 --- a/superset/config.py +++ b/superset/config.py @@ -389,7 +389,7 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: # a custom security config could potentially give access to setting filters on # tables that users do not have access to. "ROW_LEVEL_SECURITY": True, - "EMBEDDED_SUPERSET": False, + "EMBEDDED_SUPERSET": False, # This requires that the public role be available # Enables Alerts and reports new implementation "ALERT_REPORTS": False, # Enable experimental feature to search for other dashboards diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 96584784f449b..3d49978702fa2 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -16,6 +16,7 @@ # under the License. from typing import Any, Optional +from flask import g from flask_appbuilder.security.sqla.models import Role from flask_babel import lazy_gettext as _ from sqlalchemy import and_, or_ @@ -25,7 +26,8 @@ from superset.models.core import FavStar from superset.models.dashboard import Dashboard from superset.models.slice import Slice -from superset.views.base import BaseFilter, get_user_roles, is_user_admin +from superset.security.guest_token import GuestTokenResourceType, GuestUser +from superset.views.base import BaseFilter, is_user_admin from superset.views.base_api import BaseFavoriteFilter @@ -112,7 +114,7 @@ def apply(self, query: Query, value: Any) -> Query: ) ) - dashboard_rbac_or_filters = [] + feature_flagged_filters = [] if is_feature_enabled("DASHBOARD_RBAC"): roles_based_query = ( db.session.query(Dashboard.id) @@ -121,19 +123,31 @@ def apply(self, query: Query, value: Any) -> Query: and_( Dashboard.published.is_(True), dashboard_has_roles, - Role.id.in_([x.id for x in get_user_roles()]), + Role.id.in_([x.id for x in security_manager.get_user_roles()]), ), ) ) - dashboard_rbac_or_filters.append(Dashboard.id.in_(roles_based_query)) + feature_flagged_filters.append(Dashboard.id.in_(roles_based_query)) + + if is_feature_enabled("EMBEDDED_SUPERSET") and security_manager.is_guest_user( + g.user + ): + guest_user: GuestUser = g.user + embedded_dashboard_ids = [ + r["id"] + for r in guest_user.resources + if r["type"] == GuestTokenResourceType.DASHBOARD + ] + if len(embedded_dashboard_ids) != 0: + feature_flagged_filters.append(Dashboard.id.in_(embedded_dashboard_ids)) query = query.filter( or_( Dashboard.id.in_(owner_ids_query), Dashboard.id.in_(datasource_perm_query), Dashboard.id.in_(users_favorite_dash_query), - *dashboard_rbac_or_filters, + *feature_flagged_filters, ) ) diff --git a/superset/security/api.py b/superset/security/api.py index c0a4a77b24225..54efcd07e0dbd 100644 --- a/superset/security/api.py +++ b/superset/security/api.py @@ -35,7 +35,7 @@ class UserSchema(Schema): class ResourceSchema(Schema): - type = fields.String(required=True) + type = fields.String(required=True) # todo figure out how to make this an enum id = fields.String(required=True) rls = fields.String() diff --git a/superset/security/guest_token.py b/superset/security/guest_token.py index cbef52f008e34..60add8175400d 100644 --- a/superset/security/guest_token.py +++ b/superset/security/guest_token.py @@ -14,6 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from enum import Enum from typing import List, Optional, TypedDict, Union from flask_appbuilder.security.sqla.models import Role @@ -26,17 +27,24 @@ class GuestTokenUser(TypedDict, total=False): last_name: str +class GuestTokenResourceType(Enum): + DASHBOARD = "dashboard" + + class GuestTokenResource(TypedDict): - type: str + type: GuestTokenResourceType id: Union[str, int] rls: Optional[str] +GuestTokenResources = List[GuestTokenResource] + + class GuestToken(TypedDict): iat: float exp: float user: GuestTokenUser - resources: List[GuestTokenResource] + resources: GuestTokenResources class GuestUser(AnonymousUserMixin): @@ -50,7 +58,7 @@ class GuestUser(AnonymousUserMixin): def is_authenticated(self) -> bool: """ This is set to true because guest users should be considered authenticated, - at least in most places. The treatment of this flag is pretty inconsistent. + at least in most places. The treatment of this flag is kind of inconsistent. """ return True diff --git a/superset/security/manager.py b/superset/security/manager.py index 1a0a4532f5cd2..6bd31f2e98336 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -68,6 +68,7 @@ from superset.security.guest_token import ( GuestToken, GuestTokenResource, + GuestTokenResources, GuestTokenUser, GuestUser, ) @@ -278,7 +279,7 @@ def can_access(self, permission_name: str, view_name: str) -> bool: """ user = g.user - if user.is_anonymous: + if user.is_anonymous and not self.is_guest_user(user): return self.is_item_public(permission_name, view_name) return self._has_view_access(user, permission_name, view_name) @@ -1097,6 +1098,12 @@ def get_user_by_username( def get_anonymous_user(self) -> User: # pylint: disable=no-self-use return AnonymousUserMixin() + def get_user_roles(self) -> List[Role]: + if g.user.is_anonymous: + public_role = current_app.config.get("AUTH_ROLE_PUBLIC") + return [self.get_public_role()] if public_role else [] + return g.user.roles + def get_rls_filters(self, table: "BaseDatasource") -> List[SqlaQuery]: """ Retrieves the appropriate row level security filters for the current user and @@ -1195,8 +1202,7 @@ def raise_for_user_activity_access(user_id: int) -> None: ) ) - @staticmethod - def raise_for_dashboard_access(dashboard: "Dashboard") -> None: + def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: """ Raise an exception if the user cannot access the dashboard. @@ -1206,14 +1212,15 @@ def raise_for_dashboard_access(dashboard: "Dashboard") -> None: # pylint: disable=import-outside-toplevel from superset import is_feature_enabled from superset.dashboards.commands.exceptions import DashboardAccessDeniedError - from superset.views.base import get_user_roles, is_user_admin + from superset.views.base import is_user_admin from superset.views.utils import is_owner has_rbac_access = True if is_feature_enabled("DASHBOARD_RBAC"): has_rbac_access = any( - dashboard_role.id in [user_role.id for user_role in get_user_roles()] + dashboard_role.id + in [user_role.id for user_role in self.get_user_roles()] for dashboard_role in dashboard.roles ) @@ -1255,7 +1262,7 @@ def _get_current_epoch_time() -> float: return time.time() def create_guest_access_token( - self, user: GuestTokenUser, resources: List[GuestTokenResource] + self, user: GuestTokenUser, resources: GuestTokenResources ) -> bytes: secret = current_app.config["GUEST_TOKEN_JWT_SECRET"] algo = current_app.config["GUEST_TOKEN_JWT_ALGO"] @@ -1319,3 +1326,7 @@ def parse_jwt_guest_token(raw_token: str) -> GuestToken: if token.get("resources") is None: raise ValueError("Guest token does not contain a resources claim") return cast(GuestToken, token) + + @staticmethod + def is_guest_user(user: Any) -> bool: + return hasattr(user, "is_guest_user") and user.is_guest_user diff --git a/superset/views/base.py b/superset/views/base.py index 72dc8053d461a..e5fc08200da08 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -263,15 +263,8 @@ def create_table_permissions(table: models.SqlaTable) -> None: security_manager.add_permission_view_menu("schema_access", table.schema_perm) -def get_user_roles() -> List[Role]: - if g.user.is_anonymous: - public_role = conf.get("AUTH_ROLE_PUBLIC") - return [security_manager.find_role(public_role)] if public_role else [] - return g.user.roles - - def is_user_admin() -> bool: - user_roles = [role.name.lower() for role in list(get_user_roles())] + user_roles = [role.name.lower() for role in list(security_manager.get_user_roles())] return "admin" in user_roles diff --git a/superset/views/core.py b/superset/views/core.py index 9557c30e83d98..68f6e78ed64b0 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -135,7 +135,6 @@ data_payload_response, generate_download_headers, get_error_msg, - get_user_roles, handle_api_exception, json_error_response, json_errors_response, @@ -1886,7 +1885,9 @@ def publish( # pylint: disable=no-self-use f"ERROR: cannot find dashboard {dashboard_id}", status=404 ) - edit_perm = is_owner(dash, g.user) or admin_role in get_user_roles() + edit_perm = ( + is_owner(dash, g.user) or admin_role in security_manager.get_user_roles() + ) if not edit_perm: username = g.user.username if hasattr(g.user, "username") else "user" return json_error_response( From ad3572e54304323dac86023fc8907e54acb15686 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Sat, 11 Dec 2021 02:20:07 -0800 Subject: [PATCH 02/40] guest token dashboard authz --- superset/security/manager.py | 51 +++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 6bd31f2e98336..9a77528301082 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -69,6 +69,7 @@ GuestToken, GuestTokenResource, GuestTokenResources, + GuestTokenResourceType, GuestTokenUser, GuestUser, ) @@ -1068,11 +1069,17 @@ def raise_for_access( assert datasource + is_dashboard_access_check_applicable = feature_flag_manager.is_feature_enabled( + "DASHBOARD_RBAC" + ) or feature_flag_manager.is_feature_enabled( + "EMBEDDED_SUPERSET" + ) + if not ( self.can_access_schema(datasource) or self.can_access("datasource_access", datasource.perm or "") or ( - feature_flag_manager.is_feature_enabled("DASHBOARD_RBAC") + is_dashboard_access_check_applicable and self.can_access_based_on_dashboard(datasource) ) ): @@ -1215,19 +1222,25 @@ def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: from superset.views.base import is_user_admin from superset.views.utils import is_owner - has_rbac_access = True - - if is_feature_enabled("DASHBOARD_RBAC"): - has_rbac_access = any( + def has_rbac_access() -> bool: + return is_feature_enabled("DASHBOARD_RBAC") and any( dashboard_role.id in [user_role.id for user_role in self.get_user_roles()] for dashboard_role in dashboard.roles ) + has_published_access = ( + not is_feature_enabled("DASHBOARD_RBAC") + and not self.is_guest_user(g.user) + and dashboard.published + ) + can_access = ( is_user_admin() or is_owner(dashboard, g.user) - or (dashboard.published and has_rbac_access) + or has_published_access + or has_rbac_access() + or self.has_guest_access(GuestTokenResourceType.DASHBOARD, dashboard.id) or (not dashboard.published and not dashboard.roles) ) @@ -1329,4 +1342,30 @@ def parse_jwt_guest_token(raw_token: str) -> GuestToken: @staticmethod def is_guest_user(user: Any) -> bool: + # pylint: disable=import-outside-toplevel + from superset import is_feature_enabled + + if not is_feature_enabled("EMBEDDED_SUPERSET"): + return False return hasattr(user, "is_guest_user") and user.is_guest_user + + def get_current_guest_user_if_guest(self) -> Optional[GuestUser]: + # pylint: disable=import-outside-toplevel + from superset.extensions import feature_flag_manager + + if self.is_guest_user(g.user): + return g.user + return None + + def has_guest_access( + self, resource_type: GuestTokenResourceType, id: Union[str, int] + ) -> bool: + user = self.get_current_guest_user_if_guest() + if not user: + return False + + strid = str(id) + for resource in user.resources: + if resource["type"] == resource_type.value and str(resource["id"]) == strid: + return True + return False From 4fd8715b42185477706bdbd1493635636abe3a22 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Sat, 11 Dec 2021 02:35:24 -0800 Subject: [PATCH 03/40] adjust csrf exempt list --- superset/config.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/superset/config.py b/superset/config.py index 2ff2f1a2389d7..f7a5bc521f5a7 100644 --- a/superset/config.py +++ b/superset/config.py @@ -191,7 +191,11 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: WTF_CSRF_ENABLED = True # Add endpoints that need to be exempt from CSRF protection -WTF_CSRF_EXEMPT_LIST = ["superset.views.core.log", "superset.charts.data.api.data"] +WTF_CSRF_EXEMPT_LIST = [ + "superset.views.core.log", + "superset.views.core.explore_json", + "superset.charts.data.api.data", +] # Whether to run the web server in debug mode or not DEBUG = os.environ.get("FLASK_ENV") == "development" From 7353826f4fc73c752df0d5193b1e888ebcebcc6b Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Sat, 11 Dec 2021 02:35:35 -0800 Subject: [PATCH 04/40] eums don't work that way --- superset/dashboards/filters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/dashboards/filters.py b/superset/dashboards/filters.py index 3d49978702fa2..e398af97b744a 100644 --- a/superset/dashboards/filters.py +++ b/superset/dashboards/filters.py @@ -137,7 +137,7 @@ def apply(self, query: Query, value: Any) -> Query: embedded_dashboard_ids = [ r["id"] for r in guest_user.resources - if r["type"] == GuestTokenResourceType.DASHBOARD + if r["type"] == GuestTokenResourceType.DASHBOARD.value ] if len(embedded_dashboard_ids) != 0: feature_flagged_filters.append(Dashboard.id.in_(embedded_dashboard_ids)) From 5c0a7f5bbf029fdeaeab075f48e5abb0cabae67f Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian <1858430+suddjian@users.noreply.github.com> Date: Mon, 20 Dec 2021 13:31:39 -0800 Subject: [PATCH 05/40] Remove unnecessary import --- superset/security/manager.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 9a77528301082..e9e2c614a5e28 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1350,8 +1350,6 @@ def is_guest_user(user: Any) -> bool: return hasattr(user, "is_guest_user") and user.is_guest_user def get_current_guest_user_if_guest(self) -> Optional[GuestUser]: - # pylint: disable=import-outside-toplevel - from superset.extensions import feature_flag_manager if self.is_guest_user(g.user): return g.user From 2141f2dee0fe4dd53a9f2b978dcd86676b4cded1 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Tue, 21 Dec 2021 18:45:06 -0800 Subject: [PATCH 06/40] move row level security tests to their own file --- .../security/row_level_security_tests.py | 195 ++++++++++++++++++ tests/integration_tests/security_tests.py | 169 --------------- 2 files changed, 195 insertions(+), 169 deletions(-) create mode 100644 tests/integration_tests/security/row_level_security_tests.py diff --git a/tests/integration_tests/security/row_level_security_tests.py b/tests/integration_tests/security/row_level_security_tests.py new file mode 100644 index 0000000000000..d159c27fa8b18 --- /dev/null +++ b/tests/integration_tests/security/row_level_security_tests.py @@ -0,0 +1,195 @@ +# 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. +# isort:skip_file +import re +from typing import Any, Dict + +import pytest +from flask import g + +from superset import db, security_manager +from superset.connectors.sqla.models import RowLevelSecurityFilter, SqlaTable +from ..base_tests import SupersetTestCase + + +class TestRowLevelSecurity(SupersetTestCase): + """ + Testing Row Level Security + """ + + rls_entry = None + query_obj: Dict[str, Any] = dict( + groupby=[], + metrics=None, + filter=[], + is_timeseries=False, + columns=["value"], + granularity=None, + from_dttm=None, + to_dttm=None, + extras={}, + ) + NAME_AB_ROLE = "NameAB" + NAME_Q_ROLE = "NameQ" + NAMES_A_REGEX = re.compile(r"name like 'A%'") + NAMES_B_REGEX = re.compile(r"name like 'B%'") + NAMES_Q_REGEX = re.compile(r"name like 'Q%'") + BASE_FILTER_REGEX = re.compile(r"gender = 'boy'") + + def setUp(self): + session = db.session + + # Create roles + security_manager.add_role(self.NAME_AB_ROLE) + security_manager.add_role(self.NAME_Q_ROLE) + gamma_user = security_manager.find_user(username="gamma") + gamma_user.roles.append(security_manager.find_role(self.NAME_AB_ROLE)) + gamma_user.roles.append(security_manager.find_role(self.NAME_Q_ROLE)) + self.create_user_with_roles("NoRlsRoleUser", ["Gamma"]) + session.commit() + + # Create regular RowLevelSecurityFilter (energy_usage, unicode_test) + self.rls_entry1 = RowLevelSecurityFilter() + self.rls_entry1.tables.extend( + session.query(SqlaTable) + .filter(SqlaTable.table_name.in_(["energy_usage", "unicode_test"])) + .all() + ) + self.rls_entry1.filter_type = "Regular" + self.rls_entry1.clause = "value > {{ cache_key_wrapper(1) }}" + self.rls_entry1.group_key = None + self.rls_entry1.roles.append(security_manager.find_role("Gamma")) + self.rls_entry1.roles.append(security_manager.find_role("Alpha")) + db.session.add(self.rls_entry1) + + # Create regular RowLevelSecurityFilter (birth_names name starts with A or B) + self.rls_entry2 = RowLevelSecurityFilter() + self.rls_entry2.tables.extend( + session.query(SqlaTable) + .filter(SqlaTable.table_name.in_(["birth_names"])) + .all() + ) + self.rls_entry2.filter_type = "Regular" + self.rls_entry2.clause = "name like 'A%' or name like 'B%'" + self.rls_entry2.group_key = "name" + self.rls_entry2.roles.append(security_manager.find_role("NameAB")) + db.session.add(self.rls_entry2) + + # Create Regular RowLevelSecurityFilter (birth_names name starts with Q) + self.rls_entry3 = RowLevelSecurityFilter() + self.rls_entry3.tables.extend( + session.query(SqlaTable) + .filter(SqlaTable.table_name.in_(["birth_names"])) + .all() + ) + self.rls_entry3.filter_type = "Regular" + self.rls_entry3.clause = "name like 'Q%'" + self.rls_entry3.group_key = "name" + self.rls_entry3.roles.append(security_manager.find_role("NameQ")) + db.session.add(self.rls_entry3) + + # Create Base RowLevelSecurityFilter (birth_names boys) + self.rls_entry4 = RowLevelSecurityFilter() + self.rls_entry4.tables.extend( + session.query(SqlaTable) + .filter(SqlaTable.table_name.in_(["birth_names"])) + .all() + ) + self.rls_entry4.filter_type = "Base" + self.rls_entry4.clause = "gender = 'boy'" + self.rls_entry4.group_key = "gender" + self.rls_entry4.roles.append(security_manager.find_role("Admin")) + db.session.add(self.rls_entry4) + + db.session.commit() + + def tearDown(self): + session = db.session + session.delete(self.rls_entry1) + session.delete(self.rls_entry2) + session.delete(self.rls_entry3) + session.delete(self.rls_entry4) + session.delete(security_manager.find_role("NameAB")) + session.delete(security_manager.find_role("NameQ")) + session.delete(self.get_user("NoRlsRoleUser")) + session.commit() + + @pytest.mark.usefixtures("load_energy_table_with_slice") + def test_rls_filter_alters_energy_query(self): + g.user = self.get_user(username="alpha") + tbl = self.get_table(name="energy_usage") + sql = tbl.get_query_str(self.query_obj) + assert tbl.get_extra_cache_keys(self.query_obj) == [1] + assert "value > 1" in sql + + @pytest.mark.usefixtures("load_energy_table_with_slice") + def test_rls_filter_doesnt_alter_energy_query(self): + g.user = self.get_user( + username="admin" + ) # self.login() doesn't actually set the user + tbl = self.get_table(name="energy_usage") + sql = tbl.get_query_str(self.query_obj) + assert tbl.get_extra_cache_keys(self.query_obj) == [] + assert "value > 1" not in sql + + @pytest.mark.usefixtures("load_unicode_dashboard_with_slice") + def test_multiple_table_filter_alters_another_tables_query(self): + g.user = self.get_user( + username="alpha" + ) # self.login() doesn't actually set the user + tbl = self.get_table(name="unicode_test") + sql = tbl.get_query_str(self.query_obj) + assert tbl.get_extra_cache_keys(self.query_obj) == [1] + assert "value > 1" in sql + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_rls_filter_alters_gamma_birth_names_query(self): + g.user = self.get_user(username="gamma") + tbl = self.get_table(name="birth_names") + sql = tbl.get_query_str(self.query_obj) + + # establish that the filters are grouped together correctly with + # ANDs, ORs and parens in the correct place + assert ( + "WHERE ((name like 'A%'\n or name like 'B%')\n OR (name like 'Q%'))\n AND (gender = 'boy');" + in sql + ) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_rls_filter_alters_no_role_user_birth_names_query(self): + g.user = self.get_user(username="NoRlsRoleUser") + tbl = self.get_table(name="birth_names") + sql = tbl.get_query_str(self.query_obj) + + # gamma's filters should not be present query + assert not self.NAMES_A_REGEX.search(sql) + assert not self.NAMES_B_REGEX.search(sql) + assert not self.NAMES_Q_REGEX.search(sql) + # base query should be present + assert self.BASE_FILTER_REGEX.search(sql) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_rls_filter_doesnt_alter_admin_birth_names_query(self): + g.user = self.get_user(username="admin") + tbl = self.get_table(name="birth_names") + sql = tbl.get_query_str(self.query_obj) + + # no filters are applied for admin user + assert not self.NAMES_A_REGEX.search(sql) + assert not self.NAMES_B_REGEX.search(sql) + assert not self.NAMES_Q_REGEX.search(sql) + assert not self.BASE_FILTER_REGEX.search(sql) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 2168e8c1f41f3..15f06577b496a 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -1048,175 +1048,6 @@ def test_raise_for_access_viz(self, mock_can_access_schema, mock_can_access): security_manager.raise_for_access(viz=test_viz) -class TestRowLevelSecurity(SupersetTestCase): - """ - Testing Row Level Security - """ - - rls_entry = None - query_obj: Dict[str, Any] = dict( - groupby=[], - metrics=None, - filter=[], - is_timeseries=False, - columns=["value"], - granularity=None, - from_dttm=None, - to_dttm=None, - extras={}, - ) - NAME_AB_ROLE = "NameAB" - NAME_Q_ROLE = "NameQ" - NAMES_A_REGEX = re.compile(r"name like 'A%'") - NAMES_B_REGEX = re.compile(r"name like 'B%'") - NAMES_Q_REGEX = re.compile(r"name like 'Q%'") - BASE_FILTER_REGEX = re.compile(r"gender = 'boy'") - - def setUp(self): - session = db.session - - # Create roles - security_manager.add_role(self.NAME_AB_ROLE) - security_manager.add_role(self.NAME_Q_ROLE) - gamma_user = security_manager.find_user(username="gamma") - gamma_user.roles.append(security_manager.find_role(self.NAME_AB_ROLE)) - gamma_user.roles.append(security_manager.find_role(self.NAME_Q_ROLE)) - self.create_user_with_roles("NoRlsRoleUser", ["Gamma"]) - session.commit() - - # Create regular RowLevelSecurityFilter (energy_usage, unicode_test) - self.rls_entry1 = RowLevelSecurityFilter() - self.rls_entry1.tables.extend( - session.query(SqlaTable) - .filter(SqlaTable.table_name.in_(["energy_usage", "unicode_test"])) - .all() - ) - self.rls_entry1.filter_type = "Regular" - self.rls_entry1.clause = "value > {{ cache_key_wrapper(1) }}" - self.rls_entry1.group_key = None - self.rls_entry1.roles.append(security_manager.find_role("Gamma")) - self.rls_entry1.roles.append(security_manager.find_role("Alpha")) - db.session.add(self.rls_entry1) - - # Create regular RowLevelSecurityFilter (birth_names name starts with A or B) - self.rls_entry2 = RowLevelSecurityFilter() - self.rls_entry2.tables.extend( - session.query(SqlaTable) - .filter(SqlaTable.table_name.in_(["birth_names"])) - .all() - ) - self.rls_entry2.filter_type = "Regular" - self.rls_entry2.clause = "name like 'A%' or name like 'B%'" - self.rls_entry2.group_key = "name" - self.rls_entry2.roles.append(security_manager.find_role("NameAB")) - db.session.add(self.rls_entry2) - - # Create Regular RowLevelSecurityFilter (birth_names name starts with Q) - self.rls_entry3 = RowLevelSecurityFilter() - self.rls_entry3.tables.extend( - session.query(SqlaTable) - .filter(SqlaTable.table_name.in_(["birth_names"])) - .all() - ) - self.rls_entry3.filter_type = "Regular" - self.rls_entry3.clause = "name like 'Q%'" - self.rls_entry3.group_key = "name" - self.rls_entry3.roles.append(security_manager.find_role("NameQ")) - db.session.add(self.rls_entry3) - - # Create Base RowLevelSecurityFilter (birth_names boys) - self.rls_entry4 = RowLevelSecurityFilter() - self.rls_entry4.tables.extend( - session.query(SqlaTable) - .filter(SqlaTable.table_name.in_(["birth_names"])) - .all() - ) - self.rls_entry4.filter_type = "Base" - self.rls_entry4.clause = "gender = 'boy'" - self.rls_entry4.group_key = "gender" - self.rls_entry4.roles.append(security_manager.find_role("Admin")) - db.session.add(self.rls_entry4) - - db.session.commit() - - def tearDown(self): - session = db.session - session.delete(self.rls_entry1) - session.delete(self.rls_entry2) - session.delete(self.rls_entry3) - session.delete(self.rls_entry4) - session.delete(security_manager.find_role("NameAB")) - session.delete(security_manager.find_role("NameQ")) - session.delete(self.get_user("NoRlsRoleUser")) - session.commit() - - @pytest.mark.usefixtures("load_energy_table_with_slice") - def test_rls_filter_alters_energy_query(self): - g.user = self.get_user(username="alpha") - tbl = self.get_table(name="energy_usage") - sql = tbl.get_query_str(self.query_obj) - assert tbl.get_extra_cache_keys(self.query_obj) == [1] - assert "value > 1" in sql - - @pytest.mark.usefixtures("load_energy_table_with_slice") - def test_rls_filter_doesnt_alter_energy_query(self): - g.user = self.get_user( - username="admin" - ) # self.login() doesn't actually set the user - tbl = self.get_table(name="energy_usage") - sql = tbl.get_query_str(self.query_obj) - assert tbl.get_extra_cache_keys(self.query_obj) == [] - assert "value > 1" not in sql - - @pytest.mark.usefixtures("load_unicode_dashboard_with_slice") - def test_multiple_table_filter_alters_another_tables_query(self): - g.user = self.get_user( - username="alpha" - ) # self.login() doesn't actually set the user - tbl = self.get_table(name="unicode_test") - sql = tbl.get_query_str(self.query_obj) - assert tbl.get_extra_cache_keys(self.query_obj) == [1] - assert "value > 1" in sql - - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - def test_rls_filter_alters_gamma_birth_names_query(self): - g.user = self.get_user(username="gamma") - tbl = self.get_table(name="birth_names") - sql = tbl.get_query_str(self.query_obj) - - # establish that the filters are grouped together correctly with - # ANDs, ORs and parens in the correct place - assert ( - "WHERE ((name like 'A%'\n or name like 'B%')\n OR (name like 'Q%'))\n AND (gender = 'boy');" - in sql - ) - - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - def test_rls_filter_alters_no_role_user_birth_names_query(self): - g.user = self.get_user(username="NoRlsRoleUser") - tbl = self.get_table(name="birth_names") - sql = tbl.get_query_str(self.query_obj) - - # gamma's filters should not be present query - assert not self.NAMES_A_REGEX.search(sql) - assert not self.NAMES_B_REGEX.search(sql) - assert not self.NAMES_Q_REGEX.search(sql) - # base query should be present - assert self.BASE_FILTER_REGEX.search(sql) - - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - def test_rls_filter_doesnt_alter_admin_birth_names_query(self): - g.user = self.get_user(username="admin") - tbl = self.get_table(name="birth_names") - sql = tbl.get_query_str(self.query_obj) - - # no filters are applied for admin user - assert not self.NAMES_A_REGEX.search(sql) - assert not self.NAMES_B_REGEX.search(sql) - assert not self.NAMES_Q_REGEX.search(sql) - assert not self.BASE_FILTER_REGEX.search(sql) - - class TestAccessRequestEndpoints(SupersetTestCase): def test_access_request_disabled(self): with patch.object(AccessRequestsModelView, "is_enabled", return_value=False): From bfdbad5955404ea6a40c91c8c16691a9c95f8ffb Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Wed, 22 Dec 2021 12:09:43 -0800 Subject: [PATCH 07/40] a bit of refactoring --- superset/security/manager.py | 14 +- .../security/guest_token_security_tests.py | 47 +++++ tox.ini | 180 ------------------ 3 files changed, 55 insertions(+), 186 deletions(-) create mode 100644 tests/integration_tests/security/guest_token_security_tests.py delete mode 100644 tox.ini diff --git a/superset/security/manager.py b/superset/security/manager.py index e9e2c614a5e28..73e818491d718 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1071,8 +1071,8 @@ def raise_for_access( is_dashboard_access_check_applicable = feature_flag_manager.is_feature_enabled( "DASHBOARD_RBAC" - ) or feature_flag_manager.is_feature_enabled( - "EMBEDDED_SUPERSET" + ) or self.is_guest_user( + g.user ) if not ( @@ -1315,10 +1315,12 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]: logger.warning("Invalid guest token", exc_info=True) return None else: - return self.guest_user_cls( - token=token, - roles=[self.find_role(current_app.config["GUEST_ROLE_NAME"])], - ) + return self.get_guest_user_from_token(token) + + def get_guest_user_from_token(self, token: GuestToken) -> GuestUser: + return self.guest_user_cls( + token=token, roles=[self.find_role(current_app.config["GUEST_ROLE_NAME"])], + ) @staticmethod def parse_jwt_guest_token(raw_token: str) -> GuestToken: diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py new file mode 100644 index 0000000000000..5f4a60a54ea8c --- /dev/null +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -0,0 +1,47 @@ +# 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. +"""Unit tests for Superset""" +from unittest import mock + +import pytest +from flask import g + +from integration_tests.base_tests import SupersetTestCase +from superset import db, security_manager +from superset.models.dashboard import Dashboard +from superset.security.guest_token import GuestUser +from tests.integration_tests.fixtures.birth_names_dashboard import ( + load_birth_names_dashboard_with_slices, +) +from tests.integration_tests.fixtures.public_role import public_role_like_gamma +from tests.integration_tests.fixtures.query_context import get_query_context + + +@mock.patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=True, +) +class TestDashboardGuestTokenSecurity(SupersetTestCase): + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_dashboard_access_filter_as_guest(self): + dash = db.session.query(Dashboard).filter_by(slug="births").first() + g.user = security_manager.get_guest_user_from_token({ + "user": {}, + "resources": [{"type": "dashboard", "id": dash.id}] + }) + + security_manager.raise_for_access(datasource=dash.datasources[0]) diff --git a/tox.ini b/tox.ini deleted file mode 100644 index 839bce55867ec..0000000000000 --- a/tox.ini +++ /dev/null @@ -1,180 +0,0 @@ -# -# 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. -# - -# Remember to start celery workers to run celery tests, e.g. -# celery --app=superset.tasks.celery_app:app worker -Ofair -c 2 -[testenv] -basepython = python3.8 -ignore_basepython_conflict = true -commands = - superset db upgrade - superset init - # use -s to be able to use break pointers. - # no args or tests/* can be passed as an argument to run all tests - pytest -s {posargs} -deps = - -rrequirements/testing.txt -setenv = - PYTHONPATH = {toxinidir} - SUPERSET_TESTENV = true - SUPERSET_CONFIG = tests.integration_tests.superset_test_config - SUPERSET_HOME = {envtmpdir} - mysql: SUPERSET__SQLALCHEMY_DATABASE_URI = mysql://mysqluser:mysqluserpassword@localhost/superset?charset=utf8 - postgres: SUPERSET__SQLALCHEMY_DATABASE_URI = postgresql+psycopg2://superset:superset@localhost/test - sqlite: SUPERSET__SQLALCHEMY_DATABASE_URI = sqlite:////{envtmpdir}/superset.db - mysql-presto: SUPERSET__SQLALCHEMY_DATABASE_URI = mysql://mysqluser:mysqluserpassword@localhost/superset?charset=utf8 - # docker run -p 8080:8080 --name presto prestosql/presto - mysql-presto: SUPERSET__SQLALCHEMY_EXAMPLES_URI = presto://localhost:8080/memory/default - # based on https://github.com/big-data-europe/docker-hadoop - # clone the repo & run docker-compose up -d to test locally - mysql-hive: SUPERSET__SQLALCHEMY_DATABASE_URI = mysql://mysqluser:mysqluserpassword@localhost/superset?charset=utf8 - mysql-hive: SUPERSET__SQLALCHEMY_EXAMPLES_URI = hive://localhost:10000/default - # make sure that directory is accessible by docker - hive: UPLOAD_FOLDER = /tmp/.superset/app/static/uploads/ -usedevelop = true -allowlist_externals = - npm - pkill - -[testenv:cypress] -setenv = - PYTHONPATH = {toxinidir} - SUPERSET_TESTENV = true - SUPERSET_CONFIG = tests.integration_tests.superset_test_config - SUPERSET_HOME = {envtmpdir} - ENABLE_REACT_CRUD_VIEWS = true -commands = - npm install -g npm@'>=6.5.0' - pip install -e {toxinidir}/ - {toxinidir}/superset-frontend/cypress_build.sh -commands_post = - pkill -if "python {envbindir}/flask" - -[testenv:cypress-dashboard] -setenv = - PYTHONPATH = {toxinidir} - SUPERSET_TESTENV = true - SUPERSET_CONFIG = tests.integration_tests.superset_test_config - SUPERSET_HOME = {envtmpdir} - ENABLE_REACT_CRUD_VIEWS = true -commands = - npm install -g npm@'>=6.5.0' - pip install -e {toxinidir}/ - {toxinidir}/superset-frontend/cypress_build.sh dashboard -commands_post = - pkill -if "python {envbindir}/flask" - -[testenv:cypress-explore] -setenv = - PYTHONPATH = {toxinidir} - SUPERSET_TESTENV = true - SUPERSET_CONFIG = tests.integration_tests.superset_test_config - SUPERSET_HOME = {envtmpdir} - ENABLE_REACT_CRUD_VIEWS = true -commands = - npm install -g npm@'>=6.5.0' - pip install -e {toxinidir}/ - {toxinidir}/superset-frontend/cypress_build.sh explore -commands_post = - pkill -if "python {envbindir}/flask" - -[testenv:cypress-sqllab] -setenv = - PYTHONPATH = {toxinidir} - SUPERSET_TESTENV = true - SUPERSET_CONFIG = tests.integration_tests.superset_test_config - SUPERSET_HOME = {envtmpdir} - ENABLE_REACT_CRUD_VIEWS = true -commands = - npm install -g npm@'>=6.5.0' - pip install -e {toxinidir}/ - {toxinidir}/superset-frontend/cypress_build.sh sqllab -commands_post = - pkill -if "python {envbindir}/flask" - -[testenv:cypress-sqllab-backend-persist] -setenv = - PYTHONPATH = {toxinidir} - SUPERSET_TESTENV = true - SUPERSET_CONFIG = tests.integration_tests.superset_test_config - SUPERSET_HOME = {envtmpdir} - ENABLE_REACT_CRUD_VIEWS = true -commands = - npm install -g npm@'>=6.5.0' - pip install -e {toxinidir}/ - {toxinidir}/superset-frontend/cypress_build.sh sqllab -commands_post = - pkill -if "python {envbindir}/flask" - -[testenv:eslint] -changedir = {toxinidir}/superset-frontend -commands = - npm run lint -deps = - -[testenv:fossa] -commands = - {toxinidir}/scripts/fossa.sh -deps = -passenv = * - -[testenv:javascript] -commands = - npm install -g npm@'>=6.5.0' - {toxinidir}/superset-frontend/js_build.sh -deps = - -[testenv:license-check] -commands = - {toxinidir}/scripts/check_license.sh -passenv = * -whitelist_externals = - {toxinidir}/scripts/check_license.sh -deps = - -[testenv:pre-commit] -commands = - pre-commit run --all-files -deps = - -rrequirements/integration.txt -skip_install = true - -[testenv:pylint] -commands = - pylint superset -deps = - -rrequirements/testing.txt - -[testenv:thumbnails] -setenv = - SUPERSET_CONFIG = tests.integration_tests.superset_test_config_thumbnails -deps = - -rrequirements/testing.txt - -[tox] -envlist = - cypress-dashboard - cypress-explore - cypress-sqllab - cypress-sqllab-backend-persist - eslint - fossa - javascript - license-check - pre-commit - pylint -skipsdist = true From 36a1224496eb494806bbe7f1cc85167f87f23a28 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Wed, 22 Dec 2021 12:11:24 -0800 Subject: [PATCH 08/40] add guest token security tests --- .../security/guest_token_security_tests.py | 47 +++++++++++++++---- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index 5f4a60a54ea8c..a962b68e8436c 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -19,9 +19,10 @@ import pytest from flask import g - from integration_tests.base_tests import SupersetTestCase + from superset import db, security_manager +from superset.exceptions import SupersetSecurityException from superset.models.dashboard import Dashboard from superset.security.guest_token import GuestUser from tests.integration_tests.fixtures.birth_names_dashboard import ( @@ -34,14 +35,42 @@ @mock.patch.dict( "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=True, ) -class TestDashboardGuestTokenSecurity(SupersetTestCase): - - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") +@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") +class TestGuestTokenSecurity(SupersetTestCase): def test_dashboard_access_filter_as_guest(self): dash = db.session.query(Dashboard).filter_by(slug="births").first() - g.user = security_manager.get_guest_user_from_token({ - "user": {}, - "resources": [{"type": "dashboard", "id": dash.id}] - }) + dataset = list(dash.datasources)[0] + g.user = security_manager.get_guest_user_from_token( + {"user": {}, "resources": [{"type": "dashboard", "id": dash.id}]} + ) + + security_manager.raise_for_access(datasource=dataset) + + def test_dashboard_access_filter_as_unauthorized_guest(self): + dash = db.session.query(Dashboard).filter_by(slug="births").first() + dataset = list(dash.datasources)[0] + g.user = security_manager.get_guest_user_from_token( + {"user": {}, "resources": [{"type": "dashboard", "id": dash.id + 1}]} + ) + + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access(datasource=dataset) + + def test_chart_access_filter_as_guest(self): + dash = db.session.query(Dashboard).filter_by(slug="births").first() + chart = dash.slices[0] + g.user = security_manager.get_guest_user_from_token( + {"user": {}, "resources": [{"type": "dashboard", "id": dash.id}]} + ) + + security_manager.raise_for_access(viz=chart) + + def test_chart_access_filter_as_unauthorized_guest(self): + dash = db.session.query(Dashboard).filter_by(slug="births").first() + chart = dash.slices[0] + g.user = security_manager.get_guest_user_from_token( + {"user": {}, "resources": [{"type": "dashboard", "id": dash.id + 1}]} + ) - security_manager.raise_for_access(datasource=dash.datasources[0]) + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access(viz=chart) From 7181333a89c69d3b1d9724a1edac5706d12000c0 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Wed, 5 Jan 2022 17:53:01 -0800 Subject: [PATCH 09/40] refactor tests --- .../security/guest_token_security_tests.py | 37 +++++++++---------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index a962b68e8436c..42cc7407f49c7 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -37,40 +37,37 @@ ) @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") class TestGuestTokenSecurity(SupersetTestCase): - def test_dashboard_access_filter_as_guest(self): - dash = db.session.query(Dashboard).filter_by(slug="births").first() - dataset = list(dash.datasources)[0] - g.user = security_manager.get_guest_user_from_token( - {"user": {}, "resources": [{"type": "dashboard", "id": dash.id}]} + def setUp(self) -> None: + self.dash = db.session.query(Dashboard).filter_by(slug="births").first() + self.authorized_guest = security_manager.get_guest_user_from_token( + {"user": {}, "resources": [{"type": "dashboard", "id": self.dash.id}]} + ) + self.unauthorized_guest = security_manager.get_guest_user_from_token( + {"user": {}, "resources": [{"type": "dashboard", "id": self.dash.id + 1}]} ) + def test_dashboard_access_filter_as_guest(self): + dataset = list(self.dash.datasources)[0] + g.user = self.authorized_guest + security_manager.raise_for_access(datasource=dataset) def test_dashboard_access_filter_as_unauthorized_guest(self): - dash = db.session.query(Dashboard).filter_by(slug="births").first() - dataset = list(dash.datasources)[0] - g.user = security_manager.get_guest_user_from_token( - {"user": {}, "resources": [{"type": "dashboard", "id": dash.id + 1}]} - ) + dataset = list(self.dash.datasources)[0] + g.user = self.unauthorized_guest with self.assertRaises(SupersetSecurityException): security_manager.raise_for_access(datasource=dataset) def test_chart_access_filter_as_guest(self): - dash = db.session.query(Dashboard).filter_by(slug="births").first() - chart = dash.slices[0] - g.user = security_manager.get_guest_user_from_token( - {"user": {}, "resources": [{"type": "dashboard", "id": dash.id}]} - ) + chart = self.dash.slices[0] + g.user = self.authorized_guest security_manager.raise_for_access(viz=chart) def test_chart_access_filter_as_unauthorized_guest(self): - dash = db.session.query(Dashboard).filter_by(slug="births").first() - chart = dash.slices[0] - g.user = security_manager.get_guest_user_from_token( - {"user": {}, "resources": [{"type": "dashboard", "id": dash.id + 1}]} - ) + chart = self.dash.slices[0] + g.user = self.unauthorized_guest with self.assertRaises(SupersetSecurityException): security_manager.raise_for_access(viz=chart) From 27a2ec3dbfc26ceea68f5cc216eda43c6d09fe00 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Wed, 5 Jan 2022 17:54:44 -0800 Subject: [PATCH 10/40] clean imports --- tests/integration_tests/security_tests.py | 24 +++-------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 15f06577b496a..7b3edfd5d8bf2 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -15,27 +15,25 @@ # specific language governing permissions and limitations # under the License. # isort:skip_file -import dataclasses import inspect -import re import time import unittest from collections import namedtuple from unittest import mock from unittest.mock import Mock, patch -from typing import Any, Dict +from typing import Any import jwt import prison import pytest -from flask import current_app, g +from flask import current_app from superset.models.dashboard import Dashboard from superset import app, appbuilder, db, security_manager, viz, ConnectorRegistry from superset.connectors.druid.models import DruidCluster, DruidDatasource -from superset.connectors.sqla.models import RowLevelSecurityFilter, SqlaTable +from superset.connectors.sqla.models import SqlaTable from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import SupersetSecurityException from superset.models.core import Database @@ -45,22 +43,6 @@ from superset.views.access_requests import AccessRequestsModelView from .base_tests import SupersetTestCase -from tests.integration_tests.fixtures.birth_names_dashboard import ( - load_birth_names_dashboard_with_slices, -) -from tests.integration_tests.fixtures.energy_dashboard import ( - load_energy_table_with_slice, -) -from tests.integration_tests.fixtures.public_role import ( - public_role_like_gamma, - public_role_like_test_role, -) -from tests.integration_tests.fixtures.unicode_dashboard import ( - load_unicode_dashboard_with_slice, -) -from tests.integration_tests.fixtures.world_bank_dashboard import ( - load_world_bank_dashboard_with_slices, -) NEW_SECURITY_CONVERGE_VIEWS = ( "Annotation", From 09aed96e678e5fdb02b0c1b2b34446ccfa399390 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 14:19:51 -0800 Subject: [PATCH 11/40] variable names can be too long apparently --- superset/security/manager.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 73e818491d718..4d9f77c0fa443 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1069,17 +1069,15 @@ def raise_for_access( assert datasource - is_dashboard_access_check_applicable = feature_flag_manager.is_feature_enabled( + should_check_dashboard_access = feature_flag_manager.is_feature_enabled( "DASHBOARD_RBAC" - ) or self.is_guest_user( - g.user - ) + ) or self.is_guest_user(g.user) if not ( self.can_access_schema(datasource) or self.can_access("datasource_access", datasource.perm or "") or ( - is_dashboard_access_check_applicable + should_check_dashboard_access and self.can_access_based_on_dashboard(datasource) ) ): From c7bfa96dd17d417695d92ca3c95846efb3cc56d1 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 14:25:13 -0800 Subject: [PATCH 12/40] missing argument to get_user_roles --- superset/security/manager.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 4d9f77c0fa443..933ff1d6f3d9a 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1103,11 +1103,11 @@ def get_user_by_username( def get_anonymous_user(self) -> User: # pylint: disable=no-self-use return AnonymousUserMixin() - def get_user_roles(self) -> List[Role]: - if g.user.is_anonymous: + def get_user_roles(self, user: User) -> List[Role]: + if user.is_anonymous: public_role = current_app.config.get("AUTH_ROLE_PUBLIC") return [self.get_public_role()] if public_role else [] - return g.user.roles + return user.roles def get_rls_filters(self, table: "BaseDatasource") -> List[SqlaQuery]: """ @@ -1223,7 +1223,7 @@ def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: def has_rbac_access() -> bool: return is_feature_enabled("DASHBOARD_RBAC") and any( dashboard_role.id - in [user_role.id for user_role in self.get_user_roles()] + in [user_role.id for user_role in self.get_user_roles(g.user)] for dashboard_role in dashboard.roles ) From aa672b9b7ac270c59914362c59878ff1c212b8c3 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 14:26:25 -0800 Subject: [PATCH 13/40] don't redefine builtins --- superset/security/manager.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 933ff1d6f3d9a..fa8ab71d67e7d 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1356,13 +1356,13 @@ def get_current_guest_user_if_guest(self) -> Optional[GuestUser]: return None def has_guest_access( - self, resource_type: GuestTokenResourceType, id: Union[str, int] + self, resource_type: GuestTokenResourceType, resource_id: Union[str, int] ) -> bool: user = self.get_current_guest_user_if_guest() if not user: return False - strid = str(id) + strid = str(resource_id) for resource in user.resources: if resource["type"] == resource_type.value and str(resource["id"]) == strid: return True From 091786d1e60af9931d6b392604fe2c5a89bb0d19 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 14:28:07 -0800 Subject: [PATCH 14/40] remove unused imports --- superset/security/manager.py | 1 - superset/views/base.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index fa8ab71d67e7d..48d3c9eec6a1a 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -67,7 +67,6 @@ from superset.exceptions import SupersetSecurityException from superset.security.guest_token import ( GuestToken, - GuestTokenResource, GuestTokenResources, GuestTokenResourceType, GuestTokenUser, diff --git a/superset/views/base.py b/superset/views/base.py index e5fc08200da08..5b84ec36751d7 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -38,7 +38,7 @@ from flask_appbuilder.actions import action from flask_appbuilder.forms import DynamicForm from flask_appbuilder.models.sqla.filters import BaseFilter -from flask_appbuilder.security.sqla.models import Role, User +from flask_appbuilder.security.sqla.models import User from flask_appbuilder.widgets import ListWidget from flask_babel import get_locale, gettext as __, lazy_gettext as _ from flask_jwt_extended.exceptions import NoAuthorizationError From 5e23ce9dcef758ad00ade9311006efd0fc0d7cf6 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 14:33:41 -0800 Subject: [PATCH 15/40] fix test import --- tests/integration_tests/security/guest_token_security_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index 42cc7407f49c7..2fdb1bd1db1d2 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -19,12 +19,12 @@ import pytest from flask import g -from integration_tests.base_tests import SupersetTestCase from superset import db, security_manager from superset.exceptions import SupersetSecurityException from superset.models.dashboard import Dashboard from superset.security.guest_token import GuestUser +from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, ) From a62b2f516ef0ae4c9177bce46db18cc30a00b6f3 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 14:57:43 -0800 Subject: [PATCH 16/40] default to global user when getting roles --- superset/security/manager.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 48d3c9eec6a1a..0dfaafa533ac6 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1102,7 +1102,9 @@ def get_user_by_username( def get_anonymous_user(self) -> User: # pylint: disable=no-self-use return AnonymousUserMixin() - def get_user_roles(self, user: User) -> List[Role]: + def get_user_roles(self, user: Optional[User] = None) -> List[Role]: + if not user: + user = g.user if user.is_anonymous: public_role = current_app.config.get("AUTH_ROLE_PUBLIC") return [self.get_public_role()] if public_role else [] From 4d5a69158be7fc83d5001a4ac8ad7f223441b595 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 15:35:39 -0800 Subject: [PATCH 17/40] missing import --- tests/integration_tests/security_tests.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 087b3fee6793c..ebbe18aafcb91 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -53,6 +53,7 @@ ) from tests.integration_tests.fixtures.world_bank_dashboard import ( load_world_bank_dashboard_with_slices, + load_world_bank_data, ) NEW_SECURITY_CONVERGE_VIEWS = ( From 13a2038ad1fd8b4e706f0c37922dd287fbd4cc7a Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 15:53:15 -0800 Subject: [PATCH 18/40] mock it --- .../security/guest_token_security_tests.py | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index 2fdb1bd1db1d2..840ef6944724e 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -16,6 +16,7 @@ # under the License. """Unit tests for Superset""" from unittest import mock +from unittest.mock import patch import pytest from flask import g @@ -46,28 +47,32 @@ def setUp(self) -> None: {"user": {}, "resources": [{"type": "dashboard", "id": self.dash.id + 1}]} ) - def test_dashboard_access_filter_as_guest(self): + @patch("superset.security.manager.g") + def test_dashboard_access_filter_as_guest(self, mock_g): dataset = list(self.dash.datasources)[0] - g.user = self.authorized_guest + mock_g.user = self.authorized_guest security_manager.raise_for_access(datasource=dataset) - def test_dashboard_access_filter_as_unauthorized_guest(self): + @patch("superset.security.manager.g") + def test_dashboard_access_filter_as_unauthorized_guest(self, mock_g): dataset = list(self.dash.datasources)[0] - g.user = self.unauthorized_guest + mock_g.user = self.unauthorized_guest with self.assertRaises(SupersetSecurityException): security_manager.raise_for_access(datasource=dataset) - def test_chart_access_filter_as_guest(self): + @patch("superset.security.manager.g") + def test_chart_access_filter_as_guest(self, mock_g): chart = self.dash.slices[0] - g.user = self.authorized_guest + mock_g.user = self.authorized_guest security_manager.raise_for_access(viz=chart) - def test_chart_access_filter_as_unauthorized_guest(self): + @patch("superset.security.manager.g") + def test_chart_access_filter_as_unauthorized_guest(self, mock_g): chart = self.dash.slices[0] - g.user = self.unauthorized_guest + mock_g.user = self.unauthorized_guest with self.assertRaises(SupersetSecurityException): security_manager.raise_for_access(viz=chart) From 0be28dab75023d565b6566a7945627a825545ecb Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 15:57:40 -0800 Subject: [PATCH 19/40] test get_user_roles --- .../security/guest_token_security_tests.py | 11 +++++++++++ tests/integration_tests/security_tests.py | 13 +++++++++++++ 2 files changed, 24 insertions(+) diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index 840ef6944724e..b74fae1223d5b 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -76,3 +76,14 @@ def test_chart_access_filter_as_unauthorized_guest(self, mock_g): with self.assertRaises(SupersetSecurityException): security_manager.raise_for_access(viz=chart) + + def test_get_guest_user_roles_explicit(self): + roles = security_manager.get_user_roles(self.authorized_guest) + self.assertEqual(self.authorized_guest.roles, roles) + + @patch("superset.security.manager.g") + def test_get_guest_user_roles_implicit(self, mock_g): + mock_g.user = self.authorized_guest + + roles = security_manager.get_user_roles(self.authorized_guest) + self.assertEqual(self.authorized_guest.roles, roles) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index ebbe18aafcb91..2f6480aecd90c 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -1041,6 +1041,19 @@ def test_raise_for_access_viz(self, mock_can_access_schema, mock_can_access): with self.assertRaises(SupersetSecurityException): security_manager.raise_for_access(viz=test_viz) + @patch("superset.security.manager.g") + def test_get_user_roles(self, mock_g): + admin = security_manager.find_user("admin") + mock_g.user = admin + roles = security_manager.get_user_roles() + self.assertEqual(admin.roles, roles) + + @patch("superset.security.manager.g") + def test_get_anonymous_roles(self, mock_g): + mock_g.user = security_manager.get_anonymous_user() + roles = security_manager.get_user_roles() + self.assertEqual([security_manager.get_public_role()], roles) + class TestAccessRequestEndpoints(SupersetTestCase): def test_access_request_disabled(self): From c5bded9bb24b0037d1e945ce03769bd68df9e801 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 17:11:34 -0800 Subject: [PATCH 20/40] infer g.user for ease of tests --- superset/security/manager.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 0dfaafa533ac6..40620ebbc3c70 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1068,9 +1068,10 @@ def raise_for_access( assert datasource - should_check_dashboard_access = feature_flag_manager.is_feature_enabled( - "DASHBOARD_RBAC" - ) or self.is_guest_user(g.user) + should_check_dashboard_access = ( + feature_flag_manager.is_feature_enabled("DASHBOARD_RBAC") + or self.is_guest_user() + ) if not ( self.can_access_schema(datasource) @@ -1230,7 +1231,7 @@ def has_rbac_access() -> bool: has_published_access = ( not is_feature_enabled("DASHBOARD_RBAC") - and not self.is_guest_user(g.user) + and not self.is_guest_user() and dashboard.published ) @@ -1342,17 +1343,19 @@ def parse_jwt_guest_token(raw_token: str) -> GuestToken: return cast(GuestToken, token) @staticmethod - def is_guest_user(user: Any) -> bool: + def is_guest_user(user: Optional[Any] = None) -> bool: # pylint: disable=import-outside-toplevel from superset import is_feature_enabled if not is_feature_enabled("EMBEDDED_SUPERSET"): return False + if not user: + user = g.user return hasattr(user, "is_guest_user") and user.is_guest_user def get_current_guest_user_if_guest(self) -> Optional[GuestUser]: - if self.is_guest_user(g.user): + if self.is_guest_user(): return g.user return None From ebf9400fff1787142ddb0c7b0882f672a8c22a96 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 22:13:37 -0800 Subject: [PATCH 21/40] remove redundant check --- superset/security/manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 40620ebbc3c70..e6b1527473915 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -279,7 +279,7 @@ def can_access(self, permission_name: str, view_name: str) -> bool: """ user = g.user - if user.is_anonymous and not self.is_guest_user(user): + if user.is_anonymous: return self.is_item_public(permission_name, view_name) return self._has_view_access(user, permission_name, view_name) From deceb33b6ecc4e2c507447eb100635cdadd35114 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 22:14:35 -0800 Subject: [PATCH 22/40] tests for guest user security manager fns --- .../security/guest_token_security_tests.py | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index b74fae1223d5b..e877f8f36ac72 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -19,12 +19,10 @@ from unittest.mock import patch import pytest -from flask import g from superset import db, security_manager from superset.exceptions import SupersetSecurityException from superset.models.dashboard import Dashboard -from superset.security.guest_token import GuestUser from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, @@ -87,3 +85,23 @@ def test_get_guest_user_roles_implicit(self, mock_g): roles = security_manager.get_user_roles(self.authorized_guest) self.assertEqual(self.authorized_guest.roles, roles) + + def test_user_is_not_guest(self): + is_guest = security_manager.is_guest_user(security_manager.find_user("admin")) + self.assertTrue(is_guest) + + def test_anon_is_not_guest(self): + is_guest = security_manager.is_guest_user(security_manager.get_anonymous_user()) + self.assertTrue(is_guest) + + def test_guest_is_guest(self): + is_guest = security_manager.is_guest_user(self.authorized_guest) + self.assertTrue(is_guest) + + @mock.patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", + EMBEDDED_SUPERSET=False, + ) + def test_is_guest_user_flag_off(self): + is_guest = security_manager.is_guest_user(self.authorized_guest) + self.assertFalse(is_guest) From 128f3a09f9b2db1c1d156fefb1947da6ba984eae Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 13 Jan 2022 22:15:22 -0800 Subject: [PATCH 23/40] use algo to get rid of warning messages --- tests/integration_tests/security_tests.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 2f6480aecd90c..48ded730aad1b 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -1170,7 +1170,11 @@ def test_create_guest_access_token(self, get_time_mock): token = security_manager.create_guest_access_token(user, resources) # unfortunately we cannot mock time in the jwt lib - decoded_token = jwt.decode(token, self.app.config["GUEST_TOKEN_JWT_SECRET"]) + decoded_token = jwt.decode( + token, + self.app.config["GUEST_TOKEN_JWT_SECRET"], + algorithms=[self.app.config["GUEST_TOKEN_JWT_ALGO"]], + ) self.assertEqual(user, decoded_token["user"]) self.assertEqual(resources, decoded_token["resources"]) From ede136716c8d35d3d2d88baa7abf31f242f8e23d Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Tue, 18 Jan 2022 01:57:08 -0800 Subject: [PATCH 24/40] tweaking access checks --- superset/security/manager.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index e6b1527473915..22ed41569761b 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1222,25 +1222,20 @@ def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: from superset.views.base import is_user_admin from superset.views.utils import is_owner - def has_rbac_access() -> bool: - return is_feature_enabled("DASHBOARD_RBAC") and any( + has_rbac_access = True + + if is_feature_enabled("DASHBOARD_RBAC"): + has_rbac_access = any( dashboard_role.id - in [user_role.id for user_role in self.get_user_roles(g.user)] + in [user_role.id for user_role in self.get_user_roles()] for dashboard_role in dashboard.roles ) - has_published_access = ( - not is_feature_enabled("DASHBOARD_RBAC") - and not self.is_guest_user() - and dashboard.published - ) - can_access = ( is_user_admin() or is_owner(dashboard, g.user) - or has_published_access - or has_rbac_access() or self.has_guest_access(GuestTokenResourceType.DASHBOARD, dashboard.id) + or (dashboard.published and has_rbac_access) or (not dashboard.published and not dashboard.roles) ) From 38a89ae3eeab6236b7fbdde6d087dd4c940ec0ed Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Tue, 18 Jan 2022 12:13:36 -0800 Subject: [PATCH 25/40] fix guest token security tests --- .../security/guest_token_security_tests.py | 64 ++++++++++--------- 1 file changed, 33 insertions(+), 31 deletions(-) diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index e877f8f36ac72..df76417d6fd39 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -16,9 +16,9 @@ # under the License. """Unit tests for Superset""" from unittest import mock -from unittest.mock import patch import pytest +from flask import g from superset import db, security_manager from superset.exceptions import SupersetSecurityException @@ -26,16 +26,35 @@ from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, + load_birth_names_data, ) -from tests.integration_tests.fixtures.public_role import public_role_like_gamma -from tests.integration_tests.fixtures.query_context import get_query_context + + +@mock.patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=True, +) +class TestGuestUserSecurity(SupersetTestCase): + def test_user_is_not_guest(self): + is_guest = security_manager.is_guest_user(security_manager.find_user("admin")) + self.assertFalse(is_guest) + + def test_anon_is_not_guest(self): + is_guest = security_manager.is_guest_user(security_manager.get_anonymous_user()) + self.assertFalse(is_guest) + + def test_guest_is_guest(self): + authorized_guest = security_manager.get_guest_user_from_token( + {"user": {}, "resources": []} + ) + is_guest = security_manager.is_guest_user(authorized_guest) + self.assertTrue(is_guest) @mock.patch.dict( "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=True, ) @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") -class TestGuestTokenSecurity(SupersetTestCase): +class TestGuestTokenDashboardAccess(SupersetTestCase): def setUp(self) -> None: self.dash = db.session.query(Dashboard).filter_by(slug="births").first() self.authorized_guest = security_manager.get_guest_user_from_token( @@ -45,32 +64,28 @@ def setUp(self) -> None: {"user": {}, "resources": [{"type": "dashboard", "id": self.dash.id + 1}]} ) - @patch("superset.security.manager.g") - def test_dashboard_access_filter_as_guest(self, mock_g): + def test_dashboard_access_filter_as_guest(self): dataset = list(self.dash.datasources)[0] - mock_g.user = self.authorized_guest + g.user = self.authorized_guest security_manager.raise_for_access(datasource=dataset) - @patch("superset.security.manager.g") - def test_dashboard_access_filter_as_unauthorized_guest(self, mock_g): + def test_dashboard_access_filter_as_unauthorized_guest(self): dataset = list(self.dash.datasources)[0] - mock_g.user = self.unauthorized_guest + g.user = self.unauthorized_guest with self.assertRaises(SupersetSecurityException): security_manager.raise_for_access(datasource=dataset) - @patch("superset.security.manager.g") - def test_chart_access_filter_as_guest(self, mock_g): + def test_chart_access_filter_as_guest(self): chart = self.dash.slices[0] - mock_g.user = self.authorized_guest + g.user = self.authorized_guest security_manager.raise_for_access(viz=chart) - @patch("superset.security.manager.g") - def test_chart_access_filter_as_unauthorized_guest(self, mock_g): + def test_chart_access_filter_as_unauthorized_guest(self): chart = self.dash.slices[0] - mock_g.user = self.unauthorized_guest + g.user = self.unauthorized_guest with self.assertRaises(SupersetSecurityException): security_manager.raise_for_access(viz=chart) @@ -79,25 +94,12 @@ def test_get_guest_user_roles_explicit(self): roles = security_manager.get_user_roles(self.authorized_guest) self.assertEqual(self.authorized_guest.roles, roles) - @patch("superset.security.manager.g") - def test_get_guest_user_roles_implicit(self, mock_g): - mock_g.user = self.authorized_guest + def test_get_guest_user_roles_implicit(self): + g.user = self.authorized_guest roles = security_manager.get_user_roles(self.authorized_guest) self.assertEqual(self.authorized_guest.roles, roles) - def test_user_is_not_guest(self): - is_guest = security_manager.is_guest_user(security_manager.find_user("admin")) - self.assertTrue(is_guest) - - def test_anon_is_not_guest(self): - is_guest = security_manager.is_guest_user(security_manager.get_anonymous_user()) - self.assertTrue(is_guest) - - def test_guest_is_guest(self): - is_guest = security_manager.is_guest_user(self.authorized_guest) - self.assertTrue(is_guest) - @mock.patch.dict( "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=False, From 1927a1ceb22cc6f539b45c7b7552ed8e9dd99391 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Tue, 18 Jan 2022 14:32:17 -0800 Subject: [PATCH 26/40] missing imports --- .../security/row_level_security_tests.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/integration_tests/security/row_level_security_tests.py b/tests/integration_tests/security/row_level_security_tests.py index d159c27fa8b18..665666cb61f5b 100644 --- a/tests/integration_tests/security/row_level_security_tests.py +++ b/tests/integration_tests/security/row_level_security_tests.py @@ -24,6 +24,18 @@ from superset import db, security_manager from superset.connectors.sqla.models import RowLevelSecurityFilter, SqlaTable from ..base_tests import SupersetTestCase +from tests.integration_tests.fixtures.birth_names_dashboard import ( + load_birth_names_dashboard_with_slices, + load_birth_names_data, +) +from tests.integration_tests.fixtures.energy_dashboard import ( + load_energy_table_with_slice, + load_energy_table_data, +) +from tests.integration_tests.fixtures.unicode_dashboard import ( + load_unicode_dashboard_with_slice, + load_unicode_data, +) class TestRowLevelSecurity(SupersetTestCase): From 128b90cfa43f226c782af61b9929d50a3e1025a7 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Tue, 18 Jan 2022 15:31:09 -0800 Subject: [PATCH 27/40] more tests --- .../security/guest_token_security_tests.py | 82 ++++++++++++++++++- 1 file changed, 78 insertions(+), 4 deletions(-) diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index df76417d6fd39..6d1efe1f17c1f 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -23,6 +23,7 @@ from superset import db, security_manager from superset.exceptions import SupersetSecurityException from superset.models.dashboard import Dashboard +from superset.security.guest_token import GuestTokenResourceType from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, @@ -34,27 +35,100 @@ "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=True, ) class TestGuestUserSecurity(SupersetTestCase): - def test_user_is_not_guest(self): + def test_is_guest_user__regular_user(self): is_guest = security_manager.is_guest_user(security_manager.find_user("admin")) self.assertFalse(is_guest) - def test_anon_is_not_guest(self): + def test_is_guest_user__anonymous(self): is_guest = security_manager.is_guest_user(security_manager.get_anonymous_user()) self.assertFalse(is_guest) - def test_guest_is_guest(self): + def test_is_guest_user__guest_user(self): authorized_guest = security_manager.get_guest_user_from_token( {"user": {}, "resources": []} ) is_guest = security_manager.is_guest_user(authorized_guest) self.assertTrue(is_guest) + def test_get_guest_user__regular_user(self): + g.user = security_manager.find_user("admin") + guest_user = security_manager.get_current_guest_user_if_guest() + self.assertIsNone(guest_user) + + def test_get_guest_user__anonymous_user(self): + g.user = security_manager.get_anonymous_user() + guest_user = security_manager.get_current_guest_user_if_guest() + self.assertIsNone(guest_user) + + def test_get_guest_user__guest_user(self): + g.user = security_manager.get_guest_user_from_token( + {"user": {}, "resources": []} + ) + guest_user = security_manager.get_current_guest_user_if_guest() + self.assertEqual(guest_user, g.user) + + def test_has_guest_access__regular_user(self): + g.user = security_manager.find_user("admin") + has_guest_access = security_manager.has_guest_access( + GuestTokenResourceType.DASHBOARD, 42 + ) + self.assertFalse(has_guest_access) + + def test_has_guest_access__anonymous_user(self): + g.user = security_manager.get_anonymous_user() + has_guest_access = security_manager.has_guest_access( + GuestTokenResourceType.DASHBOARD, 42 + ) + self.assertFalse(has_guest_access) + + def test_has_guest_access__authorized_guest_user(self): + g.user = security_manager.get_guest_user_from_token( + {"user": {}, "resources": [{"type": "dashboard", "id": 42}]} + ) + has_guest_access = security_manager.has_guest_access( + GuestTokenResourceType.DASHBOARD, 42 + ) + self.assertTrue(has_guest_access) + + def test_has_guest_access__authorized_guest_user__non_zero_resource_index(self): + g.user = security_manager.get_guest_user_from_token( + { + "user": {}, + "resources": [ + {"type": "dashboard", "id": 24}, + {"type": "dashboard", "id": 42}, + ], + } + ) + has_guest_access = security_manager.has_guest_access( + GuestTokenResourceType.DASHBOARD, 42 + ) + self.assertTrue(has_guest_access) + + def test_has_guest_access__unauthorized_guest_user__different_resource_id(self): + g.user = security_manager.get_guest_user_from_token( + {"user": {}, "resources": [{"type": "dashboard", "id": 24}]} + ) + has_guest_access = security_manager.has_guest_access( + GuestTokenResourceType.DASHBOARD, 42 + ) + self.assertFalse(has_guest_access) + + def test_has_guest_access__unauthorized_guest_user__different_resource_type(self): + g.user = security_manager.get_guest_user_from_token( + {"user": {}, "resources": [{"type": "dirt", "id": 42}]} + ) + has_guest_access = security_manager.has_guest_access( + GuestTokenResourceType.DASHBOARD, 42 + ) + self.assertFalse(has_guest_access) + @mock.patch.dict( "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=True, ) @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") -class TestGuestTokenDashboardAccess(SupersetTestCase): +class TestGuestUserDashboardAccess(SupersetTestCase): def setUp(self) -> None: self.dash = db.session.query(Dashboard).filter_by(slug="births").first() self.authorized_guest = security_manager.get_guest_user_from_token( From b73a81fdd97168a2bbb12b71ab529e29539e2aa5 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Wed, 19 Jan 2022 21:15:37 -0800 Subject: [PATCH 28/40] more testing and also some small refactoring --- superset/security/manager.py | 34 +++-- .../security/guest_token_security_tests.py | 131 +++++++++++------- 2 files changed, 99 insertions(+), 66 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 22ed41569761b..566e36e76130c 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1222,22 +1222,28 @@ def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: from superset.views.base import is_user_admin from superset.views.utils import is_owner - has_rbac_access = True - - if is_feature_enabled("DASHBOARD_RBAC"): - has_rbac_access = any( - dashboard_role.id - in [user_role.id for user_role in self.get_user_roles()] - for dashboard_role in dashboard.roles + def has_rbac_access() -> bool: + return ( + is_feature_enabled("DASHBOARD_RBAC") + and dashboard.published + and any( + dashboard_role.id + in [user_role.id for user_role in self.get_user_roles()] + for dashboard_role in dashboard.roles + ) ) - can_access = ( - is_user_admin() - or is_owner(dashboard, g.user) - or self.has_guest_access(GuestTokenResourceType.DASHBOARD, dashboard.id) - or (dashboard.published and has_rbac_access) - or (not dashboard.published and not dashboard.roles) - ) + if self.is_guest_user(): + can_access = self.has_guest_access( + GuestTokenResourceType.DASHBOARD, dashboard.id + ) + else: + can_access = ( + is_user_admin() + or is_owner(dashboard, g.user) + or has_rbac_access() + or (not dashboard.published and not dashboard.roles) + ) if not can_access: raise DashboardAccessDeniedError() diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index 6d1efe1f17c1f..9ca34198dbdf2 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -21,9 +21,11 @@ from flask import g from superset import db, security_manager +from superset.dashboards.commands.exceptions import DashboardAccessDeniedError from superset.exceptions import SupersetSecurityException from superset.models.dashboard import Dashboard from superset.security.guest_token import GuestTokenResourceType +from superset.sql_parse import Table from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, @@ -35,6 +37,16 @@ "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=True, ) class TestGuestUserSecurity(SupersetTestCase): + # This test doesn't use a dashboard fixture, the next test does. + # That way tests are faster. + + resource_id = 42 + + def authorized_guest(self): + return security_manager.get_guest_user_from_token( + {"user": {}, "resources": [{"type": "dashboard", "id": self.resource_id}]} + ) + def test_is_guest_user__regular_user(self): is_guest = security_manager.is_guest_user(security_manager.find_user("admin")) self.assertFalse(is_guest) @@ -44,12 +56,17 @@ def test_is_guest_user__anonymous(self): self.assertFalse(is_guest) def test_is_guest_user__guest_user(self): - authorized_guest = security_manager.get_guest_user_from_token( - {"user": {}, "resources": []} - ) - is_guest = security_manager.is_guest_user(authorized_guest) + is_guest = security_manager.is_guest_user(self.authorized_guest()) self.assertTrue(is_guest) + @mock.patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", + EMBEDDED_SUPERSET=False, + ) + def test_is_guest_user__flag_off(self): + is_guest = security_manager.is_guest_user(self.authorized_guest()) + self.assertFalse(is_guest) + def test_get_guest_user__regular_user(self): g.user = security_manager.find_user("admin") guest_user = security_manager.get_current_guest_user_if_guest() @@ -61,68 +78,76 @@ def test_get_guest_user__anonymous_user(self): self.assertIsNone(guest_user) def test_get_guest_user__guest_user(self): - g.user = security_manager.get_guest_user_from_token( - {"user": {}, "resources": []} - ) + g.user = self.authorized_guest() guest_user = security_manager.get_current_guest_user_if_guest() self.assertEqual(guest_user, g.user) def test_has_guest_access__regular_user(self): g.user = security_manager.find_user("admin") has_guest_access = security_manager.has_guest_access( - GuestTokenResourceType.DASHBOARD, 42 + GuestTokenResourceType.DASHBOARD, self.resource_id ) self.assertFalse(has_guest_access) def test_has_guest_access__anonymous_user(self): g.user = security_manager.get_anonymous_user() has_guest_access = security_manager.has_guest_access( - GuestTokenResourceType.DASHBOARD, 42 + GuestTokenResourceType.DASHBOARD, self.resource_id ) self.assertFalse(has_guest_access) def test_has_guest_access__authorized_guest_user(self): - g.user = security_manager.get_guest_user_from_token( - {"user": {}, "resources": [{"type": "dashboard", "id": 42}]} - ) + g.user = self.authorized_guest() has_guest_access = security_manager.has_guest_access( - GuestTokenResourceType.DASHBOARD, 42 + GuestTokenResourceType.DASHBOARD, self.resource_id ) self.assertTrue(has_guest_access) def test_has_guest_access__authorized_guest_user__non_zero_resource_index(self): - g.user = security_manager.get_guest_user_from_token( - { - "user": {}, - "resources": [ - {"type": "dashboard", "id": 24}, - {"type": "dashboard", "id": 42}, - ], - } - ) + guest = self.authorized_guest() + guest.resources = [ + {"type": "dashboard", "id": self.resource_id - 1} + ] + guest.resources + g.user = guest + has_guest_access = security_manager.has_guest_access( - GuestTokenResourceType.DASHBOARD, 42 + GuestTokenResourceType.DASHBOARD, self.resource_id ) self.assertTrue(has_guest_access) def test_has_guest_access__unauthorized_guest_user__different_resource_id(self): g.user = security_manager.get_guest_user_from_token( - {"user": {}, "resources": [{"type": "dashboard", "id": 24}]} + { + "user": {}, + "resources": [{"type": "dashboard", "id": self.resource_id - 1}], + } ) has_guest_access = security_manager.has_guest_access( - GuestTokenResourceType.DASHBOARD, 42 + GuestTokenResourceType.DASHBOARD, self.resource_id ) self.assertFalse(has_guest_access) def test_has_guest_access__unauthorized_guest_user__different_resource_type(self): g.user = security_manager.get_guest_user_from_token( - {"user": {}, "resources": [{"type": "dirt", "id": 42}]} + {"user": {}, "resources": [{"type": "dirt", "id": self.resource_id}]} ) has_guest_access = security_manager.has_guest_access( - GuestTokenResourceType.DASHBOARD, 42 + GuestTokenResourceType.DASHBOARD, self.resource_id ) self.assertFalse(has_guest_access) + def test_get_guest_user_roles_explicit(self): + guest = self.authorized_guest() + roles = security_manager.get_user_roles(guest) + self.assertEqual(guest.roles, roles) + + def test_get_guest_user_roles_implicit(self): + guest = self.authorized_guest() + g.user = guest + + roles = security_manager.get_user_roles() + self.assertEqual(guest.roles, roles) + @mock.patch.dict( "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=True, @@ -138,46 +163,48 @@ def setUp(self) -> None: {"user": {}, "resources": [{"type": "dashboard", "id": self.dash.id + 1}]} ) - def test_dashboard_access_filter_as_guest(self): - dataset = list(self.dash.datasources)[0] + def test_chart_raise_for_access_as_guest(self): + chart = self.dash.slices[0] g.user = self.authorized_guest - security_manager.raise_for_access(datasource=dataset) + security_manager.raise_for_access(viz=chart) - def test_dashboard_access_filter_as_unauthorized_guest(self): - dataset = list(self.dash.datasources)[0] + def test_chart_raise_for_access_as_unauthorized_guest(self): + chart = self.dash.slices[0] g.user = self.unauthorized_guest with self.assertRaises(SupersetSecurityException): - security_manager.raise_for_access(datasource=dataset) + security_manager.raise_for_access(viz=chart) - def test_chart_access_filter_as_guest(self): - chart = self.dash.slices[0] + def test_dataset_raise_for_access_as_guest(self): + dataset = self.dash.slices[0].datasource g.user = self.authorized_guest - security_manager.raise_for_access(viz=chart) + security_manager.raise_for_access(datasource=dataset) - def test_chart_access_filter_as_unauthorized_guest(self): - chart = self.dash.slices[0] + def test_dataset_raise_for_access_as_unauthorized_guest(self): + dataset = self.dash.slices[0].datasource g.user = self.unauthorized_guest with self.assertRaises(SupersetSecurityException): - security_manager.raise_for_access(viz=chart) + security_manager.raise_for_access(datasource=dataset) - def test_get_guest_user_roles_explicit(self): - roles = security_manager.get_user_roles(self.authorized_guest) - self.assertEqual(self.authorized_guest.roles, roles) + def test_guest_token_does_not_grant_access_to_underlying_table(self): + sqla_table = self.dash.slices[0].table + table = Table(table=sqla_table.table_name) - def test_get_guest_user_roles_implicit(self): g.user = self.authorized_guest - roles = security_manager.get_user_roles(self.authorized_guest) - self.assertEqual(self.authorized_guest.roles, roles) + with self.assertRaises(Exception): + security_manager.raise_for_access(table=table, database=sqla_table.database) - @mock.patch.dict( - "superset.extensions.feature_flag_manager._feature_flags", - EMBEDDED_SUPERSET=False, - ) - def test_is_guest_user_flag_off(self): - is_guest = security_manager.is_guest_user(self.authorized_guest) - self.assertFalse(is_guest) + def test_raise_for_dashboard_access_as_guest(self): + g.user = self.authorized_guest + + security_manager.raise_for_dashboard_access(self.dash) + + def test_raise_for_dashboard_access_as_unauthorized_guest(self): + g.user = self.unauthorized_guest + + with self.assertRaises(DashboardAccessDeniedError): + security_manager.raise_for_dashboard_access(self.dash) From eccec4d4ba41ab2285068d439a6f8d73f6c29c08 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 20 Jan 2022 13:01:35 -0800 Subject: [PATCH 29/40] move validation out of parsing --- superset/security/manager.py | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 566e36e76130c..14b6d89601164 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1310,13 +1310,17 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]: try: token = self.parse_jwt_guest_token(raw_token) + if token.get("user") is None: + raise ValueError("Guest token does not contain a user claim") + if token.get("resources") is None: + raise ValueError("Guest token does not contain a resources claim") except Exception: # pylint: disable=broad-except # The login manager will handle sending 401s. # We don't need to send a special error message. logger.warning("Invalid guest token", exc_info=True) return None else: - return self.get_guest_user_from_token(token) + return self.get_guest_user_from_token(cast(GuestToken, token)) def get_guest_user_from_token(self, token: GuestToken) -> GuestUser: return self.guest_user_cls( @@ -1324,24 +1328,15 @@ def get_guest_user_from_token(self, token: GuestToken) -> GuestUser: ) @staticmethod - def parse_jwt_guest_token(raw_token: str) -> GuestToken: + def parse_jwt_guest_token(raw_token: str) -> Dict[str, Any]: """ - Parses and validates a guest token. - Raises an error if the jwt is invalid: - if it is not signed with our secret, - or if required claims are not present. + Parses a guest token. Raises an error if the jwt fails standard claims checks. :param raw_token: the token gotten from the request :return: the same token that was passed in, tested but unchanged """ secret = current_app.config["GUEST_TOKEN_JWT_SECRET"] algo = current_app.config["GUEST_TOKEN_JWT_ALGO"] - - token = jwt.decode(raw_token, secret, algorithms=[algo]) - if token.get("user") is None: - raise ValueError("Guest token does not contain a user claim") - if token.get("resources") is None: - raise ValueError("Guest token does not contain a resources claim") - return cast(GuestToken, token) + return jwt.decode(raw_token, secret, algorithms=[algo]) @staticmethod def is_guest_user(user: Optional[Any] = None) -> bool: From e833ef57bc77a051c013c89831585c90237106fb Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 20 Jan 2022 15:13:18 -0800 Subject: [PATCH 30/40] fix dashboard access check again --- superset/security/manager.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 14b6d89601164..5993f952f3473 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1212,6 +1212,8 @@ def raise_for_user_activity_access(user_id: int) -> None: def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: """ Raise an exception if the user cannot access the dashboard. + This does not check for the required role/permission pairs, + it only concerns itself with entity relationships. :param dashboard: Dashboard the user wants access to :raises DashboardAccessDeniedError: If the user cannot access the resource @@ -1223,14 +1225,10 @@ def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: from superset.views.utils import is_owner def has_rbac_access() -> bool: - return ( - is_feature_enabled("DASHBOARD_RBAC") - and dashboard.published - and any( - dashboard_role.id - in [user_role.id for user_role in self.get_user_roles()] - for dashboard_role in dashboard.roles - ) + return (not is_feature_enabled("DASHBOARD_RBAC")) or any( + dashboard_role.id + in [user_role.id for user_role in self.get_user_roles()] + for dashboard_role in dashboard.roles ) if self.is_guest_user(): @@ -1241,7 +1239,7 @@ def has_rbac_access() -> bool: can_access = ( is_user_admin() or is_owner(dashboard, g.user) - or has_rbac_access() + or (dashboard.published and has_rbac_access()) or (not dashboard.published and not dashboard.roles) ) From 1527c41193a6f09a1bb49dcc90c4a7db8fa7fa24 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Tue, 4 Jan 2022 15:41:56 -0800 Subject: [PATCH 31/40] rls rules for guest tokens --- superset/connectors/sqla/models.py | 20 ++++++-- superset/security/api.py | 47 +++++++++++++++---- superset/security/guest_token.py | 8 +++- superset/security/manager.py | 29 +++++++++--- tests/integration_tests/security/api_tests.py | 8 +++- 5 files changed, 90 insertions(+), 22 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 22516934db139..311889b090596 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -963,14 +963,28 @@ def _get_sqla_row_level_filters( :returns: A list of SQL clauses to be ANDed together. :rtype: List[str] """ - filters_grouped: Dict[Union[int, str], List[str]] = defaultdict(list) + all_filters: List[TextClause] = [] + filter_groups: Dict[Union[int, str], List[TextClause]] = defaultdict(list) try: for filter_ in security_manager.get_rls_filters(self): clause = self.text( f"({template_processor.process_template(filter_.clause)})" ) - filters_grouped[filter_.group_key or filter_.id].append(clause) - return [or_(*clauses) for clauses in filters_grouped.values()] + if filter_.group_key: + filter_groups[filter_.group_key].append(clause) + else: + all_filters.append(clause) + + if is_feature_enabled("EMBEDDED_SUPERSET"): + for rule in security_manager.get_guest_rls_filters(self): + clause = self.text( + f"({template_processor.process_template(rule['clause'])})" + ) + all_filters.append(clause) + + grouped_filters = [or_(*clauses) for clauses in filter_groups.values()] + all_filters.extend(grouped_filters) + return all_filters except TemplateError as ex: raise QueryObjectValidationError( _("Error in jinja expression in RLS filters: %(msg)s", msg=ex.message,) diff --git a/superset/security/api.py b/superset/security/api.py index 54efcd07e0dbd..97720a64f3c61 100644 --- a/superset/security/api.py +++ b/superset/security/api.py @@ -14,35 +14,62 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +import dataclasses import logging +from enum import Enum +from typing import Any, Dict from flask import request, Response from flask_appbuilder import expose from flask_appbuilder.api import BaseApi, safe from flask_appbuilder.security.decorators import permission_name, protect from flask_wtf.csrf import generate_csrf -from marshmallow import fields, Schema, ValidationError +from marshmallow import EXCLUDE, fields, post_load, Schema, ValidationError +from marshmallow_enum import EnumField from superset.extensions import event_logger +from superset.security.guest_token import GuestTokenResourceType logger = logging.getLogger(__name__) -class UserSchema(Schema): +class PermissiveSchema(Schema): + """ + A marshmallow schema that ignores unexpected fields, instead of throwing an error. + """ + + class Meta: + unknown = EXCLUDE + + +class UserSchema(PermissiveSchema): username = fields.String() first_name = fields.String() last_name = fields.String() -class ResourceSchema(Schema): - type = fields.String(required=True) # todo figure out how to make this an enum +class ResourceSchema(PermissiveSchema): + type = EnumField(GuestTokenResourceType, by_value=True, required=True) id = fields.String(required=True) - rls = fields.String() + + @post_load + def convert_enum_to_value( + self, data: Dict[str, Any], **kwargs: Any + ) -> Dict[str, Any]: + # we don't care about the enum, we want the value inside + data["type"] = data["type"].value + return data + + +class RlsRuleSchema(PermissiveSchema): + dataset = fields.Integer(required=True) # todo make this optional when possible + clause = fields.String(required=True) # todo other options? -class GuestTokenCreateSchema(Schema): +class GuestTokenCreateSchema(PermissiveSchema): user = fields.Nested(UserSchema) - resource = fields.Nested(ResourceSchema, required=True) + resources = fields.List(fields.Nested(ResourceSchema), required=True) + rls = fields.List(fields.Nested(RlsRuleSchema), required=True) guest_token_create_schema = GuestTokenCreateSchema() @@ -117,12 +144,12 @@ def guest_token(self) -> Response: """ try: body = guest_token_create_schema.load(request.json) - # validate stuff: - # make sure the resource id is valid + # todo validate stuff: + # make sure the resource ids are valid # make sure username doesn't reference an existing user # check rls rules for validity? token = self.appbuilder.sm.create_guest_access_token( - body["user"], [body["resource"]] + body["user"], body["resources"], body["rls"] ) return self.response(200, token=token) except ValidationError as error: diff --git a/superset/security/guest_token.py b/superset/security/guest_token.py index 60add8175400d..c1f7ea9e9677c 100644 --- a/superset/security/guest_token.py +++ b/superset/security/guest_token.py @@ -34,17 +34,22 @@ class GuestTokenResourceType(Enum): class GuestTokenResource(TypedDict): type: GuestTokenResourceType id: Union[str, int] - rls: Optional[str] GuestTokenResources = List[GuestTokenResource] +class GuestTokenRlsRule(TypedDict): + dataset: str + clause: str + + class GuestToken(TypedDict): iat: float exp: float user: GuestTokenUser resources: GuestTokenResources + rls_rules: List[GuestTokenRlsRule] class GuestUser(AnonymousUserMixin): @@ -79,3 +84,4 @@ def __init__(self, token: GuestToken, roles: List[Role]): self.last_name = user.get("last_name", "User") self.roles = roles self.resources = token["resources"] + self.rls = token["rls_rules"] diff --git a/superset/security/manager.py b/superset/security/manager.py index 5993f952f3473..161bdcff1e3f0 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -69,6 +69,7 @@ GuestToken, GuestTokenResources, GuestTokenResourceType, + GuestTokenRlsRule, GuestTokenUser, GuestUser, ) @@ -1111,6 +1112,18 @@ def get_user_roles(self, user: Optional[User] = None) -> List[Role]: return [self.get_public_role()] if public_role else [] return user.roles + def get_guest_rls_filters(self, table: "BaseDatasource") -> List[GuestTokenRlsRule]: + """ + Retrieves the row level security filters for the current user and the table, + if the user is authenticated with a guest token. + :param table: The table to check against + :return: A list of filters + """ + guest_user = self.get_current_guest_user_if_guest() + if guest_user: + return [rule for rule in guest_user.rls if rule["dataset"] == table.id] + return [] + def get_rls_filters(self, table: "BaseDatasource") -> List[SqlaQuery]: """ Retrieves the appropriate row level security filters for the current user and @@ -1119,7 +1132,7 @@ def get_rls_filters(self, table: "BaseDatasource") -> List[SqlaQuery]: :param table: The table to check against :returns: A list of filters """ - if hasattr(g, "user") and hasattr(g.user, "id"): + if hasattr(g, "user"): # pylint: disable=import-outside-toplevel from superset.connectors.sqla.models import ( RLSFilterRoles, @@ -1127,11 +1140,7 @@ def get_rls_filters(self, table: "BaseDatasource") -> List[SqlaQuery]: RowLevelSecurityFilter, ) - user_roles = ( - self.get_session.query(assoc_user_role.c.role_id) - .filter(assoc_user_role.c.user_id == g.user.get_id()) - .subquery() - ) + user_roles = [role.id for role in self.get_user_roles()] regular_filter_roles = ( self.get_session.query(RLSFilterRoles.c.rls_filter_id) .join(RowLevelSecurityFilter) @@ -1274,7 +1283,10 @@ def _get_current_epoch_time() -> float: return time.time() def create_guest_access_token( - self, user: GuestTokenUser, resources: GuestTokenResources + self, + user: GuestTokenUser, + resources: GuestTokenResources, + rls: List[GuestTokenRlsRule], ) -> bytes: secret = current_app.config["GUEST_TOKEN_JWT_SECRET"] algo = current_app.config["GUEST_TOKEN_JWT_ALGO"] @@ -1286,6 +1298,7 @@ def create_guest_access_token( claims = { "user": user, "resources": resources, + "rls_rules": rls, # standard jwt claims: "iat": now, # issued at "exp": exp, # expiration time @@ -1312,6 +1325,8 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]: raise ValueError("Guest token does not contain a user claim") if token.get("resources") is None: raise ValueError("Guest token does not contain a resources claim") + if token.get("rls_rules") is None: + raise ValueError("Guest token does not contain an rls_rules claim") except Exception: # pylint: disable=broad-except # The login manager will handle sending 401s. # We don't need to send a special error message. diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index fcacd7ce668f2..dc413870c40e2 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -77,7 +77,7 @@ def test_post_guest_token_unauthorized(self): response = self.client.post(self.uri) self.assert403(response) - def test_post_embed_token_authorized(self): + def test_post_guest_token_authorized(self): self.login(username="admin") user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} resource = {"type": "dashboard", "id": "blah", "rls": "1 = 1"} @@ -92,3 +92,9 @@ def test_post_embed_token_authorized(self): self.assertEqual(user, decoded_token["user"]) self.assertEqual(resource, decoded_token["resources"][0]) + + def test_post_guest_token_multiple_resources(self): + ... + + def test_post_guest_token_rls(self): + ... From 07debeedc3530b21810821209a77ff0be44883c2 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 20 Jan 2022 10:43:03 -0800 Subject: [PATCH 32/40] test guest token rls rules --- superset/security/manager.py | 10 +- .../security/row_level_security_tests.py | 150 +++++++++++++----- 2 files changed, 115 insertions(+), 45 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 161bdcff1e3f0..01a5d084b1f80 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1112,16 +1112,18 @@ def get_user_roles(self, user: Optional[User] = None) -> List[Role]: return [self.get_public_role()] if public_role else [] return user.roles - def get_guest_rls_filters(self, table: "BaseDatasource") -> List[GuestTokenRlsRule]: + def get_guest_rls_filters( + self, dataset: "BaseDatasource" + ) -> List[GuestTokenRlsRule]: """ - Retrieves the row level security filters for the current user and the table, + Retrieves the row level security filters for the current user and the dataset, if the user is authenticated with a guest token. - :param table: The table to check against + :param dataset: The dataset to check against :return: A list of filters """ guest_user = self.get_current_guest_user_if_guest() if guest_user: - return [rule for rule in guest_user.rls if rule["dataset"] == table.id] + return [rule for rule in guest_user.rls if rule["dataset"] == dataset.id] return [] def get_rls_filters(self, table: "BaseDatasource") -> List[SqlaQuery]: diff --git a/tests/integration_tests/security/row_level_security_tests.py b/tests/integration_tests/security/row_level_security_tests.py index 665666cb61f5b..d933fc3fec253 100644 --- a/tests/integration_tests/security/row_level_security_tests.py +++ b/tests/integration_tests/security/row_level_security_tests.py @@ -16,13 +16,15 @@ # under the License. # isort:skip_file import re -from typing import Any, Dict +from typing import Any, Dict, List +from unittest import mock import pytest from flask import g from superset import db, security_manager from superset.connectors.sqla.models import RowLevelSecurityFilter, SqlaTable +from superset.security.guest_token import GuestTokenRlsRule, GuestTokenResourceType from ..base_tests import SupersetTestCase from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, @@ -38,39 +40,39 @@ ) +query_obj: Dict[str, Any] = dict( + groupby=[], + metrics=None, + filter=[], + is_timeseries=False, + columns=["value"], + granularity=None, + from_dttm=None, + to_dttm=None, + extras={}, +) +NAME_AB_ROLE = "NameAB" +NAME_Q_ROLE = "NameQ" +NAMES_A_REGEX = re.compile(r"name like 'A%'") +NAMES_B_REGEX = re.compile(r"name like 'B%'") +NAMES_Q_REGEX = re.compile(r"name like 'Q%'") +BASE_FILTER_REGEX = re.compile(r"gender = 'boy'") + + class TestRowLevelSecurity(SupersetTestCase): """ Testing Row Level Security """ - rls_entry = None - query_obj: Dict[str, Any] = dict( - groupby=[], - metrics=None, - filter=[], - is_timeseries=False, - columns=["value"], - granularity=None, - from_dttm=None, - to_dttm=None, - extras={}, - ) - NAME_AB_ROLE = "NameAB" - NAME_Q_ROLE = "NameQ" - NAMES_A_REGEX = re.compile(r"name like 'A%'") - NAMES_B_REGEX = re.compile(r"name like 'B%'") - NAMES_Q_REGEX = re.compile(r"name like 'Q%'") - BASE_FILTER_REGEX = re.compile(r"gender = 'boy'") - def setUp(self): session = db.session # Create roles - security_manager.add_role(self.NAME_AB_ROLE) - security_manager.add_role(self.NAME_Q_ROLE) + security_manager.add_role(NAME_AB_ROLE) + security_manager.add_role(NAME_Q_ROLE) gamma_user = security_manager.find_user(username="gamma") - gamma_user.roles.append(security_manager.find_role(self.NAME_AB_ROLE)) - gamma_user.roles.append(security_manager.find_role(self.NAME_Q_ROLE)) + gamma_user.roles.append(security_manager.find_role(NAME_AB_ROLE)) + gamma_user.roles.append(security_manager.find_role(NAME_Q_ROLE)) self.create_user_with_roles("NoRlsRoleUser", ["Gamma"]) session.commit() @@ -144,8 +146,8 @@ def tearDown(self): def test_rls_filter_alters_energy_query(self): g.user = self.get_user(username="alpha") tbl = self.get_table(name="energy_usage") - sql = tbl.get_query_str(self.query_obj) - assert tbl.get_extra_cache_keys(self.query_obj) == [1] + sql = tbl.get_query_str(query_obj) + assert tbl.get_extra_cache_keys(query_obj) == [1] assert "value > 1" in sql @pytest.mark.usefixtures("load_energy_table_with_slice") @@ -154,8 +156,8 @@ def test_rls_filter_doesnt_alter_energy_query(self): username="admin" ) # self.login() doesn't actually set the user tbl = self.get_table(name="energy_usage") - sql = tbl.get_query_str(self.query_obj) - assert tbl.get_extra_cache_keys(self.query_obj) == [] + sql = tbl.get_query_str(query_obj) + assert tbl.get_extra_cache_keys(query_obj) == [] assert "value > 1" not in sql @pytest.mark.usefixtures("load_unicode_dashboard_with_slice") @@ -164,15 +166,15 @@ def test_multiple_table_filter_alters_another_tables_query(self): username="alpha" ) # self.login() doesn't actually set the user tbl = self.get_table(name="unicode_test") - sql = tbl.get_query_str(self.query_obj) - assert tbl.get_extra_cache_keys(self.query_obj) == [1] + sql = tbl.get_query_str(query_obj) + assert tbl.get_extra_cache_keys(query_obj) == [1] assert "value > 1" in sql @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_rls_filter_alters_gamma_birth_names_query(self): g.user = self.get_user(username="gamma") tbl = self.get_table(name="birth_names") - sql = tbl.get_query_str(self.query_obj) + sql = tbl.get_query_str(query_obj) # establish that the filters are grouped together correctly with # ANDs, ORs and parens in the correct place @@ -185,23 +187,89 @@ def test_rls_filter_alters_gamma_birth_names_query(self): def test_rls_filter_alters_no_role_user_birth_names_query(self): g.user = self.get_user(username="NoRlsRoleUser") tbl = self.get_table(name="birth_names") - sql = tbl.get_query_str(self.query_obj) + sql = tbl.get_query_str(query_obj) # gamma's filters should not be present query - assert not self.NAMES_A_REGEX.search(sql) - assert not self.NAMES_B_REGEX.search(sql) - assert not self.NAMES_Q_REGEX.search(sql) + assert not NAMES_A_REGEX.search(sql) + assert not NAMES_B_REGEX.search(sql) + assert not NAMES_Q_REGEX.search(sql) # base query should be present - assert self.BASE_FILTER_REGEX.search(sql) + assert BASE_FILTER_REGEX.search(sql) @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_rls_filter_doesnt_alter_admin_birth_names_query(self): g.user = self.get_user(username="admin") tbl = self.get_table(name="birth_names") - sql = tbl.get_query_str(self.query_obj) + sql = tbl.get_query_str(query_obj) # no filters are applied for admin user - assert not self.NAMES_A_REGEX.search(sql) - assert not self.NAMES_B_REGEX.search(sql) - assert not self.NAMES_Q_REGEX.search(sql) - assert not self.BASE_FILTER_REGEX.search(sql) + assert not NAMES_A_REGEX.search(sql) + assert not NAMES_B_REGEX.search(sql) + assert not NAMES_Q_REGEX.search(sql) + assert not BASE_FILTER_REGEX.search(sql) + + +RLS_ALICE_REGEX = re.compile(r"name = 'Alice'") +RLS_GENDER_REGEX = re.compile(r"gender = 'girl'") + + +@mock.patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=True, +) +class GuestTokenRowLevelSecurityTests(SupersetTestCase): + def default_rls_rule(self): + return { + "dataset": self.get_table(name="birth_names").id, + "clause": "name = 'Alice'", + } + + def guest_user_with_rls(self, rules: List[Any] = None): + if rules is None: + rules = [self.default_rls_rule()] + return security_manager.get_guest_user_from_token( + { + "user": {}, + "resources": [{"type": GuestTokenResourceType.DASHBOARD.value}], + "rls_rules": rules, + } + ) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_rls_filter_alters_query(self): + g.user = self.guest_user_with_rls() + tbl = self.get_table(name="birth_names") + sql = tbl.get_query_str(query_obj) + + self.assertRegexpMatches(sql, RLS_ALICE_REGEX) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_rls_filter_does_not_alter_unrelated_query(self): + g.user = self.guest_user_with_rls( + rules=[ + { + "dataset": self.get_table(name="birth_names").id + 1, + "clause": "name = 'Alice'", + } + ] + ) + tbl = self.get_table(name="birth_names") + sql = tbl.get_query_str(query_obj) + + self.assertNotRegexpMatches(sql, RLS_ALICE_REGEX) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_multiple_rls_filters_are_unionized(self): + g.user = self.guest_user_with_rls( + rules=[ + self.default_rls_rule(), + { + "dataset": self.get_table(name="birth_names").id, + "clause": "gender = 'girl'", + }, + ] + ) + tbl = self.get_table(name="birth_names") + sql = tbl.get_query_str(query_obj) + + self.assertRegexpMatches(sql, RLS_ALICE_REGEX) + self.assertRegexpMatches(sql, RLS_GENDER_REGEX) From f16ae106719d4ab7492c4d97828f51fdeb11a4ad Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 20 Jan 2022 12:29:28 -0800 Subject: [PATCH 33/40] more flexible rls rules --- superset/security/api.py | 2 +- superset/security/guest_token.py | 2 +- superset/security/manager.py | 7 +++- .../security/row_level_security_tests.py | 37 +++++++++++++++++-- 4 files changed, 41 insertions(+), 7 deletions(-) diff --git a/superset/security/api.py b/superset/security/api.py index 97720a64f3c61..e1ba8c50c25e6 100644 --- a/superset/security/api.py +++ b/superset/security/api.py @@ -62,7 +62,7 @@ def convert_enum_to_value( class RlsRuleSchema(PermissiveSchema): - dataset = fields.Integer(required=True) # todo make this optional when possible + dataset = fields.Integer() clause = fields.String(required=True) # todo other options? diff --git a/superset/security/guest_token.py b/superset/security/guest_token.py index c1f7ea9e9677c..13a88a10fe2e4 100644 --- a/superset/security/guest_token.py +++ b/superset/security/guest_token.py @@ -40,7 +40,7 @@ class GuestTokenResource(TypedDict): class GuestTokenRlsRule(TypedDict): - dataset: str + dataset: Optional[str] clause: str diff --git a/superset/security/manager.py b/superset/security/manager.py index 01a5d084b1f80..5ca81b2a9546e 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1123,7 +1123,12 @@ def get_guest_rls_filters( """ guest_user = self.get_current_guest_user_if_guest() if guest_user: - return [rule for rule in guest_user.rls if rule["dataset"] == dataset.id] + return [ + rule + for rule in guest_user.rls + if not rule.get("dataset") + or str(rule.get("dataset")) == str(dataset.id) + ] return [] def get_rls_filters(self, table: "BaseDatasource") -> List[SqlaQuery]: diff --git a/tests/integration_tests/security/row_level_security_tests.py b/tests/integration_tests/security/row_level_security_tests.py index d933fc3fec253..b7db7242af037 100644 --- a/tests/integration_tests/security/row_level_security_tests.py +++ b/tests/integration_tests/security/row_level_security_tests.py @@ -16,7 +16,7 @@ # under the License. # isort:skip_file import re -from typing import Any, Dict, List +from typing import Any, Dict, List, Optional from unittest import mock import pytest @@ -24,7 +24,11 @@ from superset import db, security_manager from superset.connectors.sqla.models import RowLevelSecurityFilter, SqlaTable -from superset.security.guest_token import GuestTokenRlsRule, GuestTokenResourceType +from superset.security.guest_token import ( + GuestTokenRlsRule, + GuestTokenResourceType, + GuestUser, +) from ..base_tests import SupersetTestCase from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, @@ -210,7 +214,7 @@ def test_rls_filter_doesnt_alter_admin_birth_names_query(self): RLS_ALICE_REGEX = re.compile(r"name = 'Alice'") -RLS_GENDER_REGEX = re.compile(r"gender = 'girl'") +RLS_GENDER_REGEX = re.compile(r"AND \(gender = 'girl'\)") @mock.patch.dict( @@ -223,7 +227,7 @@ def default_rls_rule(self): "clause": "name = 'Alice'", } - def guest_user_with_rls(self, rules: List[Any] = None): + def guest_user_with_rls(self, rules: Optional[List[Any]] = None) -> GuestUser: if rules is None: rules = [self.default_rls_rule()] return security_manager.get_guest_user_from_token( @@ -273,3 +277,28 @@ def test_multiple_rls_filters_are_unionized(self): self.assertRegexpMatches(sql, RLS_ALICE_REGEX) self.assertRegexpMatches(sql, RLS_GENDER_REGEX) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @pytest.mark.usefixtures("load_energy_table_with_slice") + def test_rls_filter_for_all_datasets(self): + births = self.get_table(name="birth_names") + energy = self.get_table(name="energy_usage") + guest = self.guest_user_with_rls(rules=[{"clause": "name = 'Alice'"}]) + guest.resources.append({type: "dashboard", id: energy.id}) + g.user = guest + births_sql = births.get_query_str(query_obj) + energy_sql = energy.get_query_str(query_obj) + + self.assertRegexpMatches(births_sql, RLS_ALICE_REGEX) + self.assertRegexpMatches(energy_sql, RLS_ALICE_REGEX) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_dataset_id_can_be_string(self): + dataset = self.get_table(name="birth_names") + str_id = str(dataset.id) + g.user = self.guest_user_with_rls( + rules=[{"dataset": str_id, "clause": "name = 'Alice'"}] + ) + sql = dataset.get_query_str(query_obj) + + self.assertRegexpMatches(sql, RLS_ALICE_REGEX) From bfafc130e2ac975ba05ecef7eb04428ce92ab111 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 20 Jan 2022 16:53:46 -0800 Subject: [PATCH 34/40] lint --- superset/security/api.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/superset/security/api.py b/superset/security/api.py index e1ba8c50c25e6..b919e29f78ddd 100644 --- a/superset/security/api.py +++ b/superset/security/api.py @@ -14,9 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -import dataclasses import logging -from enum import Enum from typing import Any, Dict from flask import request, Response @@ -38,7 +36,7 @@ class PermissiveSchema(Schema): A marshmallow schema that ignores unexpected fields, instead of throwing an error. """ - class Meta: + class Meta: # pylint: disable=too-few-public-methods unknown = EXCLUDE @@ -53,8 +51,8 @@ class ResourceSchema(PermissiveSchema): id = fields.String(required=True) @post_load - def convert_enum_to_value( - self, data: Dict[str, Any], **kwargs: Any + def convert_enum_to_value( # pylint: disable=no-self-use + self, data: Dict[str, Any], **kwargs: Any # pylint: disable=unused-argument ) -> Dict[str, Any]: # we don't care about the enum, we want the value inside data["type"] = data["type"].value From bf302f41f10202c171f48fcf20b1f85fdf1cf386 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 20 Jan 2022 18:19:43 -0800 Subject: [PATCH 35/40] fix tests --- tests/integration_tests/security_tests.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 48ded730aad1b..55451af20dc44 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -1161,14 +1161,22 @@ class FakeRequest: class TestGuestTokens(SupersetTestCase): + def create_guest_token(self): + user = {"username": "test_guest"} + resources = [{"some": "resource"}] + rls = [{"dataset": 1, "clause": "access = 1"}] + return security_manager.create_guest_access_token(user, resources, rls) + @patch("superset.security.SupersetSecurityManager._get_current_epoch_time") def test_create_guest_access_token(self, get_time_mock): now = time.time() get_time_mock.return_value = now # so we know what it should = - user = {"any": "data"} + + user = {"username": "test_guest"} resources = [{"some": "resource"}] + rls = [{"dataset": 1, "clause": "access = 1"}] + token = security_manager.create_guest_access_token(user, resources, rls) - token = security_manager.create_guest_access_token(user, resources) # unfortunately we cannot mock time in the jwt lib decoded_token = jwt.decode( token, @@ -1185,9 +1193,7 @@ def test_create_guest_access_token(self, get_time_mock): ) def test_get_guest_user(self): - user = {"username": "test_guest"} - resources = [{"type": "dashboard", "id": 1}] - token = security_manager.create_guest_access_token(user, resources) + token = self.create_guest_token() fake_request = FakeRequest() fake_request.headers[current_app.config["GUEST_TOKEN_HEADER_NAME"]] = token @@ -1202,9 +1208,7 @@ def test_get_guest_user_expired_token(self, get_time_mock): get_time_mock.return_value = ( time.time() - (self.app.config["GUEST_TOKEN_JWT_EXP_SECONDS"] * 1000) - 1 ) - user = {"username": "test_guest"} - resources = [{"type": "dashboard", "id": 1}] - token = security_manager.create_guest_access_token(user, resources) + token = self.create_guest_token() fake_request = FakeRequest() fake_request.headers[current_app.config["GUEST_TOKEN_HEADER_NAME"]] = token From bdd842baf904c73a5597c52ba7531b65c709c95c Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 20 Jan 2022 21:17:15 -0800 Subject: [PATCH 36/40] fix test --- tests/integration_tests/security/api_tests.py | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index dc413870c40e2..d7b365985d9b2 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -80,21 +80,16 @@ def test_post_guest_token_unauthorized(self): def test_post_guest_token_authorized(self): self.login(username="admin") user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} - resource = {"type": "dashboard", "id": "blah", "rls": "1 = 1"} - params = {"user": user, "resource": resource} + resource = {"type": "dashboard", "id": "blah"} + rls_rule = {"dataset": 1, "clause": "1=1"} + params = {"user": user, "resources": [resource], "rls": [rls_rule]} response = self.client.post( self.uri, data=json.dumps(params), content_type="application/json" ) + self.assert200(response) token = json.loads(response.data)["token"] decoded_token = jwt.decode(token, self.app.config["GUEST_TOKEN_JWT_SECRET"]) - self.assertEqual(user, decoded_token["user"]) self.assertEqual(resource, decoded_token["resources"][0]) - - def test_post_guest_token_multiple_resources(self): - ... - - def test_post_guest_token_rls(self): - ... From 28b69df495e241243b0d7d1e5c47dd7ab9ac191e Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 20 Jan 2022 21:51:57 -0800 Subject: [PATCH 37/40] defaults --- superset/security/guest_token.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/security/guest_token.py b/superset/security/guest_token.py index 13a88a10fe2e4..af86da326288c 100644 --- a/superset/security/guest_token.py +++ b/superset/security/guest_token.py @@ -84,4 +84,4 @@ def __init__(self, token: GuestToken, roles: List[Role]): self.last_name = user.get("last_name", "User") self.roles = roles self.resources = token["resources"] - self.rls = token["rls_rules"] + self.rls = token.get("rls_rules", []) From 1c41a74561f19758e7827703e47826b65efcc4ca Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Fri, 21 Jan 2022 15:22:41 -0800 Subject: [PATCH 38/40] fix some tests --- .../integration_tests/security/row_level_security_tests.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration_tests/security/row_level_security_tests.py b/tests/integration_tests/security/row_level_security_tests.py index 0542a13f3d2b4..06610bbd06de8 100644 --- a/tests/integration_tests/security/row_level_security_tests.py +++ b/tests/integration_tests/security/row_level_security_tests.py @@ -73,10 +73,10 @@ def setUp(self): # Create roles self.role_ab = security_manager.add_role(self.NAME_AB_ROLE) - security_manager.add_role(self.NAME_Q_ROLE) + self.role_q = security_manager.add_role(self.NAME_Q_ROLE) gamma_user = security_manager.find_user(username="gamma") - gamma_user.roles.append(security_manager.find_role(self.NAME_AB_ROLE)) - gamma_user.roles.append(security_manager.find_role(self.NAME_Q_ROLE)) + gamma_user.roles.append(self.role_ab) + gamma_user.roles.append(self.role_q) self.create_user_with_roles("NoRlsRoleUser", ["Gamma"]) session.commit() From 0853eccc8c484f646d2c609c6f0bc8659b5a690a Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Fri, 21 Jan 2022 15:37:42 -0800 Subject: [PATCH 39/40] fix some tests --- tests/integration_tests/security_tests.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 140bd9f70d709..46ca679deebc8 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -1220,7 +1220,8 @@ def test_get_guest_user_expired_token(self, get_time_mock): def test_get_guest_user_no_user(self): user = None resources = [{"type": "dashboard", "id": 1}] - token = security_manager.create_guest_access_token(user, resources) + rls = {} + token = security_manager.create_guest_access_token(user, resources, rls) fake_request = FakeRequest() fake_request.headers[current_app.config["GUEST_TOKEN_HEADER_NAME"]] = token guest_user = security_manager.get_guest_user_from_request(fake_request) @@ -1231,10 +1232,11 @@ def test_get_guest_user_no_user(self): def test_get_guest_user_no_resource(self): user = {"username": "test_guest"} resources = [] - token = security_manager.create_guest_access_token(user, resources) + rls = {} + token = security_manager.create_guest_access_token(user, resources, rls) fake_request = FakeRequest() fake_request.headers[current_app.config["GUEST_TOKEN_HEADER_NAME"]] = token - guest_user = security_manager.get_guest_user_from_request(fake_request) + security_manager.get_guest_user_from_request(fake_request) self.assertRaisesRegex( ValueError, "Guest token does not contain a resources claim" From ff3e034d0b7d726afe0d473483c987f564fa706d Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Fri, 21 Jan 2022 15:45:49 -0800 Subject: [PATCH 40/40] lint --- superset/security/guest_token.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/superset/security/guest_token.py b/superset/security/guest_token.py index 16b18e7c1a5cb..af86da326288c 100644 --- a/superset/security/guest_token.py +++ b/superset/security/guest_token.py @@ -44,9 +44,6 @@ class GuestTokenRlsRule(TypedDict): clause: str -GuestTokenResources = List[GuestTokenResource] - - class GuestToken(TypedDict): iat: float exp: float