Enable login_via_existing_session in local dev/test Synapse - #5830
Enable login_via_existing_session in local dev/test Synapse#5830FadhlanR wants to merge 2 commits into
Conversation
Turn on Synapse's login_via_existing_session feature in the dev and test homeserver configs so the MSC3882 endpoint POST /_matrix/client/v1/login/get_token is available. An access-token holder can mint a short-lived (2m), single-use login token without being re-prompted for the password (require_ui_auth: false), which lets a pre-authenticated client hand off a session to the browser. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Verify the MSC3882 endpoint the test homeserver now enables: an access-token holder mints a short-lived login token via POST /_matrix/client/v1/login/get_token and exchanges it for a fresh session on a new device. Also assert the endpoint is recognized and auth-enforced (401 M_MISSING_TOKEN) and that a login token is single-use. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Reviewed with two lenses: what the flipped switch actually does at runtime in the pinned Synapse (v1.126.0 — the endpoint's real operational envelope, not just its existence), and whether the new spec pins this repo's contract or upstream's. The config hunks themselves are three lines each, correctly placed at the top level of the document; the substance sits around them.
Bottom line: no blocking issues — the config is valid for the pinned Synapse and the endpoint behaves as the description says. Two things need a decision rather than a fix: the security rationale is slightly stronger than the mechanism supports, and the new spec asserts mostly upstream behavior while the boxel-side ?loginToken path it was written to unblock is already testable today.
Verified against the branch and the pinned upstream source:
- The block is valid config for v1.126.0.
synapse/config/auth.pyreadslogin_via_existing_session.enabled(defaultFalse) to gateregister_servlets,require_ui_auth(defaultTrue, so the explicitfalseis load-bearing) to skipvalidate_user_via_ui_auth, andtoken_timeoutthroughparse_duration— surfacing asexpires_in_ms, hence"2m"→ the spec's120_000. The unauthenticated401 M_MISSING_TOKENholds becauseauth.get_user_by_reqruns first inon_POST, before the body parse and the rate limiter. - Enabling this changes
GET /_matrix/client/v3/login: the existingm.login.tokenflow entry gainsget_login_token: true. No login-UI fallout —detectGoogleSsoinpackages/host/app/components/matrix/login.gtsis the only consumer ofloginFlows()and filters onm.login.ssoalone. m.login.tokenis accepted onPOST /loginregardless of that advertisement:LoginRestServlet.on_POSTdispatches on the submitted type with no_get_login_token_enabledguard; the flag only shapes whatGET /loginadvertises. So the exchange half of the flow was already reachable, and this change is what makes the minting half exist.
Recommendations:
- The "adds no new capability" claim needs a caveat (no inline thread — this is about the description and applies to all three copies of the block equally). It is right that an access-token holder can already act as the account. What it cannot do without this endpoint is create additional login sessions: the exchange mints a new device with its own access token, unrelated to the device that minted it — which the third test half-demonstrates with
device_idnot matching. That collides with the session-revocation runbook, whose first step is "deactivate the user's matrix device in Synapse — this removes the ability to mint". With this endpoint enabled, deactivating the one device an operator knows about no longer removes the ability to mint: a stolen token can spawn devices ahead of that step, and any surviving device keeps satisfying thematrixService.isLoggedIngate that lets a client re-mint realm JWTs aftersessions_revoked_atmoves. Dev and test only in this change, so nothing to fix here — but the runbook step generalizes to "log out every device", and the description is the artifact whatever enables this in a deployed config will cite. Suggest softening it to something like: no new capability over the account, but a new session-spawning primitive, so revocation has to cover all devices. get_tokenis hardcoded to one request per minute per user, unrelaxable by config or by the admin override API — see the thread on the block intest/homeserver.yaml. The tests are safe only because each one registers a fresh user; that invariant deserves a line in the yaml, sincerc_loginsitting a few lines above suggests login rate limits are configurable here.- Coverage/scope of the new spec — see the file-level thread on
login-via-existing-session.spec.ts. Two of the three tests pin upstream behavior, the second is subsumed by the first, and the end-to-end test that would guard this repo's own path can be written now. - The third template looks like dead config — see the file-level thread on
test-without-registration-token/homeserver.yaml. No caller in the repo selects it, so that hunk is unverifiable by construction.
Adjacent, out of scope. The ?loginToken consumer in packages/host/app/components/matrix/login.gts is Google-branded throughout: the placeholder reads "Signing you in with Google", both error branches read "Google sign-in failed", and the surrounding names follow (exchangingSsoToken, loginWithSsoToken, consumeSsoLoginToken). A pre-authenticated handoff that has nothing to do with Google will show Google's name to the user on both the happy path and the failure path. Worth generalizing in whichever change adds the handoff, not here.
Generated by Claude Code
| login_via_existing_session: | ||
| enabled: true | ||
| require_ui_auth: false | ||
| token_timeout: "2m" |
There was a problem hiding this comment.
[Claude Code 🤖] The 2-minute token_timeout is configurable; the request rate is not. POST /_matrix/client/v1/login/get_token carries a hardcoded limit of one request per 60 seconds per user, and nothing in this file can relax it — worth writing down right here, because the generous rc_login block earlier in this same file implies the opposite.
Mechanism. In Synapse v1.126.0 (the version support/synapse/index.ts pins), LoginTokenRequestServlet.__init__ builds its own limiter instead of reading any rc_* setting:
# Ratelimit aggressively to a maximum of 1 request per minute.
self._ratelimiter = Ratelimiter(
store=..., clock=...,
cfg=RatelimitSettings(key="<login token request>", per_second=1 / 60, burst_count=1),
)and invokes it as await self._ratelimiter.ratelimit(None, requester.user.to_string().lower()). Two things follow from that one call:
- The key is the user id, so the budget is per account — not per device, not per IP.
requesterisNone, andRatelimiter.can_do_actionconsults theratelimit_overridetable only insideif requester:. So the admin exemption API (POST /_synapse/admin/v1/users/<user>/override_ratelimit) doesn't reach this endpoint either. There is no config knob and no admin escape hatch; an over-limit caller gets429 M_LIMIT_EXCEEDEDwithretry_after_ms.
Why it matters here. The new spec is safe purely by construction — every test calls createUser, so each mint lands on a fresh unique user id. Nothing states that invariant, and it is an easy one to break: a later test that mints for one of the long-lived shared accounts, combined with retries: 2 in the Playwright config, re-mints for the same user inside the same minute and fails deterministically on the retry — a "flake" that gets worse the more CI retries it.
Ask. Add a line to the block (in each copy) naming the constraint:
login_via_existing_session:
enabled: true
require_ui_auth: false
token_timeout: "2m"
# Synapse rate-limits get_token to one request per minute per user id. That
# limit is hardcoded in the servlet — no rc_* setting and no admin
# override_ratelimit relaxes it — so anything minting more than one token
# per minute needs a distinct account per mint.Class: pre-existing upstream behavior that this change makes load-bearing. Non-blocking.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Nothing in the repo selects this template, so this third of the diff is config that never loads. Not this change's doing — but worth knowing before treating "parity across all three templates" as a cost worth paying.
Verified. synapseStart resolves a template via opts?.template ?? 'test' (support/synapse/index.ts), and repo-wide there are exactly two call sites: scripts/synapse.ts passes template: 'dev', and tests/global.setup.ts calls synapseStart() with no options, taking the 'test' default. A repo-wide grep for without-registration (excluding node_modules, across .ts/.sh/.yml/.yaml/.json/.md) returns nothing outside the directory itself — no script, workflow, mise task, or spec names it. Its only distinguishing content is registration_requires_token: false (plus a missing presence block and the test OIDC block).
The practical consequence is that an error in this copy is undetectable: cfgDirFromTemplate never renders it and Synapse never boots it, so a bad key here fails nowhere. That is the difference between this hunk and the identical hunk in test/ (which the suite boots on every run, so the spec's expires_in_ms assertion actually proves the block took effect).
Ask. Either point out what selects it that I've missed, or file the follow-up to delete the directory — in which case this hunk goes with it. Class: pre-existing orphan; follow-up, not this change. Non-blocking.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Most of what this file asserts is Synapse's behavior rather than ours — and the boxel-side path it was written to unblock is already testable today. Non-blocking, but it is the substantive question about this file: what regression does each test catch?
Background. The config switch has exactly two observable consequences this repo owns: the endpoint exists, and token_timeout: "2m" reaches Synapse. Everything else on the wire — single-use tokens, a fresh device per exchange, 403 on reuse, 401 M_MISSING_TOKEN when unauthenticated — is upstream behavior no change in this repo can move. A Synapse version bump is the only thing that shifts it, and then the red test reads as "our test broke", not "our feature broke".
Sorting the assertions against that:
expires_in_ms === 120_000earns its place. It is the only assertion that fails if the yaml block regresses in a way a status code wouldn't catch, and the value it checks is one this repo sets. Verified end to end in v1.126.0:synapse/config/auth.pyreadslogin_via_existing_session.token_timeoutthroughparse_duration, and the servlet returns that same millisecond value asexpires_in_ms, so"2m"→120_000is a real check on the config reaching Synapse.- The second test is subsumed by the first.
register_servletsregisters the servlet onlyif hs.config.auth.login_via_existing_enabled, so with the feature off the route is unregistered and both tests change — the first one'sexpect(status).toBe(200)fails on the 404 before the second one says anything. The only thing the second adds is that an unauthenticated caller is rejected asM_MISSING_TOKENspecifically, which isauth.get_user_by_reqbehavior shared by every authenticated client-API route. Its comment also narrates the pre-change response, which reads as history rather than as the current contract. - Single-use and the device-independence assertion are upstream guarantees. They are cheap to run, so keeping them costs little, but they document rather than guard.
The test that would guard this repo has both halves already in place. The host consumer exists: packages/host/app/components/matrix/login.gts reads ?loginToken in consumeSsoLoginToken, hands it to matrixService.loginWithSsoToken (→ client.loginWithToken), then calls matrixService.start. The isTesting() branch in that constructor only adds a console.warn, so the path is live under Playwright. Sketch:
// illustrative — helper names/assertions to taste
let { username, credentials } = await createSubscribedUser('login-token');
let login_token = await mintLoginToken(credentials.accessToken);
await page.goto(`${appURL}?loginToken=${login_token}`);
await assertLoggedIn(page, { userId: `@${username}:localhost`, displayName: username });That covers what the three current tests do not: the query-param plumbing, the exchange through matrixService, and session start. It also exercises the token_timeout budget for real, since the 2 minutes must cover mint → page load → matrix-sdk load → exchange.
Ask. A decision, not necessarily a change: keep the upstream-behavior tests, or trade them for the end-to-end one. If they stay, dropping the second test loses no coverage. Class: coverage/scope.
Generated by Claude Code
What
Enables Synapse's
login_via_existing_sessionfeature in the local dev and test homeserver configs, so the MSC3882 endpointPOST /_matrix/client/v1/login/get_tokenis available.Added to all three templates for parity:
packages/matrix/support/synapse/dev/homeserver.yamlpackages/matrix/support/synapse/test/homeserver.yamlpackages/matrix/support/synapse/test-without-registration-token/homeserver.yamlWhy
Part 1 of the "open the Boxel app pre-authenticated" work. A client holding an access token can mint a short-lived (2m), single-use login token and hand a session off to the browser without the user re-typing credentials. Enabling the
testconfig also unblocks an end-to-end matrix test for the host-side?loginTokenconsumer (separate PR).Security: an access-token holder can already do anything with the account; letting it mint 2-minute single-use login tokens adds no new capability.
Verification
Booted an isolated synapse from the changed
devtemplate (own container/port, so the shared dev synapse was untouched):homeserver.yamlcarries the block verbatim.POST /_matrix/client/v1/login/get_token(unauthenticated) returns401 M_MISSING_TOKEN— i.e. the endpoint is now recognized and enforcing auth. Before this change it returnedM_UNRECOGNIZED(feature disabled).Local config is regenerated from the template on every synapse start (
cfgDirFromTemplaterewriteshomeserver.yamlunconditionally), sopnpm stop:synapse && pnpm start:synapseis enough to pick this up — no data-dir reset needed.🤖 Generated with Claude Code