From 591df78361a7f129a71bf162dd50e4b6e4f25b80 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 30 Dec 2013 11:27:02 -0800 Subject: [PATCH] Bind patches, file content and raw diffs bind policies to their originating objects Summary: Fixes T4270. When you download raw file content, diffs, and patches we currently give them default (all users) visibility. Instead, bind them to the repository or revision in question. (This code could use a bit of cleanup at some point.) Test Plan: Hit the patch and content download links in Diffusion and the patch download link in Differential, got restricted files with accurate policy bindings. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4270 Differential Revision: https://secure.phabricator.com/D7849 --- .../DifferentialRevisionViewController.php | 10 ++++++++++ .../DiffusionBrowseFileController.php | 11 ++++++++++- .../controller/DiffusionCommitController.php | 8 ++++++++ .../PhabricatorFileInfoController.php | 2 ++ .../files/storage/PhabricatorFile.php | 18 +++++++++++++++--- 5 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 60a0acf210..14d37ffb29 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -70,6 +70,7 @@ final class DifferentialRevisionViewController extends DifferentialController { if ($request->getExists('download')) { return $this->buildRawDiffResponse( + $revision, $changesets, $vs_changesets, $vs_map, @@ -850,6 +851,7 @@ final class DifferentialRevisionViewController extends DifferentialController { * @return @{class:AphrontRedirectResponse} */ private function buildRawDiffResponse( + DifferentialRevision $revision, array $changesets, array $vs_changesets, array $vs_map, @@ -910,8 +912,16 @@ final class DifferentialRevisionViewController extends DifferentialController { $raw_diff, array( 'name' => $file_name, + 'ttl' => (60 * 60 * 24), + 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, )); + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $file->attachToObject( + $this->getRequest()->getUser(), + $revision->getPHID()); + unset($unguarded); + return id(new AphrontRedirectResponse())->setURI($file->getBestURI()); } diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php index f3da2d3583..c996b7602a 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php @@ -810,12 +810,21 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController { } private function loadFileForData($path, $data) { - return PhabricatorFile::buildFromFileDataOrHash( + $file = PhabricatorFile::buildFromFileDataOrHash( $data, array( 'name' => basename($path), 'ttl' => time() + 60 * 60 * 24, + 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, )); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $file->attachToObject( + $this->getRequest()->getUser(), + $this->getDiffusionRequest()->getRepository()->getPHID()); + unset($unguarded); + + return $file; } private function buildRawResponse($path, $data) { diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 557338a46e..ccad796246 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -1049,8 +1049,16 @@ final class DiffusionCommitController extends DiffusionController { $raw_diff, array( 'name' => $drequest->getCommit().'.diff', + 'ttl' => (60 * 60 * 24), + 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, )); + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $file->attachToObject( + $this->getRequest()->getUser(), + $drequest->getRepository()->getPHID()); + unset($unguarded); + return id(new AphrontRedirectResponse())->setURI($file->getBestURI()); } diff --git a/src/applications/files/controller/PhabricatorFileInfoController.php b/src/applications/files/controller/PhabricatorFileInfoController.php index b224c9b55d..197401ddc7 100644 --- a/src/applications/files/controller/PhabricatorFileInfoController.php +++ b/src/applications/files/controller/PhabricatorFileInfoController.php @@ -33,6 +33,8 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { $this->loadHandles($handle_phids); $header = id(new PHUIHeaderView()) + ->setUser($user) + ->setPolicyObject($file) ->setHeader($file->getName()); $ttl = $file->getTTL(); diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index c54e07949c..d611f1b062 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -175,13 +175,17 @@ final class PhabricatorFile extends PhabricatorFileDAO $file_ttl = idx($params, 'ttl'); $authorPHID = idx($params, 'authorPHID'); - $new_file = new PhabricatorFile(); + $new_file = new PhabricatorFile(); $new_file->setName($file_name); $new_file->setByteSize($copy_of_byteSize); $new_file->setAuthorPHID($authorPHID); $new_file->setTtl($file_ttl); + if (idx($params, 'viewPolicy')) { + $new_file->setViewPolicy($params['viewPolicy']); + } + $new_file->setContentHash($hash); $new_file->setStorageEngine($copy_of_storage_engine); $new_file->setStorageHandle($copy_of_storage_handle); @@ -262,6 +266,10 @@ final class PhabricatorFile extends PhabricatorFileDAO $file->setTtl($file_ttl); $file->setContentHash(self::hashFileContent($data)); + if (idx($params, 'viewPolicy')) { + $file->setViewPolicy($params['viewPolicy']); + } + $file->setStorageEngine($engine_identifier); $file->setStorageHandle($data_handle); @@ -877,8 +885,12 @@ final class PhabricatorFile extends PhabricatorFileDAO } public function getPolicy($capability) { - // TODO: Implement proper per-object policies. - return PhabricatorPolicies::POLICY_USER; + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + return $this->getViewPolicy(); + case PhabricatorPolicyCapability::CAN_EDIT: + return PhabricatorPolicies::POLICY_NOONE; + } } public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {