schizophrenicmaniac commented on code in PR #45593:
URL: https://github.com/apache/superset/pull/45593#discussion_r4395004255
##########
tests/unit_tests/views/test_auth_login.py:
##########
@@ -75,3 +75,32 @@ def test_login_page_without_flash_messages(client) -> None:
assert response.status_code == 200
data = unescape(response.data.decode())
assert '"auth_messages":[]' in data.replace(" ", "")
+
+
+def test_log_beacon_from_login_page_returns_401(client) -> None:
+ """An anonymous ``/log/`` beacon gets a 401, not a redirect to
``/login/``."""
+ assert client.get("/login/").status_code == 200
+
+ response = client.post(
+ "/log/?explode=events",
+ data={"events": '[{"source": "spa", "path": "/login/"}]'},
+ )
+
+ assert response.status_code == 401
+ assert "Location" not in response.headers
+
+
+def test_log_beacon_from_login_page_leaves_session_untouched(client) -> None:
+ """The beacon must not rewrite the session cookie of the login page.
+
+ A new ``Set-Cookie`` here can land after the one that stores the OAuth
+ state and drop it, failing the login with ``mismatching_state``.
+ """
+ assert client.get("/login/").status_code == 200
+
+ response = client.post("/log/?explode=events", data={"events": "[]"})
+
+ assert response.status_code == 401
+ assert "Set-Cookie" not in response.headers
Review Comment:
You're right. I popped `g._login_user` before the POST and the 401 came back
with `Set-Cookie: session={"_fresh":false,"locale":"en"}`, so the backend
change cuts it from two rewrites to one, not zero. I dropped the Set-Cookie
assertion and renamed the test to
`test_log_beacon_from_login_page_does_not_flash`, so it only checks what the
decorator swap actually guarantees (401, no `_flashes` in the session). I also
fixed the claim and step 4 in the description. The frontend guard is what stops
the login page from sending the beacon at all.
##########
superset/views/core.py:
##########
@@ -666,7 +666,9 @@ def dashboard_permalink(
return redirect(url)
@api
- @has_access
+ # has_access_api answers anonymous beacons with a 401; has_access would
+ # flash and redirect to /login/, whose response rewrites the session cookie
+ @has_access_api
Review Comment:
Good catch, I hadn't noticed the `@api` wrapper turning the `abort(403)`
into a 500. I added `test_log_beacon_without_permission_returns_403`. With
`@has_access` put back it fails with `500 == 403`. I also added the embedded
dashboard / guest token case to the description.
--
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]
