Page MenuHomePhabricator
Files F15439725

D13328.diff
No OneTemporary
Actions

D13328.diff
View Options

diff --git a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php
--- a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php
+++ b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php
@@ -353,6 +353,37 @@
$this->assertEqual(array(), $result);
}
+ public function testPolicyStrength() {
+ $public = PhabricatorPolicyQuery::getGlobalPolicy(
+ PhabricatorPolicies::POLICY_PUBLIC);
+ $user = PhabricatorPolicyQuery::getGlobalPolicy(
+ PhabricatorPolicies::POLICY_USER);
+ $admin = PhabricatorPolicyQuery::getGlobalPolicy(
+ PhabricatorPolicies::POLICY_ADMIN);
+ $noone = PhabricatorPolicyQuery::getGlobalPolicy(
+ PhabricatorPolicies::POLICY_NOONE);
+
+ $this->assertFalse($public->isStrongerThan($public));
+ $this->assertFalse($public->isStrongerThan($user));
+ $this->assertFalse($public->isStrongerThan($admin));
+ $this->assertFalse($public->isStrongerThan($noone));
+
+ $this->assertTrue($user->isStrongerThan($public));
+ $this->assertFalse($user->isStrongerThan($user));
+ $this->assertFalse($user->isStrongerThan($admin));
+ $this->assertFalse($user->isStrongerThan($noone));
+
+ $this->assertTrue($admin->isStrongerThan($public));
+ $this->assertTrue($admin->isStrongerThan($user));
+ $this->assertFalse($admin->isStrongerThan($admin));
+ $this->assertFalse($admin->isStrongerThan($noone));
+
+ $this->assertTrue($noone->isStrongerThan($public));
+ $this->assertTrue($noone->isStrongerThan($user));
+ $this->assertTrue($noone->isStrongerThan($admin));
+ $this->assertFalse($admin->isStrongerThan($noone));
+ }
+
/**
* Test an object for visibility across multiple user specifications.
diff --git a/src/applications/policy/controller/PhabricatorPolicyExplainController.php b/src/applications/policy/controller/PhabricatorPolicyExplainController.php
--- a/src/applications/policy/controller/PhabricatorPolicyExplainController.php
+++ b/src/applications/policy/controller/PhabricatorPolicyExplainController.php
@@ -69,44 +69,115 @@
$capability_name = $capobj->getCapabilityName();
}
- $space_info = null;
- if ($object instanceof PhabricatorSpacesInterface) {
- if (PhabricatorSpacesNamespaceQuery::getViewerSpacesExist($viewer)) {
- $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID(
- $object);
-
- $handles = $viewer->loadHandles(array($space_phid));
+ $dialog = id(new AphrontDialogView())
+ ->setUser($viewer)
+ ->setClass('aphront-access-dialog');
- $space_info = array(
- pht(
- 'This object is in %s, and can only be seen by users with '.
- 'access to that space.',
- $handles[$space_phid]->renderLink()),
- phutil_tag('br'),
- phutil_tag('br'),
- );
- }
- }
+ $this->appendSpaceInformation($dialog, $object, $policy, $capability);
- $content = array(
- $space_info,
- pht('Users with the "%s" capability:', $capability_name),
- $auto_info,
- );
+ $intro = pht(
+ 'Users with the "%s" capability for this object:',
+ $capability_name);
$object_name = pht(
'%s %s',
$handle->getTypeName(),
$handle->getObjectName());
- $dialog = id(new AphrontDialogView())
- ->setUser($viewer)
- ->setClass('aphront-access-dialog')
+ return $dialog
->setTitle(pht('Policy Details: %s', $object_name))
- ->appendChild($content)
+ ->appendParagraph($intro)
+ ->appendChild($auto_info)
->addCancelButton($object_uri, pht('Done'));
+ }
+
+ private function appendSpaceInformation(
+ AphrontDialogView $dialog,
+ PhabricatorPolicyInterface $object,
+ PhabricatorPolicy $policy,
+ $capability) {
+ $viewer = $this->getViewer();
+
+ if (!($object instanceof PhabricatorSpacesInterface)) {
+ return;
+ }
+
+ if (!PhabricatorSpacesNamespaceQuery::getSpacesExist($viewer)) {
+ return;
+ }
+
+ // NOTE: We're intentionally letting users through here, even if they only
+ // have access to one space. The intent is to help users in "space jail"
+ // understand who objects they create are visible to:
+
+ $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID(
+ $object);
+
+ $handles = $viewer->loadHandles(array($space_phid));
+ $doc_href = PhabricatorEnv::getDoclink('Spaces User Guide');
+
+ $dialog->appendParagraph(
+ array(
+ pht(
+ 'This object is in %s, and can only be seen or edited by users with '.
+ 'access to view objects in the space.',
+ $handles[$space_phid]->renderLink()),
+ ' ',
+ phutil_tag(
+ 'strong',
+ array(),
+ phutil_tag(
+ 'a',
+ array(
+ 'href' => $doc_href,
+ 'target' => '_blank',
+ ),
+ pht('Learn More'))),
+ ));
+
+ $spaces = PhabricatorSpacesNamespaceQuery::getViewerSpaces($viewer);
+ $space = idx($spaces, $space_phid);
+ if (!$space) {
+ return;
+ }
+
+ $space_policies = PhabricatorPolicyQuery::loadPolicies($viewer, $space);
+ $space_policy = idx($space_policies, PhabricatorPolicyCapability::CAN_VIEW);
+ if (!$space_policy) {
+ return;
+ }
+
+ $space_explanation = PhabricatorPolicy::getPolicyExplanation(
+ $viewer,
+ $space_policy->getPHID());
+ $items = array();
+ $items[] = $space_explanation;
+
+ foreach ($items as $key => $item) {
+ $items[$key] = phutil_tag('li', array(), $item);
+ }
+
+ $dialog->appendParagraph(pht('Users who can see objects in this space:'));
+ $dialog->appendChild(phutil_tag('ul', array(), $items));
+
+ $view_capability = PhabricatorPolicyCapability::CAN_VIEW;
+ if ($capability == $view_capability) {
+ $stronger = $space_policy->isStrongerThan($policy);
+ if ($stronger) {
+ $dialog->appendParagraph(
+ pht(
+ 'The space this object is in has a more restrictive view '.
+ 'policy ("%s") than the object does ("%s"), so the space\'s '.
+ 'view policy is shown as a hint instead of the object policy.',
+ $space_policy->getShortName(),
+ $policy->getShortName()));
+ }
+ }
- return id(new AphrontDialogResponse())->setDialog($dialog);
+ $dialog->appendParagraph(
+ pht(
+ 'After a user passes space policy checks, they must still pass '.
+ 'object policy checks.'));
}
}
diff --git a/src/applications/policy/storage/PhabricatorPolicy.php b/src/applications/policy/storage/PhabricatorPolicy.php
--- a/src/applications/policy/storage/PhabricatorPolicy.php
+++ b/src/applications/policy/storage/PhabricatorPolicy.php
@@ -362,6 +362,60 @@
}
+ /**
+ * Return `true` if this policy is stronger (more restrictive) than some
+ * other policy.
+ *
+ * Because policies are complicated, determining which policies are
+ * "stronger" is not trivial. This method uses a very coarse working
+ * definition of policy strength which is cheap to compute, unambiguous,
+ * and intuitive in the common cases.
+ *
+ * This method returns `true` if the //class// of this policy is stronger
+ * than the other policy, even if the policies are (or might be) the same in
+ * practice. For example, "Members of Project X" is considered a stronger
+ * policy than "All Users", even though "Project X" might (in some rare
+ * cases) contain every user.
+ *
+ * Generally, the ordering here is:
+ *
+ * - Public
+ * - All Users
+ * - (Everything Else)
+ * - No One
+ *
+ * In the "everything else" bucket, we can't make any broad claims about
+ * which policy is stronger (and we especially can't make those claims
+ * cheaply).
+ *
+ * Even if we fully evaluated each policy, the two policies might be
+ * "Members of X" and "Members of Y", each of which permits access to some
+ * set of unique users. In this case, neither is strictly stronger than
+ * the other.
+ *
+ * @param PhabricatorPolicy Other policy.
+ * @return bool `true` if this policy is more restrictive than the other
+ * policy.
+ */
+ public function isStrongerThan(PhabricatorPolicy $other) {
+ $this_policy = $this->getPHID();
+ $other_policy = $other->getPHID();
+
+ $strengths = array(
+ PhabricatorPolicies::POLICY_PUBLIC => -2,
+ PhabricatorPolicies::POLICY_USER => -1,
+ // (Default policies have strength 0.)
+ PhabricatorPolicies::POLICY_NOONE => 1,
+ );
+
+ $this_strength = idx($strengths, $this->getPHID(), 0);
+ $other_strength = idx($strengths, $other->getPHID(), 0);
+
+ return ($this_strength > $other_strength);
+ }
+
+
+
/* -( PhabricatorPolicyInterface )----------------------------------------- */
diff --git a/src/view/phui/PHUIHeaderView.php b/src/view/phui/PHUIHeaderView.php
--- a/src/view/phui/PHUIHeaderView.php
+++ b/src/view/phui/PHUIHeaderView.php
@@ -285,6 +285,32 @@
return null;
}
+ // If an object is in a Space with a strictly stronger (more restrictive)
+ // policy, we show the more restrictive policy. This better aligns the
+ // UI hint with the actual behavior.
+
+ // NOTE: We'll do this even if the viewer has access to only one space, and
+ // show them information about the existence of spaces if they click
+ // through.
+ if ($object instanceof PhabricatorSpacesInterface) {
+ $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID(
+ $object);
+
+ $spaces = PhabricatorSpacesNamespaceQuery::getViewerSpaces($viewer);
+ $space = idx($spaces, $space_phid);
+ if ($space) {
+ $space_policies = PhabricatorPolicyQuery::loadPolicies(
+ $viewer,
+ $space);
+ $space_policy = idx($space_policies, $view_capability);
+ if ($space_policy) {
+ if ($space_policy->isStrongerThan($policy)) {
+ $policy = $space_policy;
+ }
+ }
+ }
+ }
+
$phid = $object->getPHID();
$icon = id(new PHUIIconView())

File Metadata

Mime Type
text/plain
Expires
Thu, Mar 27, 9:05 AM (1 w, 1 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7624143
Default Alt Text
D13328.diff (10 KB)

Event Timeline

Phabricator · Built by Phacility