sadpandajoe opened a new pull request, #45876:
URL: https://github.com/apache/superset/pull/45876
### SUMMARY
Scheduled CSV and XLSX chart reports fail on every run with `Chart data
request failed: category=http status=401` once the report owner has had a
password change or reset (or any other session invalidation). The chart itself
loads fine in the UI, and the failure happens before any query runs.
#### Root Cause
`MachineAuthProvider.get_auth_cookies` logs the report owner in with a bare
`login_user(user)` inside a throwaway request context and replays the resulting
`session` cookie against `/api/v1/chart/data`. Unlike a normal login, which
stamps `session["_login_at"]` through `on_user_login`, that session carries
only the Flask-Login keys.
`enforce_session_validity` runs before every request and compares
`_login_at` with the user's `sessions_invalidated_at` epoch.
`is_session_invalidated(None, epoch)` deliberately fails closed, so once the
user has an epoch the hook logs the machine-minted session out, the request
continues anonymously, and `@protect()` falls through to the JWT check and
answers `401 Missing Authorization Header`. The report code then surfaces that
as the generic chart-data HTTP error.
The line itself is old, and the missing stamp only matters since the
session-invalidation hook (#40695) and the change that started stamping the
epoch on password change and reset for active users (#42934, and the later
reset and recovery flows). Users who never changed their password have no epoch
and are unaffected, which is why reports worked until a routine password reset.
The fix stamps the login time right after a successful `login_user()`, the
same way a normal login does, so a session minted after the epoch is valid. The
stamp is only applied when `login_user()` succeeds: an inactive user is still
refused and `get_auth_cookies` still returns `{}`, which is the empty-cookie
case the report code reports as missing authentication cookies. The hook, and
its fail-closed handling of sessions that predate it, are unchanged: a session
whose login time is before the epoch is still rejected.
#### Not addressed here
- A session minted within the same wall-clock second as the password change
can still be rejected, because the epoch is rounded up to the next whole
second. This also affects regular logins and clears on the next run.
- A custom `MACHINE_AUTH_PROVIDER_CLASS` that calls `login_user` itself
needs the same stamp.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
UI QA: not-required (backend-only change to how a server-side session is
minted; no user-visible UI surface). The regression test is the evidence.
Before (the regression test run without the fix),
`tests/unit_tests/utils/machine_auth_test.py`:
```
FAILED test_machine_auth_cookie_survives_epoch
AssertionError: machine-minted session was rejected by
enforce_session_validity (status 401); the session carries no '_login_at', so
it is treated as older than the epoch
assert 401 == 200
1 failed, 3 passed
```
After (this PR): `5 passed`.
### TESTING INSTRUCTIONS
Automated:
```
pytest tests/unit_tests/utils/machine_auth_test.py
```
The new tests mint a session with `MachineAuthProvider.get_auth_cookies`,
replay the cookie through a real request that passes
`enforce_session_validity`, and assert the result:
- `test_machine_auth_cookie_survives_epoch` fails without the fix (401) and
passes with it. It stamps the epoch in the past through the real
`invalidate_sessions_for_user`.
- Controls: no epoch (200), a session stamped after the epoch (200), a
session stamped before the epoch (401, invalidation of real sessions is
unchanged), and an inactive user (`{}`, no cookie).
Manual:
1. Enable alerts and reports with a working SMTP server, a Celery worker and
beat.
2. As an admin, change a user's password (Settings > List Users > edit), or
run `superset fab reset-password`.
3. As that user, create a CSV report on a chart that has a saved query,
scheduled every minute.
4. Before this change the run fails with `Chart data request failed:
category=http status=401`; with it the email carries the CSV and the run
succeeds.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] 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
--
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]
