diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -15,6 +15,9 @@ $defined_keys = PhabricatorApplicationConfigOptions::loadAllOptions(); + $stack = PhabricatorEnv::getConfigSourceStack(); + $stack = $stack->getStack(); + foreach ($all_keys as $key) { if (isset($defined_keys[$key])) { continue; @@ -48,9 +51,6 @@ ->setName($name) ->setSummary($summary); - $stack = PhabricatorEnv::getConfigSourceStack(); - $stack = $stack->getStack(); - $found = array(); $found_local = false; $found_database = false; @@ -85,6 +85,101 @@ } } + $options = PhabricatorApplicationConfigOptions::loadAllOptions(); + foreach ($defined_keys as $key => $value) { + $option = idx($options, $key); + if (!$option) { + continue; + } + + if (!$option->getLocked()) { + continue; + } + + $found_database = false; + foreach ($stack as $source_key => $source) { + $value = $source->getKeys(array($key)); + if ($value) { + if ($source instanceof PhabricatorConfigDatabaseSource) { + $found_database = true; + break; + } + } + } + + if (!$found_database) { + continue; + } + + // NOTE: These are values which we don't let you edit directly, but edit + // via other UI workflows. For now, don't raise this warning about them. + // In the future, before we stop reading database configuration for + // locked values, we either need to add a flag which lets these values + // continue reading from the database or move them to some other storage + // mechanism. + $soft_locks = array( + 'phabricator.uninstalled-applications', + 'phabricator.application-settings', + 'config.ignore-issues', + ); + $soft_locks = array_fuse($soft_locks); + if (isset($soft_locks[$key])) { + continue; + } + + $doc_name = 'Configuration Guide: Locked and Hidden Configuration'; + $doc_href = PhabricatorEnv::getDoclink($doc_name); + + $set_command = phutil_tag( + 'tt', + array(), + csprintf( + 'bin/config set %R ', + $key)); + + $summary = pht( + 'Configuration value "%s" is locked, but has a value in the database.', + $key); + $message = pht( + 'The configuration value "%s" is locked (so it can not be edited '. + 'from the web UI), but has a database value. Usually, this means '. + 'that it was previously not locked, you set it using the web UI, '. + 'and it later became locked.'. + "\n\n". + 'You should copy this configuration value in a local configuration '. + 'source (usually by using %s) and then remove it from the database '. + 'with the command below.'. + "\n\n". + 'For more information on locked and hidden configuration, including '. + 'details about this setup issue, see %s.'. + "\n\n". + 'This database value is currently respected, but a future version '. + 'of Phabricator will stop respecting database values for locked '. + 'configuration.', + $key, + $set_command, + phutil_tag( + 'a', + array( + 'href' => $doc_href, + 'target' => '_blank', + ), + $doc_name)); + $command = csprintf( + 'phabricator/ $ ./bin/config delete --database %R', + $key); + + $this->newIssue('config.locked.'.$key) + ->setShortName(pht('Deprecated Config Source')) + ->setName( + pht( + 'Locked Configuration Option "%s" Has Database Value', + $key)) + ->setSummary($summary) + ->setMessage($message) + ->addCommand($command) + ->addPhabricatorConfig($key); + } if (PhabricatorEnv::getEnvConfig('feed.http-hooks')) { $this->newIssue('config.deprecated.feed.http-hooks') diff --git a/src/applications/config/option/PhabricatorPHDConfigOptions.php b/src/applications/config/option/PhabricatorPHDConfigOptions.php --- a/src/applications/config/option/PhabricatorPHDConfigOptions.php +++ b/src/applications/config/option/PhabricatorPHDConfigOptions.php @@ -41,7 +41,12 @@ "If you are running a cluster, this limit applies separately ". "to each instance of `phd`. For example, if this limit is set ". "to `4` and you have three hosts running daemons, the effective ". - "global limit will be 12.")), + "global limit will be 12.". + "\n\n". + "After changing this value, you must restart the daemons. Most ". + "configuration changes are picked up by the daemons ". + "automatically, but pool sizes can not be changed without a ". + "restart.")), $this->newOption('phd.verbose', 'bool', false) ->setLocked(true) ->setBoolOptions( diff --git a/src/docs/user/configuration/configuration_locked.diviner b/src/docs/user/configuration/configuration_locked.diviner --- a/src/docs/user/configuration/configuration_locked.diviner +++ b/src/docs/user/configuration/configuration_locked.diviner @@ -111,6 +111,55 @@ ``` +Locked Configuration With Database Values +========================================= + +You may receive a setup issue warning you that a locked configuration key has a +value set in the database. Most commonly, this is because: + + - In some earlier version of Phabricator, this configuration was not locked. + - In the past, you or some other administrator used the web UI to set a + value. This value was written to the database. + - In a later version of the software, the value became locked. + +When Phabricator was originally released, locked configuration did not yet +exist. Locked configuration was introduced later, and then configuration options +were gradually locked for a long time after that. + +In some cases the meaning of a value changed and it became possible to use it +to break an install or the configuration became a security risk. In other +cases, we identified an existing security risk or arrived at some other reason +to lock the value. + +Locking values was more common in the past, and it is now relatively rare for +an unlocked value to become locked: when new values are introduced, they are +generally locked or hidden appropriately. In most cases, this setup issue only +affects installs that have used Phabricator for a long time. + +At time of writing (February 2019), Phabricator currently respects these old +database values. However, some future version of Phabricator will refuse to +read locked configuration from the database, because this improves security if +an attacker manages to find a way to bypass restrictions on editing locked +configuration from the web UI. + +To clear this setup warning and avoid surprise behavioral changes in the future, +you should move these configuration values from the database to a local config +file. Usually, you'll do this by first copying the value from the database: + +``` +phabricator/ $ ./bin/config set +``` + +...and then removing the database value: + +``` +phabricator/ $ ./bin/config delete --database +``` + +See @{Configuration User Guide: Advanced Configuration} for some more detailed +discussion of different configuration sources. + + Next Steps ==========