From 1999e1098efc653f63d582673d0c4047e6d7146d Mon Sep 17 00:00:00 2001 From: Keith Holliday Date: Thu, 8 Jun 2017 13:45:24 -0700 Subject: [PATCH] Allow guilds edit (#8800) * test: test that admin users can update guilds * test: test admin removeMember privileges * fix: allow admins to edit guilds * fix: add edit guild options for admins * test: test that admin can't remove current leader * Add error msg for removing current leader * Taskwoods Quest Line (#8156) * feat(content): Gold Quest 2016-10 * chore(news): Bailey * chore(i18n): update locales * chore(sprites): compile * 3.49.0 * chore: update express * Fix for the ReDOS vulnerability habitica is currently affected by the high-severity [ReDOS vulnerability](https://snyk.io/vuln/npm:tough-cookie:20160722). Vulnerable module: `tough-cookie` Introduced through: ` request` This PR fixes the ReDOS vulnerability by upgrading ` request` to version 2.74.0 Check out the [Snyk test report](https://snyk.io/test/github/HabitRPG/habitica) to review other vulnerabilities that affect this repo. [Watch the repo](https://snyk.io/add) to * get alerts if newly disclosed vulnerabilities affect this repo in the future. * generate pull requests with the fixes you want, or let us do the work: when a newly disclosed vulnerability affects you, we'll submit a fix to you right away. Stay secure, The Snyk team * Documentation - coupon closes #8109 * fix(client): Allow member hp to be clickable fixes #8016 closes #8155 * chore(npm): shrinkwrap * test: test isAbleToEditGroup * Add isAbleToEditGroup to groupsCtrl * Remove unnecessary ternary * Fix linting * Move edit permission logic out to groupsCtrl * fix: change ternary to boolean * Fix linting * Fixed merge issues --- .../POST-groups_id_removeMember.test.js | 29 +++++++++++++-- .../v3/integration/groups/PUT-groups.test.js | 16 +++++++-- .../spec/controllers/groupCtrlSpec.js | 35 +++++++++++++++++++ .../client-old/js/controllers/groupsCtrl.js | 6 ++++ website/common/locales/en/groups.json | 1 + website/server/controllers/api-v3/groups.js | 31 +++++++++------- website/views/options/social/group.jade | 10 +++--- 7 files changed, 106 insertions(+), 22 deletions(-) diff --git a/test/api/v3/integration/groups/POST-groups_id_removeMember.test.js b/test/api/v3/integration/groups/POST-groups_id_removeMember.test.js index 2806fffa3c..59bf23e3d8 100644 --- a/test/api/v3/integration/groups/POST-groups_id_removeMember.test.js +++ b/test/api/v3/integration/groups/POST-groups_id_removeMember.test.js @@ -11,6 +11,7 @@ describe('POST /groups/:groupId/removeMember/:memberId', () => { let guild; let member; let member2; + let adminUser; beforeEach(async () => { let { group, groupLeader, invitees, members } = await createAndPopulateGroup({ @@ -28,6 +29,7 @@ describe('POST /groups/:groupId/removeMember/:memberId', () => { invitedUser = invitees[0]; member = members[0]; member2 = members[1]; + adminUser = await generateUser({ 'contributor.admin': true }); }); context('All Groups', () => { @@ -42,7 +44,7 @@ describe('POST /groups/:groupId/removeMember/:memberId', () => { }); }); - it('returns an error when user is a non-leader member of a group', async () => { + it('returns an error when user is a non-leader member of a group and not an admin', async () => { expect(member2.post(`/groups/${guild._id}/removeMember/${member._id}`)) .to.eventually.be.rejected.and.eql({ code: 401, @@ -87,7 +89,30 @@ describe('POST /groups/:groupId/removeMember/:memberId', () => { let invitedUserWithoutInvite = await invitedUser.get('/user'); - expect(_.findIndex(invitedUserWithoutInvite.invitations.guilds, {id: guild._id})).eql(-1); + expect(_.findIndex(invitedUserWithoutInvite.invitations.guilds, { id: guild._id })).eql(-1); + }); + + it('allows an admin to remove other members', async () => { + await adminUser.post(`/groups/${guild._id}/removeMember/${member._id}`); + let memberRemoved = await member.get('/user'); + + expect(memberRemoved.guilds.indexOf(guild._id)).eql(-1); + }); + + it('allows an admin to remove other invites', async () => { + await adminUser.post(`/groups/${guild._id}/removeMember/${invitedUser._id}`); + + let invitedUserWithoutInvite = await invitedUser.get('/user'); + + expect(_.findIndex(invitedUserWithoutInvite.invitations.guilds, { id: guild._id })).eql(-1); + }); + + it('does not allow an admin to remove a leader', async () => { + expect(adminUser.post(`/groups/${guild._id}/removeMember/${leader._id}`)) + .to.eventually.be.rejected.and.eql({ + code: 401, + text: t('cannotRemoveCurrentLeader'), + }); }); it('sends email to user with rescinded invite', async () => { diff --git a/test/api/v3/integration/groups/PUT-groups.test.js b/test/api/v3/integration/groups/PUT-groups.test.js index a5e4e091cd..84b49d91bd 100644 --- a/test/api/v3/integration/groups/PUT-groups.test.js +++ b/test/api/v3/integration/groups/PUT-groups.test.js @@ -1,10 +1,11 @@ import { createAndPopulateGroup, + generateUser, translate as t, } from '../../../../helpers/api-v3-integration.helper'; describe('PUT /group', () => { - let leader, nonLeader, groupToUpdate; + let leader, nonLeader, groupToUpdate, adminUser; let groupName = 'Test Public Guild'; let groupType = 'guild'; let groupUpdatedName = 'Test Public Guild Updated'; @@ -18,13 +19,13 @@ describe('PUT /group', () => { }, members: 1, }); - + adminUser = await generateUser({ 'contributor.admin': true }); groupToUpdate = group; leader = groupLeader; nonLeader = members[0]; }); - it('returns an error when a non group leader tries to update', async () => { + it('returns an error when a user that is not an admin or group leader tries to update', async () => { await expect(nonLeader.put(`/groups/${groupToUpdate._id}`, { name: groupUpdatedName, })).to.eventually.be.rejected.and.eql({ @@ -44,6 +45,15 @@ describe('PUT /group', () => { expect(updatedGroup.name).to.equal(groupUpdatedName); }); + it('allows an admin to update a guild', async () => { + let updatedGroup = await adminUser.put(`/groups/${groupToUpdate._id}`, { + name: groupUpdatedName, + }); + expect(updatedGroup.leader._id).to.eql(leader._id); + expect(updatedGroup.leader.profile.name).to.eql(leader.profile.name); + expect(updatedGroup.name).to.equal(groupUpdatedName); + }); + it('allows a leader to change leaders', async () => { let updatedGroup = await leader.put(`/groups/${groupToUpdate._id}`, { name: groupUpdatedName, diff --git a/test/client-old/spec/controllers/groupCtrlSpec.js b/test/client-old/spec/controllers/groupCtrlSpec.js index f45513c124..5b3521595f 100644 --- a/test/client-old/spec/controllers/groupCtrlSpec.js +++ b/test/client-old/spec/controllers/groupCtrlSpec.js @@ -112,6 +112,41 @@ describe('Groups Controller', function() { }); }); + describe('isAbleToEditGroup', () => { + var guild; + + beforeEach(() => { + user.contributor = {}; + guild = specHelper.newGroup({ + _id: 'unique-guild-id', + type: 'guild', + members: ['not-user-id'], + $save: sandbox.spy(), + }); + }); + + it('returns true if user is an admin', () => { + guild.leader = 'not-user-id'; + user.contributor.admin = true; + expect(scope.isAbleToEditGroup(guild)).to.be.ok; + }); + + it('returns true if user is group leader', () => { + guild.leader = {_id: user._id} + expect(scope.isAbleToEditGroup(guild)).to.be.ok; + }); + + it('returns false is user is not a leader or admin', () => { + expect(scope.isAbleToEditGroup(guild)).to.not.be.ok; + }); + + it('returns false is user is an admin but group is a party', () => { + guild.type = 'party'; + user.contributor.admin = true; + expect(scope.isAbleToEditGroup(guild)).to.not.be.ok; + }); + }); + describe('editGroup', () => { var guild; diff --git a/website/client-old/js/controllers/groupsCtrl.js b/website/client-old/js/controllers/groupsCtrl.js index fa3cc0acb9..6eb339daef 100644 --- a/website/client-old/js/controllers/groupsCtrl.js +++ b/website/client-old/js/controllers/groupsCtrl.js @@ -35,6 +35,12 @@ habitrpg.controller("GroupsCtrl", ['$scope', '$rootScope', 'Shared', 'Groups', ' return ~(memberIds.indexOf(userid)); }; + $scope.isAbleToEditGroup = function (group) { + if (group.leader._id === User.user._id) return true; + if (User.user.contributor.admin && group.type === "guild") return true; + return false; + }; + $scope.isMember = function (user, group) { return ~(group.members.indexOf(user._id)); }; diff --git a/website/common/locales/en/groups.json b/website/common/locales/en/groups.json index b56f9291f7..012bc259dc 100644 --- a/website/common/locales/en/groups.json +++ b/website/common/locales/en/groups.json @@ -185,6 +185,7 @@ "questLeaderCannotLeaveGroup": "You cannot leave your party when you have started a quest. Abort the quest first.", "cannotLeaveWhileActiveQuest": "You cannot leave party during an active quest. Please leave the quest first.", "onlyLeaderCanRemoveMember": "Only group leader can remove a member!", + "cannotRemoveCurrentLeader": "You cannot remove the group leader. Assign a new a leader first.", "memberCannotRemoveYourself": "You cannot remove yourself!", "groupMemberNotFound": "User not found among group's members", "mustBeGroupMember": "Must be member of the group.", diff --git a/website/server/controllers/api-v3/groups.js b/website/server/controllers/api-v3/groups.js index 35858e2f73..3edb20ac97 100644 --- a/website/server/controllers/api-v3/groups.js +++ b/website/server/controllers/api-v3/groups.js @@ -397,7 +397,7 @@ api.getGroup = { * @apiUse groupIdRequired * @apiUse GroupNotFound * - * @apiPermission GroupLeader + * @apiPermission GroupLeader, Admin */ api.updateGroup = { method: 'PUT', @@ -410,11 +410,13 @@ api.updateGroup = { let validationErrors = req.validationErrors(); if (validationErrors) throw validationErrors; + let optionalMembership = Boolean(user.contributor.admin); + let group = await Group.getGroup({user, groupId: req.params.groupId, optionalMembership}); - let group = await Group.getGroup({user, groupId: req.params.groupId}); if (!group) throw new NotFound(res.t('groupNotFound')); - if (group.leader !== user._id) throw new NotAuthorized(res.t('messageGroupOnlyLeaderCanUpdate')); + if (group.leader !== user._id && group.type === 'party') throw new NotAuthorized(res.t('messageGroupOnlyLeaderCanUpdate')); + else if (group.leader !== user._id && !user.contributor.admin) throw new NotAuthorized(res.t('messageGroupOnlyLeaderCanUpdate')); if (req.body.leader !== user._id && group.hasNotCancelled()) throw new NotAuthorized(res.t('cannotChangeLeaderWithActiveGroupPlan')); @@ -473,7 +475,7 @@ api.joinGroup = { let validationErrors = req.validationErrors(); if (validationErrors) throw validationErrors; - // Works even if the user is not yet a member of the group + // Works even if the user is not yet a member of the group let group = await Group.getGroup({user, groupId: req.params.groupId, optionalMembership: true}); // Do not fetch chat and work even if the user is not yet a member of the group if (!group) throw new NotFound(res.t('groupNotFound')); @@ -761,7 +763,7 @@ function _sendMessageToRemoved (group, removedUser, message, isInGroup) { * * @apiSuccess {Object} data An empty object * - * @apiPermission GroupLeader + * @apiPermission GroupLeader, Admin * * @apiUse groupIdRequired * @apiUse GroupNotFound @@ -778,13 +780,18 @@ api.removeGroupMember = { let validationErrors = req.validationErrors(); if (validationErrors) throw validationErrors; + let optionalMembership = Boolean(user.contributor.admin); + let group = await Group.getGroup({user, groupId: req.params.groupId, optionalMembership, fields: '-chat'}); // Do not fetch chat - let group = await Group.getGroup({user, groupId: req.params.groupId, fields: '-chat'}); // Do not fetch chat if (!group) throw new NotFound(res.t('groupNotFound')); let uuid = req.params.memberId; - if (group.leader !== user._id) throw new NotAuthorized(res.t('onlyLeaderCanRemoveMember')); + if (group.leader !== user._id && group.type === 'party') throw new NotAuthorized(res.t('onlyLeaderCanRemoveMember')); + if (group.leader !== user._id && !user.contributor.admin) throw new NotAuthorized(res.t('onlyLeaderCanRemoveMember')); + + if (group.leader === uuid && user.contributor.admin) throw new NotAuthorized(res.t('cannotRemoveCurrentLeader')); + if (user._id === uuid) throw new NotAuthorized(res.t('memberCannotRemoveYourself')); let member = await User.findOne({_id: uuid}).exec(); @@ -947,12 +954,12 @@ async function _inviteByEmail (invite, group, inviter, req, res) { if (!invite.email) throw new BadRequest(res.t('inviteMissingEmail')); let userToContact = await User.findOne({$or: [ - {'auth.local.email': invite.email}, - {'auth.facebook.emails.value': invite.email}, - {'auth.google.emails.value': invite.email}, + {'auth.local.email': invite.email}, + {'auth.facebook.emails.value': invite.email}, + {'auth.google.emails.value': invite.email}, ]}) - .select({_id: true, 'preferences.emailNotifications': true}) - .exec(); + .select({_id: true, 'preferences.emailNotifications': true}) + .exec(); if (userToContact) { userReturnInfo = await _inviteByUUID(userToContact._id, group, inviter, req, res); diff --git a/website/views/options/social/group.jade b/website/views/options/social/group.jade index db1929121d..7cac400daa 100644 --- a/website/views/options/social/group.jade +++ b/website/views/options/social/group.jade @@ -39,7 +39,7 @@ a.pull-right.gem-wallet(ng-if='group.type!="party"', popover-trigger='mouseenter span.glyphicon.glyphicon-ban-circle =env.t('leave') a.btn.btn-success.pull-right(ng-if='!isMemberOfGroup(User.user._id, group)', ng-click='join(group)')=env.t('join') - span(ng-if='group.leader._id == user.id') + span(ng-if='isAbleToEditGroup(group)') button.btn.btn-sm.btn-primary.pull-right(ng-click='cancelEdit(group)', ng-hide='!group._editing')=env.t('cancel') button.btn.btn-sm.btn-primary.pull-right(ng-click='saveEdit(group)', ng-show='group._editing')=env.t('save') button.btn.btn-sm.btn-default.pull-right(ng-click='editGroup(group)', ng-hide='group._editing')=env.t('editGroup') @@ -107,8 +107,8 @@ a.pull-right.gem-wallet(ng-if='group.type!="party"', popover-trigger='mouseenter table.table.table-striped(ng-show='::group.memberCount > 1 || group.type !== "party"' bindonce='group') tr(ng-repeat='member in group.members track by member._id') td.media - // allow leaders to ban members - .pull-left(ng-show='group.leader._id == user.id && member._id != user._id') + // allow leaders and administrators to ban members + .pull-left(ng-show='(isAbleToEditGroup(group) && member._id != user._id)') a.media-object(ng-click='removeMember(group, member, true)') span.glyphicon.glyphicon-ban-circle(tooltip=env.t('banTip')) a.media-body @@ -130,8 +130,8 @@ a.pull-right.gem-wallet(ng-if='group.type!="party"', popover-trigger='mouseenter table.table.table-striped tr(ng-repeat='invite in group.invites') td.media - // allow leaders to ban members - .pull-left(ng-show='group.leader._id == user.id') + // allow leaders and administrators to ban members + .pull-left(ng-show='isAbleToEditGroup(group)') a.media-object(ng-click='removeMember(group, invite, false)') span.glyphicon.glyphicon-ban-circle(tooltip=env.t('banTip')) a.media-body