feat(security): terminate active sessions when an account is disabled#40695
feat(security): terminate active sessions when an account is disabled#40695rusackas wants to merge 9 commits into
Conversation
rusackas
requested review from
betodealmeida,
eschutho,
michael-s-molina,
mistercrunch and
sadpandajoe
as code owners
dosubot
Bot
added
authentication
Code Review Agent Run #ca6ef4Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue regarding a race condition in the To resolve this, you can use a database-level upsert. Since Superset supports multiple database backends, the most robust approach is to use SQLAlchemy's Given the context, a simple and effective fix is to use a def invalidate_user_sessions(connection: Any, user_id: int) -> None:
from superset.models.user_attributes import UserAttribute
table = UserAttribute.__table__
now = _utcnow().replace(tzinfo=None)
# Serialize the check-and-insert using a row-level lock
with connection.begin_nested():
existing = connection.execute(
table.select().where(table.c.user_id == user_id).with_for_update()
).fetchone()
if existing:
connection.execute(
table.update().where(table.c.user_id == user_id).values(sessions_invalidated_at=now, changed_on=now)
)
else:
connection.execute(
table.insert().values(user_id=user_id, sessions_invalidated_at=now, created_on=now, changed_on=now)
)Would you like me to check the other comments on this PR and implement fixes for them as well? superset/security/session_invalidation.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40695 +/- ##
==========================================
+ Coverage 64.04% 64.05% +0.01%
==========================================
Files 2663 2665 +2
Lines 143633 144044 +411
Branches 33036 33112 +76
==========================================
+ Hits 91988 92272 +284
- Misses 50043 50151 +108
- Partials 1602 1621 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #09c6a4Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Implements Part A2 of the session/token-lifecycle SIP (#40674): a backend-agnostic per-user invalidation epoch. - `UserAttribute.sessions_invalidated_at` (migration) records when a user's sessions were invalidated. - Login stamps `session["_login_at"]`; a `before_request` hook forces logout of any session that predates the user's epoch, then lets the request continue as anonymous so each route responds correctly for its type (401 for the REST API, redirect-to-login for HTML views). - A SQLAlchemy `after_update` listener stamps the epoch when `active` flips to False, so it fires regardless of the disable path (admin UI, REST API, CLI), for both client-side cookie and server-side session backends. Inert for users that were never disabled (NULL epoch) — backwards compatible by default. Comparison treats the naive UTC column correctly. Validated end-to-end against a local Docker stack (login -> disable -> forced 401) plus unit and integration tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… upsert Add a unique constraint on user_attribute.user_id (de-duping any existing rows first) and make invalidate_user_sessions retry as an update when a concurrent disable wins the insert race, preventing duplicate attribute rows. Also picks up pre-commit auto-fixes (auto-walrus, ruff-format). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
SQLite has no ALTER ... ADD/DROP CONSTRAINT, so wrap the user_attribute.user_id unique constraint create/drop in batch_alter_table. Also picks up a ruff-format normalization in the unit test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Claude Code <noreply@anthropic.com>
rusackas
force-pushed
the
feat/session-invalidation-epoch
branch
from
2e58d95 to
1244f59
Compare
| # ``extra_attributes[0]``). Enforce that invariant so the session-invalidation | ||
| # upsert is race-safe. Drop any pre-existing duplicates, keeping the lowest | ||
| # ``id`` per user, before adding the unique constraint. | ||
| op.execute( |
There was a problem hiding this comment.
This drops avatar_url / welcome_dashboard_id from any non-MIN(id) row without merging them into the kept row. Operators upgrading from deployments with duplicates will silently lose those settings. Shouldn't we merge non-NULL fields first, or fail the migration with operator instructions for the duplicate-row case.
There was a problem hiding this comment.
Good catch — fixed in fe314e5. Before deleting duplicates the migration now backfills the kept (MIN id) row from any higher-id sibling: for each of avatar_url, welcome_dashboard_id and sessions_invalidated_at, a kept row that is NULL takes the lowest-id non-NULL value from its duplicates. So a setting that lived only on a non-MIN row is merged forward rather than dropped. The subqueries are plain correlated SQL so they run on SQLite/MySQL/Postgres alike.
|
|
||
|
|
||
| def upgrade(): | ||
| add_columns(TABLE, sa.Column(COLUMN, sa.DateTime(), nullable=True)) |
There was a problem hiding this comment.
If an account is disabled today and re-enabled after this lands, the old session has no _login_at and no epoch. is_session_invalidated(None, None) returns False and the stale session revives. UPDATING.md says re-enabling starts a fresh session, but the code doesn't enforce that for pre-existing disabled accounts. We should backfill sessions_invalidated_at for ab_user.active=false users here, or stamp an epoch on re-enable when none exists.
There was a problem hiding this comment.
Good catch — fixed in 9ec28e3. The migration now backfills sessions_invalidated_at for any account that is already disabled (ab_user.active is false) at upgrade time, upserting one user_attribute row per such user. That way re-enabling a previously-disabled account does not revive a pre-existing session (which carries no recorded login time), so the behavior matches what UPDATING.md describes for accounts disabled before this lands as well.
| assert attribute.sessions_invalidated_at is not None | ||
|
|
||
| # ...so the previously-authenticated session is now forced out. | ||
| assert self.client.get("/api/v1/me/").status_code != 200 |
There was a problem hiding this comment.
Should we use == 401 here? != 200 means the test would pass with 500 or 403. If we wanted to have more status codes we accept, we could maybe compare the status_code to an array of status codes we accept?
There was a problem hiding this comment.
Agreed — tightened it to == 401. The enforce_session_validity hook clears the session and continues anonymous, so the protected REST route (/api/v1/me/) answers 401 specifically; asserting the exact code catches a regression that turned a forced-logout into an unrelated 500/403. Fixed in 29e0ded.
…recision MySQL DATETIME columns store no fractional seconds and truncate the stored instant. A guest token or session minted in the same wall-clock second as a revoke/disable carried a timestamp equal to the truncated epoch and survived the strict `<` comparison, leaving an outstanding token/session unrevoked. Round the stored epoch up to the next whole second so it strictly exceeds any same-second issuance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #c3b63e
Actionable Suggestions - 1
-
superset/security/manager.py - 1
- Test coverage gap for session stamping · Line 635-641
Additional Suggestions - 1
-
superset/dashboards/api.py - 1
-
Missing unit tests for revoke_embedded · Line 2160-2209The new `revoke_embedded` endpoint lacks functional tests. BITO.md rule [11730] requires unit tests covering success paths, error scenarios, and edge cases for new tools. Currently only the permission name is verified in `test_info_security_dashboard` (line 729), but no test validates actual revocation behavior or access control for this endpoint.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/migrations/versions/2026-06-02_10-00_f7a1c93e0b21_add_sessions_invalidated_at.py - 1
- MIGRATION: Column already exists in model · Line 46-46
Review Details
-
Files reviewed - 13 · Commit Range:
9814382..09aeec7- superset/daos/dashboard.py
- superset/dashboards/api.py
- superset/initialization/__init__.py
- superset/migrations/versions/2026-06-02_10-00_f7a1c93e0b21_add_sessions_invalidated_at.py
- superset/migrations/versions/2026-06-02_12-00_c4d5e6f7a8b9_add_guest_token_revoked_before.py
- superset/models/embedded_dashboard.py
- superset/models/user_attributes.py
- superset/security/manager.py
- superset/security/session_invalidation.py
- tests/integration_tests/dashboards/api_tests.py
- tests/integration_tests/security/guest_token_revocation_tests.py
- tests/integration_tests/security/session_invalidation_tests.py
- tests/unit_tests/security/test_session_invalidation.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
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
…s check Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Preserve avatar_url / welcome_dashboard_id / sessions_invalidated_at that exist only on a higher-id duplicate row by backfilling the kept (MIN id) row before deleting the duplicates, so operators upgrading from deployments with duplicate rows do not silently lose those settings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MySQL rejects referencing the UPDATE/DELETE target table in a subquery (error 1093), which broke the correlated-subquery backfill on test-mysql. Read the duplicate rows and resolve the merge winner in Python instead; this behaves identically on SQLite, MySQL and Postgres. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Accounts disabled before this feature lands had no epoch, so re-enabling one could revive a pre-existing session (which also carries no recorded login time). The migration now stamps sessions_invalidated_at for users whose ab_user.active is false, upserting one row per user. Done in Python to stay portable across SQLite/MySQL/Postgres. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #b0775fActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
SUMMARY
Implements Part A2 of the session/token-lifecycle SIP (#40674) as a self-contained, tested change: a backend-agnostic per-user session-invalidation epoch that terminates a disabled account's outstanding sessions on their next request.
Previously, disabling a user only audit-logged; access lingered until a passive check, and for client-side cookie sessions there was no server-side session to delete at all.
How it works
UserAttribute.sessions_invalidated_at(new column + migration) records the invalidation epoch.on_user_loginstampssession["_login_at"].before_requesthook forces logout of any session whose login predates the user's epoch, then lets the request continue as anonymous so each route responds correctly for its type (401 for the REST API, redirect-to-login for HTML views) — no route-kind branching needed. Fails open on any error (never locks everyone out on a bug).after_updatelistener stamps the epoch whenactiveflips toFalse, so it fires regardless of the disable path (admin UI, REST API, or CLI) and for both client-side cookie and server-side session backends. Deleted users are already rejected by Flask-Login's loader, so deletion needs no epoch.Backwards compatible by default: inert for users that were never disabled (NULL epoch). The naive-UTC column comparison is handled explicitly.
Closes part of #40674 (A2). A1/A3/Part B remain in the SIP.
TESTING INSTRUCTIONS
Validated end-to-end against a local Docker stack:
/api/v1/me/returns 200 → admin disables the user → same session →/api/v1/me/returns 401.Unit tests cover the epoch comparison incl. the UTC/naive-datetime correctness; the integration test covers the login → disable → forced-logout flow and the "active user is unaffected" case.
ADDITIONAL INFORMATION
🤖 Generated with Claude Code