revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-22 14:52:41 +01:00
Code Issues Releases Wiki Activity

Remove requireCapabilities() from ApplicationTransactionEditor and require CAN_EDIT by default

Browse source 
Summary:
Depends on D19585. Ref T13164.

Almost all transactions require CAN_EDIT on the object, but they generally do not enforce this directly today. Instead, this is effectively enforced by Controllers, API methods, and EditEngine doing a `CAN_EDIT` check when loading the object to be edited.

A small number of transactions do not require CAN_EDIT, and instead require only a weaker/lesser permission. These are:

  - Joining a project which you have CAN_JOIN on.
  - Leaving a project which isn't locked.
  - Joining a Conpherence thread you can see (today, no separate CAN_JOIN permission for Conpherence).
  - Leaving a Conpherence thread.
  - Unsubscribing.
  - Using the special `!history` command from email.

Additionally, these require CAN_INTERACT, which is weaker than CAN_EDIT:

  - Adding comments.
  - Subscribing.
  - Awarding tokens.

Soon, I want to add "disabling users" to this list, so that you can disable users if you have "Can Disable User" permission, even if you can not otherwise edit users.

It's possible this list isn't exhaustive, so this change might break something by adding a policy check to a place where we previously didn't have one. If so, we can go weaken that policy check to the appropriate level.

Enforcement of these special cases is currently weird:

  - We mostly don't actually enforce CAN_EDIT in the Editor; instead, it's enforced before you get to the editor (in EditEngine/Controllers).
  - To apply a weaker requirement (like leaving comments or leaving a project), we let you get through the Controller without CAN_EDIT, then apply the weaker policy check in the Editor.
  - Some transactions apply a confusing/redundant explicit CAN_EDIT policy check. These mostly got cleaned up in previous changes.

Instead, the new world order is:

  - Every transaction has capability/policy requirements.
  - The default is CAN_EDIT, but transactions can weaken this explicitly they want.
  - So now we'll get requirements right in the Editor, even if Controllers or API endpoints make a mistake.
  - And you don't have to copy/paste a bunch of code to say "yes, every transaction should require CAN_EDIT".

Test Plan:
- Tried to add members to a Conpherence thread I could not edit (permissions error).
- Left a Conpherence thread I could not edit (worked properly).
- Joined a thread I could see but could not edit (worked properly).
- Tried to join a thread I could not see (permissions error).
- Implemented `requireCapabilites()` on ManiphestTransactionEditor and tried to edit a task (upgrade guidance error).
- Mentioned an object I can not edit on another object (works).
- Mentioned another object on an object I can not edit (works).
- Added a `{F...}` reference to an object I can not edit (works).
- Awarded tokens to an object I can not edit (works).
- Subscribed/unsubscribed from an object I can not edit (works).
- Muted/unmuted an object I can not edit (works).
- Tried to do other types of edits to an object I can not edit (correctly results in a permissions error).
- Joined and left a project I can not edit (works).
- Tried to edit and add members to a project I can not edit (permissions error).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19586
This commit is contained in:
epriestley 2018-08-14 14:29:23 -07:00
parent b584834b19
commit 4d89afcc61
6 changed files with 214 additions and 123 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

43
src/applications/conpherence/editor/ConpherenceEditor.php
View file

