add core updater with file access check (#18594)

* add core updater with file access check

add core updater with file access check

* update screen shot

update screen shot

* Update en.json

update to multiple support

* Update plugins/CoreUpdater/Updater.php

Co-authored-by: Justin Velluppillai <justin@innocraft.com>

* Update plugins/CoreUpdater/Updater.php

Co-authored-by: Justin Velluppillai <justin@innocraft.com>

* Update en.json

* Update en.json

update wording

* Adds update test

* fix branch before merge

Co-authored-by: Justin Velluppillai <justin@innocraft.com>
Co-authored-by: sgiehl <stefan@matomo.org>

5 files changed, 65 insertions, 3 deletions

diff --git a/plugins/CoreUpdater/Updater.php b/plugins/CoreUpdater/Updater.php
index 64593a2907..42a965e1d0 100644
--- a/plugins/CoreUpdater/Updater.php
+++ b/plugins/CoreUpdater/Updater.php
@@ -186,7 +186,7 @@ class Updater
         if (!isset($newVersion)) {
             $newVersion = Version::VERSION;
         }
-        
+
         // we also need to make sure to create a new instance here as otherwise we would change the "global"
         // environment, but we only want to change piwik version temporarily for this task here
         $environment = StaticContainer::getContainer()->make('Piwik\Plugins\Marketplace\Environment');
@@ -300,7 +300,7 @@ class Updater
         foreach ($plugins as $plugin) {
             $plugin->reloadPluginInformation();
         }
-        
+
         $incompatiblePlugins = $this->getIncompatiblePlugins($version);
         $disabledPluginNames = array();
 
@@ -322,6 +322,9 @@ class Updater
 
         $model = new Model();
 
+        // Check if the target directories are writable
+        $this->checkFolderPermissions($extractedArchiveDirectory, PIWIK_INCLUDE_PATH);
+
         /*
          * Copy all files to PIWIK_INCLUDE_PATH.
          * These files are accessed through the dispatcher.
@@ -378,4 +381,34 @@ class Updater
     {
         return PluginManager::getInstance()->getIncompatiblePlugins($piwikVersion);
     }
+
+
+    /**
+     * check if the target file directory is writeable
+     * @param string $source
+     * @param string $target
+     * @throws Exception
+     */
+    private function checkFolderPermissions($source, $target)
+    {
+        $wrongPermissionDir = [];
+        if (is_dir($source)) {
+            $d = dir($source);
+            while (false !== ($entry = $d->read())) {
+                if ($entry == '.' || $entry == '..') {
+                    continue;
+                }
+                $sourcePath = $source . '/' . $entry;
+                if (is_dir($sourcePath) && !is_writable($target . '/' . $entry)) {
+                    //add the wrong permission to the array
+                    $wrongPermissionDir[] = $target . '/' . $entry;
+                }
+            }
+        }
+
+        if (!empty($wrongPermissionDir)) {
+            throw new Exception($this->translator->translate('CoreUpdater_ExceptionDirWrongPermission',
+              implode(', ', $wrongPermissionDir)));
+        }
+    }
 }
diff --git a/plugins/CoreUpdater/lang/en.json b/plugins/CoreUpdater/lang/en.json
index 0101fe7958..b831ea4a33 100644
--- a/plugins/CoreUpdater/lang/en.json
+++ b/plugins/CoreUpdater/lang/en.json
@@ -20,6 +20,7 @@
         "ExceptionArchiveEmpty": "Empty archive.",
         "ExceptionArchiveIncompatible": "Incompatible archive: %s",
         "ExceptionArchiveIncomplete": "Archive is incomplete: some files are missing (eg. %s).",
+        "ExceptionDirWrongPermission": "Some folders are not writable. Please ensure the following folders are writable and try again: %s.",
         "FeedbackRequest": "Feel free to share your ideas and suggestions with the Matomo Team here:",
         "HelpMessageContent": "Check the %1$s Matomo FAQ %2$s which explains most common errors during update. %3$s Ask your system administrator - they may be able to help you with the error which is most likely related to your server or MySQL setup.",
         "HelpMessageIntroductionWhenError": "The above is the core error message. It should help explain the cause, but if you require further help please:",
@@ -99,4 +100,4 @@
         "SkipCacheClearDesc": "Skips clearing of caches before updating. This is only useful if you can ensure that instances running this command have not created a cache at all yet, and if clearing the cache for many Matomo accounts can become a bottleneck.",
         "SkipCacheClear": "Skipping clearing caches."
     }
-}
\ No newline at end of file
+}
diff --git a/tests/PHPUnit/Fixtures/LatestStableInstall.php b/tests/PHPUnit/Fixtures/LatestStableInstall.php
index 515e7c9204..4c15b8e847 100644
--- a/tests/PHPUnit/Fixtures/LatestStableInstall.php
+++ b/tests/PHPUnit/Fixtures/LatestStableInstall.php
@@ -79,6 +79,16 @@ class LatestStableInstall extends Fixture
         }
 
         shell_exec('mv "' . $installSubdirectory . '"/piwik/* "' . $installSubdirectory . '"');
+
+        /**
+         * The additional permissions check was added within Matomo 4.8 development. Therefor the OneClickUpdate UI tests
+         * would not already perform this check, as it uses the latest stable version to perform an update the the git checkout.
+         * As soon as 4.8 has been release, which should include the permission check, this won't be needed anymore.
+         *
+         * @todo remove this after Matomo 4.8 has been released
+         */
+        shell_exec('curl https://raw.githubusercontent.com/matomo-org/matomo/4.x-dev/plugins/CoreUpdater/Updater.php > ' . $installSubdirectory . '/plugins/CoreUpdater/Updater.php');
+        shell_exec('curl https://raw.githubusercontent.com/matomo-org/matomo/4.x-dev/plugins/CoreUpdater/lang/en.json > ' . $installSubdirectory . '/plugins/CoreUpdater/lang/en.json');
     }
 
     private function installSubdirectoryInstall()
diff --git a/tests/UI/expected-screenshots/OneClickUpdate_update_fail_permission.png b/tests/UI/expected-screenshots/OneClickUpdate_update_fail_permission.png
new file mode 100644
index 0000000000..1a88203b40
--- /dev/null
+++ b/tests/UI/expected-screenshots/OneClickUpdate_update_fail_permission.png
@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:459df3d386cee7e977800e30f00f703de8d04f9c6a7f49d68a0e435a71561248
+size 66383
diff --git a/tests/UI/specs/OneClickUpdate_spec.js b/tests/UI/specs/OneClickUpdate_spec.js
index e0029c36ba..a8f8c1a78d 100644
--- a/tests/UI/specs/OneClickUpdate_spec.js
+++ b/tests/UI/specs/OneClickUpdate_spec.js
@@ -7,6 +7,9 @@
  * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
  */
 
+var fs = require('fs'),
+  path = require('../../lib/screenshot-testing/support/path');
+
 const request = require('request-promise');
 const exec = require('child_process').exec;
 
@@ -49,7 +52,19 @@ describe("OneClickUpdate", function () {
         expect(await page.screenshot({ fullPage: true })).to.matchImage('update_fail');
     });
 
+    it('should fail when a directory is not writable', async function () {
+        fs.chmodSync(path.join(PIWIK_INCLUDE_PATH, '/latestStableInstall/core'), 0o555);
+        await page.waitForTimeout(100);
+        await page.click('#updateUsingHttp');
+        await page.waitForNetworkIdle();
+        expect(await page.screenshot({ fullPage: true })).to.matchImage('update_fail_permission');
+    });
+
     it('should update successfully and show the finished update screen', async function () {
+        fs.chmodSync(path.join(PIWIK_INCLUDE_PATH, '/latestStableInstall/core'), 0o777);
+        await page.waitForTimeout(100);
+        var url = await page.getWholeCurrentUrl();
+        await page.goBack();
         await page.click('#updateUsingHttp');
         await page.waitForNetworkIdle();
         await page.waitForSelector('.content');