EnxDev commented on code in PR #37773:
URL: https://github.com/apache/superset/pull/37773#discussion_r4087768752
##########
superset/security/manager.py:
##########
@@ -3277,6 +3288,47 @@ def clean_perms(self) -> None:
if deleted_count := pvms.delete():
logger.info("Deleted %i faulty permissions", deleted_count)
+ @staticmethod
+ def _legacy_password_views_to_skip() -> tuple[type[Any], ...]:
+ """
+ Return the legacy FAB password reset views that must not be registered
+ under the current configuration.
+
+ With ``ENABLE_LEGACY_FAB_PASSWORD_VIEWS`` off (the default) the admin
+ reset form (``ResetPasswordView``) is always skipped, and the
+ self-service form (``ResetMyPasswordView``) is skipped too unless
+ ``ENABLE_FORCE_PASSWORD_CHANGE`` is on, since that flow redirects users
+ to it. Both flags are read once at app startup, like every other FAB
+ view-registration setting; ``sync_role_definitions`` (``superset
init``)
+ applies the same rule to the persisted permissions.
+
+ :returns: the view classes to skip, empty when nothing is skipped
+ """
+ if current_app.config.get("ENABLE_LEGACY_FAB_PASSWORD_VIEWS", False):
+ return ()
+ if current_app.config.get("ENABLE_FORCE_PASSWORD_CHANGE", False):
+ return (ResetPasswordView,)
+ return (ResetPasswordView, ResetMyPasswordView)
+
+ def _legacy_password_view_menus_to_exclude(self) -> set[str]:
+ """
+ Return FAB view-menu names for legacy password reset views whose
+ permissions should be excluded from role synchronization because their
+ registration is currently disabled.
+
+ Mirrors ``_legacy_password_views_to_skip`` so that an upgraded
+ installation with a persisted ``ResetPasswordView`` (or
+ ``ResetMyPasswordView``) permission/view-menu row, created before
+ ``ENABLE_LEGACY_FAB_PASSWORD_VIEWS`` existed, doesn't have Admin (or
+ any other role) retain that permission once the flag is off, even
+ though the view itself is no longer registered. The rows themselves are
+ kept, so re-enabling the views and running ``superset init`` restores
+ the assignments.
+
+ :returns: view-menu names to exclude from role assignment
+ """
+ return {view.__name__ for view in
self._legacy_password_views_to_skip()}
Review Comment:
If the `superset init` step gets missed after turning
`ENABLE_FORCE_PASSWORD_CHANGE` on, flagged users don't get an access-denied
page, they get an endless redirect: with no built-in role holding
`can_this_form_get` on `ResetMyPasswordView`, the form bounces to `/login/` and
the hook sends `/login/` straight back (reproduced locally, Admin included
since its perms go through this filter too).
Could `ResetMyPasswordView` stay out of this exclusion set? A perm on an
unrouted view is harmless, and upgraded installs would then keep working
whichever way the flag gets flipped.
##########
superset/security/manager.py:
##########
@@ -3277,6 +3288,47 @@ def clean_perms(self) -> None:
if deleted_count := pvms.delete():
logger.info("Deleted %i faulty permissions", deleted_count)
+ @staticmethod
+ def _legacy_password_views_to_skip() -> tuple[type[Any], ...]:
+ """
+ Return the legacy FAB password reset views that must not be registered
+ under the current configuration.
+
+ With ``ENABLE_LEGACY_FAB_PASSWORD_VIEWS`` off (the default) the admin
+ reset form (``ResetPasswordView``) is always skipped, and the
+ self-service form (``ResetMyPasswordView``) is skipped too unless
+ ``ENABLE_FORCE_PASSWORD_CHANGE`` is on, since that flow redirects users
+ to it. Both flags are read once at app startup, like every other FAB
+ view-registration setting; ``sync_role_definitions`` (``superset
init``)
+ applies the same rule to the persisted permissions.
+
+ :returns: the view classes to skip, empty when nothing is skipped
+ """
+ if current_app.config.get("ENABLE_LEGACY_FAB_PASSWORD_VIEWS", False):
+ return ()
+ if current_app.config.get("ENABLE_FORCE_PASSWORD_CHANGE", False):
+ return (ResetPasswordView,)
+ return (ResetPasswordView, ResetMyPasswordView)
Review Comment:
This is what's turning `unit-tests` red: the positive control at the end of
`test_enforcement_exempts_health_blueprint`
(`tests/unit_tests/security/test_password_change.py:146`) expects `/` to
redirect to `/resetmypassword/form`, but that test's app boots with
`ENABLE_FORCE_PASSWORD_CHANGE` off, so the view gets skipped here and the hook
falls back to `/logout/`. Passes on master, fails GET and HEAD on this branch.
Booting the app with the flag on fixes it (that file plus `manager_test.py`
go 202/202 locally):
```python
@pytest.mark.parametrize("app", [{"ENABLE_FORCE_PASSWORD_CHANGE": True}],
indirect=True)
@pytest.mark.parametrize("method", ["GET", "HEAD"])
def test_enforcement_exempts_health_blueprint(
```
--
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]
