Tidy up API controllers (pa11y#88)

We now use Express param callbacks for these, which reduces repeated logic in the routes themselves.
DamickDoni · Feb 21, 2018 · 4afa744 · 4afa744
1 parent 94debea
commit 4afa744
Show file tree

Hide file tree

Showing 7 changed files with 132 additions and 148 deletions.
diff --git a/controller/api-v1/docs.js b/controller/api-v1/docs.js
@@ -8,6 +8,7 @@
  */
 function initDocsController(dashboard, router) {
 
+	// The base path of the API redirects to the documentation
 	router.get('/', (request, response) => {
 		response.redirect('/docs/api/v1');
 	});

diff --git a/controller/api-v1/me.js b/controller/api-v1/me.js
@@ -14,40 +14,49 @@ function initMeController(dashboard, router) {
 	const Key = dashboard.model.Key;
 	const User = dashboard.model.User;
 
-	// Listing and viewing user/keys
+	// Require authentication for all of these routes
+	router.use('/me', requireAuth());
+
+	// Add param callback for current user API key IDs
+	router.param('myKeyId', async (request, response, next, myKeyId) => {
+		try {
+			request.keyFromParam = await Key.fetchOneByIdAndUserId(myKeyId, request.authUser.id);
+			return next(request.keyFromParam ? undefined : httpError(404));
+		} catch (error) {
+			return next(error);
+		}
+	});
 
 	// Get the currently authenticated user
-	router.get('/me', requireAuth(), (request, response) => {
+	router.get('/me', (request, response) => {
 		response.send(request.authUser);
 	});
 
-	// Get the currently authenticated user's keys
-	router.get('/me/keys', requireAuth(), async (request, response, next) => {
+	// Update the currently authenticated user
+	router.patch('/me', express.json(), async (request, response, next) => {
 		try {
-			response.send(await Key.fetchByUserId(request.authUser.id));
+			const user = await User.fetchOneById(request.authUser.id);
+			await user.update({
+				email: request.body.email,
+				password: request.body.password
+			});
+			response.status(200).send(user);
 		} catch (error) {
 			return next(error);
 		}
 	});
 
-	// Get a single currently authenticated user key by ID
-	router.get('/me/keys/:keyId', requireAuth(), async (request, response, next) => {
+	// List the currently authenticated user's keys
+	router.get('/me/keys', async (request, response, next) => {
 		try {
-			const key = await Key.fetchOneByIdAndUserId(request.params.keyId, request.authUser.id);
-			if (!key) {
-				return next();
-			}
-			response.send(key);
+			response.send(await Key.fetchByUserId(request.authUser.id));
 		} catch (error) {
 			return next(error);
 		}
 	});
 
-
-	// Creating keys
-
 	// Create a new key for the currently authenticated user
-	router.post('/me/keys', requireAuth(), express.json(), async (request, response, next) => {
+	router.post('/me/keys', express.json(), async (request, response, next) => {
 		try {
 			const secret = Key.generateSecret();
 			const key = await Key.create({
@@ -64,30 +73,20 @@ function initMeController(dashboard, router) {
 		}
 	});
 
-
-	// Updating users/keys
-
-	// Update the currently authenticated user
-	router.patch('/me', requireAuth(), express.json(), async (request, response, next) => {
+	// Get a single currently authenticated user key by ID
+	router.get('/me/keys/:myKeyId', (request, response, next) => {
 		try {
-			const user = await User.fetchOneById(request.authUser.id);
-			await user.update({
-				email: request.body.email,
-				password: request.body.password
-			});
-			response.status(200).send(user);
+			const key = request.keyFromParam;
+			response.send(key);
 		} catch (error) {
 			return next(error);
 		}
 	});
 
 	// Update a single currently authenticated user key
-	router.patch('/me/keys/:keyId', requireAuth(), express.json(), async (request, response, next) => {
+	router.patch('/me/keys/:myKeyId', express.json(), async (request, response, next) => {
 		try {
-			const key = await Key.fetchOneByIdAndUserId(request.params.keyId, request.authUser.id);
-			if (!key) {
-				return next();
-			}
+			const key = request.keyFromParam;
 			await key.update({
 				description: request.body.description
 			});
@@ -98,15 +97,12 @@ function initMeController(dashboard, router) {
 	});
 
 	// Delete a single currently authenticated user key
-	router.delete('/me/keys/:keyId', requireAuth(), async (request, response, next) => {
+	router.delete('/me/keys/:myKeyId', async (request, response, next) => {
 		try {
-			if (request.authKey && request.params.keyId === request.authKey.id) {
+			const key = request.keyFromParam;
+			if (request.authKey && key.get('id') === request.authKey.id) {
 				return next(httpError(403, 'You are not authorized to delete the key currently being used to authenticate'));
 			}
-			const key = await Key.fetchOneByIdAndUserId(request.params.keyId, request.authUser.id);
-			if (!key) {
-				return next();
-			}
 			await key.destroy();
 			response.status(204).send({});
 		} catch (error) {

diff --git a/controller/api-v1/sites.js b/controller/api-v1/sites.js
@@ -1,7 +1,7 @@
 'use strict';
 
 const express = require('express');
-// const httpError = require('http-errors');
+const httpError = require('http-errors');
 const requirePermission = require('../../lib/middleware/require-permission');
 
 /**
@@ -13,7 +13,17 @@ const requirePermission = require('../../lib/middleware/require-permission');
 function initSitesController(dashboard, router) {
 	const Site = dashboard.model.Site;
 
-	// Get a list of all sites
+	// Add a param callback for site IDs
+	router.param('siteId', async (request, response, next, siteId) => {
+		try {
+			request.siteFromParam = await Site.fetchOneById(siteId);
+			return next(request.siteFromParam ? undefined : httpError(404));
+		} catch (error) {
+			return next(error);
+		}
+	});
+
+	// List all sites
 	router.get('/sites', requirePermission('read'), async (request, response, next) => {
 		try {
 			response.send(await Site.fetchAll());
@@ -40,12 +50,9 @@ function initSitesController(dashboard, router) {
 	});
 
 	// Get a single site by ID
-	router.get('/sites/:siteId', requirePermission('read'), async (request, response, next) => {
+	router.get('/sites/:siteId', requirePermission('read'), (request, response, next) => {
 		try {
-			const site = await Site.fetchOneById(request.params.siteId);
-			if (!site) {
-				return next();
-			}
+			const site = request.siteFromParam;
 			response.send(site);
 		} catch (error) {
 			return next(error);
@@ -55,10 +62,7 @@ function initSitesController(dashboard, router) {
 	// Update a site by ID
 	router.patch('/sites/:siteId', requirePermission('write'), express.json(), async (request, response, next) => {
 		try {
-			const site = await Site.fetchOneById(request.params.siteId);
-			if (!site) {
-				return next();
-			}
+			const site = request.siteFromParam;
 			await site.update({
 				name: request.body.name,
 				base_url: request.body.baseUrl,
@@ -76,10 +80,7 @@ function initSitesController(dashboard, router) {
 	// Delete a site by ID
 	router.delete('/sites/:siteId', requirePermission('delete'), async (request, response, next) => {
 		try {
-			const site = await Site.fetchOneById(request.params.siteId);
-			if (!site) {
-				return next();
-			}
+			const site = request.siteFromParam;
 			await site.destroy();
 			response.status(204).send({});
 		} catch (error) {