refactor: automatically authenticate all requests setup through route helpers (#9357)

* refactor: automatically authenticate all requests setup through route helpers

* fix: removed connect-ensure-login dependency

* fix: bug with some middlewares not defined outside route helper methods

install/package.json

@@ -45,7 +45,6 @@
         "commander": "^7.1.0",
         "compare-versions": "3.6.0",
         "compression": "^1.7.4",
-        "connect-ensure-login": "^0.1.1",
         "connect-flash": "^0.1.1",
         "connect-mongo": "4.3.0",
         "connect-multiparty": "^2.2.0",

src/controllers/index.js

@@ -109,7 +109,7 @@ Controllers.login = async function (req, res) {
 		req.session.returnTo = req.headers['x-return-to'];
 	}
 
-	// Occasionally, x-return-to is passed a full url. Also, connect-ensure-login passes the relative path. Strip both.
+	// Occasionally, x-return-to is passed a full url.
 	req.session.returnTo = req.session.returnTo && req.session.returnTo.replace(nconf.get('base_url'), '').replace(nconf.get('relative_path'), '');
 
 	data.alternate_logins = loginStrategies.length > 0;

src/middleware/index.js

@@ -5,7 +5,6 @@ const path = require('path');
 const csrf = require('csurf');
 const validator = require('validator');
 const nconf = require('nconf');
-const ensureLoggedIn = require('connect-ensure-login');
 const toobusy = require('toobusy-js');
 const LRU = require('lru-cache');
 const util = require('util');
@@ -52,7 +51,13 @@ middleware.applyCSRF = function (req, res, next) {
 };
 middleware.applyCSRFasync = util.promisify(middleware.applyCSRF);
 
-middleware.ensureLoggedIn = ensureLoggedIn.ensureLoggedIn(`${relative_path}/login`);
+middleware.ensureLoggedIn = (req, res, next) => {
+	if (!req.loggedIn) {
+		return controllers.helpers.notAllowed(req, res);
+	}
+
+	setImmediate(next);
+};
 
 Object.assign(middleware, {
 	admin: require('./admin'),

src/middleware/user.js

@@ -83,6 +83,7 @@ module.exports = function (middleware) {
 	}
 
 	middleware.authenticate = helpers.try(async (req, res, next) => {
+		winston.warn(`[middleware] middleware.authenticate has been deprecated, page and API routes are now automatically authenticated via setup(Page|API)Route. Use middleware.authenticateRequest (if not using route helper) and middleware.ensureLoggedIn instead. (request path: ${req.path})`);
 		if (!await authenticate(req, res)) {
 			return;
 		}
@@ -92,7 +93,7 @@ module.exports = function (middleware) {
 		next();
 	});
 
-	middleware.authenticateOrGuest = helpers.try(async (req, res, next) => {
+	middleware.authenticateRequest = helpers.try(async (req, res, next) => {
 		if (!await authenticate(req, res)) {
 			return;
 		}

src/routes/admin.js

@@ -61,15 +61,15 @@ module.exports = function (app, name, middleware, controllers) {
 
 
 function apiRoutes(router, name, middleware, controllers) {
-	router.get(`/api/${name}/users/csv`, middleware.authenticate, helpers.tryRoute(controllers.admin.users.getCSV));
-	router.get(`/api/${name}/groups/:groupname/csv`, middleware.authenticate, helpers.tryRoute(controllers.admin.groups.getCSV));
-	router.get(`/api/${name}/analytics`, middleware.authenticate, helpers.tryRoute(controllers.admin.dashboard.getAnalytics));
-	router.get(`/api/${name}/advanced/cache/dump`, middleware.authenticate, helpers.tryRoute(controllers.admin.cache.dump));
+	router.get(`/api/${name}/users/csv`, middleware.ensureLoggedIn, helpers.tryRoute(controllers.admin.users.getCSV));
+	router.get(`/api/${name}/groups/:groupname/csv`, middleware.ensureLoggedIn, helpers.tryRoute(controllers.admin.groups.getCSV));
+	router.get(`/api/${name}/analytics`, middleware.ensureLoggedIn, helpers.tryRoute(controllers.admin.dashboard.getAnalytics));
+	router.get(`/api/${name}/advanced/cache/dump`, middleware.ensureLoggedIn, helpers.tryRoute(controllers.admin.cache.dump));
 
 	const multipart = require('connect-multiparty');
 	const multipartMiddleware = multipart();
 
-	const middlewares = [multipartMiddleware, middleware.validateFiles, middleware.applyCSRF, middleware.authenticate];
+	const middlewares = [multipartMiddleware, middleware.validateFiles, middleware.applyCSRF, middleware.ensureLoggedIn];
 
 	router.post(`/api/${name}/category/uploadpicture`, middlewares, helpers.tryRoute(controllers.admin.uploads.uploadCategoryPicture));
 	router.post(`/api/${name}/uploadfavicon`, middlewares, helpers.tryRoute(controllers.admin.uploads.uploadFavicon));

src/routes/api.js

@@ -8,7 +8,7 @@ module.exports = function (app, middleware, controllers) {
 	const router = express.Router();
 	app.use('/api', router);
 
-	router.get('/config', middleware.applyCSRF, middleware.authenticateOrGuest, controllers.api.getConfig);
+	router.get('/config', middleware.applyCSRF, middleware.authenticateRequest, controllers.api.getConfig);
 
 	router.get('/self', controllers.user.getCurrentUser);
 	router.get('/user/uid/:uid', middleware.canViewUsers, controllers.user.getUserByUID);
@@ -21,7 +21,7 @@ module.exports = function (app, middleware, controllers) {
 
 	router.get('/categories/:cid/moderators', controllers.api.getModerators);
 	router.get('/recent/posts/:term?', controllers.posts.getRecentPosts);
-	router.get('/unread/total', middleware.authenticate, controllers.unread.unreadTotal);
+	router.get('/unread/total', middleware.authenticateRequest, middleware.ensureLoggedIn, controllers.unread.unreadTotal);
 	router.get('/topic/teaser/:topic_id', controllers.topics.teaser);
 	router.get('/topic/pagination/:topic_id', controllers.topics.pagination);
 
@@ -30,5 +30,5 @@ module.exports = function (app, middleware, controllers) {
 	const middlewares = [middleware.maintenanceMode, multipartMiddleware, middleware.validateFiles, middleware.applyCSRF];
 	router.post('/post/upload', middlewares, uploadsController.uploadPost);
 
-	router.post('/user/:userslug/uploadpicture', middlewares.concat([middleware.exposeUid, middleware.authenticate, middleware.canViewUsers, middleware.checkAccountPermissions]), controllers.accounts.edit.uploadPicture);
+	router.post('/user/:userslug/uploadpicture', middlewares.concat([middleware.exposeUid, middleware.authenticateRequest, middleware.ensureLoggedIn, middleware.canViewUsers, middleware.checkAccountPermissions]), controllers.accounts.edit.uploadPicture);
 };

src/routes/helpers.js

@@ -1,15 +1,17 @@
 'use strict';
 
 const helpers = module.exports;
+const middleware = require('../middleware');
 const controllerHelpers = require('../controllers/helpers');
 
 helpers.setupPageRoute = function (router, name, middleware, middlewares, controller) {
 	middlewares = [
 		middleware.maintenanceMode,
 		middleware.registrationComplete,
-		middleware.pageView,
+		middleware.authenticateRequest,
 		middleware.pluginHooks,
-	].concat(middlewares);
+		middleware.pageView,
+		...middlewares];
 
 	router.get(
 		name,
@@ -28,6 +30,14 @@ helpers.setupAdminPageRoute = function (router, name, middleware, middlewares, c
 };
 
 helpers.setupApiRoute = function (router, verb, name, middlewares, controller) {
+	middlewares = [
+		middleware.maintenanceMode,
+		middleware.registrationComplete,
+		middleware.authenticateRequest,
+		middleware.pluginHooks,
+		...middlewares,
+	];
+
 	router[verb](name, middlewares, helpers.tryRoute(controller, (err, res) => {
 		controllerHelpers.formatApiResponse(400, res, err);
 	}));

src/routes/index.js

@@ -75,7 +75,7 @@ _mounts.category = (app, name, middleware, controllers) => {
 	setupPageRoute(app, '/popular', middleware, [], controllers.popular.get);
 	setupPageRoute(app, '/recent', middleware, [], controllers.recent.get);
 	setupPageRoute(app, '/top', middleware, [], controllers.top.get);
-	setupPageRoute(app, '/unread', middleware, [middleware.authenticate], controllers.unread.get);
+	setupPageRoute(app, '/unread', middleware, [middleware.ensureLoggedIn], controllers.unread.get);
 
 	setupPageRoute(app, `/${name}/:category_id/:slug/:topic_index`, middleware, [], controllers.category.get);
 	setupPageRoute(app, `/${name}/:category_id/:slug?`, middleware, [], controllers.category.get);
@@ -100,7 +100,6 @@ module.exports = async function (app, middleware) {
 	router.render = function (...args) {
 		app.render(...args);
 	};
-	const ensureLoggedIn = require('connect-ensure-login');
 
 	// Allow plugins/themes to mount some routes elsewhere
 	const remountable = ['admin', 'category', 'topic', 'post', 'users', 'user', 'groups', 'tags'];
@@ -125,8 +124,8 @@ module.exports = async function (app, middleware) {
 	});
 
 	router.all('(/+api|/+api/*?)', middleware.prepareAPI);
-	router.all(`(/+api/admin|/+api/admin/*?${mounts.admin !== 'admin' ? `|/+api/${mounts.admin}|/+api/${mounts.admin}/*?` : ''})`, middleware.authenticate, middleware.admin.checkPrivileges);
-	router.all(`(/+admin|/+admin/*?${mounts.admin !== 'admin' ? `|/+${mounts.admin}|/+${mounts.admin}/*?` : ''})`, ensureLoggedIn.ensureLoggedIn(`${nconf.get('relative_path')}/login?local=1`), middleware.applyCSRF, middleware.admin.checkPrivileges);
+	router.all(`(/+api/admin|/+api/admin/*?${mounts.admin !== 'admin' ? `|/+api/${mounts.admin}|/+api/${mounts.admin}/*?` : ''})`, middleware.authenticateRequest, middleware.ensureLoggedIn, middleware.admin.checkPrivileges);
+	router.all(`(/+admin|/+admin/*?${mounts.admin !== 'admin' ? `|/+${mounts.admin}|/+${mounts.admin}/*?` : ''})`, middleware.ensureLoggedIn, middleware.applyCSRF, middleware.admin.checkPrivileges);
 
 	app.use(middleware.stripLeadingSlashes);
 

src/routes/user.js

@@ -52,7 +52,7 @@ module.exports = function (app, name, middleware, controllers) {
 		res.redirect(`${nconf.get('relative_path')}/api/v3/users/${res.locals.uid}/sessions/${req.params.uuid}`);
 	});
 
-	setupPageRoute(app, '/notifications', middleware, [middleware.authenticate], controllers.accounts.notifications.get);
+	setupPageRoute(app, '/notifications', middleware, [middleware.ensureLoggedIn], controllers.accounts.notifications.get);
 	setupPageRoute(app, `/${name}/:userslug/chats/:roomid?`, middleware, middlewares, controllers.accounts.chats.get);
-	setupPageRoute(app, '/chats/:roomid?', middleware, [middleware.authenticate], controllers.accounts.chats.redirectToChat);
+	setupPageRoute(app, '/chats/:roomid?', middleware, [middleware.ensureLoggedIn], controllers.accounts.chats.redirectToChat);
 };

src/routes/write/admin.js

@@ -8,7 +8,7 @@ const routeHelpers = require('../helpers');
 const { setupApiRoute } = routeHelpers;
 
 module.exports = function () {
-	const middlewares = [middleware.authenticate];
+	const middlewares = [middleware.ensureLoggedIn];
 
 	setupApiRoute(router, 'put', '/settings/:setting', [...middlewares, middleware.checkRequired.bind(null, ['value'])], controllers.write.admin.updateSetting);
 

src/routes/write/categories.js

@@ -8,10 +8,10 @@ const routeHelpers = require('../helpers');
 const { setupApiRoute } = routeHelpers;
 
 module.exports = function () {
-	const middlewares = [middleware.authenticate];
+	const middlewares = [middleware.ensureLoggedIn];
 
 	setupApiRoute(router, 'post', '/', [...middlewares, middleware.checkRequired.bind(null, ['name'])], controllers.write.categories.create);
-	setupApiRoute(router, 'get', '/:cid', [middleware.authenticateOrGuest], controllers.write.categories.get);
+	setupApiRoute(router, 'get', '/:cid', [], controllers.write.categories.get);
 	setupApiRoute(router, 'put', '/:cid', [...middlewares], controllers.write.categories.update);
 	setupApiRoute(router, 'delete', '/:cid', [...middlewares], controllers.write.categories.delete);
 

src/routes/write/files.js

@@ -8,7 +8,7 @@ const routeHelpers = require('../helpers');
 const { setupApiRoute } = routeHelpers;
 
 module.exports = function () {
-	const middlewares = [middleware.authenticate];
+	const middlewares = [middleware.ensureLoggedIn];
 
 	// setupApiRoute(router, 'put', '/', [
 	// 	...middlewares,

src/routes/write/groups.js

@@ -8,7 +8,7 @@ const routeHelpers = require('../helpers');
 const { setupApiRoute } = routeHelpers;
 
 module.exports = function () {
-	const middlewares = [middleware.authenticate];
+	const middlewares = [middleware.ensureLoggedIn];
 
 	setupApiRoute(router, 'post', '/', [...middlewares, middleware.checkRequired.bind(null, ['name'])], controllers.write.groups.create);
 	setupApiRoute(router, 'head', '/:slug', [middleware.assert.group], controllers.write.groups.exists);

src/routes/write/index.js

@@ -42,7 +42,7 @@ Write.reload = async (params) => {
 	router.use('/api/v3/utilities', require('./utilities')());
 
 	router.get('/api/v3/ping', writeControllers.utilities.ping.get);
-	router.post('/api/v3/ping', middleware.authenticate, writeControllers.utilities.ping.post);
+	router.post('/api/v3/ping', middleware.authenticateRequest, middleware.ensureLoggedIn, writeControllers.utilities.ping.post);
 
 	/**
 	 * Plugins can add routes to the Write API by attaching a listener to the

src/routes/write/posts.js

@@ -8,9 +8,9 @@ const routeHelpers = require('../helpers');
 const { setupApiRoute } = routeHelpers;
 
 module.exports = function () {
-	const middlewares = [middleware.authenticate];
+	const middlewares = [middleware.ensureLoggedIn];
 
-	setupApiRoute(router, 'get', '/:pid', [middleware.authenticateOrGuest], controllers.write.posts.get);
+	setupApiRoute(router, 'get', '/:pid', [], controllers.write.posts.get);
 	// There is no POST route because you POST to a topic to create a new post. Intuitive, no?
 	setupApiRoute(router, 'put', '/:pid', [...middlewares, middleware.checkRequired.bind(null, ['content'])], controllers.write.posts.edit);
 	setupApiRoute(router, 'delete', '/:pid', [...middlewares, middleware.assert.post], controllers.write.posts.purge);
@@ -26,8 +26,8 @@ module.exports = function () {
 	setupApiRoute(router, 'put', '/:pid/bookmark', [...middlewares, middleware.assert.post], controllers.write.posts.bookmark);
 	setupApiRoute(router, 'delete', '/:pid/bookmark', [...middlewares, middleware.assert.post], controllers.write.posts.unbookmark);
 
-	setupApiRoute(router, 'get', '/:pid/diffs', [middleware.authenticateOrGuest, middleware.assert.post], controllers.write.posts.getDiffs);
-	setupApiRoute(router, 'get', '/:pid/diffs/:since', [middleware.authenticateOrGuest, middleware.assert.post], controllers.write.posts.loadDiff);
+	setupApiRoute(router, 'get', '/:pid/diffs', [middleware.assert.post], controllers.write.posts.getDiffs);
+	setupApiRoute(router, 'get', '/:pid/diffs/:since', [middleware.assert.post], controllers.write.posts.loadDiff);
 	setupApiRoute(router, 'put', '/:pid/diffs/:since', [...middlewares, middleware.assert.post], controllers.write.posts.restoreDiff);
 	setupApiRoute(router, 'delete', '/:pid/diffs/:timestamp', [...middlewares, middleware.assert.post], controllers.write.posts.deleteDiff);
 

src/routes/write/topics.js

@@ -8,14 +8,14 @@ const routeHelpers = require('../helpers');
 const { setupApiRoute } = routeHelpers;
 
 module.exports = function () {
-	const middlewares = [middleware.authenticate];
+	const middlewares = [middleware.ensureLoggedIn];
 
 	const multipart = require('connect-multiparty');
 	const multipartMiddleware = multipart();
 
-	setupApiRoute(router, 'post', '/', [middleware.authenticateOrGuest, middleware.checkRequired.bind(null, ['cid', 'title', 'content'])], controllers.write.topics.create);
-	setupApiRoute(router, 'get', '/:tid', [middleware.authenticateOrGuest], controllers.write.topics.get);
-	setupApiRoute(router, 'post', '/:tid', [middleware.authenticateOrGuest, middleware.checkRequired.bind(null, ['content']), middleware.assert.topic], controllers.write.topics.reply);
+	setupApiRoute(router, 'post', '/', [middleware.checkRequired.bind(null, ['cid', 'title', 'content'])], controllers.write.topics.create);
+	setupApiRoute(router, 'get', '/:tid', [], controllers.write.topics.get);
+	setupApiRoute(router, 'post', '/:tid', [middleware.checkRequired.bind(null, ['content']), middleware.assert.topic], controllers.write.topics.reply);
 	setupApiRoute(router, 'delete', '/:tid', [...middlewares], controllers.write.topics.purge);
 
 	setupApiRoute(router, 'put', '/:tid/state', [...middlewares], controllers.write.topics.restore);
@@ -35,13 +35,13 @@ module.exports = function () {
 	setupApiRoute(router, 'put', '/:tid/tags', [...middlewares, middleware.checkRequired.bind(null, ['tags']), middleware.assert.topic], controllers.write.topics.addTags);
 	setupApiRoute(router, 'delete', '/:tid/tags', [...middlewares, middleware.assert.topic], controllers.write.topics.deleteTags);
 
-	setupApiRoute(router, 'get', '/:tid/thumbs', middleware.authenticateOrGuest, controllers.write.topics.getThumbs);
+	setupApiRoute(router, 'get', '/:tid/thumbs', [], controllers.write.topics.getThumbs);
 	setupApiRoute(router, 'post', '/:tid/thumbs', [multipartMiddleware, middleware.validateFiles, ...middlewares], controllers.write.topics.addThumb);
 	setupApiRoute(router, 'put', '/:tid/thumbs', [...middlewares, middleware.checkRequired.bind(null, ['tid'])], controllers.write.topics.migrateThumbs);
 	setupApiRoute(router, 'delete', '/:tid/thumbs', [...middlewares, middleware.checkRequired.bind(null, ['path'])], controllers.write.topics.deleteThumb);
 	setupApiRoute(router, 'put', '/:tid/thumbs/order', [...middlewares, middleware.checkRequired.bind(null, ['path', 'order'])], controllers.write.topics.reorderThumbs);
 
-	setupApiRoute(router, 'get', '/:tid/events', [middleware.authenticateOrGuest, middleware.assert.topic], controllers.write.topics.getEvents);
+	setupApiRoute(router, 'get', '/:tid/events', [middleware.assert.topic], controllers.write.topics.getEvents);
 
 	return router;
 };

src/routes/write/users.js

@@ -13,7 +13,7 @@ function guestRoutes() {
 }
 
 function authenticatedRoutes() {
-	const middlewares = [middleware.authenticate];
+	const middlewares = [middleware.ensureLoggedIn];
 
 	setupApiRoute(router, 'post', '/', [...middlewares, middleware.checkRequired.bind(null, ['username'])], controllers.write.users.create);
 	setupApiRoute(router, 'delete', '/', [...middlewares, middleware.checkRequired.bind(null, ['uids'])], controllers.write.users.deleteMany);