From 26081594e2f3c1bd01ce785089b5b5a23625e4ef Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 7 Feb 2019 09:26:07 -0800
Subject: [PATCH] Fix two very, very minor correctness issues in Slowvote

Summary:
See <https://hackerone.com/reports/492525> and <https://hackerone.com/reports/489531>. I previously awarded a bounty for <https://hackerone.com/reports/434116> so Slowvote is getting "researched" a lot.

  - Prevent users from undoing their vote by submitting the form with nothing selected.
  - Prevent users from racing between the `delete()` and `save()` to vote for multiple options in a plurality poll.

Test Plan:
  - Clicked the vote button with nothing selected in plurality and approval polls, got an error now.
  - Added a `sleep(5)` between `delete()` and `save()`. Submitted different plurality votes in different windows. Before: votes raced, invalid end state. After: votes waited on the lock, arrived in a valid end state.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20125
---
 .../PhabricatorSlowvoteVoteController.php     | 53 ++++++++++++++-----
 1 file changed, 41 insertions(+), 12 deletions(-)

diff --git a/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php b/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php
index 62913b09e3..e1a1b9df34 100644
--- a/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php
+++ b/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php
@@ -37,6 +37,19 @@ final class PhabricatorSlowvoteVoteController
     $method = $poll->getMethod();
     $is_plurality = ($method == PhabricatorSlowvotePoll::METHOD_PLURALITY);
 
+    if (!$votes) {
+      if ($is_plurality) {
+        $message = pht('You must vote for something.');
+      } else {
+        $message = pht('You must vote for at least one option.');
+      }
+
+      return $this->newDialog()
+        ->setTitle(pht('Stand For Something'))
+        ->appendParagraph($message)
+        ->addCancelButton($poll->getURI());
+    }
+
     if ($is_plurality && count($votes) > 1) {
       throw new Exception(
         pht('In this poll, you may only vote for one option.'));
@@ -52,23 +65,39 @@ final class PhabricatorSlowvoteVoteController
       }
     }
 
-    foreach ($old_votes as $old_vote) {
-      if (!idx($votes, $old_vote->getOptionID(), false)) {
+    $poll->openTransaction();
+      $poll->beginReadLocking();
+
+      $poll->reload();
+
+      $old_votes = id(new PhabricatorSlowvoteChoice())->loadAllWhere(
+        'pollID = %d AND authorPHID = %s',
+        $poll->getID(),
+        $viewer->getPHID());
+      $old_votes = mpull($old_votes, null, 'getOptionID');
+
+      foreach ($old_votes as $old_vote) {
+        if (idx($votes, $old_vote->getOptionID())) {
+          continue;
+        }
+
         $old_vote->delete();
       }
-    }
 
-    foreach ($votes as $vote) {
-      if (idx($old_votes, $vote, false)) {
-        continue;
+      foreach ($votes as $vote) {
+        if (idx($old_votes, $vote)) {
+          continue;
+        }
+
+        id(new PhabricatorSlowvoteChoice())
+          ->setAuthorPHID($viewer->getPHID())
+          ->setPollID($poll->getID())
+          ->setOptionID($vote)
+          ->save();
       }
 
-      id(new PhabricatorSlowvoteChoice())
-        ->setAuthorPHID($viewer->getPHID())
-        ->setPollID($poll->getID())
-        ->setOptionID($vote)
-        ->save();
-    }
+      $poll->endReadLocking();
+    $poll->saveTransaction();
 
     return id(new AphrontRedirectResponse())
       ->setURI($poll->getURI());