1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 05:50:55 +01:00

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
This commit is contained in:
epriestley 2018-10-31 17:14:02 -07:00
parent b950f877c5
commit 5d4970d6b2
4 changed files with 66 additions and 1 deletions

View file

@ -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()));
}

View file

@ -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');

View file

@ -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();

View file

@ -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 )----------------------------------------- */