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

Provide core policy support for Spaces

Browse Source 
Summary:
Ref T8424. No UI or interesting behavior yet, but integrates Spaces checks:

  - `PolicyFilter` now checks Spaces.
  - `PolicyAwareQuery` now automatically adds Spaces constraints.

There's one interesting design decision here: **spaces are stronger than automatic capabilities**. That means that you can't see a task in a space you don't have permission to access, //even if you are the owner//.

I //think// this is desirable. Particularly, we need to do this in order to exclude objects at the query level, which potentially makes policy filtering for spaces hugely more efficient. I also like Spaces being very strong, conceptually.

It's possible that we might want to change this; this would reduce our access to optimizations but might be a little friendlier or make more sense to users later on.

For now, at least, I'm pursuing the more aggressive line. If we stick with this, we probably need to make some additional UI affordances (e.g., show when an owner can't see a task).

This also means that you get a hard 404 instead of a policy exception when you try to access something in a space you can't see. I'd slightly prefer to show you a policy exception instead, but think this is generally a reasonable tradeoff to get the high-performance filtering at the Query layer.

Test Plan:
  - Added and executed unit tests.
  - Put objects in spaces and viewed them with multiple users.
  - Made the default space visible/invisible, viewed objects.
  - Checked the services panel and saw `spacePHID` constraints.
  - Verified that this adds only one query to each page.

Reviewers: btrahan, chad

Reviewed By: btrahan

Subscribers: chad, epriestley

Maniphest Tasks: T8424

Differential Revision: https://secure.phabricator.com/D13156
This commit is contained in:
epriestley
2015-06-04 17:46:32 -07:00
parent e595478f1d
commit c1c897b961
5 changed files with 401 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

102
src/applications/spaces/__tests__/PhabricatorSpacesTestCase.php
View File

