From e69b349b1b24c70ebf562359f655c76cd6773bd1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 28 Mar 2019 08:53:07 -0700
Subject: [PATCH] Prevent users from removing task titles with "Bulk Edit"

Summary:
See downstream <https://phabricator.wikimedia.org/T209449>.

The "Bulk Edit" flow works with `setContinueOnMissingFields(true)`, so `newRequiredError()` errors are ignored. This allows you to apply a transaction which changes the title to `""` (the empty string) without actually hitting any errors which the workflow respects.

(Normally, `setContinueOnMissingFields(...)` workflows only edit properties that can't be missing, like the status of an object, so this is an unusual flow.)

Instead, validate more narrowly:

  - Transactions which would remove the title get an "invalid" error, which is respected even under "setContinueOnMissingFields()".
  - Then, we try to raise a "missing/required" error if everything otherwise looks okay.

Test Plan:
  - Edited a task title normally.
  - Edited a task to remove the title (got an error).
  - Created a task with no title (disallowed: got an error).
  - Bulk edited a task to remove its title.
    - Before change: allowed.
    - After change: disallowed.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20339
---
 .../xaction/ManiphestTaskTitleTransaction.php | 24 ++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php
index e4ec2a132f..7dd9217760 100644
--- a/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php
+++ b/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php
@@ -64,9 +64,27 @@ final class ManiphestTaskTitleTransaction
   public function validateTransactions($object, array $xactions) {
     $errors = array();
 
-    if ($this->isEmptyTextTransaction($object->getTitle(), $xactions)) {
-      $errors[] = $this->newRequiredError(
-        pht('Tasks must have a title.'));
+    // If the user is acting via "Bulk Edit" or another workflow which
+    // continues on missing fields, they may be applying a transaction which
+    // removes the task title. Mark these transactions as invalid first,
+    // then flag the missing field error if we don't find any more specific
+    // problems.
+
+    foreach ($xactions as $xaction) {
+      $new = $xaction->getNewValue();
+      if (!strlen($new)) {
+        $errors[] = $this->newInvalidError(
+          pht('Tasks must have a title.'),
+          $xaction);
+        continue;
+      }
+    }
+
+    if (!$errors) {
+      if ($this->isEmptyTextTransaction($object->getTitle(), $xactions)) {
+        $errors[] = $this->newRequiredError(
+          pht('Tasks must have a title.'));
+      }
     }
 
     return $errors;