From c11ee4d552d0b6d0f74dbda58f8e47515e3f6396 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Mon, 1 Jun 2026 20:43:17 -0700 Subject: [PATCH 1/5] feat(security): force password change on first use [DRAFT] Add an opt-in "must change password" lifecycle for accounts (typically created by an administrator): - New per-user flag password_must_change on the UserAttribute table (+ Alembic migration b7c9d1e2f3a4, default False). - superset/security/password_change.py: helpers to set/clear/query the flag and a before_request hook that, when ENABLE_FORCE_PASSWORD_CHANGE is enabled, redirects flagged users to the password-reset page. The hook exempts auth / password-reset / static / health endpoints to avoid a redirect loop, and is a no-op (zero overhead) when the feature is disabled (the default). - SupersetSecurityManager.reset_password override clears the flag on a successful reset (covers both self-service and admin reset, which both route through reset_password). - New config ENABLE_FORCE_PASSWORD_CHANGE (default False). Unit tests cover the endpoint-exemption logic (redirect-loop prevention) and the flag query helper. DRAFT: behavior-sensitive request middleware on the auth path. Needs end-to-end validation that the exemption list matches the actual FAB endpoint names (no redirect loop) and that the reset flow clears the flag, before merge. Wiring the flag to admin-create + secure-random initial passwords is a follow-up. Co-Authored-By: Claude Opus 4.8 --- superset/config.py | 5 + superset/initialization/__init__.py | 8 ++ ...0_b7c9d1e2f3a4_add_password_must_change.py | 47 ++++++ superset/models/user_attributes.py | 5 + superset/security/manager.py | 11 ++ superset/security/password_change.py | 136 ++++++++++++++++++ .../security/test_password_change.py | 67 +++++++++ 7 files changed, 279 insertions(+) create mode 100644 superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py create mode 100644 superset/security/password_change.py create mode 100644 tests/unit_tests/security/test_password_change.py diff --git a/superset/config.py b/superset/config.py index 5380df762de2..80639b36ec1a 100644 --- a/superset/config.py +++ b/superset/config.py @@ -362,6 +362,11 @@ def _try_json_readsha(filepath: str, length: int) -> str | None: RATELIMIT_APPLICATION = "50 per second" AUTH_RATE_LIMITED = True AUTH_RATE_LIMIT = "5 per second" + +# When enabled, users whose account is flagged with ``password_must_change`` +# (e.g. accounts provisioned by an administrator) are redirected to the +# password-reset page until they set a new password. Off by default. +ENABLE_FORCE_PASSWORD_CHANGE = False # A storage location conforming to the scheme in storage-scheme. See the limits # library for allowed values: https://limits.readthedocs.io/en/stable/storage.html # RATELIMIT_STORAGE_URI = "redis://host:port" diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py index ec7f7e233a21..738bfb22984c 100644 --- a/superset/initialization/__init__.py +++ b/superset/initialization/__init__.py @@ -729,6 +729,14 @@ def register_request_handlers(self) -> None: """Register app-level request handlers""" from flask import request, Response + from superset.security.password_change import ( + register_password_change_enforcement, + ) + + # Redirect users with a pending forced password change to the reset + # page (no-op unless ENABLE_FORCE_PASSWORD_CHANGE is enabled). + register_password_change_enforcement(self.superset_app) + @self.superset_app.after_request def apply_http_headers(response: Response) -> Response: """Applies the configuration's http headers to all responses""" diff --git a/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py b/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py new file mode 100644 index 000000000000..695f81239e88 --- /dev/null +++ b/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py @@ -0,0 +1,47 @@ +# 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. +"""add password_must_change to user_attribute + +Revision ID: b7c9d1e2f3a4 +Revises: 33d7e0e21daa +Create Date: 2026-06-01 00:00:00.000000 + +""" + +import sqlalchemy as sa + +from superset.migrations.shared.utils import add_columns, drop_columns + +# revision identifiers, used by Alembic. +revision = "b7c9d1e2f3a4" +down_revision = "33d7e0e21daa" + + +def upgrade(): + add_columns( + "user_attribute", + sa.Column( + "password_must_change", + sa.Boolean(), + nullable=False, + server_default=sa.false(), + ), + ) + + +def downgrade(): + drop_columns("user_attribute", "password_must_change") diff --git a/superset/models/user_attributes.py b/superset/models/user_attributes.py index 92ac0c9dde12..054f0015b89b 100644 --- a/superset/models/user_attributes.py +++ b/superset/models/user_attributes.py @@ -17,6 +17,7 @@ from flask_appbuilder import Model from sqlalchemy import ( + Boolean, Column, DateTime, ForeignKey, @@ -57,3 +58,7 @@ class UserAttribute(Model, AuditMixinNullable): # Stamped when the account is disabled, so outstanding sessions terminate # regardless of the session backend. NULL means "never invalidated". sessions_invalidated_at = Column(DateTime, nullable=True, index=True) + # When True, the user must change their password before they can use the + # rest of the app (enforced by the before-request hook in + # superset.security.password_change when ENABLE_FORCE_PASSWORD_CHANGE is on). + password_must_change = Column(Boolean, nullable=False, default=False) diff --git a/superset/security/manager.py b/superset/security/manager.py index bd3674353c5e..89fb270b5845 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -640,6 +640,17 @@ def create_login_manager(self, app: Flask) -> LoginManager: lm.request_loader(self.request_loader) return lm + def reset_password(self, userid: int, password: str) -> None: + """Reset a user's password and clear any pending forced-change flag. + + Covers both the self-service reset and the admin "Reset Password" action, + which both route through this method. + """ + super().reset_password(userid, password) + from superset.security.password_change import clear_password_must_change + + clear_password_must_change(userid) + def on_user_login(self, user: Any) -> None: # pylint: disable=import-outside-toplevel from superset.security.session_invalidation import stamp_login_time diff --git a/superset/security/password_change.py b/superset/security/password_change.py new file mode 100644 index 000000000000..859166d13f06 --- /dev/null +++ b/superset/security/password_change.py @@ -0,0 +1,136 @@ +# 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. +"""Force-password-change-on-first-use enforcement. + +A per-user ``password_must_change`` flag (on ``UserAttribute``) marks accounts — +typically created by an administrator — that must set a new password before +they can use the rest of the application. When ``ENABLE_FORCE_PASSWORD_CHANGE`` +is enabled, a ``before_request`` hook redirects such users to the password-reset +page until they change it; the flag is cleared automatically on a successful +password reset (see ``SupersetSecurityManager.reset_password``). +""" + +from __future__ import annotations + +import logging +from typing import Any, Optional + +from flask import current_app, flash, g, redirect, request, url_for +from flask_babel import gettext as __ + +from superset.utils.decorators import transaction + +logger = logging.getLogger(__name__) + +# Endpoint substrings that must remain reachable while a password change is +# pending, otherwise the redirect would loop (login/logout, the password-reset +# and user-info views, static assets, and the health check). +_EXEMPT_ENDPOINT_TOKENS = ( + "static", + "appbuilder", + "login", + "logout", + "resetmypassword", + "resetpassword", + "userinfoedit", + "userinfo", + "auth", + "health", +) + + +def _get_user_attribute(user_id: int) -> Optional[Any]: + # Imported lazily to avoid import cycles at app-init time. + from superset.extensions import db + from superset.models.user_attributes import UserAttribute + + return ( + db.session.query(UserAttribute) + .filter(UserAttribute.user_id == user_id) + .one_or_none() + ) + + +def password_change_required(user: Any) -> bool: + """Return True if ``user`` has a pending forced password change.""" + user_id = getattr(user, "id", None) + if not user_id: + return False + attr = _get_user_attribute(user_id) + return bool(attr and attr.password_must_change) + + +@transaction() +def set_password_must_change(user_id: int, value: bool = True) -> None: + """Set (or clear) the forced-password-change flag for a user. + + Intended to be called by administrative flows when provisioning an account + that should require a password change on first use. + """ + from superset.extensions import db + from superset.models.user_attributes import UserAttribute + + attr = _get_user_attribute(user_id) + if attr is None: + attr = UserAttribute(user_id=user_id) + db.session.add(attr) + attr.password_must_change = value + + +@transaction() +def clear_password_must_change(user_id: int) -> None: + """Clear the forced-password-change flag for a user, if set.""" + attr = _get_user_attribute(user_id) + if attr and attr.password_must_change: + attr.password_must_change = False + + +def _is_exempt_endpoint(endpoint: Optional[str]) -> bool: + if not endpoint: + return True + lowered = endpoint.lower() + return any(token in lowered for token in _EXEMPT_ENDPOINT_TOKENS) + + +def register_password_change_enforcement(app: Any) -> None: + """Register the before-request hook that enforces pending password changes. + + No-op unless ``ENABLE_FORCE_PASSWORD_CHANGE`` is enabled, so there is zero + per-request overhead in the default configuration. + """ + + @app.before_request + def _enforce_password_change() -> Any: # pylint: disable=unused-variable + if not current_app.config.get("ENABLE_FORCE_PASSWORD_CHANGE"): + return None + + user = getattr(g, "user", None) + if not user or getattr(user, "is_anonymous", True): + return None + + if _is_exempt_endpoint(request.endpoint): + return None + + if not password_change_required(user): + return None + + flash(__("You must change your password before continuing."), "warning") + try: + target = url_for("ResetMyPasswordView.this_form_get") + except Exception: # noqa: BLE001 # pylint: disable=broad-except + target = "/" + return redirect(target) diff --git a/tests/unit_tests/security/test_password_change.py b/tests/unit_tests/security/test_password_change.py new file mode 100644 index 000000000000..af58eb082bdb --- /dev/null +++ b/tests/unit_tests/security/test_password_change.py @@ -0,0 +1,67 @@ +# 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 + +import pytest + +from superset.security.password_change import ( + _is_exempt_endpoint, + password_change_required, +) + + +@pytest.mark.parametrize( + "endpoint,expected", + [ + (None, True), # static file serving etc. + ("ResetMyPasswordView.this_form_get", True), + ("AuthDBView.login", True), + ("AuthDBView.logout", True), + ("appbuilder.static", True), + ("UserInfoEditView.this_form_post", True), + ("SupersetIndexView.index", False), + ("Superset.dashboard", False), + ], +) +def test_is_exempt_endpoint(endpoint, expected) -> None: + # The password-reset / auth / static endpoints must stay reachable to avoid + # a redirect loop while a change is pending. + assert _is_exempt_endpoint(endpoint) is expected + + +def test_password_change_required() -> None: + user = MagicMock() + user.id = 5 + + with patch( + "superset.security.password_change._get_user_attribute" + ) as mock_get_attr: + mock_get_attr.return_value = MagicMock(password_must_change=True) + assert password_change_required(user) is True + + mock_get_attr.return_value = MagicMock(password_must_change=False) + assert password_change_required(user) is False + + mock_get_attr.return_value = None + assert password_change_required(user) is False + + +def test_password_change_required_no_user_id() -> None: + user = MagicMock() + user.id = None + assert password_change_required(user) is False From b345be22a665051b2974f91a8eae5dc2d66874f6 Mon Sep 17 00:00:00 2001 From: Evan Date: Tue, 9 Jun 2026 17:59:02 -0700 Subject: [PATCH 2/5] fix(security): tighten endpoint exemption and migration down_revision - Match exempt view classes exactly instead of substring matching so unrelated endpoints sharing a substring are not exempted from force-password-change enforcement. - Point the migration down_revision at the current alembic head. - Use @transaction() instead of manual db.session.commit(). Co-Authored-By: Claude Opus 4.8 --- ...0_b7c9d1e2f3a4_add_password_must_change.py | 4 +- superset/security/password_change.py | 49 +++++++++++++------ .../security/test_password_change.py | 9 ++++ 3 files changed, 44 insertions(+), 18 deletions(-) diff --git a/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py b/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py index 695f81239e88..4472e9a07ad5 100644 --- a/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py +++ b/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py @@ -17,7 +17,7 @@ """add password_must_change to user_attribute Revision ID: b7c9d1e2f3a4 -Revises: 33d7e0e21daa +Revises: 31dae2559c05 Create Date: 2026-06-01 00:00:00.000000 """ @@ -28,7 +28,7 @@ # revision identifiers, used by Alembic. revision = "b7c9d1e2f3a4" -down_revision = "33d7e0e21daa" +down_revision = "31dae2559c05" def upgrade(): diff --git a/superset/security/password_change.py b/superset/security/password_change.py index 859166d13f06..717d8f1615f9 100644 --- a/superset/security/password_change.py +++ b/superset/security/password_change.py @@ -36,22 +36,31 @@ logger = logging.getLogger(__name__) -# Endpoint substrings that must remain reachable while a password change is -# pending, otherwise the redirect would loop (login/logout, the password-reset -# and user-info views, static assets, and the health check). -_EXEMPT_ENDPOINT_TOKENS = ( - "static", - "appbuilder", - "login", - "logout", - "resetmypassword", - "resetpassword", - "userinfoedit", - "userinfo", - "auth", - "health", +# Flask endpoints take the form ``.`` (or a bare name for +# function views). The following must remain reachable while a password change +# is pending, otherwise the redirect would loop: the auth views (login/logout +# for every auth backend), the password-reset and user-info-edit views, static +# assets, and the health check. We match the *view-class* component (the part +# before the dot) exactly against the allow-list below rather than doing a +# substring search, so unrelated endpoints that merely share a substring (e.g. +# an "Author"-named view, or any name containing "health"/"static") are not +# accidentally exempted from enforcement. +_EXEMPT_VIEW_CLASSES = frozenset( + { + "AuthDBView", + "AuthLDAPView", + "AuthOAuthView", + "AuthOIDView", + "AuthRemoteUserView", + "ResetMyPasswordView", + "ResetPasswordView", + "UserInfoEditView", + } ) +# Exact endpoint names (function views / Flask built-ins) that are always exempt. +_EXEMPT_ENDPOINTS = frozenset({"static", "appbuilder.static", "health", "healthcheck"}) + def _get_user_attribute(user_id: int) -> Optional[Any]: # Imported lazily to avoid import cycles at app-init time. @@ -100,10 +109,18 @@ def clear_password_must_change(user_id: int) -> None: def _is_exempt_endpoint(endpoint: Optional[str]) -> bool: + # A missing endpoint (e.g. an unmatched URL) is left to normal 404 handling. if not endpoint: return True - lowered = endpoint.lower() - return any(token in lowered for token in _EXEMPT_ENDPOINT_TOKENS) + if endpoint in _EXEMPT_ENDPOINTS: + return True + # Any blueprint's static route, e.g. ".static". + if endpoint.endswith(".static"): + return True + # Match the view-class component exactly, so e.g. "AuthDBView.login" is + # exempt but an unrelated "AuthorView.list" is not. + view_class = endpoint.split(".", 1)[0] + return view_class in _EXEMPT_VIEW_CLASSES def register_password_change_enforcement(app: Any) -> None: diff --git a/tests/unit_tests/security/test_password_change.py b/tests/unit_tests/security/test_password_change.py index af58eb082bdb..ed460b337d97 100644 --- a/tests/unit_tests/security/test_password_change.py +++ b/tests/unit_tests/security/test_password_change.py @@ -34,8 +34,17 @@ ("AuthDBView.logout", True), ("appbuilder.static", True), ("UserInfoEditView.this_form_post", True), + ("AuthOAuthView.login", True), + ("SomeBlueprint.static", True), + ("health", True), ("SupersetIndexView.index", False), ("Superset.dashboard", False), + # Substring over-matching must NOT exempt these (they merely share a + # substring with an exempt token). + ("AuthorView.list", False), + ("HealthDashboardView.show", False), + ("StaticAssetReportView.list", False), + ("UserInfoFancyView.show", False), ], ) def test_is_exempt_endpoint(endpoint, expected) -> None: From 1df7ceabe925539d21485fc44d47ab28a3484716 Mon Sep 17 00:00:00 2001 From: Evan Date: Tue, 9 Jun 2026 18:17:26 -0700 Subject: [PATCH 3/5] fix(security): harden forced-password-change reset, lookup, and redirect Address three codeant-ai review findings on the force-password-change feature: - Admin-initiated resets no longer clear the must-change flag. Only a self-service reset (acting user == target user) clears it, so an admin setting a temporary password preserves the change-at-next-login requirement. - _get_user_attribute fetches deterministically via order_by(id).first() instead of one_or_none(), so duplicate user_attribute rows (the table has no uniqueness on user_id) can't raise MultipleResultsFound / 500. - The enforcement redirect fallback now targets an exempt route (logout) and returns a 503 if no exempt target resolves, instead of redirecting to the non-exempt index and trapping flagged users in a 302 loop. Adds unit tests pinning each corrected behavior. Co-Authored-By: Claude Opus 4.8 --- ...0_b7c9d1e2f3a4_add_password_must_change.py | 4 +- superset/security/manager.py | 46 +++++- superset/security/password_change.py | 34 ++++- tests/unit_tests/security/manager_test.py | 73 +++++++++ .../security/test_password_change.py | 138 ++++++++++++++++++ 5 files changed, 279 insertions(+), 16 deletions(-) diff --git a/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py b/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py index 4472e9a07ad5..c0c153b6fd79 100644 --- a/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py +++ b/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py @@ -17,7 +17,7 @@ """add password_must_change to user_attribute Revision ID: b7c9d1e2f3a4 -Revises: 31dae2559c05 +Revises: f7a1c93e0b21 Create Date: 2026-06-01 00:00:00.000000 """ @@ -28,7 +28,7 @@ # revision identifiers, used by Alembic. revision = "b7c9d1e2f3a4" -down_revision = "31dae2559c05" +down_revision = "f7a1c93e0b21" def upgrade(): diff --git a/superset/security/manager.py b/superset/security/manager.py index 89fb270b5845..195a4aa41787 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -23,7 +23,7 @@ from collections import defaultdict from math import ceil from types import SimpleNamespace -from typing import Any, Callable, cast, NamedTuple, Optional, TYPE_CHECKING +from typing import Any, Callable, cast, NamedTuple, Optional, TYPE_CHECKING, Union from flask import current_app, Flask, g, Request from flask_appbuilder import Model @@ -640,16 +640,48 @@ def create_login_manager(self, app: Flask) -> LoginManager: lm.request_loader(self.request_loader) return lm - def reset_password(self, userid: int, password: str) -> None: - """Reset a user's password and clear any pending forced-change flag. + def reset_password(self, userid: Union[int, str], password: str) -> None: + """Reset a user's password, clearing the forced-change flag only on a + self-service reset. - Covers both the self-service reset and the admin "Reset Password" action, - which both route through this method. + Both the self-service reset (``ResetMyPasswordView``) and the admin + "Reset Password" action (``ResetPasswordView``) route through this + method. The forced-password-change flag must only be cleared when the + user resets *their own* password — an admin-initiated reset sets a + temporary password and must preserve the "must change at next login" + requirement, otherwise the first-use lifecycle would be silently + bypassed. We distinguish the two by comparing the acting user + (``g.user``) against the target ``userid``: they match for a + self-service reset and differ for an admin reset. """ super().reset_password(userid, password) - from superset.security.password_change import clear_password_must_change - clear_password_must_change(userid) + acting_user = getattr(g, "user", None) + acting_user_id = getattr(acting_user, "id", None) + # ``userid`` arrives as a string (the ``pk`` request arg) on the admin + # path, so coerce both sides before comparing. + is_self_service = acting_user_id is not None and self._same_user( + acting_user_id, userid + ) + if is_self_service: + from superset.security.password_change import ( + clear_password_must_change, + ) + + clear_password_must_change(int(userid)) + + @staticmethod + def _same_user(left: Any, right: Any) -> bool: + """Return True if two user identifiers refer to the same user. + + Identifiers may be ints or numeric strings (FAB passes the admin-reset + target as a ``pk`` request arg string), so compare them as integers and + fall back to a string comparison if coercion fails. + """ + try: + return int(left) == int(right) + except (TypeError, ValueError): + return str(left) == str(right) def on_user_login(self, user: Any) -> None: # pylint: disable=import-outside-toplevel diff --git a/superset/security/password_change.py b/superset/security/password_change.py index 717d8f1615f9..7814295bcb06 100644 --- a/superset/security/password_change.py +++ b/superset/security/password_change.py @@ -21,7 +21,9 @@ they can use the rest of the application. When ``ENABLE_FORCE_PASSWORD_CHANGE`` is enabled, a ``before_request`` hook redirects such users to the password-reset page until they change it; the flag is cleared automatically on a successful -password reset (see ``SupersetSecurityManager.reset_password``). +*self-service* password reset (see ``SupersetSecurityManager.reset_password``). +An admin-initiated reset deliberately preserves the flag so the target user is +still forced to change the temporary password at next login. """ from __future__ import annotations @@ -67,10 +69,15 @@ def _get_user_attribute(user_id: int) -> Optional[Any]: from superset.extensions import db from superset.models.user_attributes import UserAttribute + # The ``user_attribute`` table does not enforce uniqueness on ``user_id``, + # so a user could have more than one row. ``.one_or_none()`` would raise + # ``MultipleResultsFound`` (a 500) in that case; fetch deterministically by + # ordering on the primary key and taking the first row instead. return ( db.session.query(UserAttribute) .filter(UserAttribute.user_id == user_id) - .one_or_none() + .order_by(UserAttribute.id) + .first() ) @@ -146,8 +153,21 @@ def _enforce_password_change() -> Any: # pylint: disable=unused-variable return None flash(__("You must change your password before continuing."), "warning") - try: - target = url_for("ResetMyPasswordView.this_form_get") - except Exception: # noqa: BLE001 # pylint: disable=broad-except - target = "/" - return redirect(target) + # Resolve the password-reset page. If that endpoint can't be resolved + # (e.g. a custom security manager without ``ResetMyPasswordView``), fall + # back to logout, which is always exempt from this enforcement. We must + # NOT fall back to "/" or any other non-exempt route: the index re-runs + # this same hook and would trap the user in an infinite 302 loop. If no + # exempt target can be resolved at all, return an error response rather + # than redirect, so a flagged user can never get stuck looping. + for endpoint in ("ResetMyPasswordView.this_form_get", "AuthDBView.logout"): + try: + return redirect(url_for(endpoint)) + except Exception: # noqa: BLE001, S112 # pylint: disable=broad-except + # Try the next exempt fallback; a failed url_for resolution here + # is expected/benign and not worth logging per attempt. + continue + return ( + __("You must change your password before continuing."), + 503, + ) diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index f5e4190fa6bb..1968bbc9d9b1 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -1619,3 +1619,76 @@ def test_user_view_menu_names_for_guest_user_no_roles( assert result == set() mock_get_user_id.assert_not_called() + + +def test_reset_password_self_service_clears_flag( + mocker: MockerFixture, + app_context: None, +) -> None: + """A user resetting their own password clears the forced-change flag.""" + sm = SupersetSecurityManager(appbuilder) + # The target user (id 5) is the same as the acting user -> self-service. + mock_g = SimpleNamespace(user=SimpleNamespace(id=5)) + mocker.patch("superset.security.manager.g", new=mock_g) + # Avoid touching the real DB in the FAB base implementation. + mocker.patch( + "flask_appbuilder.security.manager.BaseSecurityManager.reset_password", + return_value=None, + ) + mock_clear = mocker.patch( + "superset.security.password_change.clear_password_must_change" + ) + + sm.reset_password(5, "new-password") + + mock_clear.assert_called_once_with(5) + + +def test_reset_password_admin_does_not_clear_flag( + mocker: MockerFixture, + app_context: None, +) -> None: + """An admin-initiated reset must preserve the forced-change requirement. + + FAB's ``ResetPasswordView`` passes the target as the ``pk`` request-arg + string while ``g.user`` remains the admin, so the acting user differs from + the target and the flag must NOT be cleared. + """ + sm = SupersetSecurityManager(appbuilder) + # Acting user is admin (id 1); target is a different user ("5" as a string, + # as FAB passes it from request args). + mock_g = SimpleNamespace(user=SimpleNamespace(id=1)) + mocker.patch("superset.security.manager.g", new=mock_g) + mocker.patch( + "flask_appbuilder.security.manager.BaseSecurityManager.reset_password", + return_value=None, + ) + mock_clear = mocker.patch( + "superset.security.password_change.clear_password_must_change" + ) + + sm.reset_password("5", "temp-password") + + mock_clear.assert_not_called() + + +def test_reset_password_self_service_pk_string_clears_flag( + mocker: MockerFixture, + app_context: None, +) -> None: + """Self-service identity holds even if ids arrive as mixed int/str types.""" + sm = SupersetSecurityManager(appbuilder) + mock_g = SimpleNamespace(user=SimpleNamespace(id=5)) + mocker.patch("superset.security.manager.g", new=mock_g) + mocker.patch( + "flask_appbuilder.security.manager.BaseSecurityManager.reset_password", + return_value=None, + ) + mock_clear = mocker.patch( + "superset.security.password_change.clear_password_must_change" + ) + + sm.reset_password("5", "new-password") + + # Coerced to int when clearing, regardless of the inbound id type. + mock_clear.assert_called_once_with(5) diff --git a/tests/unit_tests/security/test_password_change.py b/tests/unit_tests/security/test_password_change.py index ed460b337d97..03c136ecf65a 100644 --- a/tests/unit_tests/security/test_password_change.py +++ b/tests/unit_tests/security/test_password_change.py @@ -20,6 +20,7 @@ import pytest from superset.security.password_change import ( + _get_user_attribute, _is_exempt_endpoint, password_change_required, ) @@ -74,3 +75,140 @@ def test_password_change_required_no_user_id() -> None: user = MagicMock() user.id = None assert password_change_required(user) is False + + +def test_get_user_attribute_deterministic_with_duplicates() -> None: + # The ``user_attribute`` table does not enforce uniqueness on ``user_id``, + # so duplicate rows are possible. The query must not raise (which + # ``.one_or_none()`` would have done via ``MultipleResultsFound``); it must + # fetch a single row deterministically via ``order_by(id).first()``. + query = MagicMock() + db = MagicMock() + db.session.query.return_value = query + query.filter.return_value = query + query.order_by.return_value = query + sentinel = MagicMock(name="first_row") + query.first.return_value = sentinel + + with ( + patch("superset.extensions.db", db), + patch("superset.models.user_attributes.UserAttribute") as user_attribute, + ): + result = _get_user_attribute(5) + + # Deterministic ordering on the primary key, then ``.first()`` — never + # ``.one_or_none()``, which could 500 on duplicate rows. + query.order_by.assert_called_once_with(user_attribute.id) + query.first.assert_called_once_with() + assert not query.one_or_none.called + assert result is sentinel + + +@pytest.fixture +def enforcement_app(): + """A minimal Flask app with the enforcement hook registered and a flagged + user, used to exercise the before-request redirect behavior end to end.""" + from flask import Flask, g + + from superset.security.password_change import ( + register_password_change_enforcement, + ) + + app = Flask(__name__) + app.config["ENABLE_FORCE_PASSWORD_CHANGE"] = True + app.secret_key = "test" # noqa: S105 + + user = MagicMock() + user.id = 5 + user.is_anonymous = False + + @app.before_request + def _set_user() -> None: # pylint: disable=unused-variable + g.user = user + + # A non-exempt route that, if redirected to, would re-trigger enforcement. + @app.route("/") + def index() -> str: # pylint: disable=unused-variable + return "index" + + register_password_change_enforcement(app) + return app + + +@pytest.fixture(autouse=True) +def _no_babel_flash(): + """The minimal test app has no babel/flash messaging set up; stub them so + the enforcement hook's translation + flash calls don't blow up. These are + incidental to the redirect-target logic under test.""" + with ( + patch("superset.security.password_change.flash"), + patch("superset.security.password_change.__", side_effect=lambda s: s), + ): + yield + + +def test_enforcement_redirects_to_reset_view(enforcement_app) -> None: + # Happy path: the reset endpoint resolves, so flagged users are redirected + # there (an exempt route) — no loop. + with ( + patch( + "superset.security.password_change.password_change_required", + return_value=True, + ), + patch( + "superset.security.password_change.url_for", + return_value="/resetmypassword/form", + ), + ): + resp = enforcement_app.test_client().get("/") + assert resp.status_code == 302 + assert resp.headers["Location"].endswith("/resetmypassword/form") + + +def test_enforcement_falls_back_to_exempt_logout_not_index(enforcement_app) -> None: + # If the reset endpoint can't be resolved, the fallback must be an exempt + # route (logout) — never "/" / the index, which would loop. We make the + # reset endpoint fail and the logout endpoint resolve. + def fake_url_for(endpoint: str, *args, **kwargs) -> str: + if endpoint == "ResetMyPasswordView.this_form_get": + raise RuntimeError("no such endpoint") + if endpoint == "AuthDBView.logout": + return "/logout" + raise AssertionError(f"unexpected endpoint {endpoint}") + + with ( + patch( + "superset.security.password_change.password_change_required", + return_value=True, + ), + patch( + "superset.security.password_change.url_for", + side_effect=fake_url_for, + ), + ): + resp = enforcement_app.test_client().get("/") + assert resp.status_code == 302 + location = resp.headers["Location"] + assert location.endswith("/logout") + # Crucially, the fallback is NOT a redirect back to the non-exempt index. + assert not location.endswith("/") + + +def test_enforcement_no_resolvable_target_returns_error_not_loop( + enforcement_app, +) -> None: + # If NO exempt target can be resolved, we must return an error response + # rather than redirect, so the flagged user can never get stuck in a loop. + with ( + patch( + "superset.security.password_change.password_change_required", + return_value=True, + ), + patch( + "superset.security.password_change.url_for", + side_effect=RuntimeError("no endpoints"), + ), + ): + resp = enforcement_app.test_client().get("/") + assert resp.status_code == 503 + assert "Location" not in resp.headers From 74808105f3c5068eadbe63bca0a529eb10470b86 Mon Sep 17 00:00:00 2001 From: Evan Date: Thu, 11 Jun 2026 21:06:10 -0700 Subject: [PATCH 4/5] fix(migrations): merge sibling migration heads from master Master gained two sibling alembic heads (c8d2e3f4a5b6 and f7a1c93e0b21, both revising 31dae2559c05), so any branch adding a migration on top of just one of them fails 'superset db upgrade' with multiple heads. Make this PR's migration a merge point over both heads. Co-Authored-By: Claude Opus 4.8 --- .../2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py b/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py index c0c153b6fd79..22571c5ec211 100644 --- a/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py +++ b/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py @@ -17,7 +17,7 @@ """add password_must_change to user_attribute Revision ID: b7c9d1e2f3a4 -Revises: f7a1c93e0b21 +Revises: c8d2e3f4a5b6, f7a1c93e0b21 Create Date: 2026-06-01 00:00:00.000000 """ @@ -28,7 +28,7 @@ # revision identifiers, used by Alembic. revision = "b7c9d1e2f3a4" -down_revision = "f7a1c93e0b21" +down_revision = ("c8d2e3f4a5b6", "f7a1c93e0b21") def upgrade(): From 947778c60ce654934f83417faff1e8e4e53eefc9 Mon Sep 17 00:00:00 2001 From: Evan Date: Thu, 11 Jun 2026 21:10:50 -0700 Subject: [PATCH 5/5] fix(security): address review feedback on forced password change - Upsert-with-retry in set_password_must_change: the unique constraint on user_attribute.user_id means a concurrent insert can win between the read and the flush; catch IntegrityError in a nested transaction and update the winner's row (mirrors the session-invalidation upsert). - Derive the logout fallback from the registered auth view so non-DB auth backends (LDAP/OAuth/OID/remote-user) get a working exempt fallback, guarded so a non-exempt custom auth view can never become a loop target. - Refresh stale comments that predate the user_id unique constraint. - Typing/docstring nits: annotate migration upgrade/downgrade, test params and fixtures; add docstrings to the before-request hook and tests. Co-Authored-By: Claude Opus 4.8 --- ...0_b7c9d1e2f3a4_add_password_must_change.py | 4 +- superset/security/password_change.py | 58 +++++++++++++++---- .../security/test_password_change.py | 27 ++++++--- 3 files changed, 68 insertions(+), 21 deletions(-) diff --git a/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py b/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py index 22571c5ec211..5c7422898aa2 100644 --- a/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py +++ b/superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py @@ -31,7 +31,7 @@ down_revision = ("c8d2e3f4a5b6", "f7a1c93e0b21") -def upgrade(): +def upgrade() -> None: add_columns( "user_attribute", sa.Column( @@ -43,5 +43,5 @@ def upgrade(): ) -def downgrade(): +def downgrade() -> None: drop_columns("user_attribute", "password_must_change") diff --git a/superset/security/password_change.py b/superset/security/password_change.py index 7814295bcb06..eacd9b795cdc 100644 --- a/superset/security/password_change.py +++ b/superset/security/password_change.py @@ -33,6 +33,7 @@ from flask import current_app, flash, g, redirect, request, url_for from flask_babel import gettext as __ +from sqlalchemy.exc import IntegrityError from superset.utils.decorators import transaction @@ -69,10 +70,11 @@ def _get_user_attribute(user_id: int) -> Optional[Any]: from superset.extensions import db from superset.models.user_attributes import UserAttribute - # The ``user_attribute`` table does not enforce uniqueness on ``user_id``, - # so a user could have more than one row. ``.one_or_none()`` would raise - # ``MultipleResultsFound`` (a 500) in that case; fetch deterministically by - # ordering on the primary key and taking the first row instead. + # ``user_attribute.user_id`` carries a unique constraint, but databases + # migrated from before the constraint existed could contain duplicate rows. + # ``.one_or_none()`` would raise ``MultipleResultsFound`` (a 500) in that + # case; fetch deterministically by ordering on the primary key and taking + # the first row instead. return ( db.session.query(UserAttribute) .filter(UserAttribute.user_id == user_id) @@ -102,8 +104,21 @@ def set_password_must_change(user_id: int, value: bool = True) -> None: attr = _get_user_attribute(user_id) if attr is None: - attr = UserAttribute(user_id=user_id) - db.session.add(attr) + # ``user_attribute.user_id`` carries a unique constraint, so a + # concurrent call for the same user can win the insert between our + # read and flush. Insert in a nested transaction and, on conflict, + # fall through to update the row the winner created (mirroring the + # upsert in ``superset.security.session_invalidation``). + try: + with db.session.begin_nested(): + db.session.add( + UserAttribute(user_id=user_id, password_must_change=value) + ) + return + except IntegrityError: + attr = _get_user_attribute(user_id) + if attr is None: # pragma: no cover - the conflicting row vanished + raise attr.password_must_change = value @@ -139,6 +154,12 @@ def register_password_change_enforcement(app: Any) -> None: @app.before_request def _enforce_password_change() -> Any: # pylint: disable=unused-variable + """Redirect flagged users to the password-reset page. + + Returns ``None`` (request proceeds) for anonymous users, exempt + endpoints, and users without a pending change; otherwise returns a + redirect to an exempt target (or an error response if none resolves). + """ if not current_app.config.get("ENABLE_FORCE_PASSWORD_CHANGE"): return None @@ -155,12 +176,29 @@ def _enforce_password_change() -> Any: # pylint: disable=unused-variable flash(__("You must change your password before continuing."), "warning") # Resolve the password-reset page. If that endpoint can't be resolved # (e.g. a custom security manager without ``ResetMyPasswordView``), fall - # back to logout, which is always exempt from this enforcement. We must - # NOT fall back to "/" or any other non-exempt route: the index re-runs - # this same hook and would trap the user in an infinite 302 loop. If no + # back to logout, which is always exempt from this enforcement. The + # logout endpoint is derived from the *registered* auth view so the + # fallback works for non-DB auth backends (LDAP, OAuth, remote-user) + # too, with ``AuthDBView.logout`` as a last resort. We must NOT fall + # back to "/" or any other non-exempt route: the index re-runs this + # same hook and would trap the user in an infinite 302 loop. If no # exempt target can be resolved at all, return an error response rather # than redirect, so a flagged user can never get stuck looping. - for endpoint in ("ResetMyPasswordView.this_form_get", "AuthDBView.logout"): + candidates = ["ResetMyPasswordView.this_form_get"] + auth_view = getattr( + getattr(getattr(current_app, "appbuilder", None), "sm", None), + "auth_view", + None, + ) + # Only redirect to the registered auth view's logout if that view is + # itself exempt from this hook; otherwise the redirect would loop. + if ( + auth_view is not None + and getattr(auth_view, "endpoint", None) in _EXEMPT_VIEW_CLASSES + ): + candidates.append(f"{auth_view.endpoint}.logout") + candidates.append("AuthDBView.logout") + for endpoint in candidates: try: return redirect(url_for(endpoint)) except Exception: # noqa: BLE001, S112 # pylint: disable=broad-except diff --git a/tests/unit_tests/security/test_password_change.py b/tests/unit_tests/security/test_password_change.py index 03c136ecf65a..236d0949c991 100644 --- a/tests/unit_tests/security/test_password_change.py +++ b/tests/unit_tests/security/test_password_change.py @@ -15,9 +15,12 @@ # specific language governing permissions and limitations # under the License. +from collections.abc import Iterator +from typing import Optional from unittest.mock import MagicMock, patch import pytest +from flask import Flask from superset.security.password_change import ( _get_user_attribute, @@ -48,13 +51,15 @@ ("UserInfoFancyView.show", False), ], ) -def test_is_exempt_endpoint(endpoint, expected) -> None: +def test_is_exempt_endpoint(endpoint: Optional[str], expected: bool) -> None: + """Exempt-endpoint matching is exact per view class, never substring.""" # The password-reset / auth / static endpoints must stay reachable to avoid # a redirect loop while a change is pending. assert _is_exempt_endpoint(endpoint) is expected def test_password_change_required() -> None: + """The flag on the user's attribute row drives the required-change check.""" user = MagicMock() user.id = 5 @@ -72,14 +77,16 @@ def test_password_change_required() -> None: def test_password_change_required_no_user_id() -> None: + """A user without an id (e.g. anonymous) never requires a change.""" user = MagicMock() user.id = None assert password_change_required(user) is False def test_get_user_attribute_deterministic_with_duplicates() -> None: - # The ``user_attribute`` table does not enforce uniqueness on ``user_id``, - # so duplicate rows are possible. The query must not raise (which + """Duplicate attribute rows must yield a deterministic row, not a 500.""" + # Databases migrated from before the ``user_attribute.user_id`` unique + # constraint could contain duplicate rows. The query must not raise (which # ``.one_or_none()`` would have done via ``MultipleResultsFound``); it must # fetch a single row deterministically via ``order_by(id).first()``. query = MagicMock() @@ -105,10 +112,10 @@ def test_get_user_attribute_deterministic_with_duplicates() -> None: @pytest.fixture -def enforcement_app(): +def enforcement_app() -> Flask: """A minimal Flask app with the enforcement hook registered and a flagged user, used to exercise the before-request redirect behavior end to end.""" - from flask import Flask, g + from flask import g from superset.security.password_change import ( register_password_change_enforcement, @@ -136,7 +143,7 @@ def index() -> str: # pylint: disable=unused-variable @pytest.fixture(autouse=True) -def _no_babel_flash(): +def _no_babel_flash() -> Iterator[None]: """The minimal test app has no babel/flash messaging set up; stub them so the enforcement hook's translation + flash calls don't blow up. These are incidental to the redirect-target logic under test.""" @@ -147,7 +154,7 @@ def _no_babel_flash(): yield -def test_enforcement_redirects_to_reset_view(enforcement_app) -> None: +def test_enforcement_redirects_to_reset_view(enforcement_app: Flask) -> None: # Happy path: the reset endpoint resolves, so flagged users are redirected # there (an exempt route) — no loop. with ( @@ -165,7 +172,9 @@ def test_enforcement_redirects_to_reset_view(enforcement_app) -> None: assert resp.headers["Location"].endswith("/resetmypassword/form") -def test_enforcement_falls_back_to_exempt_logout_not_index(enforcement_app) -> None: +def test_enforcement_falls_back_to_exempt_logout_not_index( + enforcement_app: Flask, +) -> None: # If the reset endpoint can't be resolved, the fallback must be an exempt # route (logout) — never "/" / the index, which would loop. We make the # reset endpoint fail and the logout endpoint resolve. @@ -195,7 +204,7 @@ def fake_url_for(endpoint: str, *args, **kwargs) -> str: def test_enforcement_no_resolvable_target_returns_error_not_loop( - enforcement_app, + enforcement_app: Flask, ) -> None: # If NO exempt target can be resolved, we must return an error response # rather than redirect, so the flagged user can never get stuck in a loop.