From 8b71175c4658df5c24fb95b59f83928698c1f540 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 4 Dec 2017 09:27:56 -0700 Subject: [PATCH 1/3] adjusted logic flow for comment flags --- graph/mutators/action.js | 107 ++++++++++++++++++++++++++++----------- 1 file changed, 77 insertions(+), 30 deletions(-) diff --git a/graph/mutators/action.js b/graph/mutators/action.js index 93806ced9..1ef9014a8 100644 --- a/graph/mutators/action.js +++ b/graph/mutators/action.js @@ -1,28 +1,72 @@ -const ActionsService = require('../../services/actions'); -const UsersService = require('../../services/users'); const errors = require('../../errors'); const {CREATE_ACTION, DELETE_ACTION} = require('../../perms/constants'); +/** + * getActionItem will return the item that is associated with the given action. + * If it does not exist, it will throw an error. + * + * @param {Object} ctx the graphql context for the request + * @param {Object} action the action being performed + * @return {Promise} resolves to the referenced item + */ +const getActionItem = async (ctx, {item_id, item_type}) => { + const { + loaders: { + Comments, + Users, + }, + } = ctx; + + if (item_type === 'COMMENTS') { + const comment = await Comments.get.load(item_id); + if (!comment) { + throw errors.ErrNotFound; + } + + return comment; + } else if (item_type === 'USERS') { + const user = await Users.getByID.load(item_id); + if (!user) { + throw errors.ErrNotFound; + } + + return user; + } +}; + /** * Creates an action on a item. If the item is a user flag, sets the user's status to * pending. - * @param {Object} user the user performing the request - * @param {String} item_id id of the item to add the action to - * @param {String} item_type type of the item - * @param {String} action_type type of the action - * @return {Promise} resolves to the action created + * + * @param {Object} ctx the graphql context for the request + * @param {Object} action the action being created + * @return {Promise} resolves to the action created */ -const createAction = async ({user = {}, pubsub, loaders: {Comments}}, {item_id, item_type, action_type, group_id, metadata = {}}) => { +const createAction = async (ctx, {item_id, item_type, action_type, group_id, metadata = {}}) => { + const { + user = {}, + pubsub, + connectors: { + services: { + Actions, + }, + }, + } = ctx; - let comment; - if (item_type === 'COMMENTS') { - comment = await Comments.get.load(item_id); - if (!comment) { - throw new Error('Comment not found'); + // Gets the item referenced by the action. + const item = await getActionItem(ctx, {item_id, item_type}); + + if (action_type === 'FLAG' && item_type === 'USERS') { + + // The item is a user, and this is a flag. Check to see if they are staff, + // if they are, don't permit the flag. + if (item.isStaff()) { + throw errors.ErrNotAuthorized; } } - let action = await ActionsService.create({ + // Create the action itself. + let action = await Actions.create({ item_id, item_type, user_id: user.id, @@ -31,17 +75,11 @@ const createAction = async ({user = {}, pubsub, loaders: {Comments}}, {item_id, metadata }); - if (item_type === 'USERS' && action_type === 'FLAG') { + if (action_type === 'FLAG' && item_type === 'COMMENTS') { - // Set the user as pending if it was a user flag and user has no Admin, Staff or Moderation roles - let user = await UsersService.findById(item_id); - if(!user.isStaff()){ - await UsersService.setStatus(item_id, 'PENDING'); - } - } - - if (comment) { - pubsub.publish('commentFlagged', comment); + // The item is a comment, and this is a flag. Push that the comment was + // flagged, don't wait for it to finish. + pubsub.publish('commentFlagged', item); } return action; @@ -53,16 +91,25 @@ const createAction = async ({user = {}, pubsub, loaders: {Comments}}, {item_id, * @param {String} id the id of the action to delete * @return {Promise} resolves to the deleted action, or null if not found. */ -const deleteAction = ({user}, {id}) => { - return ActionsService.delete({id, user_id: user.id}); +const deleteAction = (ctx, {id}) => { + const { + user, + connectors: { + services: { + Actions, + }, + }, + } = ctx; + + return Actions.delete({id, user_id: user.id}); }; -module.exports = (context) => { - if (context.user && context.user.can(CREATE_ACTION, DELETE_ACTION)) { +module.exports = (ctx) => { + if (ctx.user && ctx.user.can(CREATE_ACTION, DELETE_ACTION)) { return { Action: { - create: (action) => createAction(context, action), - delete: (action) => deleteAction(context, action) + create: (action) => createAction(ctx, action), + delete: (action) => deleteAction(ctx, action) } }; } From 87ef0f1cb9d235c9db610ea5c93eacd706c3b3a4 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 4 Dec 2017 12:17:39 -0700 Subject: [PATCH 2/3] Adjusted flag beheviour for staff --- config.js | 7 +++++- docs/_docs/02-02-advanced-configuration.md | 5 ++++ graph/mutators/action.js | 17 ++++++++++---- graph/mutators/comment.js | 16 ++++++++++--- plugin-api/beta/server/getReactionConfig.js | 26 +++++++++++---------- 5 files changed, 50 insertions(+), 21 deletions(-) diff --git a/config.js b/config.js index 4601683c1..3e167b294 100644 --- a/config.js +++ b/config.js @@ -175,7 +175,12 @@ const CONFIG = { DISABLE_AUTOFLAG_SUSPECT_WORDS: process.env.TALK_DISABLE_AUTOFLAG_SUSPECT_WORDS === 'TRUE', // TRUST_THRESHOLDS defines the thresholds used for automoderation. - TRUST_THRESHOLDS: process.env.TRUST_THRESHOLDS || 'comment:2,-1;flag:2,-1' + TRUST_THRESHOLDS: process.env.TRUST_THRESHOLDS || 'comment:2,-1;flag:2,-1', + + // IGNORE_FLAGS_AGAINST_STAFF disables staff members from entering the + // reported queue from comments after this was enabled and from reports + // against the staff members user account. + IGNORE_FLAGS_AGAINST_STAFF: process.env.TALK_DISABLE_IGNORE_FLAGS_AGAINST_STAFF === 'TRUE', }; //============================================================================== diff --git a/docs/_docs/02-02-advanced-configuration.md b/docs/_docs/02-02-advanced-configuration.md index 74099258d..94a6b402d 100644 --- a/docs/_docs/02-02-advanced-configuration.md +++ b/docs/_docs/02-02-advanced-configuration.md @@ -456,3 +456,8 @@ Could be read as: added back to the queue. - At the moment of writing, behavior is not attached to the flagging reliability, but it is recorded. + +## TALK_DISABLE_IGNORE_FLAGS_AGAINST_STAFF + +When `TRUE`, staff members will have their accounts and comments moderated the +same as any other user in the system. (Default `FALSE`) \ No newline at end of file diff --git a/graph/mutators/action.js b/graph/mutators/action.js index 1ef9014a8..29a6e67af 100644 --- a/graph/mutators/action.js +++ b/graph/mutators/action.js @@ -1,5 +1,8 @@ const errors = require('../../errors'); const {CREATE_ACTION, DELETE_ACTION} = require('../../perms/constants'); +const { + IGNORE_FLAGS_AGAINST_STAFF, +} = require('../../config'); /** * getActionItem will return the item that is associated with the given action. @@ -56,12 +59,16 @@ const createAction = async (ctx, {item_id, item_type, action_type, group_id, met // Gets the item referenced by the action. const item = await getActionItem(ctx, {item_id, item_type}); - if (action_type === 'FLAG' && item_type === 'USERS') { + // If we are ignoring flags against staff, ensure that the target isn't a + // staff member. + if (IGNORE_FLAGS_AGAINST_STAFF) { + if (action_type === 'FLAG') { - // The item is a user, and this is a flag. Check to see if they are staff, - // if they are, don't permit the flag. - if (item.isStaff()) { - throw errors.ErrNotAuthorized; + // If the item is a user, and this is a flag. Check to see if they are + // staff, if they are, don't permit the flag. + if (item_type === 'USERS' && item.isStaff()) { + return null; + } } } diff --git a/graph/mutators/comment.js b/graph/mutators/comment.js index de3345257..bd8ab18fd 100644 --- a/graph/mutators/comment.js +++ b/graph/mutators/comment.js @@ -19,7 +19,8 @@ const { } = require('../../perms/constants'); const { - DISABLE_AUTOFLAG_SUSPECT_WORDS + DISABLE_AUTOFLAG_SUSPECT_WORDS, + IGNORE_FLAGS_AGAINST_STAFF, } = require('../../config'); const debug = require('debug')('talk:graph:mutators:tags'); @@ -292,7 +293,7 @@ const moderationPhases = [ } }, - // This phase checks to see if the comment's length exeeds maximum. + // This phase checks to see if the comment's length exceeds maximum. (context, comment, {assetSettings: {charCountEnable, charCount}}) => { // Reject if the comment is too long @@ -313,6 +314,15 @@ const moderationPhases = [ } }, + // If a given user is a staff member, always approve their comment. + (context) => { + if (IGNORE_FLAGS_AGAINST_STAFF && context.user && context.user.isStaff()) { + return { + status: 'ACCEPTED', + }; + } + }, + // This phase checks the comment if it has any links in it if the check is // enabled. (context, comment, {assetSettings: {premodLinksEnable}}) => { @@ -362,7 +372,7 @@ const moderationPhases = [ } }, - // This phase checks to see if the comment was already perscribed a status. + // This phase checks to see if the comment was already prescribed a status. (context, comment) => { // If the status was already defined, don't redefine it. It's only defined diff --git a/plugin-api/beta/server/getReactionConfig.js b/plugin-api/beta/server/getReactionConfig.js index 481990c16..2fffb4d1c 100644 --- a/plugin-api/beta/server/getReactionConfig.js +++ b/plugin-api/beta/server/getReactionConfig.js @@ -157,10 +157,22 @@ function getReactionConfig(reaction) { RootMutation: { [`create${Reaction}Action`]: async (_, {input: {item_id}}, {mutators: {Action}, pubsub, loaders: {Comments}}) => { const comment = await Comments.get.load(item_id); + if (!comment) { + throw errors.ErrNotFound; + } - let action; try { - action = await Action.create({item_id, item_type: 'COMMENTS', action_type: REACTION}); + const action = await Action.create({item_id, item_type: 'COMMENTS', action_type: REACTION}); + + if (pubsub) { + + // The comment is needed to allow better filtering e.g. by asset_id. + pubsub.publish(`${reaction}ActionCreated`, {action, comment}); + } + + return { + [reaction]: action, + }; } catch (err) { if (err instanceof errors.ErrAlreadyExists) { return err.metadata.existing; @@ -168,16 +180,6 @@ function getReactionConfig(reaction) { throw err; } - - if (pubsub) { - - // The comment is needed to allow better filtering e.g. by asset_id. - pubsub.publish(`${reaction}ActionCreated`, {action, comment}); - } - - return { - [reaction]: action, - }; }, [`delete${Reaction}Action`]: async (_, {input: {id}}, {mutators: {Action}, pubsub, loaders: {Comments}}) => { const action = await Action.delete({id}); From 3b4e69f06e9d84be53c08c96c09107b5646537d1 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 4 Dec 2017 14:06:32 -0700 Subject: [PATCH 3/3] fixed based on e2e finding a bug! --- graph/mutators/action.js | 15 +++++++++++---- services/users.js | 40 ++++++++++++++++++++++++++++------------ 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/graph/mutators/action.js b/graph/mutators/action.js index 29a6e67af..06417844b 100644 --- a/graph/mutators/action.js +++ b/graph/mutators/action.js @@ -1,4 +1,5 @@ const errors = require('../../errors'); +const UsersService = require('../../services/users'); const {CREATE_ACTION, DELETE_ACTION} = require('../../perms/constants'); const { IGNORE_FLAGS_AGAINST_STAFF, @@ -82,11 +83,17 @@ const createAction = async (ctx, {item_id, item_type, action_type, group_id, met metadata }); - if (action_type === 'FLAG' && item_type === 'COMMENTS') { + if (action_type === 'FLAG') { + if (item_type === 'USERS') { - // The item is a comment, and this is a flag. Push that the comment was - // flagged, don't wait for it to finish. - pubsub.publish('commentFlagged', item); + // Set the user's status as pending, as we need to review it. + await UsersService.setStatus(item_id, 'PENDING'); + } else if (item_type === 'COMMENTS') { + + // The item is a comment, and this is a flag. Push that the comment was + // flagged, don't wait for it to finish. + pubsub.publish('commentFlagged', item); + } } return action; diff --git a/services/users.js b/services/users.js index e0f7d1599..fbaf89609 100644 --- a/services/users.js +++ b/services/users.js @@ -395,18 +395,34 @@ module.exports = class UsersService { throw new Error(`status ${status} is not supported`); } - // TODO: current updating status behavior is weird. - // once a user has been `APPROVED` its status cannot be - // changed anymore. - const user = await UserModel.findOneAndUpdate({ - id, - status: { - $ne: 'APPROVED' - } - }, { + // Compose the query. + const query = {id}; + + // Insert extra validations into the query. + switch (status) { + case 'ACTIVE': + case 'BANNED': + case 'APPROVED': + + // A user cannot become change their status from what it is already. + query.status = { + $ne: status, + }; + break; + case 'PENDING': + + // A user cannot become pending if they are already approved, pending, or + // banned + query.status = { + $nin: [status, 'APPROVED', 'BANNED'], + }; + break; + } + + const user = await UserModel.findOneAndUpdate(query, { $set: { - status - } + status, + }, }, { new: true, }); @@ -426,8 +442,8 @@ module.exports = class UsersService { }; await MailerService.sendSimple(options); } - } + return user; }