From 8e5a2276afe929fa321574db80b99643e1ad5bb0 Mon Sep 17 00:00:00 2001 From: Julian Lam Date: Mon, 20 Jan 2020 10:19:23 -0500 Subject: [PATCH] feat: check flag values on save (assignee and state) (#8122) * feat: add assignee checking when updating flag Prior to this, it was possible to update the assignee to any value (or any user. This commit adds checking to allow only admins, global moderators, or in the case of flagged posts, moderators. Also some prep work was added for value checking `state`. * feat: value checking `state` on flag update The state should be one of the constants defined earlier in the file. --- src/flags.js | 51 ++++++++++++++++------ test/flags.js | 119 +++++++++++++++++++++++++++++++++++--------------- 2 files changed, 122 insertions(+), 48 deletions(-) diff --git a/src/flags.js b/src/flags.js index 489ebbf9d9..2f5264678d 100644 --- a/src/flags.js +++ b/src/flags.js @@ -19,6 +19,16 @@ const utils = require('../public/src/utils'); const Flags = module.exports; +Flags._constants = { + states: ['open', 'wip', 'resolved', 'rejected'], + state_class: { + open: 'info', + wip: 'warning', + resolved: 'success', + rejected: 'danger', + }, +}; + Flags.init = async function () { // Query plugins for custom filter strategies and merge into core filter strategies function prepareSets(sets, orSets, prefix, value) { @@ -162,13 +172,7 @@ Flags.list = async function (filters, uid) { 'icon:text': userObj['icon:text'], }, }; - const stateToLabel = { - open: 'info', - wip: 'warning', - resolved: 'success', - rejected: 'danger', - }; - flagObj.labelClass = stateToLabel[flagObj.state]; + flagObj.labelClass = Flags._constants.state_class[flagObj.state]; return Object.assign(flagObj, { description: validator.escape(String(flagObj.description)), @@ -344,6 +348,7 @@ Flags.getTargetCid = async function (type, id) { }; Flags.update = async function (flagId, uid, changeset) { + const current = await db.getObjectFields('flag:' + flagId, ['state', 'assignee', 'type', 'targetId']); const now = changeset.datetime || Date.now(); const notifyAssignee = async function (assigneeId) { if (assigneeId === '' || parseInt(uid, 10) === parseInt(assigneeId, 10)) { @@ -359,20 +364,40 @@ Flags.update = async function (flagId, uid, changeset) { }); await notifications.push(notifObj, [assigneeId]); }; + const isAssignable = async function (assigneeId) { + let allowed = false; + allowed = await user.isAdminOrGlobalMod(assigneeId); + + // Mods are also allowed to be assigned, if flag target is post in uid's moderated cid + if (!allowed && current.type === 'post') { + const cid = await posts.getCidByPid(current.targetId); + allowed = await user.isModerator(assigneeId, cid); + } + + return allowed; + }; - // Retrieve existing flag data to compare for history-saving purposes - const current = await db.getObjectFields('flag:' + flagId, ['state', 'assignee']); + // Retrieve existing flag data to compare for history-saving/reference purposes const tasks = []; for (var prop in changeset) { if (changeset.hasOwnProperty(prop)) { if (current[prop] === changeset[prop]) { delete changeset[prop]; } else if (prop === 'state') { - tasks.push(db.sortedSetAdd('flags:byState:' + changeset[prop], now, flagId)); - tasks.push(db.sortedSetRemove('flags:byState:' + current[prop], flagId)); + if (!Flags._constants.states.includes(changeset[prop])) { + delete changeset[prop]; + } else { + tasks.push(db.sortedSetAdd('flags:byState:' + changeset[prop], now, flagId)); + tasks.push(db.sortedSetRemove('flags:byState:' + current[prop], flagId)); + } } else if (prop === 'assignee') { - tasks.push(db.sortedSetAdd('flags:byAssignee:' + changeset[prop], now, flagId)); - tasks.push(notifyAssignee(changeset[prop])); + /* eslint-disable-next-line */ + if (!await isAssignable(parseInt(changeset[prop], 10))) { + delete changeset[prop]; + } else { + tasks.push(db.sortedSetAdd('flags:byAssignee:' + changeset[prop], now, flagId)); + tasks.push(notifyAssignee(changeset[prop])); + } } } } diff --git a/test/flags.js b/test/flags.js index b157ba26c0..75e0285412 100644 --- a/test/flags.js +++ b/test/flags.js @@ -13,40 +13,30 @@ var Groups = require('../src/groups'); var Meta = require('../src/meta'); describe('Flags', function () { - before(function (done) { + let uid1; + let uid2; + let uid3; + let category; + before(async () => { // Create some stuff to flag - async.waterfall([ - async.apply(User.create, { username: 'testUser', password: 'abcdef', email: 'b@c.com' }), - function (uid, next) { - Categories.create({ - name: 'test category', - }, function (err, category) { - if (err) { - return done(err); - } + uid1 = await User.create({ username: 'testUser', password: 'abcdef', email: 'b@c.com' }); - Topics.post({ - cid: category.cid, - uid: uid, - title: 'Topic to flag', - content: 'This is flaggable content', - }, next); - }); - }, - function (topicData, next) { - User.create({ - username: 'testUser2', password: 'abcdef', email: 'c@d.com', - }, next); - }, - function (uid, next) { - Groups.join('administrators', uid, next); - }, - function (next) { - User.create({ - username: 'unprivileged', password: 'abcdef', email: 'd@e.com', - }, next); - }, - ], done); + uid2 = await User.create({ username: 'testUser2', password: 'abcdef', email: 'c@d.com' }); + await Groups.join('administrators', uid2); + + category = await Categories.create({ + name: 'test category', + }); + await Topics.post({ + cid: category.cid, + uid: uid1, + title: 'Topic to flag', + content: 'This is flaggable content', + }); + + uid3 = await User.create({ + username: 'unprivileged', password: 'abcdef', email: 'd@e.com', + }); }); describe('.create()', function () { @@ -274,9 +264,9 @@ describe('Flags', function () { describe('.update()', function () { it('should alter a flag\'s various attributes and persist them to the database', function (done) { - Flags.update(1, 1, { + Flags.update(1, uid2, { state: 'wip', - assignee: 1, + assignee: uid2, }, function (err) { assert.ifError(err); db.getObjectFields('flag:1', ['state', 'assignee'], function (err, data) { @@ -286,7 +276,7 @@ describe('Flags', function () { assert.strictEqual('wip', data.state); assert.ok(!isNaN(parseInt(data.assignee, 10))); - assert.strictEqual(1, parseInt(data.assignee, 10)); + assert.strictEqual(uid2, parseInt(data.assignee, 10)); done(); }); }); @@ -313,6 +303,65 @@ describe('Flags', function () { done(); }); }); + + it('should allow assignment if user is an admin and do nothing otherwise', async () => { + await Flags.update(1, uid2, { + assignee: uid2, + }); + let assignee = await db.getObjectField('flag:1', 'assignee'); + assert.strictEqual(uid2, parseInt(assignee, 10)); + + await Flags.update(1, uid2, { + assignee: uid3, + }); + assignee = await db.getObjectField('flag:1', 'assignee'); + assert.strictEqual(uid2, parseInt(assignee, 10)); + }); + + it('should allow assignment if user is a global mod and do nothing otherwise', async () => { + await Groups.join('Global Moderators', uid3); + + await Flags.update(1, uid3, { + assignee: uid3, + }); + let assignee = await db.getObjectField('flag:1', 'assignee'); + assert.strictEqual(uid3, parseInt(assignee, 10)); + + await Flags.update(1, uid3, { + assignee: uid1, + }); + assignee = await db.getObjectField('flag:1', 'assignee'); + assert.strictEqual(uid3, parseInt(assignee, 10)); + + await Groups.leave('Global Moderators', uid3); + }); + + it('should allow assignment if user is a mod of the category, do nothing otherwise', async () => { + await Groups.join('cid:' + category.cid + ':privileges:moderate', uid3); + + await Flags.update(1, uid3, { + assignee: uid3, + }); + let assignee = await db.getObjectField('flag:1', 'assignee'); + assert.strictEqual(uid3, parseInt(assignee, 10)); + + await Flags.update(1, uid3, { + assignee: uid1, + }); + assignee = await db.getObjectField('flag:1', 'assignee'); + assert.strictEqual(uid3, parseInt(assignee, 10)); + + await Groups.leave('cid:' + category.cid + ':privileges:moderate', uid3); + }); + + it('should do nothing when you attempt to set a bogus state', async () => { + await Flags.update(1, uid2, { + state: 'hocus pocus', + }); + + const state = await db.getObjectField('flag:1', 'state'); + assert.strictEqual('wip', state); + }); }); describe('.getTarget()', function () {