gabotorresruiz commented on code in PR #45593:
URL: https://github.com/apache/superset/pull/45593#discussion_r4403033308
##########
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:
Thanks for going and checking it yourself rather than taking my word for it.
`test_log_beacon_from_login_page_does_not_flash` with only the `_flashes`
assertion is the honest version of that claim, and dropping the `Set-Cookie`
line is the right call: the frontend guard is what actually keeps the beacon
off the login page, so the decorator only ever needed to own the flash. Closing
this out.
##########
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:
`test_log_beacon_without_permission_returns_403` is exactly it, and putting
`@has_access` back to watch it fail on `500 == 403` is the proof I was after.
Thanks for adding the embedded and guest token case to the description as well,
that is the part a release note reader will care about. Nothing further from me.
--
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]
