Prevent duplicate challenge tasks (#11502)

* Implement atomic user challenges update

Prevents multiple concurrent requests from passing through
Fixes #11295

* Move user challenges update to separate method

* Rename challenge syncToUser to syncTasksToUser

Now that adding the challenge to user is separated, this methods main purpose is to sync the tasks

* Fix lint errors

migrations/challenges/sync-all-challenges.js

@@ -10,7 +10,7 @@ async function syncChallengeToMembers (challenges) {
 
     const promises = [];
     users.forEach(user => {
-      promises.push(challenge.syncToUser(user));
+      promises.push(challenge.syncTasksToUser(user));
       promises.push(challenge.save());
       promises.push(user.save());
     });

test/api/unit/models/challenge.test.js

@@ -90,7 +90,37 @@ describe('Challenge Model', () => {
         expect(syncedTask.tags[0]).to.eql(challenge._id);
       });
 
-      it('syncs a challenge to a user', async () => {
+      it('adds a challenge to a user', async () => {
+        const newMember = new User({
+          guilds: [guild._id],
+        });
+        await newMember.save();
+
+        const addedSuccessfully = await challenge.addToUser(newMember);
+
+        const updatedNewMember = await User.findById(newMember._id);
+
+        expect(addedSuccessfully).to.eql(true);
+        expect(updatedNewMember.challenges).to.contain(challenge._id);
+      });
+
+      it('does not add a challenge to a user that already in the challenge', async () => {
+        const newMember = new User({
+          guilds: [guild._id],
+          challenges: [challenge._id],
+        });
+        await newMember.save();
+
+        const addedSuccessfully = await challenge.addToUser(newMember);
+
+        const updatedNewMember = await User.findById(newMember._id);
+
+        expect(addedSuccessfully).to.eql(false);
+        expect(updatedNewMember.challenges).to.contain(challenge._id);
+        expect(updatedNewMember.challenges.length).to.eql(1);
+      });
+
+      it('syncs challenge tasks to a user', async () => {
         await challenge.addTasks([task]);
 
         const newMember = new User({
@@ -98,7 +128,7 @@ describe('Challenge Model', () => {
         });
         await newMember.save();
 
-        await challenge.syncToUser(newMember);
+        await challenge.syncTasksToUser(newMember);
 
         const updatedNewMember = await User.findById(newMember._id);
         const updatedNewMemberTasks = await Tasks.Task.find({ _id: { $in: updatedNewMember.tasksOrder[`${taskType}s`] } });
@@ -110,14 +140,13 @@ describe('Challenge Model', () => {
           ),
         );
 
-        expect(updatedNewMember.challenges).to.contain(challenge._id);
         expect(updatedNewMember.tags[7].id).to.equal(challenge._id);
         expect(updatedNewMember.tags[7].name).to.equal(challenge.shortName);
         expect(syncedTask).to.exist;
         expect(syncedTask.attribute).to.eql('str');
       });
 
-      it('syncs a challenge to a user with the existing task', async () => {
+      it('syncs challenge tasks to a user with the existing task', async () => {
         await challenge.addTasks([task]);
 
         let updatedLeader = await User.findOne({ _id: leader._id });
@@ -134,7 +163,7 @@ describe('Challenge Model', () => {
         task.text = newTitle;
         task.attribute = 'int';
         await task.save();
-        await challenge.syncToUser(leader);
+        await challenge.syncTasksToUser(leader);
 
         updatedLeader = await User.findOne({ _id: leader._id });
         updatedLeadersTasks = await Tasks.Task.find({ _id: { $in: updatedLeader.tasksOrder[`${taskType}s`] } });

website/server/controllers/api-v3/challenges.js

@@ -254,19 +254,23 @@ api.joinChallenge = {
 
     const challenge = await Challenge.findOne({ _id: req.params.challengeId }).exec();
     if (!challenge) throw new NotFound(res.t('challengeNotFound'));
-    if (challenge.isMember(user)) throw new NotAuthorized(res.t('userAlreadyInChallenge'));
 
     const group = await Group.getGroup({
       user, groupId: challenge.group, fields: basicGroupFields, optionalMembership: true,
     });
     if (!group || !challenge.canJoin(user, group)) throw new NotFound(res.t('challengeNotFound'));
 
+    const addedSuccessfully = await challenge.addToUser(user);
+    if (!addedSuccessfully) {
+      throw new NotAuthorized(res.t('userAlreadyInChallenge'));
+    }
+
     challenge.memberCount += 1;
 
     addUserJoinChallengeNotification(user);
 
     // Add all challenge's tasks to user's tasks and save the challenge
-    const results = await Promise.all([challenge.syncToUser(user), challenge.save()]);
+    const results = await Promise.all([challenge.syncTasksToUser(user), challenge.save()]);
 
     const response = results[1].toJSON();
     response.group = getChallengeGroupResponse(group);

website/server/models/challenge.js

@@ -94,6 +94,23 @@ schema.methods.canJoin = function canJoinChallenge (user, group) {
   return user.getGroups().indexOf(this.group) !== -1;
 };
 
+// Returns true if the challenge was successfully added to the user
+// or false if the user already in the challenge
+schema.methods.addToUser = async function addChallengeToUser (user) {
+  // Add challenge to users challenges atomically (with a condition that checks that it
+  // is not there already) to prevent multiple concurrent requests from passing through
+  // see https://github.com/HabitRPG/habitica/issues/11295
+  const result = await User.update(
+    {
+      _id: user._id,
+      challenges: { $nin: [this._id] },
+    },
+    { $push: { challenges: this._id } },
+  ).exec();
+
+  return !!result.nModified;
+};
+
 // Returns true if user can view the challenge
 // Different from canJoin because you can see challenges of groups
 // you've been removed from if you're participating in them
@@ -102,19 +119,12 @@ schema.methods.canView = function canViewChallenge (user, group) {
   return this.canJoin(user, group);
 };
 
-// Sync challenge to user, including tasks and tags.
+// Sync challenge tasks to user, including tags.
 // Used when user joins the challenge or to force sync.
-schema.methods.syncToUser = async function syncChallengeToUser (user) {
+schema.methods.syncTasksToUser = async function syncChallengeTasksToUser (user) {
   const challenge = this;
   challenge.shortName = challenge.shortName || challenge.name;
 
-  // Add challenge to user.challenges
-  if (!_.includes(user.challenges, challenge._id)) {
-    // using concat because mongoose's protection against
-    // concurrent array modification isn't working as expected.
-    // see https://github.com/HabitRPG/habitica/pull/7787#issuecomment-232972394
-    user.challenges = user.challenges.concat([challenge._id]);
-  }
   // Sync tags
   const userTags = user.tags;
   const i = _.findIndex(userTags, { id: challenge._id });