@@ -14,10 +14,23 @@ final class PhabricatorSpacesTestCase extends PhabricatorTestCase {
// Test that our helper methods work correctly.
$actor = $this->generateNewTestUser();
$this->newSpace($actor, pht('Test Space'), true);
$default = $this->newSpace($actor, pht('Test Space'), true);
$this->assertEqual(1, count($this->loadAllSpaces()));
$this->assertEqual(
1,
count(PhabricatorSpacesNamespaceQuery::getAllSpaces()));
$cache_default = PhabricatorSpacesNamespaceQuery::getDefaultSpace();
$this->assertEqual($default->getPHID(), $cache_default->getPHID());
$this->destroyAllSpaces();
$this->assertEqual(0, count($this->loadAllSpaces()));
$this->assertEqual(
0,
count(PhabricatorSpacesNamespaceQuery::getAllSpaces()));
$this->assertEqual(
null,
PhabricatorSpacesNamespaceQuery::getDefaultSpace());
}
public function testSpacesSeveralSpaces() {
@@ -27,9 +40,15 @@ final class PhabricatorSpacesTestCase extends PhabricatorTestCase {
// work fine.
$actor = $this->generateNewTestUser();
$this->newSpace($actor, pht('Default Space'), true);
$default = $this->newSpace($actor, pht('Default Space'), true);
$this->newSpace($actor, pht('Alternate Space'), false);
$this->assertEqual(2, count($this->loadAllSpaces()));
$this->assertEqual(
2,
count(PhabricatorSpacesNamespaceQuery::getAllSpaces()));
$cache_default = PhabricatorSpacesNamespaceQuery::getDefaultSpace();
$this->assertEqual($default->getPHID(), $cache_default->getPHID());
}
public function testSpacesRequireNames() {
@@ -70,6 +89,84 @@ final class PhabricatorSpacesTestCase extends PhabricatorTestCase {
$this->assertTrue(($caught instanceof Exception));
}
public function testSpacesPolicyFiltering() {
$this->destroyAllSpaces();
$creator = $this->generateNewTestUser();
$viewer = $this->generateNewTestUser();
// Create a new paste.
$paste = PhabricatorPaste::initializeNewPaste($creator)
->setViewPolicy(PhabricatorPolicies::POLICY_USER)
->setFilePHID('')
->setLanguage('')
->save();
// It should be visible.
$this->assertTrue(
PhabricatorPolicyFilter::hasCapability(
$viewer,
$paste,
PhabricatorPolicyCapability::CAN_VIEW));
// Create a default space with an open view policy.
$default = $this->newSpace($creator, pht('Default Space'), true)
->setViewPolicy(PhabricatorPolicies::POLICY_USER)
->save();
PhabricatorSpacesNamespaceQuery::destroySpacesCache();
// The paste should now be in the space implicitly, but still visible
// because the space view policy is open.
$this->assertTrue(
PhabricatorPolicyFilter::hasCapability(
$viewer,
$paste,
PhabricatorPolicyCapability::CAN_VIEW));
// Make the space view policy restrictive.
$default
->setViewPolicy(PhabricatorPolicies::POLICY_NOONE)
->save();
PhabricatorSpacesNamespaceQuery::destroySpacesCache();
// The paste should be in the space implicitly, and no longer visible.
$this->assertFalse(
PhabricatorPolicyFilter::hasCapability(
$viewer,
$paste,
PhabricatorPolicyCapability::CAN_VIEW));
// Put the paste in the space explicitly.
$paste
->setSpacePHID($default->getPHID())
->save();
PhabricatorSpacesNamespaceQuery::destroySpacesCache();
// This should still fail, we're just in the space explicitly now.
$this->assertFalse(
PhabricatorPolicyFilter::hasCapability(
$viewer,
$paste,
PhabricatorPolicyCapability::CAN_VIEW));
// Create an alternate space with more permissive policies, then move the
// paste to that space.
$alternate = $this->newSpace($creator, pht('Alternate Space'), false)
->setViewPolicy(PhabricatorPolicies::POLICY_USER)
->save();
$paste
->setSpacePHID($alternate->getPHID())
->save();
PhabricatorSpacesNamespaceQuery::destroySpacesCache();
// Now the paste should be visible again.
$this->assertTrue(
PhabricatorPolicyFilter::hasCapability(
$viewer,
$paste,
PhabricatorPolicyCapability::CAN_VIEW));
}
private function loadAllSpaces() {
return id(new PhabricatorSpacesNamespaceQuery())
->setViewer(PhabricatorUser::getOmnipotentUser())
@@ -77,6 +174,7 @@ final class PhabricatorSpacesTestCase extends PhabricatorTestCase {
}
private function destroyAllSpaces() {
PhabricatorSpacesNamespaceQuery::destroySpacesCache();
$spaces = $this->loadAllSpaces();
foreach ($spaces as $space) {
$engine = new PhabricatorDestructionEngine();

67
src/applications/spaces/query/PhabricatorSpacesNamespaceQuery.php
View File

@@ -3,6 +3,9 @@
final class PhabricatorSpacesNamespaceQuery
extends PhabricatorCursorPagedPolicyAwareQuery {
const KEY_ALL = 'spaces.all';
const KEY_DEFAULT = 'spaces.default';
private $ids;
private $phids;
private $isDefaultNamespace;
@@ -74,4 +77,68 @@ final class PhabricatorSpacesNamespaceQuery
return $this->formatWhereClause($where);
}
public static function destroySpacesCache() {
$cache = PhabricatorCaches::getRequestCache();
$cache->deleteKeys(
array(
self::KEY_ALL,
self::KEY_DEFAULT,
));
}
public static function getAllSpaces() {
$cache = PhabricatorCaches::getRequestCache();
$cache_key = self::KEY_ALL;
$spaces = $cache->getKey($cache_key);
if ($spaces === null) {
$spaces = id(new PhabricatorSpacesNamespaceQuery())
->setViewer(PhabricatorUser::getOmnipotentUser())
->execute();
$spaces = mpull($spaces, null, 'getPHID');
$cache->setKey($cache_key, $spaces);
}
return $spaces;
}
public static function getDefaultSpace() {
$cache = PhabricatorCaches::getRequestCache();
$cache_key = self::KEY_DEFAULT;
$default_space = $cache->getKey($cache_key, false);
if ($default_space === false) {
$default_space = null;
$spaces = self::getAllSpaces();
foreach ($spaces as $space) {
if ($space->getIsDefaultNamespace()) {
$default_space = $space;
break;
}
}
$cache->setKey($cache_key, $default_space);
}
return $default_space;
}
public static function getViewerSpaces(PhabricatorUser $viewer) {
$spaces = self::getAllSpaces();
$result = array();
foreach ($spaces as $key => $space) {
$can_see = PhabricatorPolicyFilter::hasCapability(
$viewer,
$space,
PhabricatorPolicyCapability::CAN_VIEW);
if ($can_see) {
$result[$key] = $space;
}
}
return $result;
}
}