From 149da578fdc4c6fe309dd17091d718a2b1b25d2d Mon Sep 17 00:00:00 2001 From: SabreCat Date: Mon, 22 Aug 2022 16:16:23 -0500 Subject: [PATCH] fix(teams): fix fix fix Removed testing banner Fixed a JS console error when assigning a user to a previously open task Fixed a potential abuse where user might be able to score someone else's task via API call Fixed an issue where finding tasks by alias could return tasks belonging to other users Fixed an issue that was appending the user's party ID to their list of Guilds Fixed an issue where group tasks were not receiving the default tag needed for filtering them on user's personal list --- test/api/unit/models/task.test.js | 19 +++++-- website/client/src/app.vue | 3 - .../header/banners/teamsTesting.vue | 56 ------------------- .../components/snackbars/notifications.vue | 2 +- .../src/components/tasks/taskSummary.vue | 3 +- website/common/script/ops/scoreTask.js | 7 +++ website/server/controllers/api-v3/tasks.js | 2 +- website/server/libs/tasks/index.js | 4 +- website/server/models/task.js | 4 +- 9 files changed, 28 insertions(+), 72 deletions(-) delete mode 100644 website/client/src/components/header/banners/teamsTesting.vue diff --git a/test/api/unit/models/task.test.js b/test/api/unit/models/task.test.js index 5921a4e2c2..5da3341cbe 100644 --- a/test/api/unit/models/task.test.js +++ b/test/api/unit/models/task.test.js @@ -246,13 +246,23 @@ describe('Task Model', () => { expect(foundTasks[0].text).to.eql(taskWithAlias.text); }); - it('scopes alias lookup to user', async () => { + it('scopes alias lookup to user when querying aliases only', async () => { await Tasks.Task.findMultipleByIdOrAlias([taskWithAlias.alias], user._id); + expect(Tasks.Task.find).to.be.calledOnce; + expect(Tasks.Task.find).to.be.calledWithMatch({ + alias: { $in: [taskWithAlias.alias] }, + userId: user._id, + }); + }); + + it('scopes alias lookup to user when querying aliases and IDs', async () => { + await Tasks.Task.findMultipleByIdOrAlias([taskWithAlias.alias, secondTask._id], user._id); + expect(Tasks.Task.find).to.be.calledOnce; expect(Tasks.Task.find).to.be.calledWithMatch({ $or: [ - { _id: { $in: [] } }, + { _id: { $in: [secondTask._id] } }, { alias: { $in: [taskWithAlias.alias] } }, ], userId: user._id, @@ -270,10 +280,7 @@ describe('Task Model', () => { expect(Tasks.Task.find).to.be.calledOnce; expect(Tasks.Task.find).to.be.calledWithMatch({ - $or: [ - { _id: { $in: [] } }, - { alias: { $in: [taskWithAlias.alias] } }, - ], + alias: { $in: [taskWithAlias.alias] }, userId: user._id, foo: 'bar', }); diff --git a/website/client/src/app.vue b/website/client/src/app.vue index 64603fd122..e9afbb2515 100644 --- a/website/client/src/app.vue +++ b/website/client/src/app.vue @@ -42,7 +42,6 @@ -
- -
- - You're previewing new Habitica Groups! - - - Have a question? - -
- - - - - - diff --git a/website/client/src/components/snackbars/notifications.vue b/website/client/src/components/snackbars/notifications.vue index 9f1705d220..30bd8d73e9 100644 --- a/website/client/src/components/snackbars/notifications.vue +++ b/website/client/src/components/snackbars/notifications.vue @@ -143,7 +143,7 @@ export default { scrollPosToCheck += this.eventPromoBannerHeight ?? 0; } - return scrollPosToCheck + 48; // teams testing banner is always on, for now + return scrollPosToCheck; }, visibleNotificationsWithoutErrors () { return this.visibleNotifications.filter(n => n.type !== 'error'); diff --git a/website/client/src/components/tasks/taskSummary.vue b/website/client/src/components/tasks/taskSummary.vue index a42928968e..109d343c73 100644 --- a/website/client/src/components/tasks/taskSummary.vue +++ b/website/client/src/components/tasks/taskSummary.vue @@ -213,7 +213,8 @@ export default { user: 'user.data', }), assignedUsernames () { - if (!this.task.group || !this.task.group.assignedUsers) return []; + if (!this.task.group || !this.task.group.assignedUsers + || !this.task.group.assignedUsersDetail) return []; const usernames = []; for (const user of this.task.group.assignedUsers) { usernames.push(this.task.group.assignedUsersDetail[user].assignedUsername); diff --git a/website/common/script/ops/scoreTask.js b/website/common/script/ops/scoreTask.js index e85b4509e0..14c0f688d8 100644 --- a/website/common/script/ops/scoreTask.js +++ b/website/common/script/ops/scoreTask.js @@ -4,6 +4,7 @@ import reduce from 'lodash/reduce'; import moment from 'moment'; import max from 'lodash/max'; import { + BadRequest, NotAuthorized, } from '../libs/errors'; import i18n from '../i18n'; @@ -247,6 +248,12 @@ export default function scoreTask (options = {}, req = {}, analytics) { // If they're trying to purchase a too-expensive reward, don't allow them to do that. if (task.value > user.stats.gp && task.type === 'reward') throw new NotAuthorized(i18n.t('messageNotEnoughGold', req.language)); + // Thanks to open group tasks, userId is not guaranteed. Don't allow scoring inaccessible tasks + if (task.userId && task.userId !== user._id) { + throw new BadRequest('Cannot score task belonging to another user.'); + } else if (user.guilds.indexOf(task.group.id) === -1 && user.party._id !== task.group.id) { + throw new BadRequest('Cannot score task belonging to another user.'); + } if (task.type === 'habit') { delta += _changeTaskValue(user, task, direction, times, cron); diff --git a/website/server/controllers/api-v3/tasks.js b/website/server/controllers/api-v3/tasks.js index 020625de7e..8a665e2da2 100644 --- a/website/server/controllers/api-v3/tasks.js +++ b/website/server/controllers/api-v3/tasks.js @@ -816,7 +816,7 @@ api.moveTask = { const group = await getGroupFromTaskAndUser(task, user); const challenge = await getChallengeFromTask(task); if (task.group.id && !task.userId) { - if (!group || user.guilds.concat(user.party._id).indexOf(group._id) === -1) { + if (!group || (user.guilds.indexOf(group._id) === -1 && user.party._id !== group._id)) { throw new NotFound(res.t('groupNotFound')); } if (task.group.assignedUsers.length !== 0 diff --git a/website/server/libs/tasks/index.js b/website/server/libs/tasks/index.js index f29839dbf7..faf36cd077 100644 --- a/website/server/libs/tasks/index.js +++ b/website/server/libs/tasks/index.js @@ -67,9 +67,7 @@ async function createTasks (req, res, options = {}) { newTask.challenge.id = challenge.id; } else if (group) { newTask.group.id = group._id; - if (taskData.requiresApproval) { - newTask.group.approval.required = true; - } + newTask.tags = [group._id]; newTask.group.managerNotes = taskData.managerNotes || ''; } else { newTask.userId = user._id; diff --git a/website/server/models/task.js b/website/server/models/task.js index e09040a877..f9601cd10c 100644 --- a/website/server/models/task.js +++ b/website/server/models/task.js @@ -205,7 +205,7 @@ TaskSchema.statics.findByIdOrAlias = async function findByIdOrAlias ( return task; }; -TaskSchema.statics.findMultipleByIdOrAlias = async function findByIdOrAlias ( +TaskSchema.statics.findMultipleByIdOrAlias = async function findMultipleByIdOrAlias ( identifiers, userId, additionalQueries = {}, @@ -226,6 +226,7 @@ TaskSchema.statics.findMultipleByIdOrAlias = async function findByIdOrAlias ( }); if (ids.length > 0 && aliases.length > 0) { + query.userId = userId; query.$or = [ { _id: { $in: ids } }, { alias: { $in: aliases } }, @@ -233,6 +234,7 @@ TaskSchema.statics.findMultipleByIdOrAlias = async function findByIdOrAlias ( } else if (ids.length > 0) { query._id = { $in: ids }; } else if (aliases.length > 0) { + query.userId = userId; query.alias = { $in: aliases }; } else { throw new Error('No identifiers found.'); // Should be covered by the !identifiers check, but..