diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e76fed12a0..f6254a8c3b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -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', diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php index df58762baf..d4d055f8ec 100644 --- a/src/aphront/AphrontRequest.php +++ b/src/aphront/AphrontRequest.php @@ -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; diff --git a/src/aphront/console/DarkConsoleController.php b/src/aphront/console/DarkConsoleController.php index cb11db7b5c..9135e1c77c 100644 --- a/src/aphront/console/DarkConsoleController.php +++ b/src/aphront/console/DarkConsoleController.php @@ -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(); diff --git a/src/aphront/console/DarkConsoleDataController.php b/src/aphront/console/DarkConsoleDataController.php index f761308ada..b860b310d7 100644 --- a/src/aphront/console/DarkConsoleDataController.php +++ b/src/aphront/console/DarkConsoleDataController.php @@ -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']; } diff --git a/src/applications/base/PhabricatorApplication.php b/src/applications/base/PhabricatorApplication.php index 4574a7bb10..938073273b 100644 --- a/src/applications/base/PhabricatorApplication.php +++ b/src/applications/base/PhabricatorApplication.php @@ -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; } diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index 8456b625e5..62c882d131 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -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(); - - $user = new PhabricatorUser(); - - $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 ($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(); + + $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); + } } + + $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); - } - } + $auth_class = 'PhabricatorApplicationAuth'; + $auth_application = PhabricatorApplication::getByClass($auth_class); - if ($this->shouldRequireLogin() && !$user->getPHID()) { - $login_controller = new PhabricatorAuthStartController($request); - $this->setCurrentApplication( - PhabricatorApplication::getByClass('PhabricatorApplicationAuth')); - return $this->delegateToController($login_controller); - } + $allow_public = $this->shouldAllowPublic() && + PhabricatorEnv::getEnvConfig('policy.allow-public'); - if ($this->shouldRequireEmailVerification()) { - $email = $user->loadPrimaryEmail(); - if (!$email) { - throw new Exception( - "No primary email address associated with this account!"); + // 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 (!$email->getIsVerified()) { - $verify_controller = new PhabricatorMustVerifyEmailController($request); - return $this->delegateToController($verify_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() { diff --git a/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php b/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php new file mode 100644 index 0000000000..1d6be0e793 --- /dev/null +++ b/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php @@ -0,0 +1,268 @@ + 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}'."); + } + } + +} diff --git a/src/applications/base/controller/__tests__/PhabricatorApplicationTest.php b/src/applications/base/controller/__tests__/PhabricatorApplicationTest.php new file mode 100644 index 0000000000..b20faaf553 --- /dev/null +++ b/src/applications/base/controller/__tests__/PhabricatorApplicationTest.php @@ -0,0 +1,38 @@ +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( + ); + } + +} + diff --git a/src/applications/base/controller/__tests__/PhabricatorTestController.php b/src/applications/base/controller/__tests__/PhabricatorTestController.php new file mode 100644 index 0000000000..cba2bc0b13 --- /dev/null +++ b/src/applications/base/controller/__tests__/PhabricatorTestController.php @@ -0,0 +1,40 @@ +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() { + + } + +} diff --git a/src/applications/meta/controller/PhabricatorApplicationsListController.php b/src/applications/meta/controller/PhabricatorApplicationsListController.php index a458e866a9..5a05aee538 100644 --- a/src/applications/meta/controller/PhabricatorApplicationsListController.php +++ b/src/applications/meta/controller/PhabricatorApplicationsListController.php @@ -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->isUnlisted()) { + continue; + } - if ($application->isBeta()) { - $item->addIcon('lint-warning', pht('Beta')); - } + $item = id(new PHUIObjectItemView()) + ->setHeader($application->getName()) + ->setHref('/applications/view/'.get_class($application).'/') + ->addAttribute($application->getShortDescription()); - $list->addItem($item); + if (!$application->isInstalled()) { + $item->addIcon('delete', pht('Uninstalled')); + } + if ($application->isBeta()) { + $item->addIcon('lint-warning', pht('Beta')); } + + $list->addItem($item); + } + return $list; } diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php index d6044e046d..44a43e62ec 100644 --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -528,6 +528,8 @@ final class PhabricatorEnv { foreach ($tmp as $source) { self::$sourceStack->pushSource($source); } + + self::dropConfigCache(); } private static function dropConfigCache() { diff --git a/src/infrastructure/testing/PhabricatorTestCase.php b/src/infrastructure/testing/PhabricatorTestCase.php index df85f87add..18c0ed7dcf 100644 --- a/src/infrastructure/testing/PhabricatorTestCase.php +++ b/src/infrastructure/testing/PhabricatorTestCase.php @@ -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."); + } + } + + }