rusackas opened a new pull request, #44857:
URL: https://github.com/apache/superset/pull/44857
### SUMMARY
Follow-up to #44626, addressing the two threads @sadpandajoe left open there
(both quoted below for context).
**1. Forced-password-change redirect loop for roles without `can_read` on
`user`**
> A flagged user is unconditionally redirected to `UserInfoView.list`, which
itself requires `can_read on user` — but the old redirect target,
`ResetMyPasswordView`, was reachable by every authenticated user regardless of
role (it was in `ACCESSIBLE_PERMS`). A custom role that lacks `can_read on
user` (for example one that was only ever granted the legacy reset permissions)
now gets redirected here, denied by FAB's own access check, and falls back only
to logout — with no way left to reach a password-change flow and clear
`password_must_change`. Is that intentional, or should the profile page (or
some reachable change-password path) stay available to any authenticated user
the way self-service reset used to be?
Traced the actual failure: `UserInfoView.list` requires `can_read` on
`user`. A role lacking it hits FAB's `has_access` denial, which redirects to
`<auth_view>.login?next=...`. Since the user is already authenticated, that
view immediately redirects to the index, which re-runs the
forced-password-change hook and redirects back to `/user_info/` — an infinite
loop (`/user_info/` → `/login/` → `/` → `/user_info/` → ...), not a clean
fallback to logout.
Fixed in `superset/security/password_change.py`: the hook now checks
`has_access("can_read", "user")` before offering the profile page as a redirect
candidate. If the user can't reach it, it skips straight to the logout fallback
with a distinct flash message explaining they need an administrator's help,
rather than looping. This keeps the RBAC boundary intact (no new permissions
are granted) and just fixes the stranding.
**2. Stale legacy permissions left on custom roles**
> This filter only feeds `set_role` calls for Admin, Alpha, Gamma, sql_lab,
and optionally Public — `sync_role_definitions` never rebuilds a custom
(operator-defined) role. A custom role that already held
`ResetPasswordView`/`ResetMyPasswordView`/`UserDBModelView`
`resetpasswords`/`resetmypassword` permissions before upgrading keeps them
after `superset init`, even though UPDATING.md and the new integration test
claim/verify only that Admin/Alpha/Gamma no longer hold them. Should custom
roles be swept for these stale permissions too, or is the UPDATING.md wording
meant to be narrower than "no role"?
Confirmed: `_is_legacy_password_pvm` filtering only applies to the pvms list
used by the four `set_role()` calls in `sync_role_definitions`; the PVM rows
themselves are explicitly left alone, so a custom role that held them keeps
them forever. This is functionally inert (the views are never registered and
the launcher actions are hidden/404 for everyone, regardless of role), but it
makes UPDATING.md's "no role" claim literally false on upgraded installs with
custom roles.
Added a data migration
(`superset/migrations/shared/security_converge.delete_pvms`, the same helper
used by #33272's permission cleanup) that removes the 6 legacy `(view,
permission)` pairs from every role that holds them, not just the four synced
built-ins, and the now fully-orphaned `ab_permission`/`ab_view_menu` rows. Pure
metadata cleanup, no behavior change. Narrowed the UPDATING.md wording to match
reality and documented the `can_read on user` prerequisite for custom roles
using `ENABLE_FORCE_PASSWORD_CHANGE`.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend-only)
### TESTING INSTRUCTIONS
- `pytest tests/unit_tests/security/test_password_change.py
tests/unit_tests/migrations/test_sweep_legacy_password_view_perms.py
tests/unit_tests/migrations/test_single_migration_head.py` — all pass (new
regression tests added for both fixes).
- `pre-commit run` on the changed files — clean.
- Manual repro of the original bug: create a custom role with no `can_read`
on `user`, assign it to a user, set `password_must_change=True` on that user's
`UserAttribute` row with `ENABLE_FORCE_PASSWORD_CHANGE` on, log in as that user
— confirms redirect to logout with the new flash message instead of looping.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [x] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [x] Migration is atomic, supports rollback & is backwards-compatible
- [x] Confirm DB migration upgrade and downgrade tested
- [x] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
Migration is a straightforward row-deletion sweep (no schema change) with a
deliberately no-op downgrade (the deleted permissions can never be recreated by
current code, so there's no meaningful prior state to restore); covered by
`test_sweep_legacy_password_view_perms.py`, including an idempotency test.
Runtime is proportional to the number of roles in the metadata DB — negligible
even on large installs since it's a handful of `(view, permission)` lookups,
not a table scan.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
