gabotorresruiz commented on code in PR #43786:
URL: https://github.com/apache/superset/pull/43786#discussion_r4048846177
##########
superset/security/session_invalidation.py:
##########
@@ -50,6 +50,10 @@
#: Session key holding the epoch-seconds timestamp of when the session logged
in.
SESSION_LOGIN_AT_KEY = "_login_at"
+# Health checks are deliberately independent of authentication and the metadata
+# database, so they must not resolve ``current_user`` or perform session
checks.
+_HEALTH_CHECK_PATHS = frozenset({"/health", "/healthcheck", "/ping"})
Review Comment:
Not a blocker, and the current form is correct. I think it can be shorter
and drift-proof though.
The description says the path check is used "because the hook runs before
request dispatch", but Flask matches the URL in `RequestContext.push()`, before
`preprocess_request()` runs the `before_request` funcs, so `request.endpoint`
and `request.url_rule` are already populated here. I verified that on this
branch by inserting a `before_request` hook ahead of the others: `GET /health`,
`GET /healthcheck` and `GET /ping` all arrive with `request.endpoint ==
"health.health"`. The sibling hook in this same package,
`_enforce_password_change` at `superset/security/password_change.py:155`,
already branches on `request.endpoint` for exactly that reason.
All three rules in `superset/views/health.py:26-28` resolve to that one
endpoint, so the set collapses:
```python
#: Every probe rule registered by ``health()`` in
``superset/views/health.py``
#: resolves to this endpoint.
_HEALTH_CHECK_ENDPOINT = "health.health"
...
if has_request_context() and request.endpoint == _HEALTH_CHECK_ENDPOINT:
return None
```
A fourth `@health_blueprint.route(...)` on `health()` is then covered for
free, which is the drift the bot keeps flagging above.
The test has to move off the bare `Flask(__name__)` app for this, since it
registers no rules and `request.endpoint` would be `None`. Driving it off the
real URL map is a stronger assertion anyway:
```python
def test_enforce_skips_health_check_before_resolving_user(app: Any) -> None:
"""Health probes must not resolve current_user or touch metadata."""
rules = [str(r) for r in app.url_map.iter_rules() if r.endpoint ==
"health.health"]
assert rules, "no health probe routes registered"
for rule in rules:
with app.test_request_context(rule):
with (
patch(f"{MODULE}.current_user") as current,
patch(f"{MODULE}.logger.warning") as warning,
patch(f"{MODULE}.logout_user") as logout,
):
assert enforce_session_validity() is None
current.__bool__.assert_not_called()
warning.assert_not_called()
logout.assert_not_called()
```
That also pins the "never resolves `current_user`" part explicitly. I ran
the variant locally: `tests/unit_tests/security/` stays at 346 passed, and the
test fails if the early return is deleted. Keep `has_request_context()` either
way, the other tests in this file call `enforce_session_validity()` with no
request context.
--
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]
