mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-30 01:10:58 +01:00
Add a "Subscribers" object policy
Summary: Ref T5681. Getting this to work correctly is a bit tricky, mostly because of the policy checks we do prior to applying an edit. I think I came up with a mostly-reasonable approach, although it's a little bit gross. It uses `spl_object_hash()` so it shouldn't be able to do anything bad/dangerous (the hints are strictly bound to the hinted object, which is a clone that we destroy moments later). Test Plan: - Added + ran a unit test. - Created a task with a "Subscribers" policy with me as a subscriber (without the hint stuff, this isn't possible: since you aren't a subscriber *yet*, you get a "you won't be able to see it" error). - Unsubscribed from a task with a "Subscribers" policy, was immediately unable to see it. - Created a task with a "subscribers" policy and a project subscriber with/without me as a member (error / success, respectively). Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T5681 Differential Revision: https://secure.phabricator.com/D13259
This commit is contained in:
parent
893c7a26c1
commit
3de3a72dd8
5 changed files with 213 additions and 0 deletions
|
@ -2645,6 +2645,7 @@ phutil_register_library_map(array(
|
||||||
'PhabricatorSubscriptionsEditor' => 'applications/subscriptions/editor/PhabricatorSubscriptionsEditor.php',
|
'PhabricatorSubscriptionsEditor' => 'applications/subscriptions/editor/PhabricatorSubscriptionsEditor.php',
|
||||||
'PhabricatorSubscriptionsListController' => 'applications/subscriptions/controller/PhabricatorSubscriptionsListController.php',
|
'PhabricatorSubscriptionsListController' => 'applications/subscriptions/controller/PhabricatorSubscriptionsListController.php',
|
||||||
'PhabricatorSubscriptionsSubscribeEmailCommand' => 'applications/subscriptions/command/PhabricatorSubscriptionsSubscribeEmailCommand.php',
|
'PhabricatorSubscriptionsSubscribeEmailCommand' => 'applications/subscriptions/command/PhabricatorSubscriptionsSubscribeEmailCommand.php',
|
||||||
|
'PhabricatorSubscriptionsSubscribersPolicyRule' => 'applications/subscriptions/policyrule/PhabricatorSubscriptionsSubscribersPolicyRule.php',
|
||||||
'PhabricatorSubscriptionsTransactionController' => 'applications/subscriptions/controller/PhabricatorSubscriptionsTransactionController.php',
|
'PhabricatorSubscriptionsTransactionController' => 'applications/subscriptions/controller/PhabricatorSubscriptionsTransactionController.php',
|
||||||
'PhabricatorSubscriptionsUIEventListener' => 'applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php',
|
'PhabricatorSubscriptionsUIEventListener' => 'applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php',
|
||||||
'PhabricatorSubscriptionsUnsubscribeEmailCommand' => 'applications/subscriptions/command/PhabricatorSubscriptionsUnsubscribeEmailCommand.php',
|
'PhabricatorSubscriptionsUnsubscribeEmailCommand' => 'applications/subscriptions/command/PhabricatorSubscriptionsUnsubscribeEmailCommand.php',
|
||||||
|
@ -6166,6 +6167,7 @@ phutil_register_library_map(array(
|
||||||
'PhabricatorSubscriptionsEditor' => 'PhabricatorEditor',
|
'PhabricatorSubscriptionsEditor' => 'PhabricatorEditor',
|
||||||
'PhabricatorSubscriptionsListController' => 'PhabricatorController',
|
'PhabricatorSubscriptionsListController' => 'PhabricatorController',
|
||||||
'PhabricatorSubscriptionsSubscribeEmailCommand' => 'MetaMTAEmailTransactionCommand',
|
'PhabricatorSubscriptionsSubscribeEmailCommand' => 'MetaMTAEmailTransactionCommand',
|
||||||
|
'PhabricatorSubscriptionsSubscribersPolicyRule' => 'PhabricatorPolicyRule',
|
||||||
'PhabricatorSubscriptionsTransactionController' => 'PhabricatorController',
|
'PhabricatorSubscriptionsTransactionController' => 'PhabricatorController',
|
||||||
'PhabricatorSubscriptionsUIEventListener' => 'PhabricatorEventListener',
|
'PhabricatorSubscriptionsUIEventListener' => 'PhabricatorEventListener',
|
||||||
'PhabricatorSubscriptionsUnsubscribeEmailCommand' => 'MetaMTAEmailTransactionCommand',
|
'PhabricatorSubscriptionsUnsubscribeEmailCommand' => 'MetaMTAEmailTransactionCommand',
|
||||||
|
|
|
@ -208,5 +208,32 @@ final class PhabricatorPolicyDataTestCase extends PhabricatorTestCase {
|
||||||
PhabricatorPolicyCapability::CAN_VIEW));
|
PhabricatorPolicyCapability::CAN_VIEW));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public function testObjectPolicyRuleSubscribers() {
|
||||||
|
$author = $this->generateNewTestUser();
|
||||||
|
|
||||||
|
$rule = new PhabricatorSubscriptionsSubscribersPolicyRule();
|
||||||
|
|
||||||
|
$task = ManiphestTask::initializeNewTask($author);
|
||||||
|
$task->setViewPolicy($rule->getObjectPolicyFullKey());
|
||||||
|
$task->save();
|
||||||
|
|
||||||
|
$this->assertFalse(
|
||||||
|
PhabricatorPolicyFilter::hasCapability(
|
||||||
|
$author,
|
||||||
|
$task,
|
||||||
|
PhabricatorPolicyCapability::CAN_VIEW));
|
||||||
|
|
||||||
|
id(new PhabricatorSubscriptionsEditor())
|
||||||
|
->setActor($author)
|
||||||
|
->setObject($task)
|
||||||
|
->subscribeExplicit(array($author->getPHID()))
|
||||||
|
->save();
|
||||||
|
|
||||||
|
$this->assertTrue(
|
||||||
|
PhabricatorPolicyFilter::hasCapability(
|
||||||
|
$author,
|
||||||
|
$task,
|
||||||
|
PhabricatorPolicyCapability::CAN_VIEW));
|
||||||
|
}
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
|
@ -97,6 +97,56 @@ abstract class PhabricatorPolicyRule {
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
/* -( Transaction Hints )-------------------------------------------------- */
|
||||||
|
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Tell policy rules about upcoming transaction effects.
|
||||||
|
*
|
||||||
|
* Before transaction effects are applied, we try to stop users from making
|
||||||
|
* edits which will lock them out of objects. We can't do this perfectly,
|
||||||
|
* since they can set a policy to "the moon is full" moments before it wanes,
|
||||||
|
* but we try to prevent as many mistakes as possible.
|
||||||
|
*
|
||||||
|
* Some policy rules depend on complex checks against object state which
|
||||||
|
* we can't set up ahead of time. For example, subscriptions require database
|
||||||
|
* writes.
|
||||||
|
*
|
||||||
|
* In cases like this, instead of doing writes, you can pass a hint about an
|
||||||
|
* object to a policy rule. The rule can then look for hints and use them in
|
||||||
|
* rendering a verdict about whether the user will be able to see the object
|
||||||
|
* or not after applying the policy change.
|
||||||
|
*
|
||||||
|
* @param PhabricatorPolicyInterface Object to pass a hint about.
|
||||||
|
* @param PhabricatorPolicyRule Rule to pass hint to.
|
||||||
|
* @param wild Hint.
|
||||||
|
* @return void
|
||||||
|
*/
|
||||||
|
public static function passTransactionHintToRule(
|
||||||
|
PhabricatorPolicyInterface $object,
|
||||||
|
PhabricatorPolicyRule $rule,
|
||||||
|
$hint) {
|
||||||
|
|
||||||
|
$cache = PhabricatorCaches::getRequestCache();
|
||||||
|
$cache->setKey(self::getObjectPolicyCacheKey($object, $rule), $hint);
|
||||||
|
}
|
||||||
|
|
||||||
|
protected function getTransactionHint(
|
||||||
|
PhabricatorPolicyInterface $object) {
|
||||||
|
|
||||||
|
$cache = PhabricatorCaches::getRequestCache();
|
||||||
|
return $cache->getKey(self::getObjectPolicyCacheKey($object, $this));
|
||||||
|
}
|
||||||
|
|
||||||
|
private static function getObjectPolicyCacheKey(
|
||||||
|
PhabricatorPolicyInterface $object,
|
||||||
|
PhabricatorPolicyRule $rule) {
|
||||||
|
$hash = spl_object_hash($object);
|
||||||
|
$rule = get_class($rule);
|
||||||
|
return 'policycache.'.$hash.'.'.$rule;
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
/* -( Implementing Object Policies )--------------------------------------- */
|
/* -( Implementing Object Policies )--------------------------------------- */
|
||||||
|
|
||||||
|
|
||||||
|
|
|
@ -0,0 +1,121 @@
|
||||||
|
<?php
|
||||||
|
|
||||||
|
final class PhabricatorSubscriptionsSubscribersPolicyRule
|
||||||
|
extends PhabricatorPolicyRule {
|
||||||
|
|
||||||
|
private $subscribed = array();
|
||||||
|
private $sourcePHIDs = array();
|
||||||
|
|
||||||
|
public function getObjectPolicyKey() {
|
||||||
|
return 'subscriptions.subscribers';
|
||||||
|
}
|
||||||
|
|
||||||
|
public function getObjectPolicyName() {
|
||||||
|
return pht('Subscribers');
|
||||||
|
}
|
||||||
|
|
||||||
|
public function getPolicyExplanation() {
|
||||||
|
return pht('Subscribers can take this action.');
|
||||||
|
}
|
||||||
|
|
||||||
|
public function getRuleDescription() {
|
||||||
|
return pht('subscribers');
|
||||||
|
}
|
||||||
|
|
||||||
|
public function canApplyToObject(PhabricatorPolicyInterface $object) {
|
||||||
|
return ($object instanceof PhabricatorSubscribableInterface);
|
||||||
|
}
|
||||||
|
|
||||||
|
public function willApplyRules(
|
||||||
|
PhabricatorUser $viewer,
|
||||||
|
array $values,
|
||||||
|
array $objects) {
|
||||||
|
|
||||||
|
// We want to let the user see the object if they're a subscriber or
|
||||||
|
// a member of any project which is a subscriber. Additionally, because
|
||||||
|
// subscriber state is complex, we need to read hints passed from
|
||||||
|
// the TransactionEditor to predict policy state after transactions apply.
|
||||||
|
|
||||||
|
$viewer_phid = $viewer->getPHID();
|
||||||
|
if (!$viewer_phid) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (empty($this->subscribed[$viewer_phid])) {
|
||||||
|
$this->subscribed[$viewer_phid] = array();
|
||||||
|
}
|
||||||
|
|
||||||
|
// Load the project PHIDs the user is a member of.
|
||||||
|
if (!isset($this->sourcePHIDs[$viewer_phid])) {
|
||||||
|
$source_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
|
||||||
|
$viewer_phid,
|
||||||
|
PhabricatorProjectMemberOfProjectEdgeType::EDGECONST);
|
||||||
|
$source_phids[] = $viewer_phid;
|
||||||
|
$this->sourcePHIDs[$viewer_phid] = $source_phids;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Look for transaction hints.
|
||||||
|
foreach ($objects as $key => $object) {
|
||||||
|
$cache = $this->getTransactionHint($object);
|
||||||
|
if ($cache === null) {
|
||||||
|
// We don't have a hint for this object, so we'll deal with it below.
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
// We have a hint, so use that as the source of truth.
|
||||||
|
unset($objects[$key]);
|
||||||
|
|
||||||
|
foreach ($this->sourcePHIDs[$viewer_phid] as $source_phid) {
|
||||||
|
if (isset($cache[$source_phid])) {
|
||||||
|
$this->subscribed[$viewer_phid][$object->getPHID()] = true;
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
$phids = mpull($objects, 'getPHID');
|
||||||
|
if (!$phids) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
$edge_query = id(new PhabricatorEdgeQuery())
|
||||||
|
->withSourcePHIDs($this->sourcePHIDs[$viewer_phid])
|
||||||
|
->withEdgeTypes(
|
||||||
|
array(
|
||||||
|
PhabricatorSubscribedToObjectEdgeType::EDGECONST,
|
||||||
|
))
|
||||||
|
->withDestinationPHIDs($phids);
|
||||||
|
|
||||||
|
$edge_query->execute();
|
||||||
|
|
||||||
|
$subscribed = $edge_query->getDestinationPHIDs();
|
||||||
|
if (!$subscribed) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
$this->subscribed[$viewer_phid] += array_fill_keys($subscribed, true);
|
||||||
|
}
|
||||||
|
|
||||||
|
public function applyRule(
|
||||||
|
PhabricatorUser $viewer,
|
||||||
|
$value,
|
||||||
|
PhabricatorPolicyInterface $object) {
|
||||||
|
|
||||||
|
$viewer_phid = $viewer->getPHID();
|
||||||
|
if (!$viewer_phid) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($object->isAutomaticallySubscribed($viewer_phid)) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
$subscribed = idx($this->subscribed, $viewer_phid);
|
||||||
|
return isset($subscribed[$object->getPHID()]);
|
||||||
|
}
|
||||||
|
|
||||||
|
public function getValueControlType() {
|
||||||
|
return self::CONTROL_TYPE_NONE;
|
||||||
|
}
|
||||||
|
|
||||||
|
}
|
|
@ -2087,6 +2087,19 @@ abstract class PhabricatorApplicationTransactionEditor
|
||||||
|
|
||||||
foreach ($xactions as $xaction) {
|
foreach ($xactions as $xaction) {
|
||||||
switch ($xaction->getTransactionType()) {
|
switch ($xaction->getTransactionType()) {
|
||||||
|
case PhabricatorTransactions::TYPE_SUBSCRIBERS:
|
||||||
|
$clone_xaction = clone $xaction;
|
||||||
|
$clone_xaction->setOldValue(array_values($this->subscribers));
|
||||||
|
$clone_xaction->setNewValue(
|
||||||
|
$this->getPHIDTransactionNewValue(
|
||||||
|
$clone_xaction));
|
||||||
|
|
||||||
|
PhabricatorPolicyRule::passTransactionHintToRule(
|
||||||
|
$copy,
|
||||||
|
new PhabricatorSubscriptionsSubscribersPolicyRule(),
|
||||||
|
array_fuse($clone_xaction->getNewValue()));
|
||||||
|
|
||||||
|
break;
|
||||||
case PhabricatorTransactions::TYPE_SPACE:
|
case PhabricatorTransactions::TYPE_SPACE:
|
||||||
$space_phid = $this->getTransactionNewValue($object, $xaction);
|
$space_phid = $this->getTransactionNewValue($object, $xaction);
|
||||||
$copy->setSpacePHID($space_phid);
|
$copy->setSpacePHID($space_phid);
|
||||||
|
|
Loading…
Reference in a new issue