bito-code-review[bot] commented on code in PR #44857:
URL: https://github.com/apache/superset/pull/44857#discussion_r4153065222
##########
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",
+ )
Review Comment:
<!-- Bito Reply -->
The change is appropriate. By moving the generic flash message into the
`else` block, you ensure that the warning only appears when the profile page is
unreachable, effectively resolving the issue of contradictory double flash
messages.
--
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]
