From c25a8fabfc1ead87df93e9011899a83b13a1a924 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 May 2022 15:10:01 -0700 Subject: [PATCH] Remove all "ObjectHasFile" edge reads and writes Summary: Ref T13603. Migrate all code which interacts with the "ObjectHasFile" edge to use the "Attachments" table instead. Test Plan: - Edited a paste's view policy, confirmed associated file secret was scrambled. - Verified I could still view paste content as a user who could not naturally view the underlying file. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13603 Differential Revision: https://secure.phabricator.com/D21819 --- .../sql/autopatches/20140904.macroattach.php | 2 +- .../files/query/PhabricatorFileQuery.php | 24 +++++++++ .../files/storage/PhabricatorFile.php | 6 --- ...habricatorApplicationTransactionEditor.php | 52 +------------------ 4 files changed, 27 insertions(+), 57 deletions(-) diff --git a/resources/sql/autopatches/20140904.macroattach.php b/resources/sql/autopatches/20140904.macroattach.php index 4761964758..4c4f4e8494 100644 --- a/resources/sql/autopatches/20140904.macroattach.php +++ b/resources/sql/autopatches/20140904.macroattach.php @@ -16,7 +16,7 @@ foreach (new LiskMigrationIterator($table) as $macro) { foreach ($phids as $phid) { $editor->addEdge( $macro->getPHID(), - PhabricatorObjectHasFileEdgeType::EDGECONST, + 25, $phid); } $editor->save(); diff --git a/src/applications/files/query/PhabricatorFileQuery.php b/src/applications/files/query/PhabricatorFileQuery.php index c19574acaa..ea0dc0c35b 100644 --- a/src/applications/files/query/PhabricatorFileQuery.php +++ b/src/applications/files/query/PhabricatorFileQuery.php @@ -20,6 +20,7 @@ final class PhabricatorFileQuery private $builtinKeys; private $isBuiltin; private $storageEngines; + private $attachedObjectPHIDs; public function withIDs(array $ids) { $this->ids = $ids; @@ -61,6 +62,11 @@ final class PhabricatorFileQuery return $this; } + public function withAttachedObjectPHIDs(array $phids) { + $this->attachedObjectPHIDs = $phids; + return $this; + } + /** * Select files which are transformations of some other file. For example, * you can use this query to find previously generated thumbnails of an image @@ -347,9 +353,20 @@ final class PhabricatorFileQuery id(new PhabricatorTransformedFile())->getTableName()); } + if ($this->shouldJoinAttachmentsTable()) { + $joins[] = qsprintf( + $conn, + 'JOIN %R attachments ON attachments.filePHID = f.phid', + new PhabricatorFileAttachment()); + } + return $joins; } + private function shouldJoinAttachmentsTable() { + return ($this->attachedObjectPHIDs !== null); + } + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { $where = parent::buildWhereClauseParts($conn); @@ -482,6 +499,13 @@ final class PhabricatorFileQuery $this->storageEngines); } + if ($this->attachedObjectPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'attachments.objectPHID IN (%Ls)', + $this->attachedObjectPHIDs); + } + return $where; } diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index cd1ee8997c..cc1b701dcd 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -1416,12 +1416,6 @@ final class PhabricatorFile extends PhabricatorFileDAO * @return this */ public function attachToObject($phid) { - $edge_type = PhabricatorObjectHasFileEdgeType::EDGECONST; - - id(new PhabricatorEdgeEditor()) - ->addEdge($phid, $edge_type, $this->getPHID()) - ->save(); - $attachment_table = new PhabricatorFileAttachment(); $attachment_conn = $attachment_table->establishConnection('w'); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 69b6ecc9bc..8e34d73809 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1300,7 +1300,6 @@ abstract class PhabricatorApplicationTransactionEditor } $xactions = $this->sortTransactions($xactions); - $file_phids = $this->extractFilePHIDs($object, $xactions); if ($is_preview) { $this->loadHandles($xactions); @@ -1389,10 +1388,6 @@ abstract class PhabricatorApplicationTransactionEditor } } - if ($file_phids) { - $this->attachFiles($object, $file_phids); - } - foreach ($xactions as $xaction) { $this->applyExternalEffects($object, $xaction); } @@ -4353,20 +4348,8 @@ abstract class PhabricatorApplicationTransactionEditor } $phids = array_unique(array_filter(array_mergev($phids))); - if (!$phids) { - return array(); - } - // Only let a user attach files they can actually see, since this would - // otherwise let you access any file by attaching it to an object you have - // view permission on. - - $files = id(new PhabricatorFileQuery()) - ->setViewer($this->getActor()) - ->withPHIDs($phids) - ->execute(); - - return mpull($files, 'getPHID'); + return $phids; } /** @@ -4379,28 +4362,6 @@ abstract class PhabricatorApplicationTransactionEditor } - /** - * @task files - */ - private function attachFiles( - PhabricatorLiskDAO $object, - array $file_phids) { - - if (!$file_phids) { - return; - } - - $editor = new PhabricatorEdgeEditor(); - - $src = $object->getPHID(); - $type = PhabricatorObjectHasFileEdgeType::EDGECONST; - foreach ($file_phids as $dst) { - $editor->addEdge($src, $type, $dst); - } - - $editor->save(); - } - private function applyInverseEdgeTransactions( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction, @@ -4847,20 +4808,11 @@ abstract class PhabricatorApplicationTransactionEditor } } - $phid = $object->getPHID(); - - $attached_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( - $phid, - PhabricatorObjectHasFileEdgeType::EDGECONST); - if (!$attached_phids) { - return; - } - $omnipotent_viewer = PhabricatorUser::getOmnipotentUser(); $files = id(new PhabricatorFileQuery()) ->setViewer($omnipotent_viewer) - ->withPHIDs($attached_phids) + ->withAttachedObjectPHIDs(array($object->getPHID())) ->execute(); foreach ($files as $file) { $view_policy = $file->getViewPolicy();