Merge pull request #6954 from nupplaphil/task/upgrade_hardening

ConfigFile upgrade hardening

diff --git a/bin/worker.php b/bin/worker.php
index c7174d81e1813ea0e246e2435c0132a7bdb8249a..2b5915f471d4fc76b9fdfdee1bad2ff42318cea0 100755 (executable)
--- a/bin/worker.php
+++ b/bin/worker.php
@@ -33,7 +33,7 @@ require dirname(__DIR__) . '/vendor/autoload.php';
 $a = Factory\DependencyFactory::setUp('worker', dirname(__DIR__));
 
 // Check the database structure and possibly fixes it
-Update::check($a->getBasePath(), true);
+Update::check($a->getBasePath(), true, $a->getMode());
 
 // Quit when in maintenance
 if (!$a->getMode()->has(App\Mode::MAINTENANCEDISABLED)) {

diff --git a/src/App.php b/src/App.php
index c0afd514d036068983afd1cf6c6b8a6f16b3f008..ff8cc2c12340dc81ede04070cffdef808326f9a3 100644 (file)
--- a/src/App.php
+++ b/src/App.php
@@ -1187,7 +1187,7 @@ class App
                        $this->module = 'maintenance';
                } else {
                        $this->checkURL();
-                       Core\Update::check($this->getBasePath(), false);
+                       Core\Update::check($this->getBasePath(), false, $this->getMode());
                        Core\Addon::loadAddons();
                        Core\Hook::loadHooks();
                }

diff --git a/src/Core/Config/Cache/ConfigCache.php b/src/Core/Config/Cache/ConfigCache.php
index f61865cee627a6e310ed8474586584c83702361b..3314e184f3519cf5f4a7ee388e37da8ce949fc41 100644 (file)
--- a/src/Core/Config/Cache/ConfigCache.php
+++ b/src/Core/Config/Cache/ConfigCache.php
@@ -188,4 +188,32 @@ class ConfigCache implements IConfigCache, IPConfigCache
        {
                return $this->config;
        }
+
+       /**
+        * Returns an array with missing categories/Keys
+        *
+        * @param array $config The array to check
+        *
+        * @return array
+        */
+       public function keyDiff(array $config)
+       {
+               $return = [];
+
+               $categories = array_keys($config);
+
+               foreach ($categories as $category) {
+                       if (is_array($config[$category])) {
+                               $keys = array_keys($config[$category]);
+
+                               foreach ($keys as $key) {
+                                       if (!isset($this->config[$category][$key])) {
+                                               $return[$category][$key] = $config[$category][$key];
+                                       }
+                               }
+                       }
+               }
+
+               return $return;
+       }
 }

diff --git a/src/Core/Update.php b/src/Core/Update.php
index 0d7b348b42c276f4587fab076c32fa7e34896d8b..8c15002a606ea66ccf3e645deec0ca617de9abc0 100644 (file)
--- a/src/Core/Update.php
+++ b/src/Core/Update.php
@@ -19,16 +19,21 @@ class Update
        /**
         * @brief Function to check if the Database structure needs an update.
         *
-        * @param string $basePath The base path of this application
-        * @param boolean $via_worker boolean Is the check run via the worker?
+        * @param string   $basePath   The base path of this application
+        * @param boolean  $via_worker Is the check run via the worker?
+        * @param App\Mode $mode       The current app mode
+        *
         * @throws \Friendica\Network\HTTPException\InternalServerErrorException
         */
