EnxDev commented on PR #39469:
URL: https://github.com/apache/superset/pull/39469#issuecomment-4993683213
EnxDev's Review Agent — apache/superset#39469 · HEAD 2ec20b8
request changes — Pillar 1 is now substantively delivered and the split was
honored, but the new migration forks the Alembic tree (again) and CI is red at
this HEAD.
Supersedes my review at f0de405. Both blockers raised there are addressed in
spirit: the plaintext-password logging is fixed (manual _log_audit_event with
an id-only payload, guarded by test_put_my_password_does_not_log_passwords on
success and failure paths), and the migration was re-pointed — but at a
revision that has since gained another child on master, so the tree forks again.
Pillar 1 status vs. the SIP (your May 3 checklist)
| SIP Item | Status |
|----------|--------|
| **Multi-session invalidation** | ✅ `user_session_auth_stamp` +
`before_request` check implemented. Integration test verifies that a cloned
session cookie receives `401` (one gap remains; see findings below). |
| **Remember-me token revocation** | ⚠️ Cookie is cleared only on the
browser where the password is changed. See the stamp-adoption finding below. |
| **Password confirmation field** | ✅ Implemented with `validates_schema`,
UI field, and integration test. |
| **Real-time strength / policy indicator** | ✅ Implemented via `GET
/me/password/policy` and `AuthDbPasswordPolicyIndicator`. |
| **Audit metadata (self/admin, source IP)** | ❌ Not implemented (see
finding below). |
| **`password_hash_algorithm` (bcrypt/argon2)** | ✅ Implemented, including
legacy hash fallback, bcrypt 72-byte handling, and test coverage. |
Split status: ✅ admin-reset UI/API and auth_audit_log are out of the diff
(existing Log reused per dpgaspar). Prior blockers verified fixed at this HEAD:
doubled UserLoggedIn audits, cache-before-commit race,
session-clear-before-commit, CSRF rotation via SupersetClient.reAuthenticate().
🔴 Functional
-
superset/migrations/versions/2026-05-13_12-00_c6219cac9270_add_user_session_auth_stamp_table.py:31
· High — down_revision = "3a8e6f2c1b95" collides with master: b1c2d3e4f5a6
(add_subjects_tables) already claims that same parent, so the merged tree has
two heads and superset db upgrade aborts — today's test-load-examples failure
is exactly Error: Multiple head revisions are present… at superset init. Third
round of this failure mode (b4a3f2e1d0c9 → 9e1f3b8c4d2a → 3a8e6f2c1b95):
hand-picking a revision goes stale as master moves. Rebase onto current master
and set down_revision from superset db heads at rebase time, in the same push.
(Note: the "Check DB migration conflict" workflow passing is meaningless — it
only posts courtesy comments, it never runs alembic heads.)
🟡 Should-fix
- superset/views/users/api.py:400 + superset/security/manager.py:1339 — the
audit-metadata SIP item is still open. UserPasswordChanged carries only
{"user_id"} — no self/admin flag, no source IP (the Log row has no IP column
and the payload doesn't include one). Worse, the admin path (reset_password,
still reachable via FAB's Reset Password action) emits no password-change audit
event at all. Since Log is being reused, put it in the JSON payload —
{"user_id", "initiated_by": "self"|"admin", "actor_id", "source_ip"} — and emit
the same event from the reset_password admin branch.
- superset/utils/auth_session_stamp.py:363 — a session with no stamp in the
cookie adopts the current DB stamp instead of being invalidated. That covers
pre-upgrade sessions (defensible rollout window) but also sessions
re-established from a Flask-Login remember cookie after a password change —
remember processing sets _user_id without running on_user_login, so a stolen
remember token survives rotation in deployments that enable remember-me
(clear_flask_login_remember_cookie:139 only clears the changing browser's
cookie). Suggest adopting only when session.get("_fresh") is truthy, or
documenting the limitation.
- superset/security/manager.py — pre-commit is red: ruff I001, the added
LOGMSG_WAR_SEC_LOGIN_FAILED import is unsorted. One-line fix; include in the
rebase push.
🔵 Nits
- superset/views/users/api.py:292 — "password" in request.json raises
TypeError (→ 500) when the JSON body is a scalar (5, true); guard with
isinstance(request.json, dict).
- superset/security/manager.py:4862 — the timing-balance fake check runs
werkzeug pbkdf2 while real users verify bcrypt/argon2, so unknown-username
failures are measurably faster — a username-enumeration timing oracle. Hash the
dummy with the configured algorithm.
🙌 Praise
- superset/views/users/api.py:369 — the optimistic lock
(filter(User.password == old_hash)) with post-commit-only cache write closes
both prior races cleanly, and the conflict/500 paths now have tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
