feat: role/user CRUD events and login/logout tracking in the action log#39121
Merged
dpgaspar merged 12 commits intoapache:masterfrom Apr 9, 2026
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39121 +/- ##
==========================================
- Coverage 64.47% 64.44% -0.03%
==========================================
Files 2541 2542 +1
Lines 131674 131834 +160
Branches 30524 30528 +4
==========================================
+ Hits 84893 84965 +72
- Misses 45315 45384 +69
- Partials 1466 1485 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Delegate audit events directly to the configured event_logger (AbstractEventLogger) instead of maintaining a separate _log_audit_event_to_db helper with its own independent session. This ensures all logger implementations (DBEventLogger, S3EventLogger, etc.) receive security audit events through the same interface. The independent session handling for DB logging already exists in DBEventLogger.log(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…b-security-events-on-superset
…b-security-events-on-superset
…include-fab-security-events-on-superset' into danielgaspar/sc-102915/visa-pcs-include-fab-security-events-on-superset
dosubot
bot
added
authentication
FAB 5.2.1 fixes the DetachedInstanceError that occurred when DBEventLogger.log committed via db.session, expiring the User object and breaking subsequent FAB logout/auth operations. The independent session workaround is no longer needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+228
to
+234
| user = MagicMock(spec=User) | ||
| user.username = "testuser" | ||
| user.id = 7 | ||
|
|
||
| sm.on_user_login_failed(user) | ||
|
|
||
| mock_log.assert_called_once_with( |
Contributor
There was a problem hiding this comment.
Suggestion: The failed-login hook in FAB is triggered with the attempted username (not an authenticated user object), so testing it with a User mock validates the wrong contract and can miss production failures. Update the test to pass a username string and assert the corresponding payload. [type error]
Severity Level: Critical 🚨
- ❌ Failed logins crash due to AttributeError in SupersetSecurityManager.
- ❌ Audit logger never records failed login attempts.
- ⚠️ Unit test misrepresents Flask-AppBuilder login-failure hook contract.
Suggested change
| user = MagicMock(spec=User) | |
| user.username = "testuser" | |
| user.id = 7 | |
| sm.on_user_login_failed(user) | |
| mock_log.assert_called_once_with( | |
| username = "testuser" | |
| sm.on_user_login_failed(username) | |
| mock_log.assert_called_once_with( | |
| "UserLoginFailed", {"username": "testuser"} |
Steps of Reproduction ✅
1. In `superset/superset/security/manager.py`,
`SupersetSecurityManager.on_user_login_failed` is defined at lines 578–582 to accept
`user: Any` and passes `{"username": user.username, "user_id": user.id}` to
`_log_audit_event("UserLoginFailed", ...)`.
2. Flask-AppBuilder 5.2.x (external dependency required by this PR) calls
`SecurityManager.on_user_login_failed` with the attempted username string on
authentication failure (no `User` object exists when login fails), so in production this
override will be invoked with a `str`, not a `User`.
3. When a user submits invalid credentials to the login endpoint (handled by FAB's auth
views using `SupersetSecurityManager` as the `security_manager_class`), FAB calls
`on_user_login_failed(username)`, and Superset's override tries to access `user.username`
and `user.id`, raising `AttributeError` on the string; the failed-login request returns a
500 and the audit event is never logged.
4. The unit test `test_on_user_login_failed_logs_event` in
`tests/unit_tests/security/audit_log_test.py` lines 225–235 instead constructs `user =
MagicMock(spec=User)` (lines 228–230), calls `sm.on_user_login_failed(user)` (line 232),
and asserts a payload with both `"username"` and `"user_id"` (line 235), thereby
validating a non-existent contract and letting the crash and wrong payload go undetected;
updating the test to pass a username string and assert `{"username": "testuser"}` would
align with FAB's hook and reveal the real bug.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/security/audit_log_test.py
**Line:** 228:234
**Comment:**
*Type Error: The failed-login hook in FAB is triggered with the attempted username (not an authenticated user object), so testing it with a `User` mock validates the wrong contract and can miss production failures. Update the test to pass a username string and assert the corresponding payload.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+579
to
+580
| _log_audit_event( | ||
| "UserLoginFailed", |
Contributor
There was a problem hiding this comment.
Suggestion: The failed-login hook assumes the argument always has .username and .id, but FAB can pass None (unknown user) or a plain username string for failed auth attempts. Accessing attributes unconditionally will raise AttributeError and can turn a normal login failure into a 500 error. Handle both object and string/None inputs before building the payload. [null pointer]
Severity Level: Critical 🚨
- ❌ Failed login attempts return 500 instead of 401 response.
- ⚠️ Audit logging misses some failed authentication attempts.
Suggested change
| _log_audit_event( | |
| "UserLoginFailed", | |
| username = user if isinstance(user, str) else getattr(user, "username", None) | |
| user_id = getattr(user, "id", None) | |
| _log_audit_event( | |
| "UserLoginFailed", | |
| {"username": username, "user_id": user_id}, |
Steps of Reproduction ✅
1. Superset config uses `SupersetSecurityManager` as the security manager via
`superset/security/__init__.py:17`, which exposes `SupersetSecurityManager` to
Flask-AppBuilder.
2. Authentication views are registered in `superset/security/manager.py:3277-3283`, wiring
`SupersetAuthView` from `superset/views/auth.py:4-13` to handle the `/login` route in
normal authentication flows.
3. Trigger a failed login for a non-existent or invalid user by POSTing bad credentials to
`/login` so that Flask-AppBuilder's auth stack invokes
`SupersetSecurityManager.on_user_login_failed(user)` with a value that is not a `User`
instance (for failed auth attempts FAB can pass `None` or the attempted username string
instead of a model object).
4. The implementation at `superset/security/manager.py:578-582` assumes `user` always has
`.username` and `.id` and executes `user.username` and `user.id` unconditionally, which
raises `AttributeError` when `user` is `None` or a string; this turns a normal login
failure into a 500 error and prevents the `UserLoginFailed` audit event from being
recorded by the configured `EVENT_LOGGER`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 579:580
**Comment:**
*Null Pointer: The failed-login hook assumes the argument always has `.username` and `.id`, but FAB can pass `None` (unknown user) or a plain username string for failed auth attempts. Accessing attributes unconditionally will raise `AttributeError` and can turn a normal login failure into a 500 error. Handle both object and string/None inputs before building the payload.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Member
Author
There was a problem hiding this comment.
This is a gap on FAB only for AUTH_DB. Will address this later, non critical for now
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #0f74e9
Actionable Suggestions - 2
-
superset/utils/log.py - 1
- Refactor to use transaction context manager · Line 415-415
-
superset/security/manager.py - 1
- Blind exception catch without specific type · Line 148-148
Additional Suggestions - 1
-
superset/security/manager.py - 1
-
Inconsistent user attribute access · Line 572-582The on_user_logout method uses getattr to safely access user attributes, but on_user_login and on_user_login_failed access them directly. This risks AttributeError if the user object lacks username or id attributes. For consistency and robustness, update the login methods to use getattr like logout does.
Code suggestion
@@ -572,11 +572,11 @@ - 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_login(self, user: Any) -> None: + _log_audit_event( + "UserLoggedIn", + {"username": getattr(user, "username", None), "user_id": getattr(user, "id", None)}, + ) + + def on_user_login_failed(self, user: Any) -> None: + _log_audit_event( + "UserLoginFailed", + {"username": getattr(user, "username", None), "user_id": getattr(user, "id", None)}, + ) +
-
Review Details
-
Files reviewed - 7 · Commit Range:
edceaa6..db88a17- pyproject.toml
- requirements/base.txt
- requirements/development.txt
- superset/security/manager.py
- superset/utils/log.py
- tests/unit_tests/security/api_test.py
- tests/unit_tests/security/audit_log_test.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Antonio-RiveroMartnez
approved these changes
Apr 9, 2026
dpgaspar
deleted the
danielgaspar/sc-102915/visa-pcs-include-fab-security-events-on-superset
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Adds security audit logging for FAB (Flask-AppBuilder) security events to Superset's action log. This enables tracking of user/role/group lifecycle events through the configured
EVENT_LOGGER, ensuring that custom implementations (e.g. S3EventLogger) also receive these events.flask-appbuilder 5.2.1 log hooks implemented here: dpgaspar/Flask-AppBuilder#2450
Events logged:
UserLoggedIn,UserLoginFailed,UserLoggedOutUserCreated,UserUpdated,UserDeletedRoleCreated,RoleUpdated,RoleDeletedGroupCreated,GroupUpdated,GroupDeletedImplementation:
post_add/post_update/post_deletehooks onSupersetRoleApi,SupersetUserApi, and newSupersetGroupApion_user_login,on_user_login_failed,on_user_logouthooks onSupersetSecurityManagerAbstractEventLoggerinterface (event_logger.log()), so any configured logger receives themDBEventLoggeruses an independent DB session to avoid expiring objects in the main scoped session (preventsDetachedInstanceError)flask-appbuilder>=5.2.1which adds the auth event hooksBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - backend-only change, no UI impact.
ADDITIONAL INFORMATION