fix: the Nextcloud login flow waits in Redis, so any replica can answer the poll
All checks were successful
Forgejo Docker Build / Root app tests (push) Successful in 1m3s
Forgejo Docker Build / Build Docker image (push) Successful in 20s
Forgejo Docker Build / End-to-end (browser) (push) Successful in 5s

The flow lived in a Map inside the web process; with more than one
replica the poll could land where the flow was never started and every
sign-in would look expired. It now sits in Redis with the same 20-minute
life and one-per-account rule, and falls back to memory when there is
no Redis, which is what tests and a single-process box always had.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dv6sqaY6Vq3ChZHMem3cnU
This commit is contained in:
Daniel 2026-09-14 07:31:52 +02:00
parent 9695e3c7f9
commit fbc6e3fa6c
3 changed files with 79 additions and 22 deletions

View file

@ -29,16 +29,11 @@ function davRoot(baseUrl, username) {
// The poll token is a credential, so the polling happens here rather than in the // The poll token is a credential, so the polling happens here rather than in the
// browser. The browser holds only an opaque handle bound to its own account. // browser. The browser holds only an opaque handle bound to its own account.
// //
// In memory, not in the database: a flow lives for minutes, and a restart // A flow lives for minutes and a restart mid-login is a retry, not a loss, so
// mid-login is a retry, not a loss. One flow per account at a time — starting a // it is kept in Redis (any replica may answer the poll), or in memory when
// second replaces the first, which is what "I clicked it again" means. // there is no Redis. One flow per account at a time — starting a second
var loginFlows = new Map(); // replaces the first, which is what "I clicked it again" means.
var LOGIN_FLOW_TTL_MS = 20 * 60 * 1000; // Nextcloud expires its side at ~20 minutes var loginFlows = require('../utils/loginFlowStore');
function sweepLoginFlows() {
var now = Date.now();
for (var [key, flow] of loginFlows) if (flow.expires < now) loginFlows.delete(key);
}
// The site's own Nextcloud. When it is set, nobody types an address: the // The site's own Nextcloud. When it is set, nobody types an address: the
// settings page shows "Sign in with Nextcloud" and nothing else, with the // settings page shows "Sign in with Nextcloud" and nothing else, with the
@ -81,15 +76,11 @@ router.post('/nextcloud/login-flow/start', authMiddleware, async function (req,
return res.status(502).json({ error: 'That server pointed the login somewhere else. Not continuing.' }); return res.status(502).json({ error: 'That server pointed the login somewhere else. Not continuing.' });
} }
sweepLoginFlows();
var handle = require('crypto').randomUUID(); var handle = require('crypto').randomUUID();
loginFlows.set(handle, { await loginFlows.put(handle, {
owner: req.user.id, url: cleanUrl, token: poll.token, endpoint: poll.endpoint, owner: req.user.id, url: cleanUrl, token: poll.token, endpoint: poll.endpoint,
folder: String(req.body.folder || '/PediatricScribe').replace(/\/+$/, ''), folder: String(req.body.folder || '/PediatricScribe').replace(/\/+$/, '')
expires: Date.now() + LOGIN_FLOW_TTL_MS
}); });
// One at a time per account.
for (var [key, flow] of loginFlows) if (flow.owner === req.user.id && key !== handle) loginFlows.delete(key);
res.json({ success: true, handle: handle, loginUrl: loginUrl }); res.json({ success: true, handle: handle, loginUrl: loginUrl });
} catch (err) { } catch (err) {
@ -100,8 +91,7 @@ router.post('/nextcloud/login-flow/start', authMiddleware, async function (req,
router.post('/nextcloud/login-flow/poll', authMiddleware, async function (req, res) { router.post('/nextcloud/login-flow/poll', authMiddleware, async function (req, res) {
try { try {
sweepLoginFlows(); var flow = await loginFlows.get(String(req.body.handle || ''));
var flow = loginFlows.get(String(req.body.handle || ''));
// Bound to the account that started it: a handle is not a bearer token. // Bound to the account that started it: a handle is not a bearer token.
if (!flow || flow.owner !== req.user.id) { if (!flow || flow.owner !== req.user.id) {
return res.status(410).json({ error: 'That sign-in attempt has expired. Start again.' }); return res.status(410).json({ error: 'That sign-in attempt has expired. Start again.' });
@ -145,7 +135,7 @@ router.post('/nextcloud/login-flow/poll', authMiddleware, async function (req, r
await db.run('UPDATE users SET nextcloud_url = ?, nextcloud_user = ?, nextcloud_token = ?, nextcloud_folder = ? WHERE id = ?', await db.run('UPDATE users SET nextcloud_url = ?, nextcloud_user = ?, nextcloud_token = ?, nextcloud_folder = ? WHERE id = ?',
[serverUrl, loginName, cryptoUtil.encryptString(appPassword, tokenContext(req.user.id)), folder, req.user.id]); [serverUrl, loginName, cryptoUtil.encryptString(appPassword, tokenContext(req.user.id)), folder, req.user.id]);
loginFlows.delete(String(req.body.handle)); await loginFlows.remove(String(req.body.handle));
logger.audit(req.user.id, 'nextcloud_connect', 'Connected Nextcloud via login flow', req, { category: 'integration' }); logger.audit(req.user.id, 'nextcloud_connect', 'Connected Nextcloud via login flow', req, { category: 'integration' });
res.json({ success: true, connected: true, username: loginName, folder: folder }); res.json({ success: true, connected: true, username: loginName, folder: folder });

View file

@ -0,0 +1,59 @@
// Where a Nextcloud login flow waits between "open this link" and "polled done".
//
// A flow lives for minutes and belongs to one account. It sits in Redis so
// that the poll can land on any replica, not only the one that started it;
// without Redis (tests, a single-process dev box) it falls back to memory,
// which is what it always was. Either way: one flow per account at a time,
// and a handle is never a bearer token — the caller checks the owner.
var { getRedis } = require('./redis');
var TTL_MS = 20 * 60 * 1000; // Nextcloud expires its side at ~20 minutes
var memory = new Map();
function flowKey(handle) { return 'nextcloud:loginflow:' + handle; }
function ownerKey(owner) { return 'nextcloud:loginflow:owner:' + owner; }
function sweepMemory() {
var now = Date.now();
for (var [key, flow] of memory) if (flow.expires < now) memory.delete(key);
}
async function put(handle, flow) {
var record = Object.assign({}, flow, { expires: Date.now() + TTL_MS });
var redis = await getRedis();
if (!redis) {
sweepMemory();
for (var [key, other] of memory) if (other.owner === flow.owner) memory.delete(key);
memory.set(handle, record);
return;
}
var previous = await redis.get(ownerKey(flow.owner));
var ttl = { PX: TTL_MS };
await redis.multi()
.set(flowKey(handle), JSON.stringify(record), ttl)
.set(ownerKey(flow.owner), handle, ttl)
.exec();
if (previous && previous !== handle) await redis.del(flowKey(previous));
}
async function get(handle) {
var redis = await getRedis();
if (!redis) {
sweepMemory();
return memory.get(handle) || null;
}
var raw = await redis.get(flowKey(handle));
if (!raw) return null;
try { return JSON.parse(raw); } catch (e) { return null; }
}
async function remove(handle) {
var redis = await getRedis();
if (!redis) { memory.delete(handle); return; }
var flow = await get(handle);
var keys = [flowKey(handle)];
if (flow && flow.owner !== undefined) keys.push(ownerKey(flow.owner));
await redis.del(keys);
}
module.exports = { put, get, remove, TTL_MS };

View file

@ -61,9 +61,17 @@ test('the app password is encrypted at rest, and bound to the row it is stored i
}); });
test('a flow expires, and starting again replaces the old one', () => { test('a flow expires, and starting again replaces the old one', () => {
assert.match(route, /LOGIN_FLOW_TTL_MS = 20 \* 60 \* 1000/); // The flow lives in the shared store (Redis when there is one, so any
assert.match(route, /function sweepLoginFlows\(\)/); // replica may answer the poll; memory otherwise), never in the route.
assert.match(start, /flow\.owner === req\.user\.id && key !== handle\) loginFlows\.delete\(key\)/); const store = read('src/utils/loginFlowStore.js');
assert.match(route, /require\('\.\.\/utils\/loginFlowStore'\)/);
assert.doesNotMatch(route, /new Map\(\)/, 'no flow state kept in the route module');
assert.match(store, /TTL_MS = 20 \* 60 \* 1000/);
assert.match(store, /\.set\(flowKey\(handle\), JSON\.stringify\(record\), ttl\)/, 'Redis keys expire with the flow');
assert.match(store, /if \(previous && previous !== handle\) await redis\.del\(flowKey\(previous\)\)/, 'one flow per account in Redis');
assert.match(store, /if \(other\.owner === flow\.owner\) memory\.delete\(key\)/, 'one flow per account in memory');
assert.match(start, /await loginFlows\.put\(handle, \{/);
assert.match(poll, /await loginFlows\.remove\(String\(req\.body\.handle\)\)/);
}); });
test('the sign-in tab is opened without a handle, which COOP would sever', () => { test('the sign-in tab is opened without a handle, which COOP would sever', () => {