diff --git a/pyproject.toml b/pyproject.toml index 40fc16a2aed2..5074a2035371 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -44,7 +44,7 @@ dependencies = [ "cryptography>=42.0.4, <45.0.0", "deprecation>=2.1.0, <2.2.0", "flask>=2.2.5, <3.0.0", - "flask-appbuilder>=4.5.4, <5.0.0", + "flask-appbuilder>=4.6.0, <5.0.0", "flask-caching>=2.1.0, <3", "flask-compress>=1.13, <2.0", "flask-talisman>=1.0.0, <2.0", diff --git a/requirements/base.txt b/requirements/base.txt index 975ff22fb545..5d6a8a84d0f9 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -108,7 +108,7 @@ flask==2.3.3 # flask-session # flask-sqlalchemy # flask-wtf -flask-appbuilder==4.5.4 +flask-appbuilder==4.6.0 # via apache-superset (pyproject.toml) flask-babel==2.0.0 # via flask-appbuilder diff --git a/requirements/development.txt b/requirements/development.txt index 84cc3ab0151e..5a17596ac681 100644 --- a/requirements/development.txt +++ b/requirements/development.txt @@ -190,7 +190,7 @@ flask==2.3.3 # flask-sqlalchemy # flask-testing # flask-wtf -flask-appbuilder==4.5.4 +flask-appbuilder==4.6.0 # via # -c requirements/base.txt # apache-superset diff --git a/superset/security/guest_token.py b/superset/security/guest_token.py index 6727330afe92..4d1b25030f46 100644 --- a/superset/security/guest_token.py +++ b/superset/security/guest_token.py @@ -16,7 +16,7 @@ # under the License. from typing import Optional, TypedDict, Union -from flask_appbuilder.security.sqla.models import Role +from flask_appbuilder.security.sqla.models import Group, Role from flask_login import AnonymousUserMixin from superset.utils.backports import StrEnum @@ -84,5 +84,6 @@ def __init__(self, token: GuestToken, roles: list[Role]): self.first_name = user.get("first_name", "Guest") self.last_name = user.get("last_name", "User") self.roles = roles + self.groups: list[Group] = [] # Guest users don't belong to any groups self.resources = token["resources"] self.rls = token.get("rls_rules", []) diff --git a/superset/security/manager.py b/superset/security/manager.py index 450265ccb989..80a1fb957dd6 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -27,7 +27,9 @@ from flask_appbuilder import Model from flask_appbuilder.security.sqla.manager import SecurityManager from flask_appbuilder.security.sqla.models import ( + assoc_group_role, assoc_permissionview_role, + assoc_user_group, assoc_user_role, Permission, PermissionView, @@ -51,6 +53,7 @@ from sqlalchemy.orm import eagerload from sqlalchemy.orm.mapper import Mapper from sqlalchemy.orm.query import Query as SqlaQuery +from sqlalchemy.sql import exists from superset.constants import RouteMethod from superset.errors import ErrorLevel, SupersetError, SupersetErrorType @@ -236,8 +239,10 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods "Log", "List Users", "List Roles", + "List Groups", "ResetPasswordView", "RoleModelView", + "UserGroupModelView", "Row Level Security", "Row Level Security Filters", "RowLevelSecurityFiltersModelView", @@ -733,13 +738,24 @@ def user_view_menu_names(self, permission_name: str) -> set[str]: ) if not g.user.is_anonymous: - # filter by user id - view_menu_names = ( - base_query.join(assoc_user_role) - .join(self.user_model) - .filter(self.user_model.id == get_user_id()) - .filter(self.permission_model.name == permission_name) - ).all() + user_id = get_user_id() + + user_roles_filter = or_( + exists().where( + (assoc_user_role.c.user_id == user_id) + & (assoc_user_role.c.role_id == self.role_model.id) + & (self.permission_model.name == permission_name) + ), + exists().where( + (assoc_user_group.c.user_id == user_id) + & (assoc_user_group.c.group_id == self.group_model.id) + & (assoc_group_role.c.group_id == self.group_model.id) + & (assoc_group_role.c.role_id == self.role_model.id) + & (self.permission_model.name == permission_name) + ), + ) + + view_menu_names = base_query.filter(user_roles_filter).all() return {s.name for s in view_menu_names} # Properly treat anonymous user @@ -2424,7 +2440,7 @@ def get_user_roles(self, user: Optional[User] = None) -> 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 user.roles + return super().get_user_roles(user) def get_guest_rls_filters( self, dataset: "BaseDatasource" diff --git a/superset/views/utils.py b/superset/views/utils.py index f2d06e7a3a4c..1f6a4348fd28 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -103,8 +103,8 @@ def bootstrap_user_data(user: User, include_perms: bool = False) -> dict[str, An def get_permissions( user: User, ) -> tuple[dict[str, list[tuple[str]]], DefaultDict[str, list[str]]]: - if not user.roles: - raise AttributeError("User object does not have roles") + if not user.roles and not user.groups: + raise AttributeError("User object does not have roles or groups") data_permissions = defaultdict(set) roles_permissions = security_manager.get_user_roles_permissions(user) diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py index 3a193b40e21d..f50a9ea96853 100644 --- a/tests/integration_tests/charts/commands_tests.py +++ b/tests/integration_tests/charts/commands_tests.py @@ -104,11 +104,13 @@ def test_export_chart_command(self, mock_g): "query_context": None, } + @patch("superset.utils.core.g") @patch("superset.security.manager.g") @pytest.mark.usefixtures("load_energy_table_with_slice") - def test_export_chart_command_no_access(self, mock_g): + def test_export_chart_command_no_access(self, utils_mock_g, manager_mock_g): """Test that users can't export datasets they don't have access to""" - mock_g.user = security_manager.find_user("gamma") + manager_mock_g.user = security_manager.find_user("gamma") + utils_mock_g.user = manager_mock_g.user example_chart = db.session.query(Slice).all()[0] command = ExportChartsCommand([example_chart.id]) diff --git a/tests/integration_tests/databases/api_tests.py b/tests/integration_tests/databases/api_tests.py index 1a37394d7738..9bed242a94f0 100644 --- a/tests/integration_tests/databases/api_tests.py +++ b/tests/integration_tests/databases/api_tests.py @@ -78,6 +78,9 @@ load_unicode_dashboard_with_position, # noqa: F401 load_unicode_data, # noqa: F401 ) +from tests.integration_tests.fixtures.users import ( + create_gamma_user_group_with_all_database, # noqa: F401 +) from tests.integration_tests.test_app import app @@ -259,6 +262,18 @@ def test_get_items_not_allowed(self): response = json.loads(rv.data.decode("utf-8")) assert response["count"] == 0 + @pytest.mark.usefixtures("create_gamma_user_group_with_all_database") + def test_get_items_gamma_group(self): + """ + Database API: Test get items gamma with group + """ + self.login("gamma_with_groups", "password1") + uri = "api/v1/database/" + rv = self.client.get(uri) + assert rv.status_code == 200 + response = json.loads(rv.data.decode("utf-8")) + assert response["count"] > 0 + def test_create_database(self): """ Database API: Test create @@ -2340,6 +2355,17 @@ def test_get_database_related_objects_not_found(self): rv = self.get_assert_metric(uri, "related_objects") assert rv.status_code == 404 + @pytest.mark.usefixtures("create_gamma_user_group_with_all_database") + def test_get_database_related_objects_gamma_group(self): + """ + Database API: Test related objects with gamma group with role all database + """ + database = get_example_database() + self.login("gamma_with_groups", "password1") + uri = f"api/v1/database/{database.id}/related_objects/" + rv = self.get_assert_metric(uri, "related_objects") + assert rv.status_code == 200 + def test_export_database(self): """ Database API: Test export database diff --git a/tests/integration_tests/fixtures/users.py b/tests/integration_tests/fixtures/users.py index 1a2073bf90b3..af03d1d4954f 100644 --- a/tests/integration_tests/fixtures/users.py +++ b/tests/integration_tests/fixtures/users.py @@ -22,6 +22,86 @@ from tests.integration_tests.constants import GAMMA_SQLLAB_NO_DATA_USERNAME +def create_role_with_permissions(role_name: str, permissions: list[tuple[str, str]]): + pvm_list = [ + security_manager.add_permission_view_menu(p[0], p[1]) for p in permissions + ] + return security_manager.add_role(role_name, pvm_list) + + +def create_user_and_group( + group_name: str, + username: str, + roles: list[Role], + password: str = "password1", # noqa: S107 +): + group = security_manager.add_group(group_name, "", "", roles=roles) + user = security_manager.add_user( + username, + "gamma", + "user", + username, + password=password, # noqa: S106 + role=[], + groups=[group], + ) + return user, group + + +def cleanup(user, group): + security_manager.get_session.delete(user) + security_manager.get_session.delete(group) + security_manager.get_session.commit() + + +@pytest.fixture +def create_gamma_user_group(app_context: AppContext): + gamma_role = security_manager.find_role("Gamma") + user, group = create_user_and_group("group1", "gamma_with_groups", [gamma_role]) + yield + cleanup(user, group) + + +@pytest.fixture +def create_user_group_with_dar(app_context: AppContext): + dar_role = create_role_with_permissions( + "dar", [("datasource_access", "[examples].[birth_names](id:1)]")] + ) + user, group = create_user_and_group("group1", "gamma_with_groups", [dar_role]) + yield + cleanup(user, group) + + +@pytest.fixture +def create_gamma_user_group_with_dar(app_context: AppContext): + dar_role = create_role_with_permissions( + "dar", + [ + ("datasource_access", "[examples].[birth_names](id:1)]"), + ("all_database_access", "all_database_access"), + ], + ) + gamma_role = security_manager.find_role("Gamma") + user, group = create_user_and_group( + "group1", "gamma_with_groups", [dar_role, gamma_role] + ) + yield + cleanup(user, group) + + +@pytest.fixture +def create_gamma_user_group_with_all_database(app_context: AppContext): + dar_role = create_role_with_permissions( + "dar", [("all_database_access", "all_database_access")] + ) + gamma_role = security_manager.find_role("Gamma") + user, group = create_user_and_group( + "group1", "gamma_with_groups", [dar_role, gamma_role] + ) + yield + cleanup(user, group) + + @pytest.fixture def create_gamma_sqllab_no_data(app_context: AppContext): gamma_role = db.session.query(Role).filter(Role.name == "Gamma").one_or_none() diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index a89bb47af78f..96886fab3b2e 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -63,6 +63,11 @@ load_world_bank_dashboard_with_slices, # noqa: F401 load_world_bank_data, # noqa: F401 ) +from tests.integration_tests.fixtures.users import ( + create_gamma_user_group, # noqa: F401 + create_user_group_with_dar, # noqa: F401 + create_gamma_user_group_with_dar, # noqa: F401 +) NEW_SECURITY_CONVERGE_VIEWS = ( "Annotation", @@ -1871,12 +1876,58 @@ def test_raise_for_access_rbac( } ) - def test_get_user_roles(self): + def test_get_admin_user_roles(self): admin = security_manager.find_user("admin") with override_user(admin): roles = security_manager.get_user_roles() assert admin.roles == roles + def test_get_gamma_user_roles(self): + admin = security_manager.find_user("gamma") + with override_user(admin): + roles = security_manager.get_user_roles() + assert admin.roles == roles + + @pytest.mark.usefixtures("create_gamma_user_group") + def test_get_user_roles_with_groups(self): + user = security_manager.find_user("gamma_with_groups") + with override_user(user): + roles = security_manager.get_user_roles() + assert user.groups[0].roles == roles + + @pytest.mark.usefixtures("create_gamma_user_group_with_dar") + def test_get_user_roles_with_groups_dar(self): + user = security_manager.find_user("gamma_with_groups") + with override_user(user): + role_names = [role.name for role in security_manager.get_user_roles()] + assert "Gamma" in role_names + assert "dar" in role_names + assert len(role_names) == 2 + + @pytest.mark.usefixtures("create_user_group_with_dar") + def test_user_view_menu_names_with_groups_dar(self): + user = security_manager.find_user("gamma_with_groups") + with override_user(user): + assert security_manager.user_view_menu_names("datasource_access") == { + "[examples].[birth_names](id:1)]" + } + + @pytest.mark.usefixtures("create_gamma_user_group_with_dar") + def test_gamma_user_view_menu_names_with_groups_dar(self): + user = security_manager.find_user("gamma_with_groups") + with override_user(user): + # assert pvm for dar role + assert security_manager.user_view_menu_names("datasource_access") == { + "[examples].[birth_names](id:1)]" + } + # assert pvm for gamma role + assert security_manager.user_view_menu_names("can_external_metadata") == { + "Datasource" + } + assert security_manager.user_view_menu_names("can_recent_activity") == { + "Log" + } + def test_get_anonymous_roles(self): with override_user(security_manager.get_anonymous_user()): roles = security_manager.get_user_roles()