aminghadersohi opened a new pull request, #44504: URL: https://github.com/apache/superset/pull/44504
### SUMMARY Follow-up promised in apache/superset#43786; addresses [SC-121598](https://app.shortcut.com/preset/story/121598). With `ENABLE_FORCE_PASSWORD_CHANGE = True`, an authenticated user whose `UserAttribute.password_must_change` is true receives a **302 to `/resetmypassword/form`** when requesting a health-blueprint route. A load balancer or Kubernetes probe that reuses that user's session cookie can consequently mark a healthy instance unhealthy. Confirmed against the real Superset app before changing the hook: `/health`, `/healthcheck`, and `/ping` resolve to `health.health`; `/version` resolves to **`health.version`**, not `health.health`. None matches the old bare `health`/`healthcheck` exemption literals. All four returned 200 with enforcement disabled and 302 with it enabled for the flagged session. The hook now compares Flask's resolved `request.blueprint` with the actual `health_blueprint.name`, removing the stale endpoint literals. This covers every route on that blueprint without maintaining a path or endpoint list, including new routes and renamed view functions. Unlike #43786's session-invalidation optimization, which deliberately targets only the user-independent `health.health` endpoint before resolving the user, this hook must exempt the entire health blueprint from password-change redirects, including `/version`. It does **not** skip session invalidation or alter `/version`'s build-detail authorization. The static, auth-view, password-reset/user-info, and unmatched-endpoint exemptions retain their existing logic. The redirect-target selection and its no-loop fallback are unchanged. Documentation describes the health-blueprint behavior. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF Backend-only; no screenshots applicable. | Flagged session, enforcement enabled | Before | After | | --- | --- | --- | | `/health`, `/healthcheck`, `/ping` | 302 to reset form | 200, `OK` | | `/version` | 302 to reset form | 200, normal version JSON | | Non-exempt `/` | 302 to reset form | 302 to reset form | ### TESTING INSTRUCTIONS ```bash pytest -q tests/unit_tests/security/ pre-commit run --files superset/security/password_change.py tests/unit_tests/security/test_password_change.py docs/admin_docs/configuration/configuring-superset.mdx pre-commit run mypy --files superset/security/password_change.py tests/unit_tests/security/test_password_change.py ``` - Regression coverage enumerates the **real app's `url_map`**, not a duplicated path list. It sends GET and HEAD requests with a session cookie, mocks user/attribute loading for a flagged user, and compares every health route's status, body, and content type with its enforcement-disabled response. A non-exempt `/` request must still redirect and check that user's password-change flag, preventing a vacuous pass with an anonymous session or missing hook. - Expanded exact-match tests cover all existing auth-view classes, both reset views, static endpoints, unmatched endpoints, and similarly named non-exempt views; existing redirect-loop/fallback tests remain green. - Full security unit suite: **375 passed**. - **Removal proof:** after the fixed suite passed, physically deleted the `request.blueprint == health_blueprint.name` early return and reran the new regression test. Both GET and HEAD cases failed with `302 == 200` on `/ping` (pytest exit 1). Restored the exemption and reran the full security suite successfully. - Independently exercised all four real routes before and after the fix; each now returns its normal 200 response for the flagged session. - Staged-file pre-commit checks, including mypy, pass. ### ADDITIONAL INFORMATION - [x] Has associated issue: [SC-121598](https://app.shortcut.com/preset/story/121598) - [x] Required feature flags: `ENABLE_FORCE_PASSWORD_CHANGE = True` (configuration setting; reproducing also requires a flagged authenticated user) - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API Risk is limited to which routes skip password-change enforcement. Other security hooks and redirect-loop protection are unchanged. Revert this commit to roll back. -- 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]