@ -146,49 +146,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
return $xactions;
}
protected function requireCapabilities(
PhabricatorLiskDAO $object,
PhabricatorApplicationTransaction $xaction) {
parent::requireCapabilities($object, $xaction);
switch ($xaction->getTransactionType()) {
case ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE:
$old_map = array_fuse($xaction->getOldValue());
$new_map = array_fuse($xaction->getNewValue());
$add = array_keys(array_diff_key($new_map, $old_map));
$rem = array_keys(array_diff_key($old_map, $new_map));
$actor_phid = $this->getActingAsPHID();
$is_join = (($add === array($actor_phid)) && !$rem);
$is_leave = (($rem === array($actor_phid)) && !$add);
if ($is_join) {
// Anyone can join a thread they can see.
} else if ($is_leave) {
// Anyone can leave a thread.
} else {
// You need CAN_EDIT to add or remove participants. For additional
// discussion, see D17696 and T4411.
PhabricatorPolicyFilter::requireCapability(
$this->requireActor(),
$object,
PhabricatorPolicyCapability::CAN_EDIT);
}
break;
case ConpherenceThreadTitleTransaction::TRANSACTIONTYPE:
case ConpherenceThreadTopicTransaction::TRANSACTIONTYPE:
PhabricatorPolicyFilter::requireCapability(
$this->requireActor(),
$object,
PhabricatorPolicyCapability::CAN_EDIT);
break;
}
}
protected function shouldSendMail(
PhabricatorLiskDAO $object,
array $xactions) {

30
src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php
View file

@ -114,4 +114,34 @@ final class ConpherenceThreadParticipantsTransaction
return $errors;
}
public function getRequiredCapabilities(
$object,
PhabricatorApplicationTransaction $xaction) {
$old_map = array_fuse($xaction->getOldValue());
$new_map = array_fuse($xaction->getNewValue());
$add = array_keys(array_diff_key($new_map, $old_map));
$rem = array_keys(array_diff_key($old_map, $new_map));
$actor_phid = $this->getActingAsPHID();
$is_join = (($add === array($actor_phid)) && !$rem);
$is_leave = (($rem === array($actor_phid)) && !$add);
if ($is_join) {
// Anyone can join a thread they can see.
return null;
}
if ($is_leave) {
// Anyone can leave a thread.
return null;
}
// You need CAN_EDIT to add or remove participants. For additional
// discussion, see D17696 and T4411.
return PhabricatorPolicyCapability::CAN_EDIT;
}
}

52
src/applications/project/editor/PhabricatorProjectTransactionEditor.php
View file

@ -115,58 +115,6 @@ final class PhabricatorProjectTransactionEditor
return $errors;
}
protected function requireCapabilities(
PhabricatorLiskDAO $object,
PhabricatorApplicationTransaction $xaction) {
switch ($xaction->getTransactionType()) {
case PhabricatorTransactions::TYPE_EDGE:
switch ($xaction->getMetadataValue('edge:type')) {
case PhabricatorProjectProjectHasMemberEdgeType::EDGECONST:
$old = $xaction->getOldValue();
$new = $xaction->getNewValue();
$add = array_keys(array_diff_key($new, $old));
$rem = array_keys(array_diff_key($old, $new));
$actor_phid = $this->requireActor()->getPHID();
$is_join = (($add === array($actor_phid)) && !$rem);
$is_leave = (($rem === array($actor_phid)) && !$add);
if ($is_join) {
// You need CAN_JOIN to join a project.
PhabricatorPolicyFilter::requireCapability(
$this->requireActor(),
$object,
PhabricatorPolicyCapability::CAN_JOIN);
} else if ($is_leave) {
// You usually don't need any capabilities to leave a project.
if ($object->getIsMembershipLocked()) {
// you must be able to edit though to leave locked projects
PhabricatorPolicyFilter::requireCapability(
$this->requireActor(),
$object,
PhabricatorPolicyCapability::CAN_EDIT);
}
} else {
if (!$this->getIsNewObject()) {
// You need CAN_EDIT to change members other than yourself.
// (PHI193) Just skip this check if we're creating a project.
PhabricatorPolicyFilter::requireCapability(
$this->requireActor(),
$object,
PhabricatorPolicyCapability::CAN_EDIT);
}
}
return;
}
break;
}
return parent::requireCapabilities($object, $xaction);
}
protected function willPublish(PhabricatorLiskDAO $object, array $xactions) {
// NOTE: We're using the omnipotent user here because the original actor
// may no longer have permission to view the object.

2
src/applications/subscriptions/controller/PhabricatorSubscriptionsMuteController.php
View file

@ -44,8 +44,6 @@ final class PhabricatorSubscriptionsMuteController
);
}
$muted_type = PhabricatorMutedByEdgeType::EDGECONST;
$xaction = id($object->getApplicationTransactionTemplate())
->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
->setMetadataValue('edge:type', $muted_type)

184
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
View file

