archive/phabricator
1
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

Make Herald rules obey policies during application

Browse Source 
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
This commit is contained in:
epriestley
2013-10-05 12:55:34 -07:00
parent ae27ce0f7d
commit e6d8e1a00a
10 changed files with 84 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
src/applications/files/controller/PhabricatorFileTransformController.php
View File

@@ -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) {

1
src/applications/herald/adapter/HeraldAdapter.php
View File

@@ -114,6 +114,7 @@ abstract class HeraldAdapter {
abstract public function getAdapterContentName();
abstract public function getAdapterApplicationClass();
abstract public function getObject();
/* -( Fields )------------------------------------------------------------- */

4
src/applications/herald/adapter/HeraldCommitAdapter.php
View File

@@ -31,6 +31,10 @@ final class HeraldCommitAdapter extends HeraldAdapter {
return 'PhabricatorApplicationDiffusion';
}
public function getObject() {
return $this->commit;
}
public function getAdapterContentType() {
return 'commit';
}

4
src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
View File

@@ -24,6 +24,10 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
return 'PhabricatorApplicationDifferential';
}
public function getObject() {
return $this->revision;
}
public function getAdapterContentType() {
return 'differential';
}

12
src/applications/herald/adapter/HeraldManiphestTaskAdapter.php
View File

@@ -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,

8
src/applications/herald/adapter/HeraldPholioMockAdapter.php
View File

@@ -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,

5
src/applications/herald/controller/HeraldNewController.php
View File

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

46
src/applications/herald/engine/HeraldEngine.php
View File

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

1
src/applications/herald/query/HeraldRuleQuery.php
View File

@@ -211,6 +211,7 @@ final class HeraldRuleQuery
}
$rule->attachValidAuthor(true);
$rule->attachAuthor($users[$author_phid]);
}
}

10
src/applications/herald/storage/HeraldRule.php
View File

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