gabotorresruiz commented on code in PR #45593:
URL: https://github.com/apache/superset/pull/45593#discussion_r4394802320
##########
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:
Worth calling out in the description, and worth a test: this swap also turns
a `500` into a `403`. On the merge base `has_access` reaches `abort(403)`, and
the `@api` decorator right above it catches `Forbidden` and rewrites it through
`json_error_response(get_error_msg())`, so any authenticated caller without
`can_log` gets a `500` plus a logged traceback. Measured on both branches, with
a logged-in `sql_lab` user and with a guest token sent as the `guest_token`
form field the way `loggerMiddleware` sends it:
| principal | merge base | this branch |
| --- | --- | --- |
| logged in, no `can_log` | 500 | 403 |
| guest token, role without `can_log` | 500 | 403 |
The second row is the one I would highlight, since `GUEST_ROLE_NAME`
defaults to `Public` and `superset init` leaves `Public` with no permissions,
so an embedded dashboard currently answers every beacon batch with a `500`. A
`test_log_beacon_without_permission_returns_403` next to the two new tests
would lock that in.
##########
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:
I do not think this assertion pins what it says. `flask_login`'s `basic`
session protection writes `sess["_fresh"] = False` the first time
`current_user` is resolved on a session that already carries a key and has no
`_id` (`_session_protection_failed`), and `has_access_api` resolves
`current_user` on its denial branch. Rendering `/login/` puts `locale` in the
session (FAB's `get_locale`), so the beacon request hits exactly that case. The
test misses it because the unit-test suite holds one app context open for the
whole test, so `flask_login` reuses the principal it cached on `g` during the
`GET /login/` and never re-resolves it. Pop `g._login_user` before the `POST`
and this assertion fails on this branch.
Driven over HTTP against a real server, the `401` on this branch comes back
with `Set-Cookie: session={"_fresh":false,"locale":"en"}`. On the merge base
the `302` itself already carried `Set-Cookie:
session={"_flashes":[...],"_fresh":false,"locale":"en"}`, before `/login/` was
ever fetched. So the swap goes from two cookie rewrites per beacon to one
rather than to zero, and the `curl` in step 4 of the test plan shows a clean
header only because it sends no session cookie at all.
Nothing is broken by this, since the frontend guard means no beacon leaves
the login page. It is this test and the claim I would adjust: either resolve
the principal inside the request here so it matches production, or drop the
`Set-Cookie` assertion and keep `test_log_beacon_from_login_page_returns_401`,
which does fail on the merge base. Or am I misreading the app-context lifetime
here?
--
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]
