From aa3b2ec5dcd0ba91e36d5a0b59f37f27ec78b57c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 18 Dec 2018 15:54:27 -0800 Subject: [PATCH] Give Pholio Images an authorPHID and use ExtendedPolicies to implement policy behavior Summary: Depends on D19912. Ref T11351. Images currently use `getMock()->getPolicy()` stuff to define policies. This causes bugs with object policies like "Subscribers", since the policy engine tries to evaluate the subscribers //for the image// when the intent is to evaluate the subscribers for the mock. Move this to ExtendedPolicies to fix the behavior, and give Images sensible policy behavior when they aren't attached to a mock (specifically: only the user who created the image can see it). Test Plan: Applied migrations, created and edited mocks and images without anything blowing up. Set mock visibility to "Subscribers", everything worked great. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T11351 Differential Revision: https://secure.phabricator.com/D19913 --- .../20181218.pholio.01.imageauthor.sql | 2 + src/__phutil_library_map__.php | 1 + .../PholioImageUploadController.php | 1 + .../controller/PholioMockEditController.php | 2 + .../controller/PholioMockViewController.php | 19 ++++---- ...PhabricatorPholioMockTestDataGenerator.php | 1 + .../pholio/storage/PholioImage.php | 46 +++++++++++++++---- 7 files changed, 53 insertions(+), 19 deletions(-) create mode 100644 resources/sql/autopatches/20181218.pholio.01.imageauthor.sql diff --git a/resources/sql/autopatches/20181218.pholio.01.imageauthor.sql b/resources/sql/autopatches/20181218.pholio.01.imageauthor.sql new file mode 100644 index 0000000000..4ff0a16258 --- /dev/null +++ b/resources/sql/autopatches/20181218.pholio.01.imageauthor.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_pholio.pholio_image + ADD authorPHID VARBINARY(64) NOT NULL; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index deaaae0e98..7767a93561 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -10947,6 +10947,7 @@ phutil_register_library_map(array( 'PholioImage' => array( 'PholioDAO', 'PhabricatorPolicyInterface', + 'PhabricatorExtendedPolicyInterface', ), 'PholioImageDescriptionTransaction' => 'PholioImageTransactionType', 'PholioImageFileTransaction' => 'PholioImageTransactionType', diff --git a/src/applications/pholio/controller/PholioImageUploadController.php b/src/applications/pholio/controller/PholioImageUploadController.php index 39dd661a4a..0ff5e061f5 100644 --- a/src/applications/pholio/controller/PholioImageUploadController.php +++ b/src/applications/pholio/controller/PholioImageUploadController.php @@ -23,6 +23,7 @@ final class PholioImageUploadController extends PholioController { } $image = PholioImage::initializeNewImage() + ->setAuthorPHID($viewer->getPHID()) ->attachFile($file) ->setName($title) ->setDescription($description) diff --git a/src/applications/pholio/controller/PholioMockEditController.php b/src/applications/pholio/controller/PholioMockEditController.php index 9a09c9bc67..fd1df77a8e 100644 --- a/src/applications/pholio/controller/PholioMockEditController.php +++ b/src/applications/pholio/controller/PholioMockEditController.php @@ -141,6 +141,7 @@ final class PholioMockEditController extends PholioController { if ($replaces_image_phid) { $replace_image = PholioImage::initializeNewImage() + ->setAuthorPHID($viewer->getPHID()) ->setReplacesImagePHID($replaces_image_phid) ->setFilePhid($file_phid) ->attachFile($file) @@ -154,6 +155,7 @@ final class PholioMockEditController extends PholioController { $posted_mock_images[] = $replace_image; } else if (!$existing_image) { // this is an add $add_image = PholioImage::initializeNewImage() + ->setAuthorPHID($viewer->getPHID()) ->setFilePhid($file_phid) ->attachFile($file) ->setName(strlen($title) ? $title : $file->getName()) diff --git a/src/applications/pholio/controller/PholioMockViewController.php b/src/applications/pholio/controller/PholioMockViewController.php index fd601f0a32..e24889875a 100644 --- a/src/applications/pholio/controller/PholioMockViewController.php +++ b/src/applications/pholio/controller/PholioMockViewController.php @@ -82,7 +82,7 @@ final class PholioMockViewController extends PholioController { $add_comment = $this->buildAddCommentView($mock, $comment_form_id); $crumbs = $this->buildApplicationCrumbs(); - $crumbs->addTextCrumb('M'.$mock->getID(), '/M'.$mock->getID()); + $crumbs->addTextCrumb($mock->getMonogram(), $mock->getURI()); $crumbs->setBorder(true); $thumb_grid = id(new PholioMockThumbGridView()) @@ -92,16 +92,17 @@ final class PholioMockViewController extends PholioController { $view = id(new PHUITwoColumnView()) ->setHeader($header) ->setCurtain($curtain) - ->setMainColumn(array( - $output, - $thumb_grid, - $details, - $timeline, - $add_comment, - )); + ->setMainColumn( + array( + $output, + $thumb_grid, + $details, + $timeline, + $add_comment, + )); return $this->newPage() - ->setTitle('M'.$mock->getID().' '.$title) + ->setTitle(pht('%s %s', $mock->getMonogram(), $title)) ->setCrumbs($crumbs) ->setPageObjectPHIDs(array($mock->getPHID())) ->addQuicksandConfig( diff --git a/src/applications/pholio/lipsum/PhabricatorPholioMockTestDataGenerator.php b/src/applications/pholio/lipsum/PhabricatorPholioMockTestDataGenerator.php index c038fcf922..041e14fcbf 100644 --- a/src/applications/pholio/lipsum/PhabricatorPholioMockTestDataGenerator.php +++ b/src/applications/pholio/lipsum/PhabricatorPholioMockTestDataGenerator.php @@ -42,6 +42,7 @@ final class PhabricatorPholioMockTestDataGenerator $images = array(); foreach ($files as $file) { $image = PholioImage::initializeNewImage() + ->setAuthorPHID($author_phid) ->setFilePHID($file->getPHID()) ->setSequence($sequence++) ->attachMock($mock); diff --git a/src/applications/pholio/storage/PholioImage.php b/src/applications/pholio/storage/PholioImage.php index 92694fb806..0f800bbb0f 100644 --- a/src/applications/pholio/storage/PholioImage.php +++ b/src/applications/pholio/storage/PholioImage.php @@ -2,8 +2,10 @@ final class PholioImage extends PholioDAO implements - PhabricatorPolicyInterface { + PhabricatorPolicyInterface, + PhabricatorExtendedPolicyInterface { + protected $authorPHID; protected $mockID; protected $filePHID; protected $name; @@ -57,8 +59,7 @@ final class PholioImage extends PholioDAO } public function getFile() { - $this->assertAttached($this->file); - return $this->file; + return $this->assertAttached($this->file); } public function attachMock(PholioMock $mock) { @@ -67,8 +68,7 @@ final class PholioImage extends PholioDAO } public function getMock() { - $this->assertAttached($this->mock); - return $this->mock; + return $this->assertAttached($this->mock); } public function attachInlineComments(array $inline_comments) { @@ -83,20 +83,46 @@ final class PholioImage extends PholioDAO } -/* -( PhabricatorPolicyInterface Implementation )-------------------------- */ +/* -( PhabricatorPolicyInterface )----------------------------------------- */ public function getCapabilities() { - return $this->getMock()->getCapabilities(); + return array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + ); } public function getPolicy($capability) { - return $this->getMock()->getPolicy($capability); + // If the image is attached to a mock, we use an extended policy to match + // the mock's permissions. + if ($this->getMockID()) { + return PhabricatorPolicies::getMostOpenPolicy(); + } + + // If the image is not attached to a mock, only the author can see it. + return $this->getAuthorPHID(); } - // really the *mock* controls who can see an image public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { - return $this->getMock()->hasAutomaticCapability($capability, $viewer); + return false; + } + + +/* -( PhabricatorExtendedPolicyInterface )--------------------------------- */ + + + public function getExtendedPolicy($capability, PhabricatorUser $viewer) { + if ($this->getMockID()) { + return array( + array( + $this->getMock(), + $capability, + ), + ); + } + + return array(); } }