EnxDev commented on PR #37773: URL: https://github.com/apache/superset/pull/37773#issuecomment-4809517044
## EnxDev's Review Agent โ apache/superset#37773 ยท HEAD 561d9321 comment โ Fix works and is tested for the default path, but a maintainer steered toward an upstream FAB config and the self-service password-change path needs confirming. ### ๐ด Functional - **`superset/security/manager.py:4218`** โ Gating un-registers both `ResetMyPasswordView` (a logged-in AUTH_DB user changing *their own* password) and `ResetPasswordView` (admin resetting another user's password), so both routes 404. Does the 6.0 SPA actually provide replacements for both? If not, every AUTH_DB principal (Gamma/Alpha/Admin) loses self-service password change and admins lose user-password reset โ a functional regression, not just a styling fix. (test: assert the SPA route / documented alternative exists before the FAB route is removed) ### ๐ก Should-fix - **Approach** โ @dpgaspar asked for this upstream in FAB via a new `FAB_ADD_SECURITY_*`-style flag and offered to review/release it; the current monkeypatch of `appbuilder.add_view_no_menu` is a workaround. Confirm a maintainer accepts the in-Superset approach, or pursue the FAB route as discussed in-thread. - **Tests** โ only the disabled (default) path is covered. Add a case with `ENABLE_LEGACY_FAB_PASSWORD_VIEWS=True` asserting the routes ARE registered, so the enable branch is guarded. ### ๐ต Nits - `superset/security/manager.py:4297` โ `continue` is the last statement in the loop body; it's a no-op, drop it. - `superset/security/manager.py:4216` โ the 'temporal change to remove the roles viewโฆ' comment now sits above `_skip_legacy_fab_password_view_registration` but describes `register_views`; move it back down with that method. - `superset/security/manager.py:4233-4236` โ `A or B and C` relies on operator precedence; wrap `(isinstance(baseview, type) and issubclass(...))` in parens for readability. <!-- enxdev-review-agent:561d9321 --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 561d9321._ -- 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]
