rusackas commented on code in PR #37773:
URL: https://github.com/apache/superset/pull/37773#discussion_r4078283208
##########
tests/integration_tests/core_tests.py:
##########
@@ -120,11 +121,18 @@ def test_slice_endpoint(self):
resp = self.client.get("/slice/-1/")
assert resp.status_code == 404
- def test_admin_only_menu_views(self):
- def assert_admin_view_menus_in(role_name, assert_func):
+ def test_admin_only_menu_views(self) -> None:
+ def assert_admin_view_menus_in(
+ role_name: str, assert_func: Callable[[str, list[str]], None]
+ ) -> None:
role = security_manager.find_role(role_name)
view_menus = [p.view_menu.name for p in role.permissions]
- assert_func("ResetPasswordView", view_menus)
+ if role_name == "Admin" and current_app.config.get(
+ "ENABLE_LEGACY_FAB_PASSWORD_VIEWS", False
+ ):
+ assert "ResetPasswordView" in view_menus
+ else:
+ assert "ResetPasswordView" not in view_menus
Review Comment:
Good catch. I dropped the Admin assertion from that integration test, so it
only checks that Alpha/Gamma never get `ResetPasswordView`, and left the
role-sync behavior to the unit tests.
##########
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:
Yeah, that was a 500 waiting to happen. With the views skipped, the matching
`resetpasswords` / `resetmypassword` actions on the user view are now hidden
from the show and list widgets, and a direct request to either answers 404
instead of hitting `url_for`.
##########
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:
No other FAB view-registration setting hot-reloads, and Superset builds the
appbuilder with `update_perms=False`, so permissions only ever move at
`superset init` anyway. I documented that in the config comment and UPDATING.md
instead: flipping either flag means a restart plus `superset init`.
##########
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:
Oops, fixed. The note (and the config comment) now name
`/resetpassword/form` and `/resetmypassword/form`.
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -4320,6 +4320,68 @@ def
test_request_loader_rejects_invalid_guest_token_before_bearer(
verify_jwt.assert_not_called()
[email protected](
+
"enable_legacy_password_views,enable_force_password_change,expected_registered",
+ [
+ (False, False, {"NonPasswordView"}),
+ (False, True, {"NonPasswordView", "ResetMyPasswordView"}),
+ (
+ True,
+ False,
+ {"NonPasswordView", "ResetMyPasswordView", "ResetPasswordView"},
+ ),
+ ],
+)
+def test_skip_legacy_fab_password_view_registration_keeps_forced_change_target(
+ app_context: None,
+ enable_legacy_password_views: bool,
+ enable_force_password_change: bool,
+ expected_registered: set[str],
+) -> None:
+ """Forced password changes require the self-service reset view."""
+ from flask import current_app
+ from flask_appbuilder.security.views import ResetMyPasswordView,
ResetPasswordView
Review Comment:
Fair, hoisted both to module level and dropped the duplicate `current_app`
import.
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -4320,6 +4320,68 @@ def
test_request_loader_rejects_invalid_guest_token_before_bearer(
verify_jwt.assert_not_called()
[email protected](
+
"enable_legacy_password_views,enable_force_password_change,expected_registered",
+ [
+ (False, False, {"NonPasswordView"}),
+ (False, True, {"NonPasswordView", "ResetMyPasswordView"}),
+ (
+ True,
+ False,
+ {"NonPasswordView", "ResetMyPasswordView", "ResetPasswordView"},
+ ),
+ ],
+)
+def test_skip_legacy_fab_password_view_registration_keeps_forced_change_target(
+ app_context: None,
+ enable_legacy_password_views: bool,
+ enable_force_password_change: bool,
+ expected_registered: set[str],
+) -> None:
+ """Forced password changes require the self-service reset view."""
+ from flask import current_app
+ from flask_appbuilder.security.views import ResetMyPasswordView,
ResetPasswordView
+
+ class NonPasswordView:
+ pass
+
+ registered: list[str] = []
+
+ def add_view_no_menu(baseview: type[Any], *args: Any, **kwargs: Any) ->
type[Any]:
+ registered.append(baseview.__name__)
+ return baseview
+
+ fake_appbuilder = SimpleNamespace(add_view_no_menu=add_view_no_menu)
+ sm = SupersetSecurityManager.__new__(SupersetSecurityManager)
+ sm.appbuilder = fake_appbuilder
+
+ previous_config = {
+ "ENABLE_LEGACY_FAB_PASSWORD_VIEWS": current_app.config[
+ "ENABLE_LEGACY_FAB_PASSWORD_VIEWS"
+ ],
+ "ENABLE_FORCE_PASSWORD_CHANGE": current_app.config[
+ "ENABLE_FORCE_PASSWORD_CHANGE"
+ ],
+ }
+ current_app.config["ENABLE_LEGACY_FAB_PASSWORD_VIEWS"] = (
+ enable_legacy_password_views
+ )
+ current_app.config["ENABLE_FORCE_PASSWORD_CHANGE"] =
enable_force_password_change
+
+ original_add_view_no_menu: Callable[..., Any] =
fake_appbuilder.add_view_no_menu
+ try:
+ original_add_view_no_menu =
sm._skip_legacy_fab_password_view_registration()
Review Comment:
Yep, dropped the initializer and the `Callable` import.
##########
tests/integration_tests/core_tests.py:
##########
@@ -120,11 +121,18 @@ def test_slice_endpoint(self):
resp = self.client.get("/slice/-1/")
assert resp.status_code == 404
- def test_admin_only_menu_views(self):
- def assert_admin_view_menus_in(role_name, assert_func):
+ def test_admin_only_menu_views(self) -> None:
+ def assert_admin_view_menus_in(
+ role_name: str, assert_func: Callable[[str, list[str]], None]
+ ) -> None:
Review Comment:
Added, thanks.
##########
tests/integration_tests/core_tests.py:
##########
@@ -133,6 +141,27 @@ def assert_admin_view_menus_in(role_name, assert_func):
assert_admin_view_menus_in("Alpha", self.assertNotIn)
assert_admin_view_menus_in("Gamma", self.assertNotIn)
+ def test_legacy_fab_password_views_are_not_registered(self) -> None:
Review Comment:
Added, thanks.
##########
superset/security/manager.py:
##########
@@ -3022,6 +3022,24 @@ def register_views(self) -> None:
) in ["/roles", "/users", "/groups", "registrations"]:
self.appbuilder.baseviews.remove(view)
+ # When legacy FAB password views are disabled, unregister their routes
+ # so direct URL access to /superset/resetpassword and
/superset/resetmypassword
+ # is no longer possible (SPA handles password changes post-porting).
+ if not current_app.config.get("ENABLE_LEGACY_FAB_PASSWORD_VIEWS",
False):
+ from flask_appbuilder.security.views import (
+ ResetMyPasswordView,
+ ResetPasswordView,
+ )
+
+ for view in list(self.appbuilder.baseviews):
+ if isinstance(view, (ResetPasswordView, ResetMyPasswordView)):
+ blueprint = getattr(view, "blueprint", None)
+ if blueprint is not None and hasattr(
+ current_app, "unregister_blueprint"
+ ):
+ current_app.unregister_blueprint(blueprint)
+ self.appbuilder.baseviews.remove(view)
Review Comment:
Closing the loop here since the FAB PR never happened: I checked FAB 5.2.3
and there's no per-view switch. `FAB_ADD_SECURITY_VIEWS = False` drops
everything, login view included, and a blueprint can't be unregistered once
it's added. So the Superset-side intercept stays for now, and I'm happy to swap
it for an upstream flag if one lands in FAB.
--
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]
