This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch fix/37700-forced-password-change-stranding in repository https://gitbox.apache.org/repos/asf/superset.git
commit c89b766407296f4e20a5461f8ac77514649d74cd Author: Evan Rusackas <[email protected]> AuthorDate: Wed Sep 30 21:31:13 2026 -0700 fix(auth): close the forced-password-change redirect loop and sweep stale legacy password perms from all roles Follow-up to #44626. UserInfoView.list requires can_read on user, which sync_role_definitions only grants to the built-in Admin/Alpha/Gamma roles. A user on a custom role lacking it who is forced to change their password got redirected into an infinite loop (profile page -> denied -> login -> already authenticated -> index -> profile page...) with no way out. The hook now checks reachability first and falls back to logout with an explanatory message instead. Separately, sync_role_definitions's legacy-password-pvm exclusion only ever touches the four built-in roles it resyncs; a custom role that already held ResetPasswordView/ResetMyPasswordView/UserDBModelView's resetpasswords/resetmypassword permissions before upgrading kept them indefinitely, contradicting UPDATING.md's "no role" claim. Added a data migration sweeping those stale, now fully inert permissions off every role, and narrowed the UPDATING.md wording to match. Co-Authored-By: Evan Rusackas <[email protected]> Co-Authored-By: Claude Sonnet 5 <[email protected]> --- UPDATING.md | 27 ++- ...ep_legacy_password_view_perms_from_all_roles.py | 114 +++++++++++++ superset/security/password_change.py | 75 +++++--- .../test_sweep_legacy_password_view_perms.py | 189 +++++++++++++++++++++ tests/unit_tests/security/test_password_change.py | 43 +++++ 5 files changed, 416 insertions(+), 32 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 56376a76778..c59c25a9d0a 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -376,14 +376,25 @@ routes answer 404, and the "Reset Password" and "Reset my password" buttons on the legacy FAB user pages that led to them are gone. `superset init` no longer assigns their permissions (`can this form get/post on ResetPasswordView` and `ResetMyPasswordView`, plus the `resetpasswords` and `resetmypassword` actions on -`UserDBModelView`) to any role. Every flow they served lives in the SPA: -administrators reset a user's password from the "New password" fields in the -Users list edit modal (`PUT /api/v1/security/users/<id>`), users change their -own from the "Reset my password" modal on their profile page (`PUT -/api/v1/me/`, which requires `current_password` when the account already has one), and a pending forced password -change (`ENABLE_FORCE_PASSWORD_CHANGE`) now redirects to that profile page -instead of the removed form. Deployments that link to either legacy route should -point at `/user_info/` or the Users list instead. +`UserDBModelView`) to the built-in Admin/Alpha/Gamma/sql_lab roles, and a data +migration removes those same permissions from every other (operator-defined) +role that already held them, so no role on any upgraded install retains them. +Every flow they served lives in the SPA: administrators reset a user's +password from the "New password" fields in the Users list edit modal (`PUT +/api/v1/security/users/<id>`), users change their own from the "Reset my +password" modal on their profile page (`PUT /api/v1/me/`, which requires +`current_password` when the account already has one), and a pending forced +password change (`ENABLE_FORCE_PASSWORD_CHANGE`) now redirects to that profile +page instead of the removed form. Deployments that link to either legacy route +should point at `/user_info/` or the Users list instead. + +Reaching the profile page requires `can_read` on `user`, which the built-in +Admin/Alpha/Gamma roles hold by default but a custom role is not guaranteed +to. A user whose only role lacks it and who gets redirected there by a forced +password change is now redirected to logout with an explanatory message +instead, so they are never trapped in a redirect loop; grant that permission +to any custom role whose members may have `ENABLE_FORCE_PASSWORD_CHANGE` +set on their account. ### Default Docker image is now batteries-included; the minimal image moves to `-lean` diff --git a/superset/migrations/versions/2026-09-30_00-00_d623a0cb6bb0_sweep_legacy_password_view_perms_from_all_roles.py b/superset/migrations/versions/2026-09-30_00-00_d623a0cb6bb0_sweep_legacy_password_view_perms_from_all_roles.py new file mode 100644 index 00000000000..7b08997c3a6 --- /dev/null +++ b/superset/migrations/versions/2026-09-30_00-00_d623a0cb6bb0_sweep_legacy_password_view_perms_from_all_roles.py @@ -0,0 +1,114 @@ +# 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. +"""sweep legacy password-view permissions from all roles (#44626 follow-up) + +#44626 removed the legacy FAB server-rendered password reset views +(``ResetPasswordView``, ``ResetMyPasswordView``) and hid the +``UserDBModelView`` actions that redirected to them (``resetpasswords``, +``resetmypassword``). ``SupersetSecurityManager.sync_role_definitions`` was +updated to stop handing their permissions to the built-in Admin/Alpha/Gamma/ +sql_lab roles on ``superset init``, but that exclusion only applies to the +pvms list those four roles are resynced from -- it never touches a role that +already held one of these permissions before upgrading, since Superset never +re-syncs operator-defined (custom) roles. An upgraded install's custom role +that was explicitly granted self-service or admin password reset therefore +keeps the dead permission forever, contradicting UPDATING.md's "no role" +claim for that release. + +The permissions are already fully inert on every upgraded install regardless +of which role holds them: the views are never registered (so the routes +404/anything routing there raises ``BuildError``), and the ``UserDBModelView`` +actions are hidden and wired to a handler that also answers 404. This +migration is therefore pure metadata cleanup with no behavior change -- it +deletes the stale (view, permission) pairs from every role that holds them +(not just the four synced built-ins), plus the underlying ``ab_permission``/ +``ab_view_menu`` rows once orphaned. + +Revision ID: d623a0cb6bb0 +Revises: 95d8a99c822e +Create Date: 2026-09-30 00:00:00.000000 + +""" + +# revision identifiers, used by Alembic. +revision = "d623a0cb6bb0" +down_revision = "95d8a99c822e" + +from alembic import op # noqa: E402 +from sqlalchemy.exc import SQLAlchemyError # noqa: E402 +from sqlalchemy.orm import Session # noqa: E402 + +from superset.migrations.shared.security_converge import ( # noqa: E402 + delete_pvms, + Pvm, +) + +# The legacy password-reset views' own form-submission permissions, plus the +# UserDBModelView actions that redirected to them. See +# ``superset.security.manager.LEGACY_PASSWORD_VIEWS`` / +# ``LEGACY_PASSWORD_LAUNCHERS`` for the code-side registration that these +# mirror. +PVM_LIST = ( + Pvm("ResetPasswordView", "can_this_form_get"), + Pvm("ResetPasswordView", "can_this_form_post"), + Pvm("ResetMyPasswordView", "can_this_form_get"), + Pvm("ResetMyPasswordView", "can_this_form_post"), + Pvm("UserDBModelView", "resetpasswords"), + Pvm("UserDBModelView", "resetmypassword"), +) + + +def do_upgrade(session: Session) -> None: + """Delete every PVM in ``PVM_LIST`` from every role that holds it. On a + clean install, or one already cleaned up, the PVMs simply don't resolve + and are skipped -- safe to run everywhere.""" + delete_pvms(session, PVM_LIST) + + +def do_downgrade(session: Session) -> None: + """Intentionally a no-op. + + These permissions were deleted because the views/actions they gated are + never registered in current code, so current code can never create them + again on its own. Recreating the bare PVM rows here would just + reintroduce dead, unreachable permissions with no code path granting or + checking them -- there is no meaningful prior state to restore. + """ + + +def upgrade() -> None: + bind = op.get_bind() + session = Session(bind=bind) + do_upgrade(session) + try: + session.commit() + except SQLAlchemyError as ex: + session.rollback() + raise Exception(f"An error occurred while upgrading permissions: {ex}") from ex + + +def downgrade() -> None: + bind = op.get_bind() + session = Session(bind=bind) + do_downgrade(session) + try: + session.commit() + except SQLAlchemyError as ex: + session.rollback() + raise Exception( + f"An error occurred while downgrading permissions: {ex}" + ) from ex diff --git a/superset/security/password_change.py b/superset/security/password_change.py index beda19df8ab..c6579bc0df7 100644 --- a/superset/security/password_change.py +++ b/superset/security/password_change.py @@ -170,6 +170,36 @@ def _is_exempt_endpoint(endpoint: Optional[str]) -> bool: return view_class in _EXEMPT_VIEW_CLASSES +def _profile_page_reachable(security_manager: Any) -> bool: + """Return whether the current user can reach ``UserInfoView.list``. + + It requires ``can_read`` on ``user``, which ``sync_role_definitions`` only + grants to the built-in Admin/Alpha/Gamma roles -- a custom role is never + guaranteed to hold it. A user without it who gets redirected there hits + FAB's own access-denied redirect (to login), which, since they're already + authenticated, bounces straight back to the index -- which re-runs this + hook and redirects to the profile page again, looping forever. + """ + return security_manager is not None and bool( + security_manager.has_access("can_read", "user") + ) + + +def _logout_fallback_candidates(security_manager: Any) -> list[str]: + """Build the ordered list of logout endpoints to try as a last resort.""" + candidates = [] + auth_view = getattr(security_manager, "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") + return candidates + + def register_password_change_enforcement(app: Any) -> None: """Register the before-request hook that enforces pending password changes. @@ -206,30 +236,27 @@ def register_password_change_enforcement(app: Any) -> None: return None flash(__("You must change your password before continuing."), "warning") - # Resolve the SPA profile page. If that endpoint can't be resolved - # (e.g. a deployment that does not register ``UserInfoView``), fall - # 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. - candidates = [_PROFILE_PAGE_ENDPOINT] - 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") + # Resolve the SPA profile page, if the user can actually reach it, with + # logout as the fallback -- 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. + security_manager = getattr(getattr(current_app, "appbuilder", None), "sm", None) + candidates = [] + if _profile_page_reachable(security_manager): + candidates.append(_PROFILE_PAGE_ENDPOINT) + else: + flash( + __( + "Your role does not have access to the profile page " + "needed to change your password. Contact an " + "administrator." + ), + "danger", + ) + candidates.extend(_logout_fallback_candidates(security_manager)) for endpoint in candidates: try: return redirect(url_for(endpoint)) diff --git a/tests/unit_tests/migrations/test_sweep_legacy_password_view_perms.py b/tests/unit_tests/migrations/test_sweep_legacy_password_view_perms.py new file mode 100644 index 00000000000..ec640ca0c5d --- /dev/null +++ b/tests/unit_tests/migrations/test_sweep_legacy_password_view_perms.py @@ -0,0 +1,189 @@ +# 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. +"""Migration-level regression tests for the #44626 follow-up (custom roles +keeping the legacy password-view permissions after upgrade). + +Imports the real dated migration module -- date-prefixed filenames aren't +valid Python identifiers, so ``importlib.import_module`` is used instead of a +plain ``import`` (see +``tests/unit_tests/migrations/test_deprecated_permissions_33272.py`` for the +same pattern). +""" + +from __future__ import annotations + +from importlib import import_module + +import pytest +from sqlalchemy.orm import Session + +from superset.migrations.shared.security_converge import ( + Base, + Permission, + PermissionView, + Pvm, + Role, + ViewMenu, +) + +migration = import_module( + "superset.migrations.versions." + "2026-09-30_00-00_d623a0cb6bb0_sweep_legacy_password_view_perms_from_all_roles" +) + +# The real migration constant -- never hand-copied. +PVM_LIST = migration.PVM_LIST + + [email protected](autouse=True) +def setup_tables(session: Session) -> None: + """``security_converge`` freezes its own partial declarative ``Base`` (see + its module docstring), so the shared in-memory SQLite ``session`` fixture + needs its tables created explicitly.""" + engine = session.get_bind() + Base.metadata.create_all(engine) + + +def _make_pvm(session: Session, view_name: str, permission_name: str) -> PermissionView: + view = session.query(ViewMenu).filter_by(name=view_name).one_or_none() + if view is None: + view = ViewMenu(name=view_name) + session.add(view) + permission = session.query(Permission).filter_by(name=permission_name).one_or_none() + if permission is None: + permission = Permission(name=permission_name) + session.add(permission) + session.flush() + pvm = PermissionView(view_menu=view, permission=permission) + session.add(pvm) + session.flush() + return pvm + + +def test_pvm_list_covers_both_legacy_views_and_the_launcher_actions() -> None: + """Pins the real list against accidental edits: both legacy + ``SimpleFormView``s' own permissions, and the two ``UserDBModelView`` + launcher actions that redirected to them.""" + assert set(PVM_LIST) == { + Pvm("ResetPasswordView", "can_this_form_get"), + Pvm("ResetPasswordView", "can_this_form_post"), + Pvm("ResetMyPasswordView", "can_this_form_get"), + Pvm("ResetMyPasswordView", "can_this_form_post"), + Pvm("UserDBModelView", "resetpasswords"), + Pvm("UserDBModelView", "resetmypassword"), + } + + +def test_sweeps_stale_perm_from_a_custom_role_not_just_builtins( + session: Session, +) -> None: + """The exact gap this migration closes: ``sync_role_definitions`` only + ever re-derives the built-in Admin/Alpha/Gamma/sql_lab roles, so a custom + (operator-defined) role that held ``resetmypassword`` before upgrading + keeps it forever without this sweep. Seeds a custom role holding one + legacy PVM plus one unrelated, valid permission, and asserts the legacy + one is gone -- from every role, not just the synced built-ins -- while the + unrelated permission survives untouched.""" + target = Pvm("UserDBModelView", "resetmypassword") + assert target in PVM_LIST, "test assumes this PVM_LIST entry still exists" + + legacy_pvm = _make_pvm(session, target.view, target.permission) + unrelated_pvm = _make_pvm(session, "UserDBModelView", "can_list") + custom_role = Role(name="ReportViewer", permissions=[legacy_pvm, unrelated_pvm]) + session.add(custom_role) + session.commit() + + migration.do_upgrade(session) + session.commit() + + # the legacy PVM and its role association are gone + assert ( + session.query(PermissionView) + .join(Permission) + .filter(Permission.name == "resetmypassword") + .count() + == 0 + ) + # its Permission is now orphaned (nothing else used "resetmypassword") + assert session.query(Permission).filter_by(name="resetmypassword").count() == 0 + + # the custom role keeps its other (untargeted) permission, unchanged + custom_role = session.query(Role).filter_by(name="ReportViewer").one() + assert len(custom_role.permissions) == 1 + assert custom_role.permissions[0].permission.name == "can_list" + + # the unrelated valid PVM, and the still-shared view menu, survive + remaining_unrelated = session.get(PermissionView, unrelated_pvm.id) + assert remaining_unrelated is not None + assert remaining_unrelated.permission.name == "can_list" + assert session.query(ViewMenu).filter_by(name="UserDBModelView").count() == 1 + + +def test_sweeps_both_legacy_views_own_form_permissions(session: Session) -> None: + """The two ``SimpleFormView``s' own permissions are removed too, including + the now fully-orphaned ``ResetPasswordView``/``ResetMyPasswordView`` view + menus once nothing else references them.""" + pvms = [ + _make_pvm(session, pvm.view, pvm.permission) + for pvm in PVM_LIST + if pvm.view in ("ResetPasswordView", "ResetMyPasswordView") + ] + role = Role(name="Admin", permissions=pvms) + session.add(role) + session.commit() + + migration.do_upgrade(session) + session.commit() + + for view_name in ("ResetPasswordView", "ResetMyPasswordView"): + assert session.query(ViewMenu).filter_by(name=view_name).count() == 0 + + role = session.query(Role).filter_by(name="Admin").one() + assert role.permissions == [] + + +def test_is_idempotent(session: Session) -> None: + """Running the migration twice must not raise or leave inconsistent + state -- the second pass finds nothing left to resolve.""" + target = PVM_LIST[0] + _make_pvm(session, target.view, target.permission) + session.commit() + + migration.do_upgrade(session) + session.commit() + migration.do_upgrade(session) + session.commit() + + assert ( + session.query(PermissionView) + .join(Permission) + .join(ViewMenu) + .filter( + Permission.name == target.permission, + ViewMenu.name == target.view, + ) + .count() + == 0 + ) + + +def test_downgrade_is_a_no_op(session: Session) -> None: + """The dead permissions have no code path that can recreate or check + them, so downgrade deliberately restores nothing.""" + migration.do_downgrade(session) + session.commit() + assert session.query(PermissionView).count() == 0 diff --git a/tests/unit_tests/security/test_password_change.py b/tests/unit_tests/security/test_password_change.py index 8b906e6862a..9964584a886 100644 --- a/tests/unit_tests/security/test_password_change.py +++ b/tests/unit_tests/security/test_password_change.py @@ -126,6 +126,9 @@ def test_enforcement_exempts_health_blueprint( app.app_context(), app.test_client() as client, patch.object(app.login_manager, "_user_callback", return_value=user), + # Whether the profile-page redirect target itself is reachable is not + # under test here, only that health routes stay exempt either way. + patch.object(type(appbuilder.sm), "has_access", return_value=True), patch( "superset.security.password_change._get_user_attribute", return_value=UserAttribute(user_id=user.id, password_must_change=True), @@ -279,6 +282,14 @@ def enforcement_app() -> Flask: def index() -> str: # pylint: disable=unused-variable return "index" + # Stand in for the real appbuilder/security manager: defaults to "the + # user can reach the profile page", which most tests in this module + # assume. Tests exercising the no-access fallback override + # `has_access.return_value` directly. + app.appbuilder = MagicMock() + app.appbuilder.sm.has_access.return_value = True + app.appbuilder.sm.auth_view = None + register_password_change_enforcement(app) return app @@ -314,6 +325,38 @@ def test_enforcement_redirects_to_profile_page(enforcement_app: Flask) -> None: mock_url_for.assert_called_once_with("UserInfoView.list") +def test_enforcement_skips_unreachable_profile_page_falls_back_to_logout( + enforcement_app: Flask, +) -> None: + # A role without `can_read` on `user` (any custom role not based on + # Admin/Alpha/Gamma) can't reach `UserInfoView.list`: redirecting there + # anyway would hit FAB's own access-denied redirect (to login), which, + # since the user is already authenticated, bounces to the index, which + # re-runs this hook and redirects back to the profile page — an infinite + # loop. The hook must recognize that up front and go straight to the + # logout fallback instead. + enforcement_app.appbuilder.sm.has_access.return_value = False + with ( + patch( + "superset.security.password_change.password_change_required", + return_value=True, + ), + patch( + "superset.security.password_change.url_for", + return_value="/logout", + ) as mock_url_for, + patch("superset.security.password_change.flash") as mock_flash, + ): + resp = enforcement_app.test_client().get("/") + assert resp.status_code == 302 + assert resp.headers["Location"].endswith("/logout") + mock_url_for.assert_called_once_with("AuthDBView.logout") + enforcement_app.appbuilder.sm.has_access.assert_called_with("can_read", "user") + assert any( + "does not have access" in call.args[0] for call in mock_flash.call_args_list + ) + + def test_enforcement_falls_back_to_exempt_logout_not_index( enforcement_app: Flask, ) -> None:
