When updating the config file failed (or when any other file is not w…

…ritable...), the Updater (for core or plugins) will now automatically throw an error and cancel the update (#10423) * When updating the config file failed (or when any other file is not writable...), the Updater (for core or plugins) will now automatically throw an error and cancel the update * add integration test to check the correct exception is thrown when config not writabel * New integration test for updater * Make test better
matomo-org · Sep 29, 2016 · 5b50704 · 5b50704
1 parent db67ae4
commit 5b50704
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 8 deletions.
diff --git a/core/Config.php b/core/Config.php
@@ -12,6 +12,7 @@
 use Exception;
 use Piwik\Application\Kernel\GlobalSettingsProvider;
 use Piwik\Container\StaticContainer;
+use Piwik\Exception\MissingFilePermissionException;
 use Piwik\ProfessionalServices\Advertising;
 
 /**
@@ -375,15 +376,18 @@ public function dumpConfig()
      */
     protected function writeConfig($clear = true)
     {
-        if ($this->doNotWriteConfigInTests) {
-            return;
-        }
-
         $output = $this->dumpConfig();
         if ($output !== null
             && $output !== false
         ) {
-            $success = @file_put_contents($this->getLocalPath(), $output);
+
+            if ($this->doNotWriteConfigInTests) {
+                // simulate whether it would be successful
+                $success = is_writable($this->getLocalPath());
+            } else {
+                $success = @file_put_contents($this->getLocalPath(), $output);
+            }
+
             if ($success === false) {
                 throw $this->getConfigNotWritableException();
             }
@@ -411,6 +415,6 @@ public function forceSave()
     public function getConfigNotWritableException()
     {
         $path = "config/" . basename($this->getLocalPath());
-        return new Exception(Piwik::translate('General_ConfigFileIsNotWritable', array("(" . $path . ")", "")));
+        return new MissingFilePermissionException(Piwik::translate('General_ConfigFileIsNotWritable', array("(" . $path . ")", "")));
     }
 }
diff --git a/core/Updater.php b/core/Updater.php
@@ -10,6 +10,7 @@
 
 use Piwik\Columns\Updater as ColumnUpdater;
 use Piwik\Container\StaticContainer;
+use Piwik\Exception\MissingFilePermissionException;
 use Piwik\Updater\UpdateObserver;
 use Zend_Db_Exception;
 
@@ -257,8 +258,8 @@ public function update($componentName)
                 $this->markComponentSuccessfullyUpdated($componentName, $fileVersion);
             } catch (UpdaterErrorException $e) {
                 $this->executeListenerHook('onError', array($componentName, $fileVersion, $e));
-
                 throw $e;
+
             } catch (\Exception $e) {
                 $warningMessages[] = $e->getMessage();
 
@@ -581,7 +582,9 @@ private function executeSingleUpdateClass($className)
             // make sure to check for them here
             if ($e instanceof Zend_Db_Exception) {
                 throw new UpdaterErrorException($e->getMessage(), $e->getCode(), $e);
-            } else {
+            } else if ($e instanceof MissingFilePermissionException) {
+                throw new UpdaterErrorException($e->getMessage(), $e->getCode(), $e);
+            }{
                 throw $e;
             }
         }

diff --git a/plugins/CoreAdminHome/tests/Integration/SetConfigTest.php b/plugins/CoreAdminHome/tests/Integration/SetConfigTest.php
@@ -36,6 +36,12 @@ public function setUp()
         parent::setUp();
     }
 
+    public function tearDown()
+    {
+        parent::tearDown();
+        $this->makeLocalConfigWritable();
+    }
+
     public function test_Command_SucceedsWhenOptionsUsed()
     {
         $code = $this->applicationTester->run(array(
@@ -80,6 +86,22 @@ public function getInvalidArgumentsForTest()
         );
     }
 
+    public function test_Command_FailsWithMissingFilePermissionException_whenConfigFileNotWritable()
+    {
+        $this->makeLocalConfigNotWritable();
+
+        $code = $this->applicationTester->run(array(
+            'command' => 'config:set',
+            'assignment' => array(
+                'MySection.other_array_value=[]',
+            ),
+            '-vvv' => false,
+        ));
+
+        $this->assertNotEquals(0, $code, $this->getCommandDisplayOutputErrorMessage());
+        $this->assertContains('[Piwik\Exception\MissingFilePermissionException]', $this->applicationTester->getDisplay());
+    }
+
     public function test_Command_SucceedsWhenArgumentsUsed()
     {
         $config = Config::getInstance();
@@ -191,4 +213,23 @@ private static function removeTestConfigFile()
             unlink($configPath);
         }
     }
+
+    protected function makeLocalConfigNotWritable()
+    {
+        $local = Config::getInstance()->getLocalPath();
+        touch($local);
+        chmod($local, 0444);
+        $this->assertFalse(is_writable($local));
+    }
+
+    protected function makeLocalConfigWritable()
+    {
+        $local = Config::getInstance()->getLocalPath();
+        @chmod(dirname($local), 0755);
+        @chmod($local, 0755);
+        $this->assertTrue(is_writable(dirname($local)));
+        if(file_exists($local)) {
+            $this->assertTrue(is_writable($local));
+        }
+    }
 }
diff --git a/tests/PHPUnit/Integration/UpdaterTest.php b/tests/PHPUnit/Integration/UpdaterTest.php
@@ -18,6 +18,21 @@
  */
 class UpdaterTest extends IntegrationTestCase
 {
+    public function test_doUpdate_reportsAnError_whenMissingFilePermissionException()
+    {
+        $updater = new Updater($pathToCoreUpdates = null, PIWIK_INCLUDE_PATH . '/tests/resources/Updater/%s/');
+        $updater->markComponentSuccessfullyUpdated('testpluginUpdates', '0.4');
+        $componentsWithUpdateFile = $updater->getComponentsWithUpdateFile(array('testpluginUpdates' => '0.5'));
+
+        $this->assertEquals(1, count($componentsWithUpdateFile));
+
+        $result = $updater->updateComponents($componentsWithUpdateFile);
+
+        $this->assertTrue( count ($result['errors']) > 0, 'when an update fails because config file is not writable, we expect the updater to report a critical error');
+        $this->assertEquals( 'make sure this exception is thrown', $result['errors'][0]);
+    }
+
+
     public function testUpdaterChecksCoreVersionAndDetectsUpdateFile()
     {
         $updater = new Updater(PIWIK_INCLUDE_PATH . '/tests/resources/Updater/core/');

diff --git a/tests/resources/Updater/testpluginUpdates/0.5.php b/tests/resources/Updater/testpluginUpdates/0.5.php
@@ -0,0 +1,12 @@
+<?php
+namespace Piwik\Plugins\testpluginUpdates;
+
+use Piwik\Updates as PiwikUpdates;
+
+class Updates_0_5 extends PiwikUpdates
+{
+    function doUpdate(\Piwik\Updater $updater)
+    {
+        throw new \Piwik\Exception\MissingFilePermissionException('make sure this exception is thrown');
+    }
+}