-       public static function check($basePath, $via_worker)
+       public static function check($basePath, $via_worker, App\Mode $mode)
        {
                if (!DBA::connected()) {
                        return;
                }
 
+               // Check if the config files are set correctly
+               self::checkConfigFile($basePath, $mode);
+
                // Don't check the status if the last update was failed
                if (Config::get('system', 'update', Update::SUCCESS, true) == Update::FAILED) {
                        return;
@@ -227,40 +232,77 @@ class Update
        /**
         * Checks the config settings and saves given config values into the config file
         *
-        * @param string   $basePath The basepath of Friendica
-        * @param App\Mode $mode     The Application mode
+        * @param string    $basePath The basepath of Friendica
+        * @param App\Mode  $mode     The current App mode
         *
         * @return bool True, if something has been saved
         */
-       public static function saveConfigToFile($basePath, App\Mode $mode)
+       public static function checkConfigFile($basePath, App\Mode $mode)
        {
+               if (empty($basePath)) {
+                       $basePath = BasePath::create(dirname(__DIR__, 2));
+               }
+
+               $config = [
+                       'config' => [
+                               'hostname' => [
+                                       'allowEmpty' => false,
+                                       'default' => '',
+                               ],
+                       ],
+                       'system' => [
+                               'basepath' => [
+                                       'allowEmpty' => false,
+                                       'default' => $basePath,
+                               ],
+                       ]
+               ];
+
                $configFileLoader = new ConfigFileLoader($basePath, $mode);
                $configCache = new Config\Cache\ConfigCache();
                $configFileLoader->setupCache($configCache, true);
-               $configFileSaver = new ConfigFileSaver($basePath);
 
-               $updated = false;
+               // checks if something is to update, otherwise skip this function at all
+               $missingConfig = $configCache->keyDiff($config);
+               if (empty($missingConfig)) {
+                       return true;
+               }
 
-               if (self::updateConfigEntry($configCache, $configFileSaver,'config', 'hostname')) {
-                       $updated = true;
-               };
+               // We just want one update process
+               if (Lock::acquire('config_update')) {
+                       $configFileSaver = new ConfigFileSaver($basePath);
 
-               if (self::updateConfigEntry($configCache, $configFileSaver,'system', 'basepath', BasePath::create(dirname(__DIR__) . '/../'))) {
-                       $updated = true;
-               }
+                       $updated = false;
+                       $toDelete = [];
 
-               // In case there is nothing to do, skip the update
-               if (!$updated) {
-                       return true;
-               }
+                       foreach ($missingConfig as $category => $keys) {
+                               foreach ($keys as $key => $value) {
+                                       if (self::updateConfigEntry($configCache, $configFileSaver, $category, $key, $value['allowEmpty'], $value['default'])) {
+                                               $toDelete[] = ['cat' => $category, 'key' => $key];
+                                               $updated = true;
+                                       };
+                               }
+                       }
 
-               if (!$configFileSaver->saveToConfigFile()) {
-                       Logger::alert('Config entry update failed - maybe wrong permission?');
-                       return false;
-               }
+                       // In case there is nothing to do, skip the update
+                       if (!$updated) {
+                               Lock::release('config_update');
+                               return true;
+                       }
+
+                       if (!$configFileSaver->saveToConfigFile()) {
+                               Logger::alert('Config entry update failed - maybe wrong permission?');
+                               Lock::release('config_update');
+                               return false;
+                       }
 
-               DBA::delete('config', ['cat' => 'config', 'k' => 'hostname']);
-               DBA::delete('config', ['cat' => 'system', 'k' => 'basepath']);
+                       // After the successful save, remove the db values
+                       foreach ($toDelete as $delete) {
+                               DBA::delete('config', ['cat' => $delete['cat'], 'k' => $delete['key']]);
+                       }
+
+                       Lock::release('config_update');
+               }
 
                return true;
        }
@@ -272,33 +314,47 @@ class Update
         * @param ConfigFileSaver $configFileSaver The config file saver
         * @param string          $cat             The config category
         * @param string          $key             The config key
+        * @param bool            $allowEmpty      If true, empty values are valid (Default there has to be a variable)
         * @param string          $default         A default value, if none of the settings are valid
         *
         * @return boolean True, if a value was updated
         *
         * @throws \Exception if DBA or Logger doesn't work
         */
-       private static function updateConfigEntry(IConfigCache $configCache, ConfigFileSaver $configFileSaver, $cat, $key, $default = '')
+       private static function updateConfigEntry(
+               IConfigCache $configCache,
+               ConfigFileSaver $configFileSaver,
+               $cat,
+               $key,
+               $allowEmpty = false,
+               $default = '')
        {
-               // check if the config file differs from the whole configuration (= The db contains other values)
-               $fileConfig = $configCache->get($cat, $key);
 
-               $savedConfig = DBA::selectFirst('config', ['v'], ['cat' => $cat, 'k' => $key]);
+               // check if the config file differs from the whole configuration (= The db contains other values)
+               $fileValue = $configCache->get($cat, $key);
+               $dbConfig  = DBA::selectFirst('config', ['v'], ['cat' => $cat, 'k' => $key]);
 
-               if (DBA::isResult($savedConfig)) {
-                       $savedValue = $savedConfig['v'];
+               if (DBA::isResult($dbConfig)) {
+                       $dbValue = $dbConfig['v'];
                } else {
-                       $savedValue = null;
+                       $dbValue = null;
                }
 
                // If the db contains a config value, check it
-               if (isset($savedValue) && $fileConfig !== $savedValue) {
-                       Logger::info('Difference in config found', ['cat' => $cat, 'key' => $key, 'file' => $fileConfig, 'saved' => $savedValue]);
-                       $configFileSaver->addConfigValue($cat, $key, $savedValue);
+               if ((
+                               ($allowEmpty && isset($dbValue)) ||
+                               (!$allowEmpty && !empty($dbValue))
+                       ) &&
+                       $fileValue !== $dbValue) {
+                       Logger::info('Difference in config found', ['cat' => $cat, 'key' => $key, 'file' => $fileValue, 'db' => $dbValue]);
+                       $configFileSaver->addConfigValue($cat, $key, $dbValue);
                        return true;
 
                // If both config values are not set, use the default value
-               } elseif (!isset($fileConfig) && !isset($savedValue)) {
+               } elseif (
+                       ($allowEmpty && !isset($fileValue) && !isset($dbValue)) ||
+                       (!$allowEmpty && empty($fileValue) && empty($dbValue) && !empty($default))) {
+
                        Logger::info('Using default for config', ['cat' => $cat, 'key' => $key, 'value' => $default]);
                        $configFileSaver->addConfigValue($cat, $key, $default);
                        return true;
@@ -306,7 +362,7 @@ class Update
                // If either the file config value isn't empty or the db value is the same as the
                // file config value, skip it
                } else {
-                       Logger::info('No Difference in config found', ['cat' => $cat, 'key' => $key, 'value' => $fileConfig, 'saved' => $savedValue]);
+                       Logger::debug('No Difference in config found', ['cat' => $cat, 'key' => $key, 'value' => $fileValue, 'db' => $dbValue]);
                        return false;
                }
        }

diff --git a/tests/src/Core/Config/Cache/ConfigCacheTest.php b/tests/src/Core/Config/Cache/ConfigCacheTest.php
index 1ee2a3f80270aa78c673936f74d9bc20fe6db418..e6ac8255e9f7b3b454aced052381a62e71a44c04 100644 (file)
--- a/tests/src/Core/Config/Cache/ConfigCacheTest.php
+++ b/tests/src/Core/Config/Cache/ConfigCacheTest.php
@@ -245,4 +245,34 @@ class ConfigCacheTest extends MockedTest
 
                $this->assertEmpty($configCache->getAll());
        }
+
+       /**
+        * Test the keyDiff() method with result
+        * @dataProvider dataTests
+        */
+       public function testKeyDiffWithResult($data)
+       {
+               $configCache = new ConfigCache($data);
+
+               $diffConfig = [
+                       'fakeCat' => [
+                               'fakeKey' => 'value',
+                       ]
+               ];
+
+               $this->assertEquals($diffConfig, $configCache->keyDiff($diffConfig));
+       }
+
+       /**
+        * Test the keyDiff() method without result
+        * @dataProvider dataTests
+        */
+       public function testKeyDiffWithoutResult($data)
+       {
+               $configCache = new ConfigCache($data);
+
+               $diffConfig = $configCache->getAll();
+
+               $this->assertEmpty($configCache->keyDiff($diffConfig));
+       }
 }

diff --git a/update.php b/update.php
index 0ef74ea00510554d33b80c4c80804bab493cff26..e619ec89dd9bbb569c3ecb4db27c863c16fdcab2 100644 (file)
--- a/update.php
+++ b/update.php
@@ -1,6 +1,5 @@
 <?php
 
-use Friendica\BaseObject;
 use Friendica\Core\Addon;
 use Friendica\Core\Config;
 use Friendica\Core\L10n;
@@ -347,17 +346,3 @@ function update_1298()
        }
        return Update::SUCCESS;
 }
-
-/**
- * @see https://github.com/friendica/friendica/pull/6920
- * @return int Success
- */
-function update_1307()
-{
-       $app = BaseObject::getApp();
-       if (Update::saveConfigToFile($app->getBasePath(), $app->getMode())) {
-               return Update::SUCCESS;
-       } else {
-               return Update::FAILED;
-       }
-}