From 48ede6e2fafce289e4dec608c6f9d621a0ec48b2 Mon Sep 17 00:00:00 2001 From: JS Ng Date: Tue, 27 Oct 2026 20:37:05 +0800 Subject: [PATCH 1/2] fix: login-session token re-acquisition presents Basic only (#117) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /logins/{id}/token serves client principals only (campus#963/A10 guard, #854): user bearers are denied outright. token() rode whatever Authorization header the shared session last set, so a with_user_session() nested inside another one — an auth-broker release reached from a route that already holds the user session, exactly the campus-classroom enrollment checks — presented the outer session's user token and was refused with 403 (surfacing downstream as BrokerConfigError 502s on every enrollment-gated submission mutation). token() now resets to the client's Basic credentials for the call and restores any prior Authorization header afterwards (also on failure), so an enclosing session keeps its authorization until it re-runs use_token() with the fresh bearer. The initial acquisition path now matches the discipline the 401-refresh hook already documented ("the session's stale bearer must not be presented mid-rotation"). Found live: campus-classroom /a/ answer-sheet submit 502s on dev (campus-classroom#14 verification pass, 2026-10-27). --- campus_python/auth/v1/logins.py | 26 +++++++++++-- tests/unit/test_token_refresh.py | 65 +++++++++++++++++++++++++++++++- 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/campus_python/auth/v1/logins.py b/campus_python/auth/v1/logins.py index 9d9fee6..47bb4bc 100644 --- a/campus_python/auth/v1/logins.py +++ b/campus_python/auth/v1/logins.py @@ -143,12 +143,32 @@ def token(self) -> campus.model.OAuthToken: nothing (C5). The response omits the refresh token — campus refresh material never leaves the auth service in this lane. + + The call presents Basic client credentials, never the + ambient bearer: the route denies user principals outright + (#963/A10), and with_user_session() nested inside another + with_user_session() (e.g. an auth-broker release reached + from a route that already holds the user session) would + otherwise present the outer session's user token and be + refused with 403. Any prior Authorization header is + restored afterwards, so an enclosing session keeps its + authorization until it re-runs use_token() with the fresh + bearer. """ # Canonical path has no trailing slash (route rule is # //token, unlike the CRUD subresources). - resp = self.client.post( - self.make_path("token") - ) + prior_authorization = self.client.headers.get("Authorization") + self.client.reset_authorization() + try: + resp = self.client.post( + self.make_path("token") + ) + finally: + if prior_authorization is not None and prior_authorization.startswith( + "Bearer "): + self.client.set_bearer_authorization( + prior_authorization[len("Bearer "):] + ) # Raise error if status code is not 2XX or 3XX resp.raise_for_status() return campus.model.OAuthToken.from_resource(resp.json()) diff --git a/tests/unit/test_token_refresh.py b/tests/unit/test_token_refresh.py index b00d0ea..7caa82e 100644 --- a/tests/unit/test_token_refresh.py +++ b/tests/unit/test_token_refresh.py @@ -43,8 +43,14 @@ def make_auth() -> tuple[AuthRoot, Mock]: - """Create an AuthRoot backed by a mock JSON client.""" + """Create an AuthRoot backed by a mock JSON client. + + headers.get defaults to None (no ambient Authorization header) — + token() restores whatever header it finds, so tests that exercise + the ambient-bearer path override this. + """ client = Mock() + client.headers.get.return_value = None return AuthRoot(json_client=client), client @@ -155,6 +161,63 @@ def test_invalid_grant_surfaces_oauth_error(self): self.assertEqual(ctx.exception.oauth_error, "invalid_grant") +class TestLoginTokenPresentsBasicOnly(unittest.TestCase): + """The session-token route serves client principals only (#963/A10): + token() must present Basic credentials even when an ambient bearer + is set — a with_user_session() nested inside another one otherwise + presents the outer session's user token and is refused with 403. + The prior header is restored afterwards.""" + + def setUp(self): + self.auth, self.client = make_auth() + self.client.post.return_value = make_response( + 200, dict(LOGIN_TOKEN_PAYLOAD) + ) + + def _call(self): + with session_context(): + flask.session["logins_login_id"] = "login-1" + return self.auth.logins.token() + + def test_forces_basic_when_bearer_is_set(self): + self.client.headers.get.return_value = "Bearer outer-user-token" + self._call() + calls = [name for name, _, _ in self.client.mock_calls + if name in ("reset_authorization", "post", + "set_bearer_authorization")] + # The exact ordering contract: drop the ambient bearer (Basic), + # make the call, then restore it. + self.assertEqual( + ["reset_authorization", "post", "set_bearer_authorization"], calls + ) + + def test_restores_ambient_bearer_after_the_call(self): + self.client.headers.get.return_value = "Bearer outer-user-token" + self._call() + self.client.set_bearer_authorization.assert_called_once_with( + "outer-user-token" + ) + + def test_failure_still_restores_ambient_bearer(self): + self.client.headers.get.return_value = "Bearer outer-user-token" + self.client.post.return_value = make_response(403, { + "error": {"code": "FORBIDDEN", "message": "denied"} + }) + with session_context(): + flask.session["logins_login_id"] = "login-1" + with self.assertRaises(errors.APIError): + self.auth.logins.token() + self.client.set_bearer_authorization.assert_called_once_with( + "outer-user-token" + ) + + def test_no_ambient_header_leaves_basic_in_place(self): + self.client.headers.get.return_value = None + self._call() + self.client.reset_authorization.assert_called_once() + self.client.set_bearer_authorization.assert_not_called() + + class TestPublicClientRefresh(unittest.TestCase): """auth.refresh(stored) presents the stored refresh token with the issuing client_id and returns the rotated pair (#87).""" From e79132f57dfc0f07de14c6f9a4047cd8599ee7cf Mon Sep 17 00:00:00 2001 From: JS Ng Date: Tue, 27 Oct 2026 20:41:18 +0800 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20expose=20JsonClient.authorization=20?= =?UTF-8?q?=E2=80=94=20header=20peek=20for=20the=20Basic-only=20token=20ca?= =?UTF-8?q?ll?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- campus_python/auth/v1/logins.py | 2 +- campus_python/json_client/__init__.py | 5 +++++ campus_python/json_client/interface.py | 5 +++++ tests/unit/test_token_refresh.py | 8 ++++---- 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/campus_python/auth/v1/logins.py b/campus_python/auth/v1/logins.py index 47bb4bc..6ec46fa 100644 --- a/campus_python/auth/v1/logins.py +++ b/campus_python/auth/v1/logins.py @@ -157,7 +157,7 @@ def token(self) -> campus.model.OAuthToken: """ # Canonical path has no trailing slash (route rule is # //token, unlike the CRUD subresources). - prior_authorization = self.client.headers.get("Authorization") + prior_authorization = self.client.authorization self.client.reset_authorization() try: resp = self.client.post( diff --git a/campus_python/json_client/__init__.py b/campus_python/json_client/__init__.py index c7904c8..2ae7cab 100644 --- a/campus_python/json_client/__init__.py +++ b/campus_python/json_client/__init__.py @@ -130,6 +130,11 @@ def reset_authorization(self) -> None: secret=env.CLIENT_SECRET ) + @property + def authorization(self) -> str | None: + """The current Authorization header value, or None when unset.""" + return self._session.headers.get("Authorization") + def set_basic_authorization(self, client_id: str, secret: str) -> None: """Set Basic Authorization header using a pre-encoded token. diff --git a/campus_python/json_client/interface.py b/campus_python/json_client/interface.py index e23b150..4a7e500 100644 --- a/campus_python/json_client/interface.py +++ b/campus_python/json_client/interface.py @@ -131,6 +131,11 @@ def __init__( def reset_authorization(self) -> None: """Reset authorization back to client credentials from env.""" + @property + @abstractmethod + def authorization(self) -> str | None: + """The current Authorization header value, or None when unset.""" + @abstractmethod def set_basic_authorization(self, client_id: str, secret: str) -> None: """Set Basic Authorization header using a pre-encoded token. diff --git a/tests/unit/test_token_refresh.py b/tests/unit/test_token_refresh.py index 7caa82e..cd0bc3f 100644 --- a/tests/unit/test_token_refresh.py +++ b/tests/unit/test_token_refresh.py @@ -50,7 +50,7 @@ def make_auth() -> tuple[AuthRoot, Mock]: the ambient-bearer path override this. """ client = Mock() - client.headers.get.return_value = None + client.authorization = None # property read in token() return AuthRoot(json_client=client), client @@ -180,7 +180,7 @@ def _call(self): return self.auth.logins.token() def test_forces_basic_when_bearer_is_set(self): - self.client.headers.get.return_value = "Bearer outer-user-token" + self.client.authorization = "Bearer outer-user-token" self._call() calls = [name for name, _, _ in self.client.mock_calls if name in ("reset_authorization", "post", @@ -192,14 +192,14 @@ def test_forces_basic_when_bearer_is_set(self): ) def test_restores_ambient_bearer_after_the_call(self): - self.client.headers.get.return_value = "Bearer outer-user-token" + self.client.authorization = "Bearer outer-user-token" self._call() self.client.set_bearer_authorization.assert_called_once_with( "outer-user-token" ) def test_failure_still_restores_ambient_bearer(self): - self.client.headers.get.return_value = "Bearer outer-user-token" + self.client.authorization = "Bearer outer-user-token" self.client.post.return_value = make_response(403, { "error": {"code": "FORBIDDEN", "message": "denied"} })