dpgaspar opened a new pull request, #44528:
URL: https://github.com/apache/superset/pull/44528
### SUMMARY
`SupersetAppInitializer` registers `enforce_session_validity` as an
unconditional
`before_request` hook, and the hook resolves `current_user` on *every*
request —
anonymous page loads, static assets, token-authenticated API calls included.
Two problems with that:
1. **It is wasteful.** `current_user` is a `LocalProxy` whose resolution can
hit
the metadata database. Nothing about an anonymous request needs it: this
hook
can only ever invalidate a *session* login, because the per-user epoch is
compared against `_login_at`, which is stamped into the session at login
time.
A request with no session login has nothing for the hook to invalidate.
2. **It assumes `current_user` never raises.** That holds for cookie
sessions,
where an unauthenticated request resolves to the anonymous user. It does
not
hold for deployments that install a Flask-Login `request_loader` — raising
from the loader is a common way to trigger a custom login redirect. Under
such
a deployment every anonymous request produces a `logger.warning` with a
full
traceback from the fail-open handler. In one deployment that is hundreds
of MB
of log noise per day, all of it describing ordinary unauthenticated
traffic.
The fix is two small changes to `enforce_session_validity`:
- Return early, **before** touching `current_user`, when the request carries
no
session login. A "remember me" cookie still counts as one, since
Flask-Login
restores that login while resolving `current_user` — i.e. after this hook
runs
— so the cookie's presence keeps the check enabled.
- Split the `except`: a JWT-based loader that finds no usable token raises
`JWTExtendedException`, which now logs at `debug` instead of `warning`. An
unauthenticated request is not a failure of this check. Any other error
keeps
the existing fail-open `warning` with `exc_info`.
Behaviour for the case the mechanism exists to serve is unchanged: a browser
session for a disabled user carries `_user_id`, so it is still checked and
still
forced out on its next request.
This follows the same reasoning as the existing health-probe early return
(#43786), generalized from one endpoint to "no session login".
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/security/test_session_invalidation.py
pytest tests/integration_tests/security/session_invalidation_tests.py
```
New unit tests cover:
- an anonymous request never resolves `current_user` (asserts `__bool__` is
not
called, mirroring the existing health-probe test);
- a request with a session login *does* reach the epoch lookup;
- a "remember me" cookie alone also reaches the epoch lookup;
- a request loader that raises `NoAuthorizationError` is logged at `debug`,
not
`warning`, and the request is allowed.
Manual: install a Flask-Login `request_loader` that raises
`NoAuthorizationError` for credential-less requests, hit any anonymous route,
and confirm the logs stay clean; then log in, disable the account, and
confirm
the next request is still forced out.
### 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]
