Skip to content

fix: login-session token re-acquisition presents Basic only (#117) - #119

Merged
ngjunsiang merged 2 commits into
mainfrom
fix/117-token-basic-only
Oct 27, 2026
Merged

ngjunsiang merged 2 commits into
mainfrom
fix/117-token-basic-only

Conversation

@nycomp

@nycomp nycomp commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Problem

POST /logins/{session_id}/token serves client principals only — campus.auth's A10 guard (authz.forbid_user_principal, #854 lineage) denies user bearers outright. But Login.token() sent its request on the shared session's ambient Authorization header, whatever it happened to hold.

The breakage shows whenever with_user_session() runs nested inside another with_user_session(): the outer session has set Authorization: Bearer <user token> on the shared auth client, so the inner session's re-acquisition presents a user token to the route → 403 FORBIDDEN → (in campus-classroom) BrokerConfigError → 502 on every enrollment-gated submission mutation.

Found live on dev, 2026-10-27, during the campus-classroom #14 verification pass: the /a/ answer sheet's Save-draft/Submit 502'd while the page render's identical broker release succeeded seconds earlier — the render path enters with_user_session with a fresh (Basic) header because the preceding app-scoped read resets it; the mutation path enters nested.

The SDK's own 401-refresh hook already documented the discipline this route needs — "the login-session token route denies user principals outright (campus#964/A10), so the session's stale bearer must not be presented mid-rotation" (refresh_bearer() revokes first) — the initial acquisition path just didn't follow it.

Fix

Login.token() now:

  1. snapshots the ambient Authorization header,
  2. reset_authorization() → the client's Basic credentials for the call,
  3. restores a prior Bearer afterwards — in a finally, so the enclosing session keeps its authorization even when the route answers 403/400.

The enclosing with_user_session() immediately overwrites the header via use_token(<fresh bearer>), so restore-then-overwrite is correct in both the top-level and nested cases.

Tests

TestLoginTokenPresentsBasicOnly (4 cases, mock-client contract style matching the file):

  • exact call ordering reset_authorization → POST /logins/{id}/token → set_bearer_authorization(<prior>) when a bearer is ambient,
  • the ambient bearer is restored after the call,
  • it is restored even when the route answers 403,
  • no ambient header → Basic stays, no spurious set_bearer.

Full suite: 321 passed.

Impact on consumers

After this, nested with_user_session() blocks work; the outer session's subsequent calls may ride the inner session's finally reset (Basic) and self-heal through the existing 401 hook (+1 round trip) — the same healing path an expired bearer already takes.

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).
@ngjunsiang
ngjunsiang merged commit 91d674b into main Oct 27, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants