From e6d8e1a00ac48f89b0b4fadb2d39b1b93a03db66 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 5 Oct 2013 12:55:34 -0700 Subject: [PATCH] Make Herald rules obey policies during application Summary: Ref T603. This closes the other major policy loophole in Herald, which was that you could write a rule like: When [Always], [Add me to CC] ...and end up getting email about everything. These rules are now enforced: - For a //personal// rule to trigger, you must be able to see the object, and you must be able to use the application the object exists in. - In contrast, //global// rules will //always// trigger. Also fixes some small bugs: - Policy control access to thumbnails was overly restrictive. - The Pholio and Maniphest Herald rules applied only the //last// "Add CC" or "Add Project" rules, since each rule overwrote previous rules. Test Plan: - Created "always cc me" herald and maniphest rules with a normal user. - Created task with "user" visibility, saw CC. - Created task with "no one" visibility, saw no CC and error message in transcript ("user can't see the object"). - Restricted Maniphest to administrators and created a task with "user" visibility. Same deal. - Created "user" and "no one" mocks and saw CC and no CC, respectively. - Thumbnail in Pholio worked properly. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T603 Differential Revision: https://secure.phabricator.com/D7224 --- .../PhabricatorFileTransformController.php | 15 +++--- .../herald/adapter/HeraldAdapter.php | 1 + .../herald/adapter/HeraldCommitAdapter.php | 4 ++ .../HeraldDifferentialRevisionAdapter.php | 4 ++ .../adapter/HeraldManiphestTaskAdapter.php | 12 ++--- .../adapter/HeraldPholioMockAdapter.php | 8 ++-- .../herald/controller/HeraldNewController.php | 5 +- .../herald/engine/HeraldEngine.php | 46 +++++++++++++++++-- .../herald/query/HeraldRuleQuery.php | 1 + .../herald/storage/HeraldRule.php | 10 ++++ 10 files changed, 84 insertions(+), 22 deletions(-) diff --git a/src/applications/files/controller/PhabricatorFileTransformController.php b/src/applications/files/controller/PhabricatorFileTransformController.php index 7a8eb7772d..4e2c7b08e6 100644 --- a/src/applications/files/controller/PhabricatorFileTransformController.php +++ b/src/applications/files/controller/PhabricatorFileTransformController.php @@ -7,21 +7,24 @@ final class PhabricatorFileTransformController private $phid; private $key; + public function shouldRequireLogin() { + return false; + } + public function willProcessRequest(array $data) { $this->transform = $data['transform']; $this->phid = $data['phid']; $this->key = $data['key']; } - public function shouldRequireLogin() { - return false; - } - public function processRequest() { $viewer = $this->getRequest()->getUser(); + // NOTE: This is a public/CDN endpoint, and permission to see files is + // controlled by knowing the secret key, not by authentication. + $file = id(new PhabricatorFileQuery()) - ->setViewer($viewer) + ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withPHIDs(array($this->phid)) ->executeOne(); if (!$file) { @@ -130,7 +133,7 @@ final class PhabricatorFileTransformController PhabricatorTransformedFile $xform) { $file = id(new PhabricatorFileQuery()) - ->setViewer($this->getRequest()->getUser()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withPHIDs(array($xform->getTransformedPHID())) ->executeOne(); if (!$file) { diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 88d726b168..3e867dedf8 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -114,6 +114,7 @@ abstract class HeraldAdapter { abstract public function getAdapterContentName(); abstract public function getAdapterApplicationClass(); + abstract public function getObject(); /* -( Fields )------------------------------------------------------------- */ diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php index 49abc2430f..fe4424e457 100644 --- a/src/applications/herald/adapter/HeraldCommitAdapter.php +++ b/src/applications/herald/adapter/HeraldCommitAdapter.php @@ -31,6 +31,10 @@ final class HeraldCommitAdapter extends HeraldAdapter { return 'PhabricatorApplicationDiffusion'; } + public function getObject() { + return $this->commit; + } + public function getAdapterContentType() { return 'commit'; } diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index 16d29ff1d8..d603796360 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php @@ -24,6 +24,10 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { return 'PhabricatorApplicationDifferential'; } + public function getObject() { + return $this->revision; + } + public function getAdapterContentType() { return 'differential'; } diff --git a/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php b/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php index 68329e7818..b4893b6eea 100644 --- a/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php +++ b/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php @@ -22,6 +22,10 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter { return $this->task; } + public function getObject() { + return $this->task; + } + private function setCcPHIDs(array $cc_phids) { $this->ccPHIDs = $cc_phids; return $this; @@ -118,11 +122,9 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter { pht('Great success at doing nothing.')); break; case self::ACTION_ADD_CC: - $add_cc = array(); foreach ($effect->getTarget() as $phid) { - $add_cc[$phid] = true; + $this->ccPHIDs[] = $phid; } - $this->setCcPHIDs(array_keys($add_cc)); $result[] = new HeraldApplyTranscript( $effect, true, @@ -143,11 +145,9 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter { pht('Assigned task.')); break; case self::ACTION_ADD_PROJECTS: - $add_projects = array(); foreach ($effect->getTarget() as $phid) { - $add_projects[$phid] = true; + $this->projectPHIDs[] = $phid; } - $this->setProjectPHIDs(array_keys($add_projects)); $result[] = new HeraldApplyTranscript( $effect, true, diff --git a/src/applications/herald/adapter/HeraldPholioMockAdapter.php b/src/applications/herald/adapter/HeraldPholioMockAdapter.php index bdf6e8a5f5..d117214de5 100644 --- a/src/applications/herald/adapter/HeraldPholioMockAdapter.php +++ b/src/applications/herald/adapter/HeraldPholioMockAdapter.php @@ -12,6 +12,10 @@ final class HeraldPholioMockAdapter extends HeraldAdapter { return 'PhabricatorApplicationPholio'; } + public function getObject() { + return $this->mock; + } + public function setMock(PholioMock $mock) { $this->mock = $mock; return $this; @@ -97,11 +101,9 @@ final class HeraldPholioMockAdapter extends HeraldAdapter { pht('Great success at doing nothing.')); break; case self::ACTION_ADD_CC: - $add_cc = array(); foreach ($effect->getTarget() as $phid) { - $add_cc[$phid] = true; + $this->ccPHIDs[] = $phid; } - $this->setCcPHIDs(array_keys($add_cc)); $result[] = new HeraldApplyTranscript( $effect, true, diff --git a/src/applications/herald/controller/HeraldNewController.php b/src/applications/herald/controller/HeraldNewController.php index ab87985440..ec4b08e810 100644 --- a/src/applications/herald/controller/HeraldNewController.php +++ b/src/applications/herald/controller/HeraldNewController.php @@ -49,7 +49,8 @@ final class HeraldNewController extends HeraldController { HeraldRuleTypeConfig::RULE_TYPE_PERSONAL => pht( 'Personal rules notify you about events. You own them, but they can '. - 'only affect you.'), + 'only affect you. Personal rules only trigger for objects you have '. + 'permission to see.'), HeraldRuleTypeConfig::RULE_TYPE_GLOBAL => phutil_implode_html( phutil_tag('br'), @@ -57,7 +58,7 @@ final class HeraldNewController extends HeraldController { array( pht( 'Global rules notify anyone about events. Global rules can '. - 'bypass access control policies.'), + 'bypass access control policies and act on any object.'), $global_link, ))), ); diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php index 83034584fe..28eeee7ff4 100644 --- a/src/applications/herald/engine/HeraldEngine.php +++ b/src/applications/herald/engine/HeraldEngine.php @@ -233,15 +233,23 @@ final class HeraldEngine { $local_version = id(new HeraldRule())->getConfigVersion(); if ($rule->getConfigVersion() > $local_version) { - $reason = "Rule could not be processed, it was created with a newer ". - "version of Herald."; + $reason = pht( + "Rule could not be processed, it was created with a newer version ". + "of Herald."); $result = false; } else if (!$conditions) { - $reason = "Rule failed automatically because it has no conditions."; + $reason = pht( + "Rule failed automatically because it has no conditions."); $result = false; } else if (!$rule->hasValidAuthor()) { - $reason = "Rule failed automatically because its owner is invalid ". - "or disabled."; + $reason = pht( + "Rule failed automatically because its owner is invalid ". + "or disabled."); + $result = false; + } else if (!$this->canAuthorViewObject($rule, $object)) { + $reason = pht( + "Rule failed automatically because it is a personal rule and its ". + "owner can not see the object."); $result = false; } else { foreach ($conditions as $condition) { @@ -361,4 +369,32 @@ final class HeraldEngine { return $effects; } + private function canAuthorViewObject( + HeraldRule $rule, + HeraldAdapter $adapter) { + + // Authorship is irrelevant for global rules. + if ($rule->isGlobalRule()) { + return true; + } + + // The author must be able to create rules for the adapter's content type. + // In particular, this means that the application must be installed and + // accessible to the user. For example, if a user writes a Differential + // rule and then loses access to Differential, this disables the rule. + $enabled = HeraldAdapter::getEnabledAdapterMap($rule->getAuthor()); + if (empty($enabled[$adapter->getAdapterContentType()])) { + return false; + } + + // Finally, the author must be able to see the object itself. You can't + // write a personal rule that CC's you on revisions you wouldn't otherwise + // be able to see, for example. + $object = $adapter->getObject(); + return PhabricatorPolicyFilter::hasCapability( + $rule->getAuthor(), + $object, + PhabricatorPolicyCapability::CAN_VIEW); + } + } diff --git a/src/applications/herald/query/HeraldRuleQuery.php b/src/applications/herald/query/HeraldRuleQuery.php index 4e1ea93545..e153888090 100644 --- a/src/applications/herald/query/HeraldRuleQuery.php +++ b/src/applications/herald/query/HeraldRuleQuery.php @@ -211,6 +211,7 @@ final class HeraldRuleQuery } $rule->attachValidAuthor(true); + $rule->attachAuthor($users[$author_phid]); } } diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index 2f97d154c6..52089c20e2 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -17,6 +17,7 @@ final class HeraldRule extends HeraldDAO private $ruleApplied = self::ATTACHABLE; // phids for which this rule has been applied private $validAuthor = self::ATTACHABLE; + private $author = self::ATTACHABLE; private $conditions; private $actions; @@ -167,6 +168,15 @@ final class HeraldRule extends HeraldDAO return $this; } + public function getAuthor() { + return $this->assertAttached($this->author); + } + + public function attachAuthor(PhabricatorUser $user) { + $this->author = $user; + return $this; + } + public function isGlobalRule() { return ($this->getRuleType() === HeraldRuleTypeConfig::RULE_TYPE_GLOBAL); }