From fbc6e3fa6c920a424027cc5f09854f313c6257a6 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 14 Sep 2026 07:31:52 +0200 Subject: [PATCH] fix: the Nextcloud login flow waits in Redis, so any replica can answer the poll 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 Claude-Session: https://claude.ai/code/session_01Dv6sqaY6Vq3ChZHMem3cnU --- src/routes/nextcloud.js | 28 +++++---------- src/utils/loginFlowStore.js | 59 +++++++++++++++++++++++++++++++ test/nextcloud-login-flow.test.js | 14 ++++++-- 3 files changed, 79 insertions(+), 22 deletions(-) create mode 100644 src/utils/loginFlowStore.js diff --git a/src/routes/nextcloud.js b/src/routes/nextcloud.js index c591e476..e44dd448 100644 --- a/src/routes/nextcloud.js +++ b/src/routes/nextcloud.js @@ -29,16 +29,11 @@ function davRoot(baseUrl, username) { // 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. // -// In memory, not in the database: a flow lives for minutes, and a restart -// mid-login is a retry, not a loss. One flow per account at a time — starting a -// second replaces the first, which is what "I clicked it again" means. -var loginFlows = new Map(); -var LOGIN_FLOW_TTL_MS = 20 * 60 * 1000; // Nextcloud expires its side at ~20 minutes - -function sweepLoginFlows() { - var now = Date.now(); - for (var [key, flow] of loginFlows) if (flow.expires < now) loginFlows.delete(key); -} +// A flow lives for minutes and a restart mid-login is a retry, not a loss, so +// it is kept in Redis (any replica may answer the poll), or in memory when +// there is no Redis. One flow per account at a time — starting a second +// replaces the first, which is what "I clicked it again" means. +var loginFlows = require('../utils/loginFlowStore'); // 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 @@ -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.' }); } - sweepLoginFlows(); var handle = require('crypto').randomUUID(); - loginFlows.set(handle, { + await loginFlows.put(handle, { owner: req.user.id, url: cleanUrl, token: poll.token, endpoint: poll.endpoint, - folder: String(req.body.folder || '/PediatricScribe').replace(/\/+$/, ''), - expires: Date.now() + LOGIN_FLOW_TTL_MS + folder: String(req.body.folder || '/PediatricScribe').replace(/\/+$/, '') }); - // 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 }); } 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) { try { - sweepLoginFlows(); - var flow = loginFlows.get(String(req.body.handle || '')); + var flow = await loginFlows.get(String(req.body.handle || '')); // Bound to the account that started it: a handle is not a bearer token. if (!flow || flow.owner !== req.user.id) { 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 = ?', [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' }); res.json({ success: true, connected: true, username: loginName, folder: folder }); diff --git a/src/utils/loginFlowStore.js b/src/utils/loginFlowStore.js new file mode 100644 index 00000000..c3f67373 --- /dev/null +++ b/src/utils/loginFlowStore.js @@ -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 }; diff --git a/test/nextcloud-login-flow.test.js b/test/nextcloud-login-flow.test.js index 9d7cacce..44a42e0a 100644 --- a/test/nextcloud-login-flow.test.js +++ b/test/nextcloud-login-flow.test.js @@ -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', () => { - assert.match(route, /LOGIN_FLOW_TTL_MS = 20 \* 60 \* 1000/); - assert.match(route, /function sweepLoginFlows\(\)/); - assert.match(start, /flow\.owner === req\.user\.id && key !== handle\) loginFlows\.delete\(key\)/); + // The flow lives in the shared store (Redis when there is one, so any + // replica may answer the poll; memory otherwise), never in the route. + 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', () => {