Nemcio/NodeBB
1
0
Fork 0
mirror of https://github.com/NodeBB/NodeBB.git synced 2026-02-26 16:41:21 +01:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
* 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>
This commit is contained in:
Julian Lam
2021-11-26 13:53:53 -05:00
committed by GitHub
parent d378bf4c9c
commit 6c07433dea
4 changed files with 71 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
src/middleware/admin.js
View File

@@ -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) // 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) { if (path) {
const privilege = privileges.admin.resolve(path); const privilege = privileges.admin.resolve(path);
if (!await privileges.admin.can(privilege, req.uid)) { if (!await privileges.admin.can(privilege, req.uid)) {

26
src/privileges/admin.js
View File

@@ -61,13 +61,13 @@ privsAdmin.routeMap = {
'extend/widgets': 'admin:settings', 'extend/widgets': 'admin:settings',
'extend/rewards': '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 // Mapping for socket call methods to a privilege
@@ -120,16 +120,8 @@ privsAdmin.resolve = (path) => {
return privsAdmin.routeMap[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) { privsAdmin.list = async function (uid) {

88
test/controllers-admin.js
View File

@@ -811,41 +811,71 @@ describe('Admin Controllers', () => {
userJar = (await helpers.loginUser('regularjoe', 'barbar')).jar; userJar = (await helpers.loginUser('regularjoe', 'barbar')).jar;
}); });
it('should allow normal user access to admin pages', async function () { describe('routeMap parsing', () => {
this.timeout(50000); it('should allow normal user access to admin pages', async function () {
function makeRequest(url) { this.timeout(50000);
return new Promise((resolve, reject) => { function makeRequest(url) {
request(url, { jar: userJar, json: true }, (err, res, body) => { return new Promise((resolve, reject) => {
if (err) reject(err); request(url, { jar: userJar, json: true }, (err, res, body) => {
else resolve(res); if (err) reject(err);
else resolve(res);
});
}); });
}); }
} for (const route of Object.keys(privileges.admin.routeMap)) {
for (const route of Object.keys(privileges.admin.routeMap)) { /* eslint-disable no-await-in-loop */
/* eslint-disable no-await-in-loop */ await privileges.admin.rescind([privileges.admin.routeMap[route]], uid);
await privileges.admin.rescind([privileges.admin.routeMap[route]], uid); let res = await makeRequest(`${nconf.get('url')}/api/admin/${route}`);
let res = await makeRequest(`${nconf.get('url')}/api/admin/${route}`); assert.strictEqual(res.statusCode, 403);
assert.strictEqual(res.statusCode, 403);
await privileges.admin.give([privileges.admin.routeMap[route]], uid); await privileges.admin.give([privileges.admin.routeMap[route]], uid);
res = await makeRequest(`${nconf.get('url')}/api/admin/${route}`); res = await makeRequest(`${nconf.get('url')}/api/admin/${route}`);
assert.strictEqual(res.statusCode, 200); assert.strictEqual(res.statusCode, 200);
await privileges.admin.rescind([privileges.admin.routeMap[route]], uid); await privileges.admin.rescind([privileges.admin.routeMap[route]], uid);
} }
for (const route of Object.keys(privileges.admin.routeMap)) { for (const route of Object.keys(privileges.admin.routeMap)) {
/* eslint-disable no-await-in-loop */ /* eslint-disable no-await-in-loop */
await privileges.admin.rescind([privileges.admin.routeMap[route]], uid); await privileges.admin.rescind([privileges.admin.routeMap[route]], uid);
let res = await makeRequest(`${nconf.get('url')}/api/admin`); let res = await makeRequest(`${nconf.get('url')}/api/admin`);
assert.strictEqual(res.statusCode, 403); assert.strictEqual(res.statusCode, 403);
await privileges.admin.give([privileges.admin.routeMap[route]], uid); await privileges.admin.give([privileges.admin.routeMap[route]], uid);
res = await makeRequest(`${nconf.get('url')}/api/admin`); res = await makeRequest(`${nconf.get('url')}/api/admin`);
assert.strictEqual(res.statusCode, 200); assert.strictEqual(res.statusCode, 200);
await privileges.admin.rescind([privileges.admin.routeMap[route]], uid); await privileges.admin.rescind([privileges.admin.routeMap[route]], uid);
} }
});
});
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);
}
});
}); });
}); });
}); });

2
test/middleware.js
View File

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