1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-20 13:52:40 +01:00

Fix controller-level access rules

Summary:
Ref T603. I had to partially revert this earlier because it accidentally blocked access to Conduit and File data for installs without "policy.allow-public", since the applications are available to "all users" but some endpoints actually need to be available even when not logged in.

This readjusts the gating in the controller to properly apply application visibility restrictions, and then adds a giant pile of unit test coverage to make sure it sticks and all the weird cases are covered.

Test Plan:
  - Added and executed unit tests.
  - Executed most of the tests manually, by using logged in / admin / public / disabled users.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7211
This commit is contained in:
epriestley 2013-10-03 19:05:47 -07:00
parent 3cf17cc67f
commit a6c4117ec4
12 changed files with 502 additions and 95 deletions

View file

@ -798,6 +798,7 @@ phutil_register_library_map(array(
'PasteReplyHandler' => 'applications/paste/mail/PasteReplyHandler.php',
'Phabricator404Controller' => 'applications/base/controller/Phabricator404Controller.php',
'PhabricatorAWSConfigOptions' => 'applications/config/option/PhabricatorAWSConfigOptions.php',
'PhabricatorAccessControlTestCase' => 'applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php',
'PhabricatorAccessLog' => 'infrastructure/PhabricatorAccessLog.php',
'PhabricatorAccessLogConfigOptions' => 'applications/config/option/PhabricatorAccessLogConfigOptions.php',
'PhabricatorActionHeaderExample' => 'applications/uiexample/examples/PhabricatorActionHeaderExample.php',
@ -866,6 +867,7 @@ phutil_register_library_map(array(
'PhabricatorApplicationSlowvote' => 'applications/slowvote/application/PhabricatorApplicationSlowvote.php',
'PhabricatorApplicationStatusView' => 'applications/meta/view/PhabricatorApplicationStatusView.php',
'PhabricatorApplicationSubscriptions' => 'applications/subscriptions/application/PhabricatorApplicationSubscriptions.php',
'PhabricatorApplicationTest' => 'applications/base/controller/__tests__/PhabricatorApplicationTest.php',
'PhabricatorApplicationTokens' => 'applications/tokens/application/PhabricatorApplicationTokens.php',
'PhabricatorApplicationTransaction' => 'applications/transactions/storage/PhabricatorApplicationTransaction.php',
'PhabricatorApplicationTransactionComment' => 'applications/transactions/storage/PhabricatorApplicationTransactionComment.php',
@ -1697,6 +1699,7 @@ phutil_register_library_map(array(
'PhabricatorTagView' => 'view/layout/PhabricatorTagView.php',
'PhabricatorTaskmasterDaemon' => 'infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php',
'PhabricatorTestCase' => 'infrastructure/testing/PhabricatorTestCase.php',
'PhabricatorTestController' => 'applications/base/controller/__tests__/PhabricatorTestController.php',
'PhabricatorTestDataGenerator' => 'applications/lipsum/generator/PhabricatorTestDataGenerator.php',
'PhabricatorTestStorageEngine' => 'applications/files/engine/PhabricatorTestStorageEngine.php',
'PhabricatorTestWorker' => 'infrastructure/daemon/workers/__tests__/PhabricatorTestWorker.php',
@ -2909,6 +2912,7 @@ phutil_register_library_map(array(
'PasteReplyHandler' => 'PhabricatorMailReplyHandler',
'Phabricator404Controller' => 'PhabricatorController',
'PhabricatorAWSConfigOptions' => 'PhabricatorApplicationConfigOptions',
'PhabricatorAccessControlTestCase' => 'PhabricatorTestCase',
'PhabricatorAccessLogConfigOptions' => 'PhabricatorApplicationConfigOptions',
'PhabricatorActionHeaderExample' => 'PhabricatorUIExample',
'PhabricatorActionHeaderView' => 'AphrontView',
@ -2974,6 +2978,7 @@ phutil_register_library_map(array(
'PhabricatorApplicationSlowvote' => 'PhabricatorApplication',
'PhabricatorApplicationStatusView' => 'AphrontView',
'PhabricatorApplicationSubscriptions' => 'PhabricatorApplication',
'PhabricatorApplicationTest' => 'PhabricatorApplication',
'PhabricatorApplicationTokens' => 'PhabricatorApplication',
'PhabricatorApplicationTransaction' =>
array(
@ -3876,6 +3881,7 @@ phutil_register_library_map(array(
'PhabricatorTagView' => 'AphrontView',
'PhabricatorTaskmasterDaemon' => 'PhabricatorDaemon',
'PhabricatorTestCase' => 'ArcanistPhutilTestCase',
'PhabricatorTestController' => 'PhabricatorController',
'PhabricatorTestStorageEngine' => 'PhabricatorFileStorageEngine',
'PhabricatorTestWorker' => 'PhabricatorWorker',
'PhabricatorTimeTestCase' => 'PhabricatorTestCase',

View file

@ -342,14 +342,23 @@ final class AphrontRequest {
$expire = time() + (60 * 60 * 24 * 365 * 5);
}
setcookie(
$name,
$value,
$expire,
$path = '/',
$base_domain,
$is_secure,
$http_only = true);
if (php_sapi_name() == 'cli') {
// Do nothing, to avoid triggering "Cannot modify header information"
// warnings.
// TODO: This is effectively a test for whether we're running in a unit
// test or not. Move this actual call to HTTPSink?
} else {
setcookie(
$name,
$value,
$expire,
$path = '/',
$base_domain,
$is_secure,
$http_only = true);
}
$_COOKIE[$name] = $value;

View file

@ -12,6 +12,10 @@ final class DarkConsoleController extends PhabricatorController {
return !PhabricatorEnv::getEnvConfig('darkconsole.always-on');
}
public function shouldRequireEnabledUser() {
return !PhabricatorEnv::getEnvConfig('darkconsole.always-on');
}
public function processRequest() {
$request = $this->getRequest();
$user = $request->getUser();

View file

@ -11,6 +11,10 @@ final class DarkConsoleDataController extends PhabricatorController {
return !PhabricatorEnv::getEnvConfig('darkconsole.always-on');
}
public function shouldRequireEnabledUser() {
return !PhabricatorEnv::getEnvConfig('darkconsole.always-on');
}
public function willProcessRequest(array $data) {
$this->key = $data['key'];
}

View file

@ -82,6 +82,10 @@ abstract class PhabricatorApplication
return false;
}
public function isUnlisted() {
return false;
}
/**
* Returns true if an application is first-party (developed by Phacility)
* and false otherwise.
@ -298,7 +302,6 @@ abstract class PhabricatorApplication
$applications = $apps;
}
return $applications;
}

View file

@ -5,14 +5,6 @@ abstract class PhabricatorController extends AphrontController {
private $handles;
public function shouldRequireLogin() {
// If this install is configured to allow public resources and the
// controller works in public mode, allow the request through.
$is_public_allowed = PhabricatorEnv::getEnvConfig('policy.allow-public');
if ($is_public_allowed && $this->shouldAllowPublic()) {
return false;
}
return true;
}
@ -29,33 +21,38 @@ abstract class PhabricatorController extends AphrontController {
}
public function shouldRequireEmailVerification() {
$need_verify = PhabricatorUserEmail::isEmailVerificationRequired();
$need_login = $this->shouldRequireLogin();
return ($need_login && $need_verify);
return PhabricatorUserEmail::isEmailVerificationRequired();
}
final public function willBeginExecution() {
$request = $this->getRequest();
if ($request->getUser()) {
// NOTE: Unit tests can set a user explicitly. Normal requests are not
// permitted to do this.
PhabricatorTestCase::assertExecutingUnitTests();
$user = $request->getUser();
} else {
$user = new PhabricatorUser();
$user = new PhabricatorUser();
$phusr = $request->getCookie('phusr');
$phsid = $request->getCookie('phsid');
$phusr = $request->getCookie('phusr');
$phsid = $request->getCookie('phsid');
if (strlen($phusr) && $phsid) {
$info = queryfx_one(
$user->establishConnection('r'),
'SELECT u.* FROM %T u JOIN %T s ON u.phid = s.userPHID
AND s.type LIKE %> AND s.sessionKey = %s',
$user->getTableName(),
'phabricator_session',
'web-',
PhabricatorHash::digest($phsid));
if ($info) {
$user->loadFromArray($info);
if (strlen($phusr) && $phsid) {
$info = queryfx_one(
$user->establishConnection('r'),
'SELECT u.* FROM %T u JOIN %T s ON u.phid = s.userPHID
AND s.type LIKE %> AND s.sessionKey = %s',
$user->getTableName(),
'phabricator_session',
'web-',
PhabricatorHash::digest($phsid));
if ($info) {
$user->loadFromArray($info);
}
}
$request->setUser($user);
}
$translation = $user->getTranslation();
@ -67,7 +64,15 @@ abstract class PhabricatorController extends AphrontController {
->addTranslations($translation->getTranslations());
}
$request->setUser($user);
$preferences = $user->loadPreferences();
if (PhabricatorEnv::getEnvConfig('darkconsole.enabled')) {
$dark_console = PhabricatorUserPreferences::PREFERENCE_DARK_CONSOLE;
if ($preferences->getPreference($dark_console) ||
PhabricatorEnv::getEnvConfig('darkconsole.always-on')) {
$console = new DarkConsoleCore();
$request->getApplicationConfiguration()->setConsole($console);
}
}
if ($user->getIsDisabled() && $this->shouldRequireEnabledUser()) {
$disabled_user_controller = new PhabricatorDisabledUserController(
@ -88,57 +93,59 @@ abstract class PhabricatorController extends AphrontController {
return $this->delegateToController($checker_controller);
}
$preferences = $user->loadPreferences();
if ($this->shouldRequireLogin()) {
// This actually means we need either:
// - a valid user, or a public controller; and
// - permission to see the application.
if (PhabricatorEnv::getEnvConfig('darkconsole.enabled')) {
$dark_console = PhabricatorUserPreferences::PREFERENCE_DARK_CONSOLE;
if ($preferences->getPreference($dark_console) ||
PhabricatorEnv::getEnvConfig('darkconsole.always-on')) {
$console = new DarkConsoleCore();
$request->getApplicationConfiguration()->setConsole($console);
}
}
if ($this->shouldRequireLogin() && !$user->getPHID()) {
$login_controller = new PhabricatorAuthStartController($request);
$this->setCurrentApplication(
PhabricatorApplication::getByClass('PhabricatorApplicationAuth'));
return $this->delegateToController($login_controller);
}
if ($this->shouldRequireEmailVerification()) {
$email = $user->loadPrimaryEmail();
if (!$email) {
throw new Exception(
"No primary email address associated with this account!");
}
if (!$email->getIsVerified()) {
$verify_controller = new PhabricatorMustVerifyEmailController($request);
return $this->delegateToController($verify_controller);
$auth_class = 'PhabricatorApplicationAuth';
$auth_application = PhabricatorApplication::getByClass($auth_class);
$allow_public = $this->shouldAllowPublic() &&
PhabricatorEnv::getEnvConfig('policy.allow-public');
// If this controller isn't public, and the user isn't logged in, require
// login.
if (!$allow_public && !$user->isLoggedIn()) {
$login_controller = new PhabricatorAuthStartController($request);
$this->setCurrentApplication($auth_application);
return $this->delegateToController($login_controller);
}
if ($user->isLoggedIn()) {
if ($this->shouldRequireEmailVerification()) {
$email = $user->loadPrimaryEmail();
if (!$email) {
throw new Exception(
"No primary email address associated with this account!");
}
if (!$email->getIsVerified()) {
$controller = new PhabricatorMustVerifyEmailController($request);
$this->setCurrentApplication($auth_application);
return $this->delegateToController($controller);
}
}
}
// If the user doesn't have access to the application, don't let them use
// any of its controllers. We query the application in order to generate
// a policy exception if the viewer doesn't have permission.
$application = $this->getCurrentApplication();
if ($application) {
id(new PhabricatorApplicationQuery())
->setViewer($user)
->withPHIDs(array($application->getPHID()))
->executeOne();
}
}
// NOTE: We do this last so that users get a login page instead of a 403
// if they need to login.
if ($this->shouldRequireAdmin() && !$user->getIsAdmin()) {
return new Aphront403Response();
}
// If the user doesn't have access to the application, don't let them use
// any of its controllers. We query the application in order to generate
// a policy exception if the viewer doesn't have permission.
$application = $this->getCurrentApplication();
if ($application) {
/*
TODO: Reenable this, but it's breaking some applications which need public
access in all cases, like Files and Conduit.
id(new PhabricatorApplicationQuery())
->setViewer($user)
->withPHIDs(array($application->getPHID()))
->executeOne();
*/
}
}
public function buildStandardPageView() {

View file

@ -0,0 +1,268 @@
<?php
final class PhabricatorAccessControlTestCase
extends PhabricatorTestCase {
protected function getPhabricatorTestCaseConfiguration() {
return array(
self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES => true,
);
}
public function testControllerAccessControls() {
$root = dirname(phutil_get_library_root('phabricator'));
require_once $root.'/support/PhabricatorStartup.php';
$application_configuration = new AphrontDefaultApplicationConfiguration();
$host = 'meow.example.com';
$_SERVER['REQUEST_METHOD'] = 'GET';
$request = id(new AphrontRequest($host, '/'))
->setApplicationConfiguration($application_configuration)
->setRequestData(array());
$controller = new PhabricatorTestController($request);
$u_public = id(new PhabricatorUser())
->setUsername('public');
$u_unverified = $this->generateNewTestUser()
->setUsername('unverified')
->save();
$u_unverified->loadPrimaryEmail()->setIsVerified(0)->save();
$u_normal = $this->generateNewTestUser()
->setUsername('normal')
->save();
$u_disabled = $this->generateNewTestUser()
->setIsDisabled(true)
->setUsername('disabled')
->save();
$u_admin = $this->generateNewTestUser()
->setIsAdmin(true)
->setUsername('admin')
->save();
$env = PhabricatorEnv::beginScopedEnv();
$env->overrideEnvConfig('phabricator.base-uri', 'http://'.$host);
$env->overrideEnvConfig('policy.allow-public', false);
$env->overrideEnvConfig('auth.require-email-verification', false);
$env->overrideEnvConfig('auth.email-domains', array());
// Test standard defaults.
$this->checkAccess(
"Default",
id(clone $controller),
$request,
array(
$u_normal,
$u_admin,
$u_unverified,
),
array(
$u_public,
$u_disabled,
));
// Test email verification.
$env->overrideEnvConfig('auth.require-email-verification', true);
$this->checkAccess(
"Email Verification Required",
id(clone $controller),
$request,
array(
$u_normal,
$u_admin,
),
array(
$u_unverified,
$u_public,
$u_disabled,
));
$this->checkAccess(
"Email Verification Required, With Exception",
id(clone $controller)->setConfig('email', false),
$request,
array(
$u_normal,
$u_admin,
$u_unverified,
),
array(
$u_public,
$u_disabled,
));
$env->overrideEnvConfig('auth.require-email-verification', false);
// Test admin access.
$this->checkAccess(
"Admin Required",
id(clone $controller)->setConfig('admin', true),
$request,
array(
$u_admin,
),
array(
$u_normal,
$u_unverified,
$u_public,
$u_disabled,
));
// Test disabled access.
$this->checkAccess(
"Allow Disabled",
id(clone $controller)->setConfig('enabled', false),
$request,
array(
$u_normal,
$u_unverified,
$u_admin,
$u_disabled,
),
array(
$u_public,
));
// Test no login required.
$this->checkAccess(
"No Login Required",
id(clone $controller)->setConfig('login', false),
$request,
array(
$u_normal,
$u_unverified,
$u_admin,
$u_public,
),
array(
$u_disabled,
));
// Test public access.
$this->checkAccess(
"No Login Required",
id(clone $controller)->setConfig('public', true),
$request,
array(
$u_normal,
$u_unverified,
$u_admin,
),
array(
$u_disabled,
$u_public,
));
$env->overrideEnvConfig('policy.allow-public', true);
$this->checkAccess(
"Public + configured",
id(clone $controller)->setConfig('public', true),
$request,
array(
$u_normal,
$u_unverified,
$u_admin,
$u_public,
),
array(
$u_disabled,
));
$env->overrideEnvConfig('policy.allow-public', false);
$app = PhabricatorApplication::getByClass('PhabricatorApplicationTest');
$app->reset();
$app->setPolicy(
PhabricatorPolicyCapability::CAN_VIEW,
PhabricatorPolicies::POLICY_NOONE);
$app_controller = id(clone $controller)->setCurrentApplication($app);
$this->checkAccess(
"Application Controller",
$app_controller,
$request,
array(
),
array(
$u_normal,
$u_unverified,
$u_admin,
$u_public,
$u_disabled,
));
$this->checkAccess(
"Application Controller",
id(clone $app_controller)->setConfig('login', false),
$request,
array(
$u_normal,
$u_unverified,
$u_admin,
$u_public,
),
array(
$u_disabled,
));
}
private function checkAccess(
$label,
$controller,
$request,
array $yes,
array $no) {
foreach ($yes as $user) {
$request->setUser($user);
$uname = $user->getUsername();
try {
$result = id(clone $controller)->willBeginExecution();
} catch (Exception $ex) {
$result = $ex;
}
$this->assertEqual(
true,
($result === null),
"Expect user '{$uname}' to be allowed access to '{$label}'.");
}
foreach ($no as $user) {
$request->setUser($user);
$uname = $user->getUsername();
try {
$result = id(clone $controller)->willBeginExecution();
} catch (Exception $ex) {
$result = $ex;
}
$this->assertEqual(
false,
($result === null),
"Expect user '{$uname}' to be denied access to '{$label}'.");
}
}
}

View file

@ -0,0 +1,38 @@
<?php
final class PhabricatorApplicationTest extends PhabricatorApplication {
private $policies = array();
public function isUnlisted() {
return true;
}
public function reset() {
$this->policies = array();
}
public function setPolicy($capability, $value) {
$this->policies[$capability] = $value;
return $this;
}
public function getPolicy($capability) {
return idx($this->policies, $capability, parent::getPolicy($capability));
}
public function shouldAppearInLaunchView() {
return false;
}
public function canUninstall() {
return false;
}
public function getRoutes() {
return array(
);
}
}

View file

@ -0,0 +1,40 @@
<?php
final class PhabricatorTestController extends PhabricatorController {
private $config = array();
public function setConfig($key, $value) {
$this->config[$key] = $value;
return $this;
}
public function getConfig($key, $default) {
return idx($this->config, $key, $default);
}
public function shouldRequireLogin() {
return $this->getConfig('login', parent::shouldRequireLogin());
}
public function shouldRequireAdmin() {
return $this->getConfig('admin', parent::shouldRequireAdmin());
}
public function shouldAllowPublic() {
return $this->getConfig('public', parent::shouldAllowPublic());
}
public function shouldRequireEmailVerification() {
return $this->getConfig('email', parent::shouldRequireEmailVerification());
}
public function shouldRequireEnabledUser() {
return $this->getConfig('enabled', parent::shouldRequireEnabledUser());
}
public function processRequest() {
}
}

View file

@ -30,22 +30,26 @@ final class PhabricatorApplicationsListController
$applications = msort($applications, 'getName');
foreach ($applications as $application) {
$item = id(new PHUIObjectItemView())
->setHeader($application->getName())
->setHref('/applications/view/'.get_class($application).'/')
->addAttribute($application->getShortDescription());
if (!$application->isInstalled()) {
$item->addIcon('delete', pht('Uninstalled'));
}
if ($application->isBeta()) {
$item->addIcon('lint-warning', pht('Beta'));
}
$list->addItem($item);
if ($application->isUnlisted()) {
continue;
}
$item = id(new PHUIObjectItemView())
->setHeader($application->getName())
->setHref('/applications/view/'.get_class($application).'/')
->addAttribute($application->getShortDescription());
if (!$application->isInstalled()) {
$item->addIcon('delete', pht('Uninstalled'));
}
if ($application->isBeta()) {
$item->addIcon('lint-warning', pht('Beta'));
}
$list->addItem($item);
}
return $list;
}

View file

@ -528,6 +528,8 @@ final class PhabricatorEnv {
foreach ($tmp as $source) {
self::$sourceStack->pushSource($source);
}
self::dropConfigCache();
}
private static function dropConfigCache() {

View file

@ -37,6 +37,7 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase {
private static $storageFixtureReferences = 0;
private static $storageFixture;
private static $storageFixtureObjectSeed = 0;
private static $testsAreRunning = 0;
protected function getPhabricatorTestCaseConfiguration() {
return array();
@ -68,6 +69,8 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase {
self::$storageFixture = $this->newStorageFixture();
}
}
++self::$testsAreRunning;
}
public function didRunTestCases(array $test_cases) {
@ -77,6 +80,8 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase {
self::$storageFixture = null;
}
}
--self::$testsAreRunning;
}
protected function willRunTests() {
@ -184,4 +189,21 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase {
return $user;
}
/**
* Throws unless tests are currently executing. This method can be used to
* guard code which is specific to unit tests and should not normally be
* reachable.
*
* If tests aren't currently being executed, throws an exception.
*/
public static function assertExecutingUnitTests() {
if (!self::$testsAreRunning) {
throw new Exception(
"Executing test code outside of test execution! This code path can ".
"only be run during unit tests.");
}
}
}