EnxDev commented on PR #39469: URL: https://github.com/apache/superset/pull/39469#issuecomment-4842299445
## EnxDev's Review Agent โ apache/superset#39469 ยท HEAD 90160b2 **comment** โ Pillar 1 is in good shape; the earlier session-clear-before-commit blocker is resolved. Remaining items are a CSRF-rotation edge case, a per-request DB query now added for *all* auth types, and minor cleanup. This updates my [earlier review at `f4d4cd6`](https://github.com/apache/superset/pull/39469#pullrequestreview-4572827197); the audit-log infra has since been dropped (good โ matches the Pillar-1 split @rusackas/@dpgaspar asked for), and the commit/session ordering bug is fixed. Re-reviewed the current diff fresh, so older bot/maintainer threads are largely stale. ### ๐ด Functional - **`superset/views/users/api.py:2557`** (`_reestablish_login_session`) ยท _Low_ โ clearing the whole session on a successful password change rotates the Flask-WTF CSRF token (`WTF_CSRF_ENABLED = True` by default), but the SPA keeps the token it loaded at bootstrap. The old `PUT /api/v1/me/` flow never rebuilt the session, so this is new: after changing their password in-place (no page reload), the next mutating request from the same tab can 400 on CSRF until a refresh. Worth confirming `SupersetClient` re-fetches the token on a CSRF failure; if not, return/refresh the token after this call. **regression test:** with CSRF enabled, change the password then issue a mutating request (e.g. PUT a chart) on the same client and assert it isn't rejected for a stale CSRF token. ### ๐ก Should-fix - **`superset/security/manager.py:937`** (`create_login_manager` โ `register_session_auth_stamp_hook`) โ the `before_request` stamp check and the login-time stamp row are wired in unconditionally, so every authenticated request in *every* deployment (OAuth/LDAP/remote-user, not just AUTH_DB) now does a `db.session.get(UserSessionAuthStamp, user_id)` and every login writes a row. For a feature scoped to AUTH_DB password change, that's a broad per-request cost. Gate the hook/stamp to AUTH_DB, or cache the stamp. - **Patch coverage 85% (33 lines uncovered, per Codecov).** The uncovered lines are the failure paths that matter most here โ `PasswordChangeConflictError`/`SQLAlchemyError` 500 in `update_my_password`, and the `IntegrityError` retry branches in `auth_session_stamp.py`. Add an integration test for the optimistic-lock conflict (concurrent change โ 400) so those branches are guarded. ### ๐ต Nits - `superset/migrations/versions/2026-05-13_12-00_c6219cac9270_...py:1634` โ docstring says `Revises: 78a40c08b4be` but `down_revision = "a7d3f1b9c2e4"`. The code is correct (`a7d3f1b9c2e4` is the current head); fix the stale docstring line to match. - `superset/views/users/api.py:2459` (`_get_client_ip`/IP fallback in `_me_password_rate_limit_key`) โ the endpoint is `@protect()`, so `g.user.id` is always present and the IP branch is dead. Harmless, but droppable. ### ๐ Praise - `superset/utils/auth_db_password_hash.py` โ clean bcrypt/argon2 split with legacy-Werkzeug fallback so existing users keep logging in until they rotate; the 72-byte bcrypt guard and the client/server common-password sync test (`test_common_passwords_stay_in_sync_with_frontend`) are nice touches. <!-- enxdev-review-agent:90160b2 --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 90160b2._ -- 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]
