diff --git a/api/src/main/middlewares/AuthMiddleware.ts b/api/src/main/middlewares/AuthMiddleware.ts index 687c108..bb68ec2 100644 --- a/api/src/main/middlewares/AuthMiddleware.ts +++ b/api/src/main/middlewares/AuthMiddleware.ts @@ -8,6 +8,7 @@ import { import { matchPath, extractApiPath } from '../utils/routeMatcher'; import { LeanOrganization, OrganizationMember, OrganizationUserRole } from '../types/models/Organization'; import { HttpMethod, OrganizationApiKeyRole } from '../types/permissions'; +import { LeanUser } from '../types/models/User'; /** * Middleware to authenticate API Keys (both User and Organization types) @@ -19,6 +20,18 @@ import { HttpMethod, OrganizationApiKeyRole } from '../types/permissions'; * Sets req.user for User API Keys * Sets req.org for Organization API Keys */ +/** + * A credential that was read and found wanting. + * + * Distinguished from every other failure on purpose. Authentication reads the + * database, so anything that can go wrong with the database - a dropped + * connection, a replica-set election, a Mongo that is simply not running - + * surfaces as an exception here too. Answering 401 to those says the caller + * sent a bad key, which is untrue and sends whoever is debugging to look at + * their credentials. + */ +class InvalidApiKeyError extends Error {} + const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: NextFunction) => { const apiKey = req.headers['x-api-key'] as string; @@ -40,11 +53,25 @@ const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: N return checkPermissions(req, res, next); } catch (err: any) { - if (!res.headersSent) { + if (res.headersSent) { + return; + } + + if (err instanceof InvalidApiKeyError) { return res.status(401).json({ error: err.message || 'Invalid API Key', }); } + + // Anything else got as far as trying and could not finish - almost always + // the database. 503 rather than 401, because the credential was never + // judged, and Retry-After because it is worth trying again. + console.error('Authentication could not be completed:', err); + res.setHeader('Retry-After', '5'); + return res.status(503).json({ + error: 'Space cannot verify credentials right now.', + details: err?.message ?? String(err), + }); } }; @@ -54,16 +81,43 @@ const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: N async function authenticateUserApiKey(req: Request, apiKey: string): Promise { const userService = container.resolve('userService'); - const user = await userService.findByApiKey(apiKey); - - if (!user) { - throw new Error('Invalid User API Key'); - } + const user = await rejectionOrOutage( + () => userService.findByApiKey(apiKey), + 'Invalid User API Key' + ); req.user = user; req.authType = 'user'; } +/** + * Run a credential lookup, telling a refusal apart from a failure. + * + * `UserService.findByApiKey` reports an unknown key by throwing rather than by + * returning nothing, using the `INVALID DATA:` prefix this codebase gives to a + * caller's own mistake. That has to keep answering 401. Anything else thrown by + * a lookup is the database being unable to answer, which is the case this + * middleware exists to stop reporting as a bad credential. + */ +async function rejectionOrOutage(lookup: () => Promise, absent: string): Promise { + let found: T; + + try { + found = await lookup(); + } catch (err: any) { + if (typeof err?.message === 'string' && err.message.startsWith('INVALID DATA:')) { + throw new InvalidApiKeyError(err.message); + } + throw err; + } + + if (!found) { + throw new InvalidApiKeyError(absent); + } + + return found; +} + /** * Authenticates an Organization API Key and populates req.org */ @@ -71,11 +125,10 @@ async function authenticateOrgApiKey(req: Request, apiKey: string): Promise organizationRepository.findByApiKey(apiKey), + 'Invalid Organization API Key' + ); req.org = { id: result.id!, diff --git a/api/src/main/routes/HealthcheckRoutes.ts b/api/src/main/routes/HealthcheckRoutes.ts index c4a92d0..e89b8f3 100644 --- a/api/src/main/routes/HealthcheckRoutes.ts +++ b/api/src/main/routes/HealthcheckRoutes.ts @@ -1,18 +1,49 @@ import express from 'express'; +import mongoose from 'mongoose'; + +/** + * Mongoose reports 1 when the driver has a usable connection. + * + * 2 is "still connecting", which matters at start-up: a container answering + * "healthy" while it is still dialling is one an orchestrator will start + * sending traffic to. + */ +const CONNECTED = 1; const loadFileRoutes = function (app: express.Application) { const baseUrl = '/api/v1'; // Public route for authentication (does not require API Key) - app - .route(`${baseUrl}/healthcheck`) - .get( - (req: any, res: any) => { - res.status(200).json({ - message: 'Service is up and running!', - }); - } - ); + app.route(`${baseUrl}/healthcheck`).get(async (req: any, res: any) => { + // The database is not a detail of this service, it is the service: every + // authenticated route reads it before it can answer anything. A check that + // proves only the HTTP listener is up reports a Space that cannot serve a + // single request as healthy, and keeps reporting it indefinitely while + // every call fails. + if (mongoose.connection.readyState !== CONNECTED) { + return res.status(503).json({ + message: 'Service is up but cannot reach its database.', + database: 'disconnected', + }); + } + + try { + // readyState is what the driver believes. A ping is what the database + // says, and the two disagree when a connection has gone stale. + await mongoose.connection.db!.admin().ping(); + } catch (error: any) { + return res.status(503).json({ + message: 'Service is up but its database is not answering.', + database: 'unreachable', + details: error?.message ?? String(error), + }); + } + + res.status(200).json({ + message: 'Service is up and running!', + database: 'connected', + }); + }); }; export default loadFileRoutes; diff --git a/api/src/test/middlewares/authOutage.test.ts b/api/src/test/middlewares/authOutage.test.ts new file mode 100644 index 0000000..890ec45 --- /dev/null +++ b/api/src/test/middlewares/authOutage.test.ts @@ -0,0 +1,93 @@ +import request from 'supertest'; +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { Server } from 'http'; +import { getApp, shutdownApp, baseUrl } from '../utils/testApp'; +import { createTestUser, deleteTestUser } from '../utils/users/userTestUtils'; +import container from '../../main/config/container'; +import { LeanUser } from '../../main/types/models/User'; + +/** + * Telling "your key is wrong" apart from "we could not check your key". + * + * Authentication reads the database, so a dropped connection, a replica-set + * election or a Mongo that is simply not running all surface as an exception in + * the same place an unknown key does. Answering 401 to those states something + * untrue about the caller's credential and sends whoever is debugging to look + * at their API key, which is the one place the problem is not. + * + * Both directions are pinned here, because the interesting part of the change + * is the boundary: a refusal must stay a refusal. + */ +describe('Authentication when the database cannot answer', function () { + let app: Server; + let user: LeanUser; + + beforeAll(async function () { + app = await getApp(); + user = await createTestUser('ADMIN'); + }); + + afterAll(async function () { + await deleteTestUser(user.username); + vi.restoreAllMocks(); + await shutdownApp(); + }); + + it('answers 503, not 401, when the lookup fails', async function () { + const userService: any = container.resolve('userService'); + const outage = vi + .spyOn(userService, 'findByApiKey') + .mockRejectedValue(new Error('MongooseServerSelectionError: connection timed out')); + + try { + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', user.apiKey); + + expect(response.status).toBe(503); + expect(response.body.error).toContain('cannot verify credentials'); + } finally { + outage.mockRestore(); + } + }); + + it('asks the caller to try again', async function () { + const userService: any = container.resolve('userService'); + const outage = vi + .spyOn(userService, 'findByApiKey') + .mockRejectedValue(new Error('MongooseServerSelectionError: connection timed out')); + + try { + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', user.apiKey); + + // A 503 without Retry-After tells a client nothing about whether waiting + // is worth it. + expect(response.headers['retry-after']).toBeDefined(); + } finally { + outage.mockRestore(); + } + }); + + it('still answers 401 to a key that was read and rejected', async function () { + // The regression this pairs with. `UserService.findByApiKey` reports an + // unknown key by throwing rather than by returning nothing, so narrowing + // the catch to a dedicated error type is not by itself enough to keep this + // a 401. + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', 'usr_nosuchkeyatall'); + + expect(response.status).toBe(401); + expect(response.body.error).toContain('INVALID DATA: Invalid API Key'); + }); + + it('still answers 401 to a key of no recognisable kind', async function () { + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', 'not-a-prefixed-key'); + + expect(response.status).toBe(401); + }); +});