From ac939e8fd4d6fd248a5ebf63a5e86edb6dcdaf3b Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Thu, 29 Mar 2018 09:44:38 +0200 Subject: [PATCH] Fix version comparison with minor and patch level requirements If an app requires a specific minor or path level server version, the version_compare prevented the installation as only the major version had been compared and that checks obviously returns `false`. Now the full version is used for comparison, making it possible to release apps for a specific minor or patch level version of Nextcloud. Signed-off-by: Christoph Wurst --- .../App/AppStore/Fetcher/AppFetcher.php | 23 +++-- lib/private/App/CompareVersion.php | 97 +++++++++++++++++++ .../App/AppStore/Fetcher/AppFetcherTest.php | 18 ++-- tests/lib/App/CompareVersionTest.php | 91 +++++++++++++++++ 4 files changed, 215 insertions(+), 14 deletions(-) create mode 100644 lib/private/App/CompareVersion.php create mode 100644 tests/lib/App/CompareVersionTest.php diff --git a/lib/private/App/AppStore/Fetcher/AppFetcher.php b/lib/private/App/AppStore/Fetcher/AppFetcher.php index 6c6fa68429..89bc7eb2c4 100644 --- a/lib/private/App/AppStore/Fetcher/AppFetcher.php +++ b/lib/private/App/AppStore/Fetcher/AppFetcher.php @@ -27,6 +27,7 @@ namespace OC\App\AppStore\Fetcher; use OC\App\AppStore\Version\VersionParser; +use OC\App\CompareVersion; use OC\Files\AppData\Factory; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Http\Client\IClientService; @@ -34,17 +35,23 @@ use OCP\IConfig; use OCP\ILogger; class AppFetcher extends Fetcher { + + /** @var CompareVersion */ + private $compareVersion; + /** * @param Factory $appDataFactory * @param IClientService $clientService * @param ITimeFactory $timeFactory * @param IConfig $config + * @param CompareVersion $compareVersion * @param ILogger $logger */ public function __construct(Factory $appDataFactory, IClientService $clientService, ITimeFactory $timeFactory, IConfig $config, + CompareVersion $compareVersion, ILogger $logger) { parent::__construct( $appDataFactory, @@ -56,6 +63,7 @@ class AppFetcher extends Fetcher { $this->fileName = 'apps.json'; $this->setEndpoint(); + $this->compareVersion = $compareVersion; } /** @@ -70,8 +78,6 @@ class AppFetcher extends Fetcher { /** @var mixed[] $response */ $response = parent::fetch($ETag, $content); - $ncVersion = $this->getVersion(); - $ncMajorVersion = explode('.', $ncVersion)[0]; foreach($response['data'] as $dataKey => $app) { $releases = []; @@ -83,12 +89,13 @@ class AppFetcher extends Fetcher { // Exclude all versions not compatible with the current version $versionParser = new VersionParser(); $version = $versionParser->getVersion($release['rawPlatformVersionSpec']); - if ( - // Major version is bigger or equals to the minimum version of the app - version_compare($ncMajorVersion, $version->getMinimumVersion(), '>=') - // Major version is smaller or equals to the maximum version of the app - && version_compare($ncMajorVersion, $version->getMaximumVersion(), '<=') - ) { + $ncVersion = $this->getVersion(); + $min = $version->getMinimumVersion(); + $max = $version->getMaximumVersion(); + $minFulfilled = $this->compareVersion->isCompatible($ncVersion, $min, '>='); + $maxFulfilled = $max !== '' && + $this->compareVersion->isCompatible($ncVersion, $max, '<='); + if ($minFulfilled && $maxFulfilled) { $releases[] = $release; } } diff --git a/lib/private/App/CompareVersion.php b/lib/private/App/CompareVersion.php new file mode 100644 index 0000000000..ee25a8b946 --- /dev/null +++ b/lib/private/App/CompareVersion.php @@ -0,0 +1,97 @@ + + * + * @author 2018 Christoph Wurst + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OC\App; + +use InvalidArgumentException; + +class CompareVersion { + + const REGEX_MAJOR = '/^\d+$/'; + const REGEX_MAJOR_MINOR = '/^\d+.\d+$/'; + const REGEX_MAJOR_MINOR_PATCH = '/^\d+.\d+.\d+$/'; + const REGEX_SERVER = '/^\d+.\d+.\d+(.\d+)?$/'; + + /** + * Checks if the given server version fulfills the given (app) version requirements. + * + * Version requirements can be 'major.minor.patch', 'major.minor' or just 'major', + * so '13.0.1', '13.0' and '13' are valid. + * + * @param string $actual version as major.minor.patch notation + * @param string $required version where major is requried and minor and patch are optional + * @param string $comparator passed to `version_compare` + * @return bool whether the requirement is fulfilled + * @throws InvalidArgumentException if versions specified in an invalid format + */ + public function isCompatible(string $actual, string $required, + string $comparator = '>='): bool { + + if (!preg_match(self::REGEX_SERVER, $actual)) { + throw new InvalidArgumentException('server version is invalid'); + } + + if (preg_match(self::REGEX_MAJOR, $required) === 1) { + return $this->compareMajor($actual, $required, $comparator); + } else if (preg_match(self::REGEX_MAJOR_MINOR, $required) === 1) { + return $this->compareMajorMinor($actual, $required, $comparator); + } else if (preg_match(self::REGEX_MAJOR_MINOR_PATCH, $required) === 1) { + return $this->compareMajorMinorPatch($actual, $required, $comparator); + } else { + throw new InvalidArgumentException('required version is invalid'); + } + } + + private function compareMajor(string $actual, string $required, + string $comparator) { + $actualMajor = explode('.', $actual)[0]; + $requiredMajor = explode('.', $required)[0]; + + return version_compare($actualMajor, $requiredMajor, $comparator); + } + + private function compareMajorMinor(string $actual, string $required, + string $comparator) { + $actualMajor = explode('.', $actual)[0]; + $actualMinor = explode('.', $actual)[1]; + $requiredMajor = explode('.', $required)[0]; + $requiredMinor = explode('.', $required)[1]; + + return version_compare("$actualMajor.$actualMinor", + "$requiredMajor.$requiredMinor", $comparator); + } + + private function compareMajorMinorPatch($actual, $required, $comparator) { + $actualMajor = explode('.', $actual)[0]; + $actualMinor = explode('.', $actual)[1]; + $actualPatch = explode('.', $actual)[2]; + $requiredMajor = explode('.', $required)[0]; + $requiredMinor = explode('.', $required)[1]; + $requiredPatch = explode('.', $required)[2]; + + return version_compare("$actualMajor.$actualMinor.$actualPatch", + "$requiredMajor.$requiredMinor.$requiredPatch", $comparator); + } + +} diff --git a/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php b/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php index 4549b05935..59dc7366cc 100644 --- a/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php +++ b/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php @@ -22,6 +22,7 @@ namespace Test\App\AppStore\Fetcher; use OC\App\AppStore\Fetcher\AppFetcher; +use OC\App\CompareVersion; use OC\Files\AppData\Factory; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\IAppData; @@ -33,18 +34,21 @@ use OCP\Http\Client\IClientService; use OCP\Http\Client\IResponse; use OCP\IConfig; use OCP\ILogger; +use PHPUnit_Framework_MockObject_MockObject; use Test\TestCase; class AppFetcherTest extends TestCase { - /** @var IAppData|\PHPUnit_Framework_MockObject_MockObject */ + /** @var IAppData|PHPUnit_Framework_MockObject_MockObject */ protected $appData; - /** @var IClientService|\PHPUnit_Framework_MockObject_MockObject */ + /** @var IClientService|PHPUnit_Framework_MockObject_MockObject */ protected $clientService; - /** @var ITimeFactory|\PHPUnit_Framework_MockObject_MockObject */ + /** @var ITimeFactory|PHPUnit_Framework_MockObject_MockObject */ protected $timeFactory; - /** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */ + /** @var IConfig|PHPUnit_Framework_MockObject_MockObject */ protected $config; - /** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */ + /** @var CompareVersion|PHPUnit_Framework_MockObject_MockObject */ + protected $compareVersion; + /** @var ILogger|PHPUnit_Framework_MockObject_MockObject */ protected $logger; /** @var AppFetcher */ protected $fetcher; @@ -57,7 +61,7 @@ EOD; public function setUp() { parent::setUp(); - /** @var Factory|\PHPUnit_Framework_MockObject_MockObject $factory */ + /** @var Factory|PHPUnit_Framework_MockObject_MockObject $factory */ $factory = $this->createMock(Factory::class); $this->appData = $this->createMock(IAppData::class); $factory->expects($this->once()) @@ -67,6 +71,7 @@ EOD; $this->clientService = $this->createMock(IClientService::class); $this->timeFactory = $this->createMock(ITimeFactory::class); $this->config = $this->createMock(IConfig::class); + $this->compareVersion = new CompareVersion(); $this->logger = $this->createMock(ILogger::class); $this->config @@ -79,6 +84,7 @@ EOD; $this->clientService, $this->timeFactory, $this->config, + $this->compareVersion, $this->logger ); } diff --git a/tests/lib/App/CompareVersionTest.php b/tests/lib/App/CompareVersionTest.php new file mode 100644 index 0000000000..60309fdeae --- /dev/null +++ b/tests/lib/App/CompareVersionTest.php @@ -0,0 +1,91 @@ + + * + * @author 2018 Christoph Wurst + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace Test\App; + +use InvalidArgumentException; +use OC\App\CompareVersion; +use Test\TestCase; + +class CompareVersionTest extends TestCase { + + /** @var CompareVersion */ + private $compare; + + protected function setUp() { + parent::setUp(); + + $this->compare = new CompareVersion(); + } + + public function comparisonData() { + return [ + // Compatible versions + ['13.0.0.3', '13.0.0', '>=', true], + ['13.0.0.3', '13.0.0', '<=', true], + ['13.0.0', '13.0.0', '>=', true], + ['13.0.0', '13.0', '<=', true], + ['13.0.0', '13', '>=', true], + ['13.0.1', '13', '>=', true], + ['13.0.1', '13', '<=', true], + // Incompatible major versions + ['13.0.0.3', '13.0.0', '<', false], + ['12.0.0', '13.0.0', '>=', false], + ['12.0.0', '13.0', '>=', false], + ['12.0.0', '13', '>=', false], + // Incompatible minor and patch versions + ['13.0.0', '13.0.1', '>=', false], + ['13.0.0', '13.1', '>=', false], + // Compatible minor and patch versions + ['13.0.1', '13.0.0', '>=', true], + ['13.2.0', '13.1', '>=', true], + ]; + } + + /** + * @dataProvider comparisonData + */ + public function testComparison(string $actualVersion, string $requiredVersion, + string $comparator, bool $expected) { + $isCompatible = $this->compare->isCompatible($actualVersion, $requiredVersion, + $comparator); + + $this->assertEquals($expected, $isCompatible); + } + + public function testInvalidServerVersion() { + $actualVersion = '13'; + $this->expectException(InvalidArgumentException::class); + + $this->compare->isCompatible($actualVersion, '13.0.0'); + } + + public function testInvalidRequiredVersion() { + $actualVersion = '13.0.0'; + $this->expectException(InvalidArgumentException::class); + + $this->compare->isCompatible($actualVersion, '13.0.0.9'); + } + +}