@ -976,6 +976,27 @@ abstract class PhabricatorApplicationTransactionEditor
$this->adjustTransactionValues($object, $xaction);
}
// Now that we've merged and combined transactions, check for required
// capabilities. Note that we're doing this before filtering
// transactions: if you try to apply an edit which you do not have
// permission to apply, we want to give you a permissions error even
// if the edit would have no effect.
$this->applyCapabilityChecks($object, $xactions);
// See T13186. Fatal hard if this object has an older
// "requireCapabilities()" method. The code may rely on this method being
// called to apply policy checks, so err on the side of safety and fatal.
// TODO: Remove this check after some time has passed.
if (method_exists($this, 'requireCapabilities')) {
throw new Exception(
pht(
'Editor (of class "%s") implements obsolete policy method '.
'requireCapabilities(). The implementation for this Editor '.
'MUST be updated. See <%s> for discussion.',
get_class($this),
'https://secure.phabricator.com/T13186'));
}
$xactions = $this->filterTransactions($object, $xactions);
// TODO: Once everything is on EditEngine, just use getIsNewObject() to
@ -994,12 +1015,6 @@ abstract class PhabricatorApplicationTransactionEditor
}
}
// Now that we've merged, filtered, and combined transactions, check for
// required capabilities.
foreach ($xactions as $xaction) {
$this->requireCapabilities($object, $xaction);
}
$xactions = $this->sortTransactions($xactions);
$file_phids = $this->extractFilePHIDs($object, $xactions);
@ -1459,34 +1474,151 @@ abstract class PhabricatorApplicationTransactionEditor
}
}
protected function requireCapabilities(
private function applyCapabilityChecks(
PhabricatorLiskDAO $object,
PhabricatorApplicationTransaction $xaction) {
array $xactions) {
assert_instances_of($xactions, 'PhabricatorApplicationTransaction');
$can_edit = PhabricatorPolicyCapability::CAN_EDIT;
if ($this->getIsNewObject()) {
return;
// If we're creating a new object, we don't need any special capabilities
// on the object. The actor has already made it through creation checks,
// and objects which haven't been created yet often can not be
// meaningfully tested for capabilities anyway.
$required_capabilities = array();
} else {
if (!$xactions && !$this->xactions) {
// If we aren't doing anything, require CAN_EDIT to improve consistency.
$required_capabilities = array($can_edit);
} else {
$required_capabilities = array();
foreach ($xactions as $xaction) {
$type = $xaction->getTransactionType();
$xtype = $this->getModularTransactionType($type);
if (!$xtype) {
$capabilities = $this->getLegacyRequiredCapabilities($xaction);
} else {
$capabilities = $xtype->getRequiredCapabilities($object, $xaction);
}
// For convenience, we allow flexibility in the return types because
// it's very unusual that a transaction actually requires multiple
// capability checks.
if ($capabilities === null) {
$capabilities = array();
} else {
$capabilities = (array)$capabilities;
}
foreach ($capabilities as $capability) {
$required_capabilities[$capability] = $capability;
}
}
}
}
$actor = $this->requireActor();
switch ($xaction->getTransactionType()) {
case PhabricatorTransactions::TYPE_COMMENT:
PhabricatorPolicyFilter::requireCapability(
$actor,
$object,
PhabricatorPolicyCapability::CAN_VIEW);
break;
case PhabricatorTransactions::TYPE_VIEW_POLICY:
case PhabricatorTransactions::TYPE_EDIT_POLICY:
case PhabricatorTransactions::TYPE_JOIN_POLICY:
case PhabricatorTransactions::TYPE_SPACE:
PhabricatorPolicyFilter::requireCapability(
$actor,
$object,
PhabricatorPolicyCapability::CAN_EDIT);
break;
$required_capabilities = array_fuse($required_capabilities);
$actor = $this->getActor();
if ($required_capabilities) {
id(new PhabricatorPolicyFilter())
->setViewer($actor)
->requireCapabilities($required_capabilities)
->raisePolicyExceptions(true)
->apply(array($object));
}
}
private function getLegacyRequiredCapabilities(
PhabricatorApplicationTransaction $xaction) {
$type = $xaction->getTransactionType();
switch ($type) {
case PhabricatorTransactions::TYPE_COMMENT:
// TODO: Comments technically require CAN_INTERACT, but this is
// currently somewhat special and handled through EditEngine. For now,
// don't enforce it here.
return null;
case PhabricatorTransactions::TYPE_SUBSCRIBERS:
// TODO: Removing subscribers other than yourself should probably
// require CAN_EDIT permission. You can do this via the API but
// generally can not via the web interface.
return null;
case PhabricatorTransactions::TYPE_TOKEN:
// TODO: This technically requires CAN_INTERACT, like comments.
return null;
case PhabricatorTransactions::TYPE_HISTORY:
// This is a special magic transaction which sends you history via
// email and is only partially supported in the upstream. You don't
// need any capabilities to apply it.
return null;
case PhabricatorTransactions::TYPE_EDGE:
return $this->getLegacyRequiredEdgeCapabilities($xaction);
default:
// For other older (non-modular) transactions, always require exactly
// CAN_EDIT. Transactions which do not need CAN_EDIT or need additional
// capabilities must move to ModularTransactions.
return PhabricatorPolicyCapability::CAN_EDIT;
}
}
private function getLegacyRequiredEdgeCapabilities(
PhabricatorApplicationTransaction $xaction) {
// You don't need to have edit permission on an object to mention it or
// otherwise add a relationship pointing toward it.
if ($this->getIsInverseEdgeEditor()) {
return null;
}
$edge_type = $xaction->getMetadataValue('edge:type');
switch ($edge_type) {
case PhabricatorMutedByEdgeType::EDGECONST:
// At time of writing, you can only write this edge for yourself, so
// you don't need permissions. If you can eventually mute an object
// for other users, this would need to be revisited.
return null;
case PhabricatorObjectMentionsObjectEdgeType::EDGECONST:
return null;
case PhabricatorProjectProjectHasMemberEdgeType::EDGECONST:
$old = $xaction->getOldValue();
$new = $xaction->getNewValue();
$add = array_keys(array_diff_key($new, $old));
$rem = array_keys(array_diff_key($old, $new));
$actor_phid = $this->requireActor()->getPHID();
$is_join = (($add === array($actor_phid)) && !$rem);
$is_leave = (($rem === array($actor_phid)) && !$add);
if ($is_join) {
// You need CAN_JOIN to join a project.
return PhabricatorPolicyCapability::CAN_JOIN;
}
if ($is_leave) {
$object = $this->object;
// You usually don't need any capabilities to leave a project...
if ($object->getIsMembershipLocked()) {
// ...you must be able to edit to leave locked projects, though.
return PhabricatorPolicyCapability::CAN_EDIT;
} else {
return null;
}
}
// You need CAN_EDIT to change members other than yourself.
return PhabricatorPolicyCapability::CAN_EDIT;
default:
return PhabricatorPolicyCapability::CAN_EDIT;
}
}
private function buildSubscribeTransaction(
PhabricatorLiskDAO $object,
array $xactions,

26
src/applications/transactions/storage/PhabricatorModularTransactionType.php
View file

@ -366,4 +366,30 @@ abstract class PhabricatorModularTransactionType
$capability);
}
/**
* Get a list of capabilities the actor must have on the object to apply
* a transaction to it.
*
* Usually, you should use this to reduce capability requirements when a
* transaction (like leaving a Conpherence thread) can be applied without
* having edit permission on the object. You can override this method to
* remove the CAN_EDIT requirement, or to replace it with a different
* requirement.
*
* If you are increasing capability requirements and need to add an
* additional capability or policy requirement above and beyond CAN_EDIT, it
* is usually better implemented as a validation check.
*
* @param object Object being edited.
* @param PhabricatorApplicationTransaction Transaction being applied.
* @return null|const|list<const> A capability constant (or list of
* capability constants) which the actor must have on the object. You can
* return `null` as a shorthand for "no capabilities are required".
*/
public function getRequiredCapabilities(
$object,
PhabricatorApplicationTransaction $xaction) {
return PhabricatorPolicyCapability::CAN_EDIT;
}
}