diff --git a/api/src/auth/router.ts b/api/src/auth/router.ts index 07e67683..633ec620 100644 --- a/api/src/auth/router.ts +++ b/api/src/auth/router.ts @@ -345,7 +345,13 @@ router.post('/nhi-token', async (req, res) => { // setSessionCookies only stamps boundIp for adminMode, which an NHI never has, so no conflict if (user.nhi.ipBinding) payload.boundIp = clientIp const token = await setSessionCookies(req, res, reqSitePath(req), payload, 'nhi-session', userOrg, { skipExchangeToken: true, exp }) - storages.globalStorage.updateLogged(user.id).catch((err: any) => internalError('nhi-update-logged', 'error while updating logged date', err)) + // Guarded on `readonly`, like confirmLog on the password path: a read-only storage cannot record a + // last-logged date, and FileStorage's updateLogged throws SYNCHRONOUSLY — it is not async — so the + // .catch() below never attaches and the rejection became a 500 that failed the whole exchange. + // Without the guard an NHI could not obtain a session at all under file storage. + if (!storages.globalStorage.readonly) { + storages.globalStorage.updateLogged(user.id).catch((err: any) => internalError('nhi-update-logged', 'error while updating logged date', err)) + } eventsLog.info('sd.auth.nhi.ok', `an NHI session was created for ${user.id}`, logContext) res.set('Cache-Control', 'no-store') res.send({ access_token: token, token_type: 'Bearer', expires_in: exp - nowSec }) diff --git a/tests/features/file-storage.unit.spec.ts b/tests/features/file-storage.unit.spec.ts index 854dcb59..ce38dc5d 100644 --- a/tests/features/file-storage.unit.spec.ts +++ b/tests/features/file-storage.unit.spec.ts @@ -53,4 +53,18 @@ test.describe('file storage interface', () => { assert.equal(ntag.department, 'dep1') assert.equal(ntag.departmentName, 'Dep 1') }) + + test('declares itself readonly, which is what callers must branch on', () => { + // The NHI token exchange and the password login both record a last-logged date, which a + // read-only storage cannot do. Both must therefore branch on this flag rather than rely on + // catching a failure — see the next test for why catching does not work. + assert.equal(storage.readonly, true) + }) + + test('updateLogged throws synchronously, so a .catch() cannot absorb it', () => { + // This is the reason the NHI exchange needs an explicit `readonly` guard rather than a .catch(): + // the method is not async, so the throw happens at the call site before any handler attaches and + // propagates as a 500. If it ever becomes an async rejection this test fails, and the guard can go. + assert.throws(() => storage.updateLogged('anyone')) + }) })