20after4 has uploaded a new change for review.
https://gerrit.wikimedia.org/r/179405
Change subject: Huge refactor of security policy enforcer stuff.
......................................................................
Huge refactor of security policy enforcer stuff.
This revision involves 3 cooperating classes to secure tasks considered either
'sensitive' or 'security bug'
1. SecurityPolicyListener applies an initial security policy to tasks submitted
with the
'security' option set to anything other than 'none'
2. The default security policy includes a new custom policy rule, implemented in
PhabricatorPolicyRuleTaskSubscribers, which allows anyone subscribed to a
task
to view/edit the task. In addition to subscribers of the task, the task
author and
any member of the corresponding 'security' project will also be able to
view/edit the task without
explicitly being subscribed.
3. SecurityPolicyEnforcerAction is a herald custom action which is used to
reset the security policy if anyone tries to override the policy to 'public'
or 'any user'
when the security flag is set to something other than 'none', this is just a
sanity
check to keep someone from inadvertantly or maliciously revealing a secure
task without
explicitly setting the security to 'none'
Change-Id: I5b1fcb35c6f390f1a54acfe4081da28f76245ab4
---
A PhabricatorPolicyRuleTaskSubscribers.php
M SecurityPolicyEnforcerAction.php
A SecurityPolicyListener.php
A WMFSecurityPolicy.php
4 files changed, 305 insertions(+), 272 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/phabricator/extensions
refs/changes/05/179405/1
diff --git a/PhabricatorPolicyRuleTaskSubscribers.php
b/PhabricatorPolicyRuleTaskSubscribers.php
new file mode 100644
index 0000000..1ae4cf9
--- /dev/null
+++ b/PhabricatorPolicyRuleTaskSubscribers.php
@@ -0,0 +1,66 @@
+<?php
+
+final class PhabricatorPolicyRuleTaskSubscribers
+ extends PhabricatorPolicyRule {
+
+ private $subscribed_to = array();
+
+ public function getRuleDescription() {
+ return pht('subscribers of maniphest task');
+ }
+
+ public function willApplyRules(PhabricatorUser $viewer, array $values) {
+ $values = array_unique(array_filter(array_mergev($values)));
+ if (!$values) {
+ return;
+ }
+ $this->subscribed_to = array();
+ $viewer_phid = $viewer->getPHID();
+ $tasks = id(new ManiphestTaskQuery())
+ ->setViewer(PhabricatorUser::getOmnipotentUser())
+ ->withPHIDs($values)
+ ->execute();
+
+ foreach($tasks as $task){
+ $ccs = $task->getCCPHIDs();
+ $this->subscribed_to[$task->getPHID()] = in_array($viewer_phid, $ccs);
+ }
+ }
+
+ public function applyRule(PhabricatorUser $viewer, $value) {
+ if (!is_array($value)){
+ $value = array($value);
+ }
+ foreach($value as $v) {
+ if (isset($this->subscribed_to[$v])) {
+ return $this->subscribed_to[$v];
+ }
+ }
+ return false;
+ }
+
+ public function getValueControlType() {
+ return self::CONTROL_TYPE_TEXT;
+ }
+
+ public function getRuleOrder() {
+ return 800;
+ }
+
+ public function getValueForDisplay(PhabricatorUser $viewer, $value) {
+ if (!is_array($value)) {
+ $value = array($value);
+ }
+ $handles = id(new PhabricatorHandleQuery())
+ ->setViewer($viewer)
+ ->withPHIDs($value)
+ ->execute();
+
+ return mpull($handles, 'getFullName', 'getPHID');
+ }
+
+ public function ruleHasEffect($value) {
+ return !empty($value);
+ }
+
+}
diff --git a/SecurityPolicyEnforcerAction.php b/SecurityPolicyEnforcerAction.php
index b413d71..9b2cd36 100644
--- a/SecurityPolicyEnforcerAction.php
+++ b/SecurityPolicyEnforcerAction.php
@@ -22,7 +22,7 @@
}
public function getActionName() {
- return "Ensure Security Task Policy Are Enforced";
+ return "Ensure Security Task Policies are Enforced";
}
public function getActionType() {
@@ -36,298 +36,58 @@
/** @var ManiphestTask */
$task = $object;
- $project = null;
- $viewer = PhabricatorUser::getOmnipotentUser();
+ $is_new = $adapter->getIsNewObject();
- $field_list = PhabricatorCustomField::getObjectFields(
- $task,
- PhabricatorCustomField::ROLE_EDIT);
+ // we set to true if/when we apply any effect
+ $applied = false;
- $field_list
- ->setViewer($viewer)
- ->readFieldsFromStorage($task);
-
- $field_value = null;
- foreach ($field_list->getFields() as $field) {
- $field_key = $field->getFieldKey();
-
- if ($field_key == 'std:maniphest:security_topic') {
- $field_value = $field->getValueForStorage();
- break;
- }
+ if ($is_new) {
+ // SecurityPolicyEventListener will take care of
+ // setting the policy for newly created tasks so
+ // this herald rule only needs to run on subsequent
+ // edits to secure tasks.
+ return new HeraldApplyTranscript($effect,$applied);
}
- $project_phids_orig = array_fuse($task->getProjectPHIDs());
+ $project = WMFSecurityPolicy::getSecurityProjectForTask($task);
+ // we only do something if this is a secure task
+ // if it's not a secure task then $project will be null
+ if ($project) {
+ $project_phids = array($project->getPHID());
- // These case statements tie into field values set in the
- // maniphest custom fields key
- $enforce = true;
- switch ($field_value) {
- case 'ops-sensitive':
- $enforce = true;
+ // These policies are too-open and would allow anyone to view
+ // the protected task. We override these if someone tries to
+ // set them on a 'secure task'
+ $rejected_policies = array(
+ PhabricatorPolicies::POLICY_PUBLIC,
+ PhabricatorPolicies::POLICY_USER,
+ );
+ if (in_array($task->getViewPolicy(), $rejected_policies)
+ ||in_array($task->getEditPolicy(), $rejected_policies)) {
- $project_phids = array($this->getProjectPHID('operations'));
-
- $policy = $this->createCustomPolicy(
+ $new_policy = WMFSecurityPolicy::createCustomPolicy(
+ $task,
$task->getAuthorPHID(),
$project_phids);
-
- $edit_policy = $view_policy = $policy->getPHID();
- break;
- case 'ops-access-request':
- $enforce = true;
-
- //operations group
- $project_phids = array($this->getProjectPHID('operations'));
-
- $policy = $this->createCustomPolicy(
- $task->getAuthorPHID(),
- $project_phids);
-
-
- $edit_policy = $view_policy = $policy->getPHID();
-
- // Make this public task depend on a corresponding 'private task'
- $edge_type = PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK;
-
- // First check for a pre-existant 'private task':
- $preexisting_tasks = PhabricatorEdgeQuery::loadDestinationPHIDs(
- $task->getPHID(),
- $edge_type);
-
- // if there isn't already a 'private task', create one:
- if (!count($preexisting_tasks)) {
- $oid = $task->getID();
- $user = id(new PhabricatorPeopleQuery())
- ->setViewer(PhabricatorUser::getOmnipotentUser())
- ->withUsernames(array('admin'))
- ->executeOne();
-
- $private_task = ManiphestTask::initializeNewTask($viewer);
- $private_task->setViewPolicy($view_policy)
- ->setEditPolicy($edit_policy)
- ->setTitle("ops access request: {$oid}")
- ->setAuthorPHID($user->getPHID())
- ->attachProjectPHIDs($project_phids)
- ->save();
-
- $project_type =
PhabricatorProjectObjectHasProjectEdgeType::EDGECONST;
- $transactions[] = id(new ManiphestTransaction())
- ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
- ->setMetadataValue('edge:type', $project_type)
- ->setNewValue(
- array(
- '=' => array_fuse($project_phids),
- ));
-
- // TODO: This should be transactional now.
- $edge_editor = id(new PhabricatorEdgeEditor());
-
- foreach($project_phids as $project_phid) {
- $edge_editor->addEdge(
- $private_task->getPHID(),
- $project_type,
- $project_phid);
- }
-
- $edge_editor
- ->addEdge(
- $task->getPHID(),
- $edge_type,
- $private_task->getPHID())
- ->save();
- }
-
- // we already set the security policy on the 'private task' but
- // not this task. Now reset the policy vars to avoid making the
- // 'public task' be private.
- $view_policy = null;
- $edit_policy = null;
-
- break;
- case 'sensitive':
- $enforce = true;
-
- //operations group
- $project = $this->getProjectByName('operations');
- $project_phids = array($project->getPHID());
- $policy = $this->createCustomPolicy(
- $task->getAuthorPHID(),
- $project_phids);
-
- $edit_policy = $view_policy = $policy->getPHID();
-
- break;
- case 'security-bug':
- $project = $this->getProjectByName('security');
- $project_phids = array($project->getPHID());
-
- $policy = $this->createCustomPolicy(
- $task->getAuthorPHID(),
- $project_phids);
-
- $edit_policy = $view_policy = $policy->getPHID();
- break;
- default:
- $enforce = false;
- }
-
- $transactions = array();
-
- if ($enforce) {
-
- if ($view_policy !== null) {
- $transactions[] = id(new ManiphestTransaction())
+ $adapter->queueTransaction(id(new ManiphestTransaction())
->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY)
- ->setNewValue($view_policy);
- }
-
- if ($edit_policy !== null) {
- $transactions[] = id(new ManiphestTransaction())
+ ->setNewValue($new_policy->getPHID()));
+ $adapter->queueTransaction(id(new ManiphestTransaction())
->setTransactionType(PhabricatorTransactions::TYPE_EDIT_POLICY)
- ->setNewValue($edit_policy);
+ ->setNewValue($new_policy->getPHID()));
+ $applied = true;
}
- $project_phids = array_fuse($project_phids);
- $new_project_phids = array_diff($project_phids, $project_phids_orig);
-
- // if we added a project, record the change
- if (count($new_project_phids)) {
- $transactions[] = id(new ManiphestTransaction())
- ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
- ->setMetadataValue(
- 'edge:type',
- PhabricatorProjectObjectHasProjectEdgeType::EDGECONST)
- ->setNewValue(
- array(
- '+' => array_fuse($new_project_phids),
- ));
- }
-
- foreach ($transactions as $transaction) {
- $adapter->queueTransaction($transaction);
- }
-
- if ($project !== null) {
- $this->sanityCheck($adapter, $project);
- }
}
-
- $applied = count($transactions) > 0;
return new HeraldApplyTranscript(
$effect,
$applied,
- pht('Set security policy'));
+ pht('Reset security policy'));
}
- /**
- * look up a project by name
- */
- protected function getProjectPHID($projectName) {
- return $this->getProjectByName($projectName)->getPHID();
- }
- /**
- * look up a project by name
- */
- protected function getProjectByName($projectName) {
- static $projects = array();
- if (isset($projects[$projectName])){
- return $projects[$projectName];
- }
-
- $query = new PhabricatorProjectQuery();
- $project = $query->setViewer(PhabricatorUser::getOmnipotentUser())
- ->withNames(array($projectName))
- ->needMembers(true)
- ->executeOne();
-
- if (!$project) {
- return null;
- }
-
- return $projects[$projectName] = $project;
- }
-
- protected function createCustomPolicy($user_phids, $project_phids) {
- if (!is_array($user_phids)){
- $user_phids = array($user_phids);
- }
- if (!is_array($project_phids)) {
- $project_phids = array($project_phids);
- }
-
- $policy = id(new PhabricatorPolicy())
- ->setRules(
- array(
- array(
- 'action' => PhabricatorPolicy::ACTION_ALLOW,
- 'rule' => 'PhabricatorPolicyRuleUsers',
- 'value' => $user_phids,
- ),
- array(
- 'action' => PhabricatorPolicy::ACTION_ALLOW,
- 'rule' => 'PhabricatorPolicyRuleProjects',
- 'value' => $project_phids,
- ),
- ))
- ->save();
- return $policy;
- }
-
-/**
- * Sanitize herald effects so that for tasks submitted to a secure project,
- * the following will be enforced:
- * * CC'd users must be a member of secure project
- * * assignee must be a member of the secure project
-*/
- protected function sanityCheck(
- HeraldManiphestTaskAdapter $adapter,
- PhabricatorProject $project) {
-
- $ccs = $adapter->getCcPHIDs();
- $members = $project->getMemberPHIDs();
- $member_phids = array();
- $sanitized_ccs = array();
- $removed_ccs = array();
- foreach($members as $key=>$phid) {
- $member_phids[$phid]=true;
- }
-
- //CC'd users must be members of secure project
- foreach($ccs as $cc) {
- if (isset($member_phids[$cc])) {
- $sanitized_ccs[] = $cc;
- } else {
- $removed_ccs[] = $cc;
- }
- }
- if (count($removed_ccs)) {
- $task = $adapter->getTask();
-
- $adapter->queueTransaction(id(new ManiphestTransaction())
- ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS)
- ->setNewValue(array('-' => array_fuse($removed_ccs) )));
-
- $task->setCcPHIDs($sanitized_ccs)
- ->save();
- }
- //assignee must be a member of secure project:
- $assign_phid = $adapter->getAssignPHID();
- if ($assign_phid) {
- if (!isset($member_phids[$assign_phid])) {
- //unassign because the assignee isn't a member of the project
- $task = $adapter->getTask();
- $task->setAssignPHID(null);
-
- $adapter->queueTransaction(id(new ManiphestTransaction())
- ->setTransactionType(ManiphestTransaction::TYPE_OWNER)
- ->setNewValue(null));
- }
- }
- }
}
diff --git a/SecurityPolicyListener.php b/SecurityPolicyListener.php
new file mode 100644
index 0000000..8e209b3
--- /dev/null
+++ b/SecurityPolicyListener.php
@@ -0,0 +1,82 @@
+<?php
+
+class SecurityPolicyEventListener
+ extends PhutilEventListener {
+
+ public function register() {
+ $this->listen(PhabricatorEventType::TYPE_MANIPHEST_WILLEDITTASK);
+ }
+
+ public function handleEvent(PhutilEvent $event) {
+ $task = $event->getValue('task');
+ $transactions = $event->getValue('transactions');
+ $is_new = $event->getValue('new');
+ $type_viewpol = PhabricatorTransactions::TYPE_VIEW_POLICY;
+ $type_editpol = PhabricatorTransactions::TYPE_EDIT_POLICY;
+ $type_edge = PhabricatorTransactions::TYPE_EDGE;
+ $type_hasproj = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST;
+
+ if ($is_new) {
+ if ($project = WMFSecurityPolicy::getSecurityProjectForTask($task)) {
+ $project_phids = array($project->getPHID() => $project->getPHID());
+ $policy = WMFSecurityPolicy::createCustomPolicy(
+ $task, $task->getAuthorPHID(), $project_phids);
+ $policy_phid = $policy->getPHID();
+ $trans = array();
+
+ $trans[$type_edge] = id(new ManiphestTransaction())
+ ->setTransactionType($type_edge)
+ ->setMetadataValue('edge:type',$type_hasproj)
+ ->setNewValue(array('+' => $project_phids));
+ $trans[$type_viewpol] = id(new ManiphestTransaction())
+ ->setTransactionType($type_viewpol)
+ ->setNewValue($policy_phid);
+ $trans[$type_editpol] = id(new ManiphestTransaction())
+ ->setTransactionType($type_editpol)
+ ->setNewValue($policy_phid);
+
+ // These transactions replace any user-generated transactions of
+ // the same type, e.g. user-supplied policy gets overwritten
+ // with custom policy.
+ foreach($transactions as $n => $t) {
+ $type = $t->getTransactionType();
+ if ($type == $type_edge) {
+ if ($t->getMetadataValue('edge:type') == $type_hasproj) {
+ $val = $t->getNewValue();
+ if (isset($val['=']) && is_array($val['='])){
+ $val['='] = array_unique(
+ array_merge(
+ $val['='], $project_phids));
+ } else {
+ $val['+'] = $project_phids;
+ }
+ $t->setNewValue($val);
+ unset($trans[$type_edge]);
+ }
+ }
+ else if (isset($trans[$type])){
+ $transactions[$n] = $trans[$type];
+ unset($trans[$type]);
+ }
+ }
+
+ $event->setValue('transactions', $transactions);
+ if (!empty($trans)) {
+ // apply remaining transactions
+ $content_source = PhabricatorContentSource::newForSource(
+ PhabricatorContentSource::SOURCE_UNKNOWN,
+ array());
+
+ $acting_as = id(new PhabricatorManiphestApplication())
+ ->getPHID();
+
+ id(new ManiphestTransactionEditor())
+ ->setActor(PhabricatorUser::getOmnipotentUser())
+ ->setActingAsPHID($acting_as)
+ ->setContentSource($content_source)
+ ->applyTransactions($task, $trans);
+ }
+ }
+ }
+ }
+}
diff --git a/WMFSecurityPolicy.php b/WMFSecurityPolicy.php
new file mode 100644
index 0000000..34a1ab3
--- /dev/null
+++ b/WMFSecurityPolicy.php
@@ -0,0 +1,125 @@
+<?php
+/**
+ * Static utility functions for dealing with projects and policies.
+ * These are used by both of our custom task policy extensions
+ * (SecurityPolicyEnforcerAction and SecurityPolicyEventListener)
+ * because this avoids much code duplication.
+ */
+final class WMFSecurityPolicy
+{
+ /**
+ * look up a project by name
+ * @param string $projectName
+ * @return PhabricatorProject|null
+ */
+ public static function getProjectByName($projectName) {
+ static $projects = array();
+ if (isset($projects[$projectName])){
+ return $projects[$projectName];
+ }
+
+ $query = new PhabricatorProjectQuery();
+ $project = $query->setViewer(PhabricatorUser::getOmnipotentUser())
+ ->withNames(array($projectName))
+ ->needMembers(true)
+ ->executeOne();
+
+ if (!$project) {
+ return null;
+ }
+
+ return $projects[$projectName] = $project;
+ }
+
+ /**
+ * Creates a custom policy for the given task having the following
properties:
+ *
+ * 1. The users listed in $user_phids can view/edit
+ * 2. Members of the project(s) in $project_phids can view/edit
+ * 3. $task Subscribers (aka CCs) can view/edit
+ *
+ * @param ManiphestTask $task
+ * @param array(PHID) $user_phids
+ * @param array(PHID) $project_phids
+ */
+ public static function createCustomPolicy($task, $user_phids,
$project_phids) {
+ if (!is_array($user_phids)) {
+ $user_phids = array($user_phids);
+ }
+ if (!is_array($project_phids)) {
+ $project_phids = array($project_phids);
+ }
+ $policy = $task->getViewPolicy();
+ if (!$policy instanceof PhabricatorPolicy) {
+ $policy = new PhabricatorPolicy();
+ }
+
+ $policy
+ ->setRules(
+ array(
+ array(
+ 'action' => PhabricatorPolicy::ACTION_ALLOW,
+ 'rule' => 'PhabricatorPolicyRuleUsers',
+ 'value' => $user_phids,
+ ),
+ array(
+ 'action' => PhabricatorPolicy::ACTION_ALLOW,
+ 'rule' => 'PhabricatorPolicyRuleTaskSubscribers',
+ 'value' => array($task->getPHID()),
+ ),
+ array(
+ 'action' => PhabricatorPolicy::ACTION_ALLOW,
+ 'rule' => 'PhabricatorPolicyRuleProjects',
+ 'value' => $project_phids,
+ ),
+ ))
+ ->setDefaultAction(PhabricatorPolicy::ACTION_DENY)
+ ->save();
+ return $policy;
+ }
+
+ /**
+ * return the value of the 'security_topic' custom field
+ * on the given $task
+ * @param ManiphestTask $task
+ * @return string the security_topic field value
+ */
+ public static function getSecurityFieldValue($task) {
+ $viewer = PhabricatorUser::getOmnipotentUser();
+
+ $field_list = PhabricatorCustomField::getObjectFields(
+ $task,
+ PhabricatorCustomField::ROLE_EDIT);
+
+ $field_list
+ ->setViewer($viewer)
+ ->readFieldsFromStorage($task);
+
+ $field_value = null;
+ foreach ($field_list->getFields() as $field) {
+ $field_key = $field->getFieldKey();
+
+ if ($field_key == 'std:maniphest:security_topic') {
+ $field_value = $field->getValueForStorage();
+ break;
+ }
+ }
+ return $field_value;
+ }
+
+ /**
+ * get the security project for a task (based on the security_topic field)
+ * @return PhabricatorProject|null the project, or null if security_topic
+ * is set to none
+ */
+ public static function getSecurityProjectForTask($task) {
+ switch (WMFSecurityPolicy::getSecurityFieldValue($task)) {
+ case 'sensitive':
+ return WMFSecurityPolicy::getProjectByName('operations');
+ case 'security-bug':
+ return WMFSecurityPolicy::getProjectByName('security');
+ default:
+ return false;
+ }
+ }
+}
--
To view, visit https://gerrit.wikimedia.org/r/179405
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5b1fcb35c6f390f1a54acfe4081da28f76245ab4
Gerrit-PatchSet: 1
Gerrit-Project: phabricator/extensions
Gerrit-Branch: master
Gerrit-Owner: 20after4 <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
