From 7fcda084ab71a2f664dc6bc31fb095d371f343a9 Mon Sep 17 00:00:00 2001
From: Tom Crasset <25140344+tcrasset@users.noreply.github.com>
Date: Fri, 16 Aug 2024 22:41:22 +0200
Subject: [PATCH] refactor user validation into middleware (#422)

---
 package.json                         |  1 +
 src/app-account.js                   |  4 +--
 src/app-gocardless/app-gocardless.js | 10 ++----
 src/app-secrets.js                   | 10 ++----
 src/app-sync.js                      | 48 ++--------------------------
 src/util/error-middleware.js         | 10 ------
 src/util/middlewares.js              | 27 ++++++++++++++++
 upcoming-release-notes/422.md        |  6 ++++
 8 files changed, 41 insertions(+), 75 deletions(-)
 delete mode 100644 src/util/error-middleware.js
 create mode 100644 src/util/middlewares.js
 create mode 100644 upcoming-release-notes/422.md

diff --git a/package.json b/package.json
index 9cce1858d..2197ea5ed 100644
--- a/package.json
+++ b/package.json
@@ -7,6 +7,7 @@
   "scripts": {
     "start": "node app",
     "lint": "eslint . --max-warnings 0",
+    "lint:fix": "eslint . --fix",
     "build": "tsc",
     "test": "NODE_ENV=test NODE_OPTIONS='--experimental-vm-modules --trace-warnings' jest  --coverage",
     "db:migrate": "NODE_ENV=development node src/run-migrations.js up",
diff --git a/src/app-account.js b/src/app-account.js
index 78c7524a4..ed9ee636c 100644
--- a/src/app-account.js
+++ b/src/app-account.js
@@ -1,5 +1,5 @@
 import express from 'express';
-import errorMiddleware from './util/error-middleware.js';
+import { errorMiddleware } from './util/middlewares.js';
 import validateUser, { validateAuthHeader } from './util/validate-user.js';
 import {
   bootstrap,
@@ -96,5 +96,3 @@ app.get('/validate', (req, res) => {
     res.send({ status: 'ok', data: { validated: true } });
   }
 });
-
-app.use(errorMiddleware);
diff --git a/src/app-gocardless/app-gocardless.js b/src/app-gocardless/app-gocardless.js
index 36e93842e..d59b775c4 100644
--- a/src/app-gocardless/app-gocardless.js
+++ b/src/app-gocardless/app-gocardless.js
@@ -11,7 +11,7 @@ import {
 } from './errors.js';
 import { handleError } from './util/handle-error.js';
 import { sha256String } from '../util/hash.js';
-import validateUser from '../util/validate-user.js';
+import { validateUserMiddleware } from '../util/middlewares.js';
 
 const app = express();
 app.get('/link', function (req, res) {
@@ -20,13 +20,7 @@ app.get('/link', function (req, res) {
 
 export { app as handlers };
 app.use(express.json());
-app.use(async (req, res, next) => {
-  let user = await validateUser(req, res);
-  if (!user) {
-    return;
-  }
-  next();
-});
+app.use(validateUserMiddleware);
 
 app.post('/status', async (req, res) => {
   res.send({
diff --git a/src/app-secrets.js b/src/app-secrets.js
index 1a6942e58..0fc9461d3 100644
--- a/src/app-secrets.js
+++ b/src/app-secrets.js
@@ -1,19 +1,13 @@
 import express from 'express';
-import validateUser from './util/validate-user.js';
 import { secretsService } from './services/secrets-service.js';
+import { validateUserMiddleware } from './util/middlewares.js';
 
 const app = express();
 
 export { app as handlers };
 app.use(express.json());
 
-app.use(async (req, res, next) => {
-  let user = await validateUser(req, res);
-  if (!user) {
-    return;
-  }
-  next();
-});
+app.use(validateUserMiddleware);
 
 app.post('/', async (req, res) => {
   const { name, value } = req.body;
diff --git a/src/app-sync.js b/src/app-sync.js
index 7bad89079..839f5e6c5 100644
--- a/src/app-sync.js
+++ b/src/app-sync.js
@@ -2,8 +2,7 @@ import fs from 'node:fs/promises';
 import { Buffer } from 'node:buffer';
 import express from 'express';
 import * as uuid from 'uuid';
-import validateUser from './util/validate-user.js';
-import errorMiddleware from './util/error-middleware.js';
+import { errorMiddleware, validateUserMiddleware } from './util/middlewares.js';
 import getAccountDb from './account-db.js';
 import { getPathForUserFile, getPathForGroupFile } from './util/paths.js';
 
@@ -16,6 +15,7 @@ app.use(errorMiddleware);
 app.use(express.json());
 app.use(express.raw({ type: 'application/actual-sync' }));
 
+app.use(validateUserMiddleware);
 export { app as handlers };
 
 const OK_RESPONSE = { status: 'ok' };
@@ -27,11 +27,6 @@ const OK_RESPONSE = { status: 'ok' };
 const SYNC_FORMAT_VERSION = 2;
 
 app.post('/sync', async (req, res) => {
-  let user = validateUser(req, res);
-  if (!user) {
-    return;
-  }
-
   let requestPb;
   try {
     requestPb = SyncProtoBuf.SyncRequest.deserializeBinary(req.body);
@@ -126,11 +121,6 @@ app.post('/sync', async (req, res) => {
 });
 
 app.post('/user-get-key', (req, res) => {
-  let user = validateUser(req, res);
-  if (!user) {
-    return;
-  }
-
   let accountDb = getAccountDb();
   let { fileId } = req.body;
 
@@ -153,10 +143,6 @@ app.post('/user-get-key', (req, res) => {
 });
 
 app.post('/user-create-key', (req, res) => {
-  let user = validateUser(req, res);
-  if (!user) {
-    return;
-  }
   let accountDb = getAccountDb();
   let { fileId, keyId, keySalt, testContent } = req.body;
 
@@ -169,10 +155,6 @@ app.post('/user-create-key', (req, res) => {
 });
 
 app.post('/reset-user-file', async (req, res) => {
-  let user = validateUser(req, res);
-  if (!user) {
-    return;
-  }
   let accountDb = getAccountDb();
   let { fileId } = req.body;
 
@@ -199,11 +181,6 @@ app.post('/reset-user-file', async (req, res) => {
 });
 
 app.post('/upload-user-file', async (req, res) => {
-  let user = validateUser(req, res);
-  if (!user) {
-    return;
-  }
-
   let accountDb = getAccountDb();
   if (typeof req.headers['x-actual-name'] !== 'string') {
     res.status(400).send('single x-actual-name is required');
@@ -293,10 +270,6 @@ app.post('/upload-user-file', async (req, res) => {
 });
 
 app.get('/download-user-file', async (req, res) => {
-  let user = validateUser(req, res);
-  if (!user) {
-    return;
-  }
   let accountDb = getAccountDb();
   let fileId = req.headers['x-actual-file-id'];
   if (typeof fileId !== 'string') {
@@ -319,10 +292,6 @@ app.get('/download-user-file', async (req, res) => {
 });
 
 app.post('/update-user-filename', (req, res) => {
-  let user = validateUser(req, res);
-  if (!user) {
-    return;
-  }
   let accountDb = getAccountDb();
   let { fileId, name } = req.body;
 
@@ -342,11 +311,6 @@ app.post('/update-user-filename', (req, res) => {
 });
 
 app.get('/list-user-files', (req, res) => {
-  let user = validateUser(req, res);
-  if (!user) {
-    return;
-  }
-
   let accountDb = getAccountDb();
   let rows = accountDb.all('SELECT * FROM files');
 
@@ -365,10 +329,6 @@ app.get('/list-user-files', (req, res) => {
 });
 
 app.get('/get-user-file-info', (req, res) => {
-  let user = validateUser(req, res);
-  if (!user) {
-    return;
-  }
   let accountDb = getAccountDb();
   let fileId = req.headers['x-actual-file-id'];
 
@@ -397,10 +357,6 @@ app.get('/get-user-file-info', (req, res) => {
 });
 
 app.post('/delete-user-file', (req, res) => {
-  let user = validateUser(req, res);
-  if (!user) {
-    return;
-  }
   let accountDb = getAccountDb();
   let { fileId } = req.body;
 
diff --git a/src/util/error-middleware.js b/src/util/error-middleware.js
deleted file mode 100644
index 311362080..000000000
--- a/src/util/error-middleware.js
+++ /dev/null
@@ -1,10 +0,0 @@
-/**
- * @param {Error} err
- * @param {import('express').Request} req
- * @param {import('express').Response} res
- * @param {import('express').NextFunction} _next
- */
-export default async function middleware(err, req, res, _next) {
-  console.log('ERROR', err);
-  res.status(500).send({ status: 'error', reason: 'internal-error' });
-}
diff --git a/src/util/middlewares.js b/src/util/middlewares.js
new file mode 100644
index 000000000..43b7402ba
--- /dev/null
+++ b/src/util/middlewares.js
@@ -0,0 +1,27 @@
+import validateUser from './validate-user.js';
+
+/**
+ * @param {Error} err
+ * @param {import('express').Request} req
+ * @param {import('express').Response} res
+ * @param {import('express').NextFunction} _next
+ */
+async function errorMiddleware(err, req, res, _next) {
+  console.log('ERROR', err);
+  res.status(500).send({ status: 'error', reason: 'internal-error' });
+}
+
+/**
+ * @param {import('express').Request} req
+ * @param {import('express').Response} res
+ * @param {import('express').NextFunction} next
+ */
+const validateUserMiddleware = async (req, res, next) => {
+  let user = await validateUser(req, res);
+  if (!user) {
+    return;
+  }
+  next();
+};
+
+export { validateUserMiddleware, errorMiddleware };
diff --git a/upcoming-release-notes/422.md b/upcoming-release-notes/422.md
new file mode 100644
index 000000000..a07140621
--- /dev/null
+++ b/upcoming-release-notes/422.md
@@ -0,0 +1,6 @@
+---
+category: Maintenance
+authors: [tcrasset]
+---
+
+Refactor user validation into middleware