From 2896cf77e0aa28ded26579e8be04efa76bf03156 Mon Sep 17 00:00:00 2001
From: Carl Vuorinen <carl.vuorinen@gmail.com>
Date: Fri, 17 Apr 2020 23:57:31 +0300
Subject: [PATCH] Handle simultaneous quest accept/reject (#12090)

* Implement atomic quest accept/reject

* Persist quest.members early to avoid simultaneous handling of accept/reject

* Fix quest accept test (missing expectation)

* PR fixes
---
 .../POST-groups_groupId_quests_accept.test.js |  2 +-
 website/server/controllers/api-v3/quests.js   | 16 ++++++----
 website/server/models/group.js                | 29 +++++++++++++++++++
 3 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/test/api/v3/integration/quests/POST-groups_groupId_quests_accept.test.js b/test/api/v3/integration/quests/POST-groups_groupId_quests_accept.test.js
index be9086d74f..deb105de7c 100644
--- a/test/api/v3/integration/quests/POST-groups_groupId_quests_accept.test.js
+++ b/test/api/v3/integration/quests/POST-groups_groupId_quests_accept.test.js
@@ -112,7 +112,7 @@ describe('POST /groups/:groupId/quests/accept', () => {
 
       await Promise.all([partyMembers[0].sync(), questingGroup.sync()]);
       expect(leader.party.quest.RSVPNeeded).to.equal(false);
-      expect(questingGroup.quest.members[partyMembers[0]._id]);
+      expect(questingGroup.quest.members[partyMembers[0]._id]).to.equal(true);
     });
 
     it('does not begin the quest if pending invitations remain', async () => {
diff --git a/website/server/controllers/api-v3/quests.js b/website/server/controllers/api-v3/quests.js
index 88b68bd088..55e578e08a 100644
--- a/website/server/controllers/api-v3/quests.js
+++ b/website/server/controllers/api-v3/quests.js
@@ -193,12 +193,14 @@ api.acceptQuest = {
     if (group.quest.active) throw new NotAuthorized(res.t('questAlreadyStartedFriendly'));
     if (group.quest.members[user._id]) throw new BadRequest(res.t('questAlreadyAccepted'));
 
+    const acceptedSuccessfully = await group.handleQuestInvitation(user, true);
+    if (!acceptedSuccessfully) {
+      throw new NotAuthorized(res.t('questAlreadyAccepted'));
+    }
+
     user.party.quest.RSVPNeeded = false;
     await user.save();
 
-    group.markModified('quest');
-    group.quest.members[user._id] = true;
-
     if (canStartQuestAutomatically(group)) {
       await group.startQuest(user);
     }
@@ -252,13 +254,15 @@ api.rejectQuest = {
     if (group.quest.members[user._id]) throw new BadRequest(res.t('questAlreadyAccepted'));
     if (group.quest.members[user._id] === false) throw new BadRequest(res.t('questAlreadyRejected'));
 
+    const rejectedSuccessfully = await group.handleQuestInvitation(user, false);
+    if (!rejectedSuccessfully) {
+      throw new NotAuthorized(res.t('questAlreadyRejected'));
+    }
+
     user.party.quest = Group.cleanQuestUser(user.party.quest.progress);
     user.markModified('party.quest');
     await user.save();
 
-    group.quest.members[user._id] = false;
-    group.markModified('quest.members');
-
     if (canStartQuestAutomatically(group)) {
       await group.startQuest(user);
     }
diff --git a/website/server/models/group.js b/website/server/models/group.js
index 7f0a4b5c18..00c0cdf882 100644
--- a/website/server/models/group.js
+++ b/website/server/models/group.js
@@ -651,6 +651,30 @@ schema.methods.sendChat = function sendChat (options = {}) {
   return newChatMessage;
 };
 
+schema.methods.handleQuestInvitation = async function handleQuestInvitation (user, accept) {
+  if (!user) throw new InternalServerError('Must provide user to handle quest invitation');
+  if (accept !== true && accept !== false) throw new InternalServerError('Must provide accept param handle quest invitation');
+
+  // Handle quest invitation atomically (update only current member when still undecided)
+  // to prevent multiple concurrent requests overriding updates
+  // see https://github.com/HabitRPG/habitica/issues/11398
+  const Group = this.constructor;
+  const result = await Group.update(
+    {
+      _id: this._id,
+      [`quest.members.${user._id}`]: { $type: 10 }, // match BSON Type Null (type number 10)
+    },
+    { $set: { [`quest.members.${user._id}`]: accept } },
+  ).exec();
+
+  if (result.nModified) {
+    // update also current instance so future operations will work correctly
+    this.quest.members[user._id] = accept;
+  }
+
+  return Boolean(result.nModified);
+};
+
 schema.methods.startQuest = async function startQuest (user) {
   // not using i18n strings because these errors are meant
   // for devs who forgot to pass some parameters
@@ -680,6 +704,11 @@ schema.methods.startQuest = async function startQuest (user) {
 
   // Changes quest.members to only include participating members
   this.quest.members = _.pickBy(this.quest.members, _.identity);
+
+  // Persist quest.members early to avoid simultaneous handling of accept/reject
+  // while processing the rest of this script
+  await this.update({ $set: { 'quest.members': this.quest.members } }).exec();
+
   const nonUserQuestMembers = _.keys(this.quest.members);
   removeFromArray(nonUserQuestMembers, user._id);