sadpandajoe commented on code in PR #44857:
URL: https://github.com/apache/superset/pull/44857#discussion_r4170768192
##########
superset/security/password_change.py:
##########
@@ -206,30 +236,27 @@ def _enforce_password_change() -> Any: # pylint:
disable=unused-variable
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))
Review Comment:
With `AUTH_REMOTE_USER` and a flagged user lacking profile access, this
logout fallback still loops: logout redirects to the index, welcome sends the
anonymous user to login, and the unchanged `REMOTE_USER` header immediately
authenticates them again before this hook sends them back to logout. Could this
case return a terminal explanatory response rather than redirecting into
automatic login?
##########
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"
Review Comment:
The current merge result contains both this revision and `884a2115ebd3`,
each revising `95d8a99c822e`, so it has two Alembic heads and the normal
`superset db upgrade` cannot resolve `head`. Could this migration be chained
after the annotation UUID migration, or joined with a merge revision?
##########
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`
Review Comment:
At the current head, this note still lists only `can_read` on `user` and
describes `ENABLE_FORCE_PASSWORD_CHANGE` as an account flag. A custom-role user
following it can open the profile but still gets denied by `/api/v1/me/`; could
you restore the `CurrentUserRestApi` read/write grants and the per-user
`password_must_change` distinction here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
