From ae3e26024e505cfae3196471525a701a2e87e7dd Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Fri, 24 Jan 2025 12:52:33 +0100 Subject: [PATCH 1/2] fix: Correctly return app id and app version for `core` styles and images Signed-off-by: Ferdinand Thiessen --- lib/private/App/AppManager.php | 10 ++- lib/private/Server.php | 1 + lib/private/TemplateLayout.php | 15 ++--- tests/lib/App/AppManagerTest.php | 103 +++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 11 deletions(-) diff --git a/lib/private/App/AppManager.php b/lib/private/App/AppManager.php index fe5d3e2faebfc..b6f7f9b13b798 100644 --- a/lib/private/App/AppManager.php +++ b/lib/private/App/AppManager.php @@ -27,6 +27,7 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserSession; +use OCP\ServerVersion; use OCP\Settings\IManager as ISettingsManager; use Psr\Log\LoggerInterface; @@ -80,6 +81,7 @@ public function __construct( private ICacheFactory $memCacheFactory, private IEventDispatcher $dispatcher, private LoggerInterface $logger, + private ServerVersion $serverVersion, ) { } @@ -786,8 +788,12 @@ public function getAppInfoByPath(string $path, ?string $lang = null): ?array { public function getAppVersion(string $appId, bool $useCache = true): string { if (!$useCache || !isset($this->appVersions[$appId])) { - $appInfo = $this->getAppInfo($appId); - $this->appVersions[$appId] = ($appInfo !== null && isset($appInfo['version'])) ? $appInfo['version'] : '0'; + if ($appId === 'core') { + $this->appVersions[$appId] = $this->serverVersion->getVersionString(); + } else { + $appInfo = $this->getAppInfo($appId); + $this->appVersions[$appId] = ($appInfo !== null && isset($appInfo['version'])) ? $appInfo['version'] : '0'; + } } return $this->appVersions[$appId]; } diff --git a/lib/private/Server.php b/lib/private/Server.php index f806c368eaf54..be9d7595e4221 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -791,6 +791,7 @@ public function __construct($webRoot, \OC\Config $config) { $c->get(ICacheFactory::class), $c->get(IEventDispatcher::class), $c->get(LoggerInterface::class), + $c->get(ServerVersion::class), ); }); $this->registerAlias(IAppManager::class, AppManager::class); diff --git a/lib/private/TemplateLayout.php b/lib/private/TemplateLayout.php index e4978916ec365..001ef3afca567 100644 --- a/lib/private/TemplateLayout.php +++ b/lib/private/TemplateLayout.php @@ -304,12 +304,7 @@ public function __construct($renderAs, $appId = '') { $this->assign('id-app-navigation', $renderAs === TemplateResponse::RENDER_AS_USER ? '#app-navigation' : null); } - /** - * @param string $path - * @param string $file - * @return string - */ - protected function getVersionHashSuffix($path = false, $file = false) { + protected function getVersionHashSuffix(string $path = '', string $file = ''): string { if ($this->config->getSystemValueBool('debug', false)) { // allows chrome workspace mapping in debug mode return ''; @@ -322,11 +317,11 @@ protected function getVersionHashSuffix($path = false, $file = false) { $hash = false; // Try the web-root first - if (is_string($path) && $path !== '') { + if ($path !== '') { $hash = $this->getVersionHashByPath($path); } // If not found try the file - if ($hash === false && is_string($file) && $file !== '') { + if ($hash === false && $file !== '') { $hash = $this->getVersionHashByPath($file); } // As a last resort we use the server version hash @@ -376,7 +371,7 @@ public static function findStylesheetFiles($styles, $compileScss = true) { /** * @param string $path - * @return string|boolean + * @return string|false */ public function getAppNamefromPath($path) { if ($path !== '' && is_string($path)) { @@ -384,6 +379,8 @@ public function getAppNamefromPath($path) { if ($pathParts[0] === 'css') { // This is a scss request return $pathParts[1]; + } elseif ($pathParts[0] === 'core') { + return 'core'; } return end($pathParts); } diff --git a/tests/lib/App/AppManagerTest.php b/tests/lib/App/AppManagerTest.php index 8169c25c1603d..fd7a065d5b3c3 100644 --- a/tests/lib/App/AppManagerTest.php +++ b/tests/lib/App/AppManagerTest.php @@ -25,6 +25,7 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserSession; +use OCP\ServerVersion; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -100,6 +101,8 @@ protected function getAppConfig() { protected IURLGenerator&MockObject $urlGenerator; + protected ServerVersion&MockObject $serverVersion; + /** @var IAppManager */ protected $manager; @@ -115,6 +118,7 @@ protected function setUp(): void { $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->logger = $this->createMock(LoggerInterface::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->serverVersion = $this->createMock(ServerVersion::class); $this->overwriteService(AppConfig::class, $this->appConfig); $this->overwriteService(IURLGenerator::class, $this->urlGenerator); @@ -136,6 +140,7 @@ protected function setUp(): void { $this->cacheFactory, $this->eventDispatcher, $this->logger, + $this->serverVersion, ); } @@ -278,6 +283,7 @@ public function testEnableAppForGroups(): void { $this->cacheFactory, $this->eventDispatcher, $this->logger, + $this->serverVersion, ]) ->onlyMethods([ 'getAppPath', @@ -331,6 +337,7 @@ public function testEnableAppForGroupsAllowedTypes(array $appInfo): void { $this->cacheFactory, $this->eventDispatcher, $this->logger, + $this->serverVersion, ]) ->onlyMethods([ 'getAppPath', @@ -392,6 +399,7 @@ public function testEnableAppForGroupsForbiddenTypes($type): void { $this->cacheFactory, $this->eventDispatcher, $this->logger, + $this->serverVersion, ]) ->onlyMethods([ 'getAppPath', @@ -596,6 +604,7 @@ public function testGetAppsNeedingUpgrade(): void { $this->cacheFactory, $this->eventDispatcher, $this->logger, + $this->serverVersion, ]) ->onlyMethods(['getAppInfo']) ->getMock(); @@ -655,6 +664,7 @@ public function testGetIncompatibleApps(): void { $this->cacheFactory, $this->eventDispatcher, $this->logger, + $this->serverVersion, ]) ->onlyMethods(['getAppInfo']) ->getMock(); @@ -785,4 +795,97 @@ public function testIsBackendRequired( $this->assertEquals($expected, $this->manager->isBackendRequired($backend)); } + + public function testGetAppVersion() { + $manager = $this->getMockBuilder(AppManager::class) + ->setConstructorArgs([ + $this->userSession, + $this->config, + $this->groupManager, + $this->cacheFactory, + $this->eventDispatcher, + $this->logger, + $this->serverVersion, + ]) + ->onlyMethods([ + 'getAppInfo', + ]) + ->getMock(); + + $manager->expects(self::once()) + ->method('getAppInfo') + ->with('myapp') + ->willReturn(['version' => '99.99.99-rc.99']); + + $this->serverVersion + ->expects(self::never()) + ->method('getVersionString'); + + $this->assertEquals( + '99.99.99-rc.99', + $manager->getAppVersion('myapp'), + ); + } + + public function testGetAppVersionCore() { + $manager = $this->getMockBuilder(AppManager::class) + ->setConstructorArgs([ + $this->userSession, + $this->config, + $this->groupManager, + $this->cacheFactory, + $this->eventDispatcher, + $this->logger, + $this->serverVersion, + ]) + ->onlyMethods([ + 'getAppInfo', + ]) + ->getMock(); + + $manager->expects(self::never()) + ->method('getAppInfo'); + + $this->serverVersion + ->expects(self::once()) + ->method('getVersionString') + ->willReturn('1.2.3-beta.4'); + + $this->assertEquals( + '1.2.3-beta.4', + $manager->getAppVersion('core'), + ); + } + + public function testGetAppVersionUnknown() { + $manager = $this->getMockBuilder(AppManager::class) + ->setConstructorArgs([ + $this->userSession, + $this->config, + $this->groupManager, + $this->cacheFactory, + $this->eventDispatcher, + $this->logger, + $this->serverVersion, + ]) + ->onlyMethods([ + 'getAppInfo', + ]) + ->getMock(); + + $manager->expects(self::once()) + ->method('getAppInfo') + ->with('unknown') + ->willReturn(null); + + $this->serverVersion + ->expects(self::never()) + ->method('getVersionString'); + + $this->assertEquals( + '0', + $manager->getAppVersion('unknown'), + ); + } + } From 99e76d7a99b68d122340b76d726b2f40f354d6f9 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Fri, 24 Jan 2025 13:14:19 +0100 Subject: [PATCH 2/2] fix(TemplateLayout): `core` is not an app but the server itself Signed-off-by: Ferdinand Thiessen --- build/psalm-baseline.xml | 9 --------- lib/private/TemplateLayout.php | 17 +++++++++++------ tests/lib/AppTest.php | 32 +++++++++++++++++++------------- 3 files changed, 30 insertions(+), 28 deletions(-) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 11b121045a292..49ce3c7b0549c 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -2576,15 +2576,6 @@ - - - - - - - - - diff --git a/lib/private/TemplateLayout.php b/lib/private/TemplateLayout.php index 001ef3afca567..e52ef702ad32d 100644 --- a/lib/private/TemplateLayout.php +++ b/lib/private/TemplateLayout.php @@ -344,13 +344,18 @@ private function getVersionHashByPath(string $path): string|false { return false; } - $appVersion = $this->appManager->getAppVersion($appId); - // For shipped apps the app version is not a single source of truth, we rather also need to consider the Nextcloud version - if ($this->appManager->isShipped($appId)) { - $appVersion .= '-' . self::$versionHash; - } + if ($appId === 'core') { + // core is not a real app but the server itself + $hash = self::$versionHash; + } else { + $appVersion = $this->appManager->getAppVersion($appId); + // For shipped apps the app version is not a single source of truth, we rather also need to consider the Nextcloud version + if ($this->appManager->isShipped($appId)) { + $appVersion .= '-' . self::$versionHash; + } - $hash = substr(md5($appVersion), 0, 8); + $hash = substr(md5($appVersion), 0, 8); + } self::$cacheBusterCache[$path] = $hash; } diff --git a/tests/lib/AppTest.php b/tests/lib/AppTest.php index 98c3a6ff158b1..3e4d762a0a4cd 100644 --- a/tests/lib/AppTest.php +++ b/tests/lib/AppTest.php @@ -12,7 +12,13 @@ use OC\AppConfig; use OCP\EventDispatcher\IEventDispatcher; use OCP\IAppConfig; -use OCP\IURLGenerator; +use OCP\ICacheFactory; +use OCP\IConfig; +use OCP\IDBConnection; +use OCP\IGroupManager; +use OCP\IUserManager; +use OCP\IUserSession; +use OCP\ServerVersion; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; @@ -463,8 +469,8 @@ public function appConfigValuesProvider() { * @dataProvider appConfigValuesProvider */ public function testEnabledApps($user, $expectedApps, $forceAll): void { - $userManager = \OC::$server->getUserManager(); - $groupManager = \OC::$server->getGroupManager(); + $userManager = \OCP\Server::get(IUserManager::class); + $groupManager = \OCP\Server::get(IGroupManager::class); $user1 = $userManager->createUser(self::TEST_USER1, 'NotAnEasyPassword123456+'); $user2 = $userManager->createUser(self::TEST_USER2, 'NotAnEasyPassword123456_'); $user3 = $userManager->createUser(self::TEST_USER3, 'NotAnEasyPassword123456?'); @@ -512,7 +518,7 @@ public function testEnabledApps($user, $expectedApps, $forceAll): void { * enabled apps more than once when a user is set. */ public function testEnabledAppsCache(): void { - $userManager = \OC::$server->getUserManager(); + $userManager = \OCP\Server::get(IUserManager::class); $user1 = $userManager->createUser(self::TEST_USER1, 'NotAnEasyPassword123456+'); \OC_User::setUserId(self::TEST_USER1); @@ -544,8 +550,8 @@ public function testEnabledAppsCache(): void { private function setupAppConfigMock() { /** @var AppConfig|MockObject */ $appConfig = $this->getMockBuilder(AppConfig::class) - ->setMethods(['getValues']) - ->setConstructorArgs([\OC::$server->getDatabaseConnection()]) + ->onlyMethods(['getValues']) + ->setConstructorArgs([\OCP\Server::get(IDBConnection::class)]) ->disableOriginalConstructor() ->getMock(); @@ -561,13 +567,13 @@ private function setupAppConfigMock() { private function registerAppConfig(AppConfig $appConfig) { $this->overwriteService(AppConfig::class, $appConfig); $this->overwriteService(AppManager::class, new AppManager( - \OC::$server->getUserSession(), - \OC::$server->getConfig(), - \OC::$server->getGroupManager(), - \OC::$server->getMemCacheFactory(), - \OC::$server->get(IEventDispatcher::class), - \OC::$server->get(LoggerInterface::class), - \OC::$server->get(IURLGenerator::class), + \OCP\Server::get(IUserSession::class), + \OCP\Server::get(IConfig::class), + \OCP\Server::get(IGroupManager::class), + \OCP\Server::get(ICacheFactory::class), + \OCP\Server::get(IEventDispatcher::class), + \OCP\Server::get(LoggerInterface::class), + \OCP\Server::get(ServerVersion::class), )); }