diff --git a/pyproject.toml b/pyproject.toml index 12d5224cb38..7798f72edd2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -48,7 +48,7 @@ dependencies = [ "cryptography>=42.0.4, <47.0.0", "deprecation>=2.1.0, <2.2.0", "flask>=2.2.5, <4.0.0", - "flask-appbuilder>=5.0.2,<6", + "flask-appbuilder>=5.2.1, <6.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 f6f27ff4ffb..983098bddce 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -120,7 +120,7 @@ flask==2.3.3 # flask-session # flask-sqlalchemy # flask-wtf -flask-appbuilder==5.2.0 +flask-appbuilder==5.2.1 # via # apache-superset (pyproject.toml) # apache-superset-core diff --git a/requirements/development.txt b/requirements/development.txt index f5e7f74b48a..7fecd31e208 100644 --- a/requirements/development.txt +++ b/requirements/development.txt @@ -259,7 +259,7 @@ flask==2.3.3 # flask-sqlalchemy # flask-testing # flask-wtf -flask-appbuilder==5.2.0 +flask-appbuilder==5.2.1 # via # -c requirements/base-constraint.txt # apache-superset diff --git a/superset/security/manager.py b/superset/security/manager.py index 2c10cd8ce02..a771754b839 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -26,7 +26,7 @@ from typing import Any, Callable, cast, NamedTuple, Optional, TYPE_CHECKING from flask import current_app, Flask, g, Request from flask_appbuilder import Model from flask_appbuilder.models.filters import BaseFilter -from flask_appbuilder.security.sqla.apis import RoleApi, UserApi +from flask_appbuilder.security.sqla.apis import GroupApi, RoleApi, UserApi from flask_appbuilder.security.sqla.manager import SecurityManager from flask_appbuilder.security.sqla.models import ( assoc_group_role, @@ -121,9 +121,38 @@ _RLSCacheKey = tuple[str, int | str] _RLSCache = dict[_RLSCacheKey, list[SqlaQuery]] +def _log_audit_event(action: str, payload: dict[str, Any]) -> None: + """Log an audit event via the configured event logger. + + Delegates to the AbstractEventLogger interface so that every + configured implementation (DBEventLogger, S3EventLogger, etc.) + receives these security audit events. + """ + from superset.extensions import ( + event_logger, # pylint: disable=import-outside-toplevel + ) + + user_id = get_user_id() + try: + event_logger.log( + user_id=user_id, + action=action, + dashboard_id=None, + duration_ms=None, + slice_id=None, + referrer=None, + curated_payload=None, + curated_form_data=None, + records=[payload], + ) + except Exception: # pylint: disable=broad-except + logger.warning("Failed to log audit event: %s", action, exc_info=True) + + class SupersetRoleApi(RoleApi): """ Overriding the RoleApi to be able to delete roles with permissions + and to add audit logging for role CRUD operations. """ def pre_delete(self, item: Model) -> None: @@ -132,6 +161,15 @@ class SupersetRoleApi(RoleApi): """ item.permissions = [] + def post_add(self, item: Model) -> None: + _log_audit_event("RoleCreated", {"role_name": item.name, "role_id": item.id}) + + def post_update(self, item: Model) -> None: + _log_audit_event("RoleUpdated", {"role_name": item.name, "role_id": item.id}) + + def post_delete(self, item: Model) -> None: + _log_audit_event("RoleDeleted", {"role_name": item.name, "role_id": item.id}) + class ExcludeUsersFilter(BaseFilter): # pylint: disable=too-few-public-methods """ @@ -159,6 +197,7 @@ class ExcludeUsersFilter(BaseFilter): # pylint: disable=too-few-public-methods class SupersetUserApi(UserApi): """ Overriding the UserApi to be able to delete users and filter excluded users + and to add audit logging for user CRUD operations. """ base_filters = [["username", ExcludeUsersFilter, lambda: []]] @@ -184,6 +223,51 @@ class SupersetUserApi(UserApi): """ item.roles = [] + def post_add(self, item: Model) -> None: + _log_audit_event( + "UserCreated", + { + "target_username": item.username, + "target_user_id": item.id, + "email": item.email, + }, + ) + + def post_update(self, item: Model) -> None: + _log_audit_event( + "UserUpdated", + { + "target_username": item.username, + "target_user_id": item.id, + "email": item.email, + "active": item.active, + }, + ) + + def post_delete(self, item: Model) -> None: + _log_audit_event( + "UserDeleted", + { + "target_username": item.username, + "target_user_id": item.id, + }, + ) + + +class SupersetGroupApi(GroupApi): + """ + Overriding the GroupApi to add audit logging for group CRUD operations. + """ + + def post_add(self, item: Model) -> None: + _log_audit_event("GroupCreated", {"group_name": item.name, "group_id": item.id}) + + def post_update(self, item: Model) -> None: + _log_audit_event("GroupUpdated", {"group_name": item.name, "group_id": item.id}) + + def post_delete(self, item: Model) -> None: + _log_audit_event("GroupDeleted", {"group_name": item.name, "group_id": item.id}) + # Limiting routes on FAB model views PermissionViewModelView.include_route_methods = {RouteMethod.LIST} @@ -267,6 +351,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods role_api = SupersetRoleApi user_api = SupersetUserApi + group_api = SupersetGroupApi USER_MODEL_VIEWS = { "RegisterUserModelView", @@ -484,6 +569,27 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods lm.request_loader(self.request_loader) return lm + def on_user_login(self, user: Any) -> None: + _log_audit_event( + "UserLoggedIn", + {"username": user.username, "user_id": user.id}, + ) + + def on_user_login_failed(self, user: Any) -> None: + _log_audit_event( + "UserLoginFailed", + {"username": user.username, "user_id": user.id}, + ) + + def on_user_logout(self, user: Any) -> None: + _log_audit_event( + "UserLoggedOut", + { + "username": getattr(user, "username", None), + "user_id": getattr(user, "id", None), + }, + ) + def request_loader(self, request: Request) -> Optional[User]: # pylint: disable=import-outside-toplevel from superset.extensions import feature_flag_manager diff --git a/superset/utils/log.py b/superset/utils/log.py index 6685010b4d2..7387cafbfcc 100644 --- a/superset/utils/log.py +++ b/superset/utils/log.py @@ -412,7 +412,7 @@ class DBEventLogger(AbstractEventLogger): logging.exception(ex) # Rollback to clean up the session state try: - db.session.rollback() + db.session.rollback() # pylint: disable=consider-using-transaction except Exception: # pylint: disable=broad-except # If rollback also fails, just continue - don't let issues crash the app logging.error( diff --git a/tests/unit_tests/security/api_test.py b/tests/unit_tests/security/api_test.py index 9e0a25131c3..b26d2687990 100644 --- a/tests/unit_tests/security/api_test.py +++ b/tests/unit_tests/security/api_test.py @@ -32,7 +32,7 @@ def test_csrf_exempt_blueprints(app_context: None) -> None: are exempt from CSRF protection. """ assert {blueprint.name for blueprint in csrf._exempt_blueprints} == { - "GroupApi", + "SupersetGroupApi", "MenuApi", "SecurityApi", "OpenApi", diff --git a/tests/unit_tests/security/audit_log_test.py b/tests/unit_tests/security/audit_log_test.py new file mode 100644 index 00000000000..276da24a6f9 --- /dev/null +++ b/tests/unit_tests/security/audit_log_test.py @@ -0,0 +1,251 @@ +# 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. + +from unittest.mock import MagicMock, patch + +from flask_appbuilder.security.sqla.models import Group, Role, User + +from superset.security.manager import ( + _log_audit_event, + SupersetGroupApi, + SupersetRoleApi, + SupersetSecurityManager, + SupersetUserApi, +) + + +@patch("superset.extensions.event_logger") +@patch("superset.security.manager.get_user_id", return_value=1) +def test_log_audit_event_calls_event_logger( + mock_get_user_id: MagicMock, + mock_event_logger: MagicMock, +) -> None: + """_log_audit_event delegates to the configured event_logger.""" + _log_audit_event("TestAction", {"key": "value"}) + + mock_event_logger.log.assert_called_once_with( + user_id=1, + action="TestAction", + dashboard_id=None, + duration_ms=None, + slice_id=None, + referrer=None, + curated_payload=None, + curated_form_data=None, + records=[{"key": "value"}], + ) + + +@patch("superset.extensions.event_logger") +@patch("superset.security.manager.get_user_id", return_value=1) +def test_log_audit_event_handles_logger_error( + mock_get_user_id: MagicMock, + mock_event_logger: MagicMock, +) -> None: + """_log_audit_event does not raise on event_logger errors.""" + mock_event_logger.log.side_effect = Exception("Logger error") + # Should not raise + _log_audit_event("TestAction", {"key": "value"}) + + +# --- Role CRUD --- + + +@patch("superset.security.manager._log_audit_event") +def test_role_api_post_add_logs_event(mock_log: MagicMock) -> None: + """SupersetRoleApi.post_add logs a RoleCreated event.""" + api = SupersetRoleApi.__new__(SupersetRoleApi) + role = MagicMock(spec=Role) + role.name = "TestRole" + role.id = 42 + api.post_add(role) + mock_log.assert_called_once_with( + "RoleCreated", {"role_name": "TestRole", "role_id": 42} + ) + + +@patch("superset.security.manager._log_audit_event") +def test_role_api_post_update_logs_event(mock_log: MagicMock) -> None: + """SupersetRoleApi.post_update logs a RoleUpdated event.""" + api = SupersetRoleApi.__new__(SupersetRoleApi) + role = MagicMock(spec=Role) + role.name = "TestRole" + role.id = 42 + api.post_update(role) + mock_log.assert_called_once_with( + "RoleUpdated", {"role_name": "TestRole", "role_id": 42} + ) + + +@patch("superset.security.manager._log_audit_event") +def test_role_api_post_delete_logs_event(mock_log: MagicMock) -> None: + """SupersetRoleApi.post_delete logs a RoleDeleted event.""" + api = SupersetRoleApi.__new__(SupersetRoleApi) + role = MagicMock(spec=Role) + role.name = "TestRole" + role.id = 42 + api.post_delete(role) + mock_log.assert_called_once_with( + "RoleDeleted", {"role_name": "TestRole", "role_id": 42} + ) + + +# --- User CRUD --- + + +@patch("superset.security.manager._log_audit_event") +def test_user_api_post_add_logs_event(mock_log: MagicMock) -> None: + """SupersetUserApi.post_add logs a UserCreated event.""" + api = SupersetUserApi.__new__(SupersetUserApi) + user = MagicMock(spec=User) + user.username = "testuser" + user.id = 7 + user.email = "test@example.com" + api.post_add(user) + mock_log.assert_called_once_with( + "UserCreated", + { + "target_username": "testuser", + "target_user_id": 7, + "email": "test@example.com", + }, + ) + + +@patch("superset.security.manager._log_audit_event") +def test_user_api_post_update_logs_event(mock_log: MagicMock) -> None: + """SupersetUserApi.post_update logs a UserUpdated event.""" + api = SupersetUserApi.__new__(SupersetUserApi) + user = MagicMock(spec=User) + user.username = "testuser" + user.id = 7 + user.email = "test@example.com" + user.active = True + api.post_update(user) + mock_log.assert_called_once_with( + "UserUpdated", + { + "target_username": "testuser", + "target_user_id": 7, + "email": "test@example.com", + "active": True, + }, + ) + + +@patch("superset.security.manager._log_audit_event") +def test_user_api_post_delete_logs_event(mock_log: MagicMock) -> None: + """SupersetUserApi.post_delete logs a UserDeleted event.""" + api = SupersetUserApi.__new__(SupersetUserApi) + user = MagicMock(spec=User) + user.username = "testuser" + user.id = 7 + api.post_delete(user) + mock_log.assert_called_once_with( + "UserDeleted", + {"target_username": "testuser", "target_user_id": 7}, + ) + + +# --- Group CRUD --- + + +@patch("superset.security.manager._log_audit_event") +def test_group_api_post_add_logs_event(mock_log: MagicMock) -> None: + """SupersetGroupApi.post_add logs a GroupCreated event.""" + api = SupersetGroupApi.__new__(SupersetGroupApi) + group = MagicMock(spec=Group) + group.name = "TestGroup" + group.id = 10 + api.post_add(group) + mock_log.assert_called_once_with( + "GroupCreated", {"group_name": "TestGroup", "group_id": 10} + ) + + +@patch("superset.security.manager._log_audit_event") +def test_group_api_post_update_logs_event(mock_log: MagicMock) -> None: + """SupersetGroupApi.post_update logs a GroupUpdated event.""" + api = SupersetGroupApi.__new__(SupersetGroupApi) + group = MagicMock(spec=Group) + group.name = "TestGroup" + group.id = 10 + api.post_update(group) + mock_log.assert_called_once_with( + "GroupUpdated", {"group_name": "TestGroup", "group_id": 10} + ) + + +@patch("superset.security.manager._log_audit_event") +def test_group_api_post_delete_logs_event(mock_log: MagicMock) -> None: + """SupersetGroupApi.post_delete logs a GroupDeleted event.""" + api = SupersetGroupApi.__new__(SupersetGroupApi) + group = MagicMock(spec=Group) + group.name = "TestGroup" + group.id = 10 + api.post_delete(group) + mock_log.assert_called_once_with( + "GroupDeleted", {"group_name": "TestGroup", "group_id": 10} + ) + + +# --- Login / Logout --- + + +@patch("superset.security.manager._log_audit_event") +def test_on_user_login_logs_event(mock_log: MagicMock) -> None: + """on_user_login logs a UserLoggedIn event.""" + sm = SupersetSecurityManager.__new__(SupersetSecurityManager) + user = MagicMock(spec=User) + user.username = "testuser" + user.id = 7 + + sm.on_user_login(user) + + mock_log.assert_called_once_with( + "UserLoggedIn", {"username": "testuser", "user_id": 7} + ) + + +@patch("superset.security.manager._log_audit_event") +def test_on_user_login_failed_logs_event(mock_log: MagicMock) -> None: + """on_user_login_failed logs a UserLoginFailed event.""" + sm = SupersetSecurityManager.__new__(SupersetSecurityManager) + user = MagicMock(spec=User) + user.username = "testuser" + user.id = 7 + + sm.on_user_login_failed(user) + + mock_log.assert_called_once_with( + "UserLoginFailed", {"username": "testuser", "user_id": 7} + ) + + +@patch("superset.security.manager._log_audit_event") +def test_on_user_logout_logs_event(mock_log: MagicMock) -> None: + """on_user_logout logs a UserLoggedOut event.""" + sm = SupersetSecurityManager.__new__(SupersetSecurityManager) + user = MagicMock(spec=User) + user.username = "testuser" + user.id = 7 + + sm.on_user_logout(user) + + mock_log.assert_called_once_with( + "UserLoggedOut", {"username": "testuser", "user_id": 7} + )