You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/10/01 04:31:52 UTC
(superset) 01/01: fix(auth): close the forced-password-change redirect loop and sweep stale legacy password perms from all roles
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 <ev...@rusackas.com>
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 <ev...@preset.io>
Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
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
+
+
+@pytest.fixture(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: