refactor: use routePrefixMap instead of routeRegexpMap, +tests (#10035)

* refactor: use routePrefixMap instead of routeRegexpMap, +tests

Currently tests fail because privilege pages resolve if passed garbage... hmm

* fix: priv check paths

remove /v3 from path as well

Co-authored-by: Barış Soner Uşaklı <barisusakli@gmail.com>

src/middleware/admin.js

@@ -117,7 +117,7 @@ middleware.checkPrivileges = helpers.try(async (req, res, next) => {
 	}
 
 	// Otherwise, check for privilege based on page (if not in mapping, deny access)
-	const path = req.path.replace(/^(\/api)?\/admin\/?/g, '');
+	const path = req.path.replace(/^(\/api)?(\/v3)?\/admin\/?/g, '');
 	if (path) {
 		const privilege = privileges.admin.resolve(path);
 		if (!await privileges.admin.can(privilege, req.uid)) {

src/privileges/admin.js

@@ -61,13 +61,13 @@ privsAdmin.routeMap = {
 	'extend/widgets': 'admin:settings',
 	'extend/rewards': 'admin:settings',
 };
-privsAdmin.routeRegexpMap = {
+privsAdmin.routePrefixMap = {
-	'^manage/categories/\\d+': 'admin:categories',
+	'manage/categories/': 'admin:categories',
-	'^manage/privileges/(\\d+|admin)': 'admin:privileges',
+	'manage/privileges/': 'admin:privileges',
-	'^manage/groups/.+$': 'admin:groups',
+	'manage/groups/': 'admin:groups',
-	'^settings/[\\w\\-]+$': 'admin:settings',
+	'settings/': 'admin:settings',
-	'^appearance/[\\w]+$': 'admin:settings',
+	'appearance/': 'admin:settings',
-	'^plugins/[\\w\\-]+$': 'admin:settings',
+	'plugins/': 'admin:settings',
 };
 
 // Mapping for socket call methods to a privilege
@@ -120,16 +120,8 @@ privsAdmin.resolve = (path) => {
 		return privsAdmin.routeMap[path];
 	}
 
-	let privilege;
+	const found = Object.entries(privsAdmin.routePrefixMap).find(entry => path.startsWith(entry[0]));
-	Object.keys(privsAdmin.routeRegexpMap).forEach((regexp) => {
+	return found ? found[1] : undefined;
-		if (!privilege) {
-			if (new RegExp(regexp).test(path)) {
-				privilege = privsAdmin.routeRegexpMap[regexp];
-			}
-		}
-	});
-
-	return privilege;
 };
 
 privsAdmin.list = async function (uid) {

test/controllers-admin.js

@@ -811,6 +811,7 @@ describe('Admin Controllers', () => {
 			userJar = (await helpers.loginUser('regularjoe', 'barbar')).jar;
 		});
 
+		describe('routeMap parsing', () => {
 			it('should allow normal user access to admin pages', async function () {
 				this.timeout(50000);
 				function makeRequest(url) {
@@ -848,4 +849,33 @@ describe('Admin Controllers', () => {
 				}
 			});
 		});
+
+		describe('routePrefixMap parsing', () => {
+			it('should allow normal user access to admin pages', async () => {
+				// this.timeout(50000);
+				function makeRequest(url) {
+					return new Promise((resolve, reject) => {
+						process.stdout.write(`calling ${url} `);
+						request(url, { jar: userJar, json: true }, (err, res, body) => {
+							process.stdout.write(`got ${res.statusCode}\n`);
+							if (err) reject(err);
+							else resolve(res);
+						});
+					});
+				}
+				for (const route of Object.keys(privileges.admin.routePrefixMap)) {
+					/* eslint-disable no-await-in-loop */
+					await privileges.admin.rescind([privileges.admin.routePrefixMap[route]], uid);
+					let res = await makeRequest(`${nconf.get('url')}/api/admin/${route}foobar/derp`);
+					assert.strictEqual(res.statusCode, 403);
+
+					await privileges.admin.give([privileges.admin.routePrefixMap[route]], uid);
+					res = await makeRequest(`${nconf.get('url')}/api/admin/${route}foobar/derp`);
+					assert.strictEqual(res.statusCode, 404);
+
+					await privileges.admin.rescind([privileges.admin.routePrefixMap[route]], uid);
+				}
+			});
+		});
+	});
 });

test/middleware.js

@@ -8,10 +8,12 @@ const groups = require('../src/groups');
 
 describe('Middlewares', () => {
 	let adminUid;
+
 	before(async () => {
 		adminUid = await user.create({ username: 'admin', password: '123456' });
 		await groups.join('administrators', adminUid);
 	});
+
 	describe('expose', () => {
 		it('should expose res.locals.isAdmin = false', (done) => {
 			const resMock = { locals: {} };