From 5d4970d6b24ac98f50990cddd5151291c10855fa Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 31 Oct 2018 17:14:02 -0700
Subject: [PATCH] Fix a bug where "View as Query" could replace a saved query
 row by ID, causing workboard 404s

Summary:
Fixes T13208. See that task for details.

The `clone $query` line is safe if `$query` is a builtin query (like "open").

However, if it's a saved query we clone not only the query parameters but the ID, too. Then when we `save()` the query later, we overwrite the original query.

So this would happen in the database. First, you run a query and save it as the workboard default (query key "abc123"):

| 123 | abc123 | {"...xxx..."} |

Then we `clone` it and change the parameters, and `save()` it. But that causes an `UPDATE ... WHERE id = 123` and the table now looks like this:

| 123 | def456 | {"...yyy..."} |

What we want is to create a new query instead, with an `INSERT ...`:

| 123 | abc123 | {"...xxx..."} |
| 124 | def456 | {"...yyy..."} |

Test Plan:
  - Followed reproduction steps from above.
    - With just the new `save()` guard, hit the guard error.
    - With the `newCopy()`, got a new copy of the query and "View as Query" remained functional without overwriting the original query row.
  - Ran migration, saw an affected board get fixed.

Reviewers: amckinley, joshuaspence

Reviewed By: joshuaspence

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13208

Differential Revision: https://secure.phabricator.com/D19768
---
 .../20181031.board.01.queryreset.php          | 50 +++++++++++++++++++
 .../PhabricatorProjectBoardViewController.php |  2 +-
 .../PhabricatorApplicationSearchEngine.php    |  8 +++
 .../search/storage/PhabricatorSavedQuery.php  |  7 +++
 4 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 resources/sql/autopatches/20181031.board.01.queryreset.php

diff --git a/resources/sql/autopatches/20181031.board.01.queryreset.php b/resources/sql/autopatches/20181031.board.01.queryreset.php
new file mode 100644
index 0000000000..781cf456ce
--- /dev/null
+++ b/resources/sql/autopatches/20181031.board.01.queryreset.php
@@ -0,0 +1,50 @@
+<?php
+
+// See T13208. It was previously possible to replace a saved query with another
+// saved query, causing loss of the first query. Find projects which have their
+// default query set to an invalid query and throw the setting away.
+
+$viewer = PhabricatorUser::getOmnipotentUser();
+
+$table = new PhabricatorProject();
+$conn = $table->establishConnection('w');
+
+$iterator = new LiskMigrationIterator($table);
+$search_engine = id(new ManiphestTaskSearchEngine())
+  ->setViewer($viewer);
+
+foreach ($iterator as $project) {
+  $default_filter = $project->getDefaultWorkboardFilter();
+  if (!strlen($default_filter)) {
+    continue;
+  }
+
+  if ($search_engine->isBuiltinQuery($default_filter)) {
+    continue;
+  }
+
+  $saved = id(new PhabricatorSavedQueryQuery())
+    ->setViewer($viewer)
+    ->withQueryKeys(array($default_filter))
+    ->executeOne();
+  if ($saved) {
+    continue;
+  }
+
+  $properties = $project->getProperties();
+  unset($properties['workboard.filter.default']);
+
+  queryfx(
+    $conn,
+    'UPDATE %T SET properties = %s WHERE id = %d',
+    $table->getTableName(),
+    phutil_json_encode($properties),
+    $project->getID());
+
+  echo tsprintf(
+    "%s\n",
+    pht(
+      'Project ("%s") had an invalid query saved as a default workboard '.
+      'query. The query has been reset. See T13208.',
+      $project->getDisplayName()));
+}
diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php
index bb048f22ea..15e0c5d075 100644
--- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php
+++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php
@@ -203,7 +203,7 @@ final class PhabricatorProjectBoardViewController
       // with the column filter. If the user currently has constraints on the
       // board, we want to add a new column or project constraint, not
       // completely replace the constraints.
-      $saved_query = clone $saved;
+      $saved_query = $saved->newCopy();
 
       if ($query_column->getProxyPHID()) {
         $project_phids = $saved_query->getParameter('projectPHIDs');
diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php
index b808291a52..a89a017e85 100644
--- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php
+++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php
@@ -103,6 +103,14 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject {
   }
 
   public function saveQuery(PhabricatorSavedQuery $query) {
+    if ($query->getID()) {
+      throw new Exception(
+        pht(
+          'Query (with ID "%s") has already been saved. Queries are '.
+          'immutable once saved.',
+          $query->getID()));
+    }
+
     $query->setEngineClassName(get_class($this));
 
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
diff --git a/src/applications/search/storage/PhabricatorSavedQuery.php b/src/applications/search/storage/PhabricatorSavedQuery.php
index 3f75027888..837f4fde42 100644
--- a/src/applications/search/storage/PhabricatorSavedQuery.php
+++ b/src/applications/search/storage/PhabricatorSavedQuery.php
@@ -63,6 +63,13 @@ final class PhabricatorSavedQuery extends PhabricatorSearchDAO
     return $this->assertAttachedKey($this->parameterMap, $key);
   }
 
+  public function newCopy() {
+    return id(new self())
+      ->setParameters($this->getParameters())
+      ->setQueryKey(null)
+      ->setEngineClassName($this->getEngineClassName());
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */