sadpandajoe commented on code in PR #37773:
URL: https://github.com/apache/superset/pull/37773#discussion_r4021144637
##########
superset/security/manager.py:
##########
@@ -5887,8 +5915,48 @@ def is_admin(self) -> bool:
role.name for role in self.get_user_roles()
]
- # temporal change to remove the roles view from the security menu,
- # after migrating all views to frontend, we will set
FAB_ADD_SECURITY_VIEWS = False
+ def _skip_legacy_fab_password_view_registration(self) -> Callable[...,
Any]:
+ """
+ Temporarily patch ``add_view_no_menu`` so legacy FAB password reset
+ views are skipped during ``register_views()``.
+
+ When ``ENABLE_LEGACY_FAB_PASSWORD_VIEWS`` is disabled,
``ResetPasswordView``
+ is always skipped, and ``ResetMyPasswordView`` is skipped unless
+ ``ENABLE_FORCE_PASSWORD_CHANGE`` is enabled (that flow still needs a
+ reachable reset form). When the flag is enabled, no patching occurs.
+
+ :returns: the original, unpatched ``add_view_no_menu`` bound method, so
+ the caller can restore it once ``register_views()`` completes.
+ """
+ original_add_view_no_menu: Callable[..., Any] =
self.appbuilder.add_view_no_menu
+
+ if current_app.config.get("ENABLE_LEGACY_FAB_PASSWORD_VIEWS", False):
+ return original_add_view_no_menu
+
+ from flask_appbuilder.security.views import (
+ ResetMyPasswordView,
+ ResetPasswordView,
+ )
+
+ legacy_password_views: tuple[type[Any], ...] = (ResetPasswordView,)
+ if not current_app.config.get("ENABLE_FORCE_PASSWORD_CHANGE", False):
+ legacy_password_views = (*legacy_password_views,
ResetMyPasswordView)
+
+ def add_view_no_menu_without_legacy_password_views(
+ baseview: Any, *args: Any, **kwargs: Any
+ ) -> Any:
+ if isinstance(baseview, legacy_password_views) or (
Review Comment:
The FAB user-info and admin-user pages still render their reset actions, but
those POST handlers call `url_for()` for the views skipped here, so clicking
either button now raises `BuildError` and returns a 500. Could the flag also
suppress or redirect those actions so every still-reachable launcher has a
valid destination?
##########
UPDATING.md:
##########
@@ -24,6 +24,16 @@ assists people when migrating to a new version.
## Next
+### Legacy FAB password reset routes are no longer registered by default
+
+The legacy Flask-AppBuilder SSR password reset views are no longer registered
+by default. `/superset/resetpassword` (admin-triggered password reset) is no
Review Comment:
These paths do not match the FAB routes: the forms are mounted at
`/resetpassword/form` and `/resetmypassword/form`, without a `/superset`
prefix. Could this migration note name the actual URLs so operators audit and
update the right links?
##########
superset/security/manager.py:
##########
@@ -3049,6 +3072,11 @@ def sync_role_definitions(self) -> None:
pvms = self._get_all_pvms()
+ if excluded_view_menus :=
self._legacy_password_view_menus_to_exclude():
Review Comment:
After `superset init` runs with the default flags, this filter removes the
reset-view permissions, but changing either flag only re-registers routes on
web startup. An operator who later enables the documented escape hatch still
gets access denied, while enabling forced password change can redirect affected
users to a view they cannot access; could route registration and role
assignment be kept in sync without requiring another undocumented init?
--
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]
