feat: role/user CRUD events and login/logout tracking in the action log (#39121)

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

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",

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

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

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

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(

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",

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}
+    )