Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/matrix/support/synapse/dev/homeserver.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ suppress_key_server_warning: true
ui_auth:
session_timeout: "300s"

login_via_existing_session:
enabled: true
require_ui_auth: false
token_timeout: "2m"

email:
smtp_host: "boxel-smtp"
smtp_port: 25
Expand Down

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ suppress_key_server_warning: true
ui_auth:
session_timeout: "300s"

login_via_existing_session:
enabled: true
require_ui_auth: false
token_timeout: "2m"

email:
smtp_host: "boxel-smtp"
smtp_port: 25
Expand Down
5 changes: 5 additions & 0 deletions packages/matrix/support/synapse/test/homeserver.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ suppress_key_server_warning: true
ui_auth:
session_timeout: "300s"

login_via_existing_session:
enabled: true
require_ui_auth: false
token_timeout: "2m"
Comment on lines +87 to +90

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.
  • requester is None, and Ratelimiter.can_do_action consults the ratelimit_override table only inside if 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 gets 429 M_LIMIT_EXCEEDED with retry_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


email:
smtp_host: "boxel-smtp"
smtp_port: 25
Expand Down
91 changes: 91 additions & 0 deletions packages/matrix/tests/login-via-existing-session.spec.ts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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_000 earns 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.py reads login_via_existing_session.token_timeout through parse_duration, and the servlet returns that same millisecond value as expires_in_ms, so "2m"120_000 is a real check on the config reaching Synapse.
  • The second test is subsumed by the first. register_servlets registers the servlet only if hs.config.auth.login_via_existing_enabled, so with the feature off the route is unregistered and both tests change — the first one's expect(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 as M_MISSING_TOKEN specifically, which is auth.get_user_by_req behavior 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

Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { expect, test } from '@playwright/test';
import { getSynapseURL } from '../support/environment-config.ts';
import { createUser } from '../helpers/index.ts';

// Exercises Synapse's login_via_existing_session feature (MSC3882), which the
// test homeserver config enables. A client holding an access token mints a
// short-lived, single-use login token and exchanges it for a fresh session —
// the mechanism that hands a pre-authenticated session off to the browser.
test.describe('login_via_existing_session', () => {
test('an access-token holder can mint a login token and exchange it for a session', async () => {
let { credentials } = await createUser('login-token');

// Mint a login token with the existing session's access token.
let getTokenResponse = await fetch(
`${getSynapseURL()}/_matrix/client/v1/login/get_token`,
{
method: 'POST',
headers: { Authorization: `Bearer ${credentials.accessToken}` },
body: JSON.stringify({}),
},
);
expect(
getTokenResponse.status,
'get_token succeeds for an authenticated caller',
).toBe(200);
let { login_token, expires_in_ms } = (await getTokenResponse.json()) as {
login_token: string;
expires_in_ms: number;
};
expect(login_token, 'a login token is returned').toBeTruthy();
// token_timeout is configured as "2m" in the test homeserver.yaml.
expect(expires_in_ms).toBe(120_000);

// Exchange the login token for a brand-new session belonging to the same user.
let loginResponse = await fetch(
`${getSynapseURL()}/_matrix/client/v3/login`,
{
method: 'POST',
body: JSON.stringify({ type: 'm.login.token', token: login_token }),
},
);
expect(loginResponse.status, 'login with the token succeeds').toBe(200);
let session = (await loginResponse.json()) as {
user_id: string;
access_token: string;
device_id: string;
};
expect(session.user_id).toBe(credentials.userId);
expect(session.access_token, 'a fresh access token is issued').toBeTruthy();
expect(session.device_id).toBeTruthy();
// The handed-off session is independent of the caller's device.
expect(session.device_id).not.toBe(credentials.deviceId);
});

test('the endpoint is recognized and requires authentication', async () => {
// Before the feature is enabled Synapse returns M_UNRECOGNIZED for this
// route; with it enabled an unauthenticated call is rejected as
// M_MISSING_TOKEN, proving the endpoint is wired up and enforcing auth.
let response = await fetch(
`${getSynapseURL()}/_matrix/client/v1/login/get_token`,
{ method: 'POST', body: JSON.stringify({}) },
);
expect(response.status).toBe(401);
let body = (await response.json()) as { errcode: string };
expect(body.errcode).toBe('M_MISSING_TOKEN');
});

test('a login token is single-use', async () => {
let { credentials } = await createUser('login-token-reuse');

let { login_token } = (await (
await fetch(`${getSynapseURL()}/_matrix/client/v1/login/get_token`, {
method: 'POST',
headers: { Authorization: `Bearer ${credentials.accessToken}` },
body: JSON.stringify({}),
})
).json()) as { login_token: string };

let first = await fetch(`${getSynapseURL()}/_matrix/client/v3/login`, {
method: 'POST',
body: JSON.stringify({ type: 'm.login.token', token: login_token }),
});
expect(first.status, 'the first exchange succeeds').toBe(200);

let second = await fetch(`${getSynapseURL()}/_matrix/client/v3/login`, {
method: 'POST',
body: JSON.stringify({ type: 'm.login.token', token: login_token }),
});
expect(second.status, 'the token cannot be reused').toBe(403);
});
});
Loading