A database outage should not be reported as 401 Unauthorized

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,28 +81,54 @@ const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: N
 async function authenticateUserApiKey(req: Request, apiKey: string): Promise<void> {
   const userService = container.resolve('userService');
 
-  const user = await userService.findByApiKey(apiKey);
-
-  if (!user) {
-    throw new Error('Invalid User API Key');
-  }
+  const user = await rejectionOrOutage<LeanUser>(
+    () => 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<T>(lookup: () => Promise<T>, absent: string): Promise<T> {
+  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
  */
 async function authenticateOrgApiKey(req: Request, apiKey: string): Promise<void> {
   const organizationRepository = container.resolve('organizationRepository');
 
   // Find organization by API Key
-  const result: LeanOrganization = await organizationRepository.findByApiKey(apiKey);
-
-  if (!result) {
-    throw new Error('Invalid Organization API Key');
-  }
+  const result: LeanOrganization = await rejectionOrOutage(
+    () => organizationRepository.findByApiKey(apiKey),
+    'Invalid Organization API Key'
+  );
 
   req.org = {
     id: result.id!,

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;

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);
+  });
+});