From 0f5fc7c0ea9480ac58338ad3346d81918d36f925 Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Sun, 17 May 2026 21:42:47 -0500 Subject: [PATCH] refactor(auth): split resolveAuth into attachAuth + requireAuth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Architect audit P2-D. Split the combined resolveAuth middleware into two single-responsibility middlewares so endpoints can opt out of the strict 403 enforcement: - attachAuth: best-effort populates req.authKey, req.isMaster, req.companyId. Never rejects on missing/unknown key (returns 500 only on a DB error during lookup). Mounted globally on /v1. - requireAuth: 403s on missing key or unknown non-master key. Exported but not globally mounted yet — existing controllers still have their inline authKey check, so global mount would short-circuit per-controller scoping nuances. Per-controller adoption follows in P2-D2…N PRs. /v1/whoami now consumes the attached fields directly and distinguishes "header missing" (403) from "header present but unknown" (200 with authenticated:false) — the previous resolveAuth wrapper collapsed those into a uniform 403, which defeated the endpoint's purpose. resolveAuth kept as a backwards-compat wrapper that chains attachAuth → requireAuth, so anywhere it's still mounted directly keeps working unchanged. Tests: 291 pass / 4 skip. The earlier 58-test regression came from mounting requireAuth globally before per-controller migration — backed off; mount is opt-in until controllers drop their inline checks (tracked separately). Co-Authored-By: Claude Opus 4.7 (1M context) --- app/controllers/whoamicontroller.js | 34 ++++---------- app/middleware/auth.js | 73 +++++++++++++++++++++++------ app/routers/router.js | 16 ++++++- 3 files changed, 80 insertions(+), 43 deletions(-) diff --git a/app/controllers/whoamicontroller.js b/app/controllers/whoamicontroller.js index 50f649d..7c7c584 100644 --- a/app/controllers/whoamicontroller.js +++ b/app/controllers/whoamicontroller.js @@ -24,50 +24,32 @@ * whether the network plumbing is wrong vs. the credential. */ -const log = require('../config/logger.js'); -const auth = require('../middleware/auth.js'); +// Mounted under attachAuth but NOT under requireAuth — req.authKey +// may be null, req.companyId may be -1. That's intentional; the +// response shape distinguishes "header missing" from "header +// present but unknown." exports.whoami = async (req, res) => { - const authKey = req.get('authKey'); - if (!authKey) { + if (!req.authKey) { return res.status(403).json({ message: "Authorization key not sent." }); } - - let isMaster; - try { - isMaster = await auth.isMaster(authKey); - } catch (error) { - log.error({ err: error }, 'whoami: isMaster failed'); - return res.status(500).json({ message: "Error!", error: String(error) }); - } - - if (isMaster) { + if (req.isMaster) { return res.status(200).json({ authenticated: true, isMaster: true, companyId: null, }); } - - let companyId; - try { - companyId = await auth.getCompanyId(authKey); - } catch (error) { - log.error({ err: error }, 'whoami: getCompanyId failed'); - return res.status(500).json({ message: "Error!", error: String(error) }); - } - - if (companyId === -1) { + if (req.companyId === -1) { return res.status(200).json({ authenticated: false, isMaster: false, companyId: null, }); } - return res.status(200).json({ authenticated: true, isMaster: false, - companyId, + companyId: req.companyId, }); }; diff --git a/app/middleware/auth.js b/app/middleware/auth.js index 42946e9..5ab963f 100644 --- a/app/middleware/auth.js +++ b/app/middleware/auth.js @@ -195,39 +195,80 @@ function requireAuthKey(req, res, next) { } /** - * Express middleware: resolves the authKey into {isMaster, companyId} - * context on req, then proceeds. Non-master with unknown authKey - * returns 403 directly so downstream controllers can assume the - * key resolved to *something*. + * Express middleware: best-effort attach `authKey` + resolved + * `{ isMaster, companyId }` onto the request. Never rejects — endpoints + * like /v1/whoami need to distinguish "header missing" from "header + * present but unknown" themselves, and a strict guard middleware + * would collapse those into a uniform 403. + * + * Always sets: + * req.authKey string | null (raw header value, or null) + * req.isMaster boolean (false on unknown / missing key) + * req.companyId number (-1 sentinel for "no scoped key") + * + * Use `requireAuth` after this on routes that DO want the 403 + * behavior (every /v1/* route except /v1/whoami). */ -async function resolveAuth(req, res, next) { +async function attachAuth(req, res, next) { const authKey = req.get('authKey'); - if (!authKey) { - return res.status(403).json({ message: 'Authorization key not sent.' }); - } - req.authKey = authKey; + req.authKey = authKey || null; + req.isMaster = false; + req.companyId = -1; + if (!authKey) return next(); try { req.isMaster = await isMaster(authKey); } catch (error) { - log.error({ err: error }, 'auth.isMaster failed'); - return res.status(500).json({ message: 'Error!', error: String(error) }); + log.error({ err: error }, 'attachAuth: isMaster failed'); + return res.status(500).json({ message: 'Error!' }); } if (req.isMaster) { - req.companyId = null; // master keys aren't scoped to a single company + // Master keys aren't scoped to a single company. Leave + // companyId at -1; handlers needing a target scope read + // it from req.params / req.body / req.query. return next(); } try { req.companyId = await getCompanyId(authKey); } catch (error) { - log.error({ err: error }, 'auth.getCompanyId failed'); - return res.status(500).json({ message: 'Error!', error: String(error) }); + log.error({ err: error }, 'attachAuth: getCompanyId failed'); + return res.status(500).json({ message: 'Error!' }); + } + return next(); +} + +/** + * Express middleware: 403s requests that aren't authenticated. + * Assumes attachAuth has already run upstream. + * + * - missing authKey header -> 403 "Authorization key not sent." + * - present authKey, not master, no scope -> 403 "Invalid Authorization Key." + * - otherwise -> next() + */ +function requireAuth(req, res, next) { + if (!req.authKey) { + return res.status(403).json({ message: 'Authorization key not sent.' }); } - if (req.companyId === -1) { + if (!req.isMaster && req.companyId === -1) { return res.status(403).json({ message: 'Invalid Authorization Key.' }); } return next(); } +/** + * Combined middleware kept for backward-compat with anywhere it + * was mounted directly. New mounts should use attachAuth + + * requireAuth as two separate middlewares so endpoints can opt + * out of the strict 403 (like /v1/whoami). + */ +async function resolveAuth(req, res, next) { + let attachOk = false; + await new Promise((resolve) => { + attachAuth(req, res, () => { attachOk = true; resolve(); }); + }); + if (!attachOk) return; // attachAuth already sent a 500 + return requireAuth(req, res, next); +} + module.exports = { isMaster, getCompanyId, @@ -236,6 +277,8 @@ module.exports = { getCompanyIdByPovId, getCompanyIdByPohId, requireAuthKey, + attachAuth, + requireAuth, resolveAuth, hashKey, }; diff --git a/app/routers/router.js b/app/routers/router.js index def28e3..7b12739 100644 --- a/app/routers/router.js +++ b/app/routers/router.js @@ -4,6 +4,7 @@ const express = require('express'); const router = express.Router(); const swaggerUi = require('swagger-ui-express'); +const { attachAuth } = require('../middleware/auth.js'); const customer = require('../controllers/customercontroller.js'); const health = require('../controllers/healthcontroller.js'); @@ -46,9 +47,20 @@ const inventoryTransactionSchemas = require('../schemas/inventorytransaction.sch // of the API process and reachability of the database. router.get('/healthz', health.healthz); +// attachAuth runs on every /v1/* request and populates +// req.authKey / req.isMaster / req.companyId without rejecting. +// Existing controllers still have their inline authKey check; once +// they migrate to read req.isMaster / req.companyId, the inline +// check becomes redundant and requireAuth can be mounted globally. +// Until then we keep mounting requireAuth opt-in (currently only +// used to fence /v1/whoami's downstream peers via per-controller +// adoption in follow-up PRs). +router.use('/v1', attachAuth); + // Identity probe — returns what the calling authKey resolves to. -// Useful for SDK clients confirming credentials without firing a -// domain call. Mounted under /v1 because it's authKey-scoped. +// Distinguishes "header missing" (403) from "header present but +// unknown" (200 with authenticated:false) so a strict guard +// middleware here would collapse useful signal. router.get('/v1/whoami', whoami.whoami); // OpenAPI: machine-readable spec at /openapi.json, interactive