From 346f5d46a11664f1246d01d8652d618abddd0147 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 10 Oct 2024 18:26:52 +0200 Subject: [PATCH] fix: Shipped apps should include the Nextcloud version in the cache buster Signed-off-by: Ferdinand Thiessen --- lib/private/TemplateLayout.php | 104 +++++++++++++++++-------------- tests/lib/TemplateLayoutTest.php | 91 +++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 47 deletions(-) create mode 100644 tests/lib/TemplateLayoutTest.php diff --git a/lib/private/TemplateLayout.php b/lib/private/TemplateLayout.php index 60c7526435e2c..2bc9ff5a2af74 100644 --- a/lib/private/TemplateLayout.php +++ b/lib/private/TemplateLayout.php @@ -31,6 +31,8 @@ class TemplateLayout extends \OC_Template { private static $versionHash = ''; + /** @var string[] */ + private static $cacheBusterCache = []; /** @var CSSResourceLocator|null */ public static $cssLocator = null; @@ -38,38 +40,29 @@ class TemplateLayout extends \OC_Template { /** @var JSResourceLocator|null */ public static $jsLocator = null; - /** @var IConfig */ - private $config; - - /** @var IInitialStateService */ - private $initialState; - - /** @var INavigationManager */ - private $navigationManager; + private IConfig $config; + private IAppManager $appManager; + private InitialStateService $initialState; + private INavigationManager $navigationManager; /** * @param string $renderAs * @param string $appId application id */ public function __construct($renderAs, $appId = '') { - /** @var IConfig */ - $this->config = \OC::$server->get(IConfig::class); - - /** @var IInitialStateService */ - $this->initialState = \OC::$server->get(IInitialStateService::class); + $this->config = \OCP\Server::get(IConfig::class); + $this->appManager = \OCP\Server::get(IAppManager::class); + $this->initialState = \OCP\Server::get(InitialStateService::class); + $this->navigationManager = \OCP\Server::get(INavigationManager::class); - // Add fallback theming variables if theming is disabled - if ($renderAs !== TemplateResponse::RENDER_AS_USER - || !\OC::$server->getAppManager()->isEnabledForUser('theming')) { + // Add fallback theming variables if not rendered as user + if ($renderAs !== TemplateResponse::RENDER_AS_USER) { // TODO cache generated default theme if enabled for fallback if server is erroring ? Util::addStyle('theming', 'default'); } // Decide which page we show if ($renderAs === TemplateResponse::RENDER_AS_USER) { - /** @var INavigationManager */ - $this->navigationManager = \OC::$server->get(INavigationManager::class); - parent::__construct('core', 'layout.user'); if (in_array(\OC_App::getCurrentApp(), ['settings','admin', 'help']) !== false) { $this->assign('bodyid', 'body-settings'); @@ -90,7 +83,7 @@ public function __construct($renderAs, $appId = '') { } // Set body data-theme $this->assign('enabledThemes', []); - if (\OC::$server->getAppManager()->isEnabledForUser('theming') && class_exists('\OCA\Theming\Service\ThemesService')) { + if ($this->appManager->isEnabledForUser('theming') && class_exists('\OCA\Theming\Service\ThemesService')) { /** @var \OCA\Theming\Service\ThemesService */ $themesService = \OC::$server->get(\OCA\Theming\Service\ThemesService::class); $this->assign('enabledThemes', $themesService->getEnabledThemes()); @@ -101,8 +94,8 @@ public function __construct($renderAs, $appId = '') { $this->assign('logoUrl', $logoUrl); // Set default entry name - $defaultEntryId = \OCP\Server::get(INavigationManager::class)->getDefaultEntryIdForUser(); - $defaultEntry = \OCP\Server::get(INavigationManager::class)->get($defaultEntryId); + $defaultEntryId = $this->navigationManager->getDefaultEntryIdForUser(); + $defaultEntry = $this->navigationManager->get($defaultEntryId); $this->assign('defaultAppName', $defaultEntry['name']); // Add navigation entry @@ -182,8 +175,7 @@ public function __construct($renderAs, $appId = '') { $showSimpleSignup = true; } - $appManager = \OCP\Server::get(IAppManager::class); - if ($appManager->isEnabledForUser('registration')) { + if ($this->appManager->isEnabledForUser('registration')) { $urlGenerator = \OCP\Server::get(IURLGenerator::class); $signUpLink = $urlGenerator->getAbsoluteURL('/index.php/apps/registration/'); } @@ -203,7 +195,7 @@ public function __construct($renderAs, $appId = '') { $this->assign('locale', $locale); $this->assign('direction', $direction); - if (\OC::$server->getSystemConfig()->getValue('installed', false)) { + if ($this->config->getSystemValueBool('installed', false)) { if (empty(self::$versionHash)) { $v = \OC_App::getAppVersions(); $v['core'] = implode('.', \OCP\Util::getVersion()); @@ -224,7 +216,7 @@ public function __construct($renderAs, $appId = '') { \OCP\Server::get(ServerVersion::class), \OCP\Util::getL10N('lib'), \OCP\Server::get(Defaults::class), - \OC::$server->getAppManager(), + $this->appManager, \OC::$server->getSession(), \OC::$server->getUserSession()->getUser(), $this->config, @@ -314,34 +306,52 @@ protected function getVersionHashSuffix($path = false, $file = false) { // allows chrome workspace mapping in debug mode return ''; } - $themingSuffix = ''; - $v = []; - if ($this->config->getSystemValueBool('installed', false)) { - if (\OC::$server->getAppManager()->isInstalled('theming')) { - $themingSuffix = '-' . $this->config->getAppValue('theming', 'cachebuster', '0'); - } - $v = \OC_App::getAppVersions(); + if ($this->config->getSystemValueBool('installed', false) === false) { + // if not installed just return the version hash + return '?v=' . self::$versionHash; } - // Try the webroot path for a match - if ($path !== false && $path !== '') { - $appName = $this->getAppNamefromPath($path); - if (array_key_exists($appName, $v)) { - $appVersion = $v[$appName]; - return '?v=' . substr(md5($appVersion), 0, 8) . $themingSuffix; - } + $hash = false; + // Try the web-root first + if (is_string($path) && $path !== '') { + $hash = $this->getVersionHashByPath($path); + } + // If not found try the file + if ($hash === false && is_string($file) && $file !== '') { + $hash = $this->getVersionHashByPath($file); } - // fallback to the file path instead - if ($file !== false && $file !== '') { - $appName = $this->getAppNamefromPath($file); - if (array_key_exists($appName, $v)) { - $appVersion = $v[$appName]; - return '?v=' . substr(md5($appVersion), 0, 8) . $themingSuffix; + // As a last resort we use the server version hash + if ($hash === false) { + $hash = self::$versionHash; + } + + // The theming app is force-enabled thus the cache buster is always available + $themingSuffix = '-' . $this->config->getAppValue('theming', 'cachebuster', '0'); + + return '?v=' . $hash . $themingSuffix; + } + + private function getVersionHashByPath(string $path): string|false { + if (array_key_exists($path, self::$cacheBusterCache) === false) { + // Not yet cached, so lets find the cache buster string + $appId = $this->getAppNamefromPath($path); + if ($appId === false || $appId === null) { + // No app Id could be guessed + 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; } + + $hash = substr(md5($appVersion), 0, 8); + self::$cacheBusterCache[$path] = $hash; } - return '?v=' . self::$versionHash . $themingSuffix; + return self::$cacheBusterCache[$path]; } /** diff --git a/tests/lib/TemplateLayoutTest.php b/tests/lib/TemplateLayoutTest.php new file mode 100644 index 0000000000000..51a1363ab8ed1 --- /dev/null +++ b/tests/lib/TemplateLayoutTest.php @@ -0,0 +1,91 @@ +createMock(IAppManager::class); + $appManager->expects(self::any()) + ->method('getAppVersion') + ->willReturnCallback(fn ($appId) => match ($appId) { + 'shippedApp' => 'shipped_1', + 'otherApp' => 'other_2', + default => "$appId", + }); + $appManager->expects(self::any()) + ->method('isShipped') + ->willReturnCallback(fn (string $app) => $app === 'shippedApp'); + + $config = $this->createMock(IConfig::class); + $config->expects(self::atLeastOnce()) + ->method('getSystemValueBool') + ->willReturnMap([ + ['installed', false, $installed], + ['debug', false, $debug], + ]); + $config->expects(self::any()) + ->method('getAppValue') + ->with('theming', 'cachebuster', '0') + ->willReturn('42'); + + $initialState = $this->createMock(InitialStateService::class); + + $this->overwriteService(IConfig::class, $config); + $this->overwriteService(IAppManager::class, $appManager); + $this->overwriteService(InitialStateService::class, $initialState); + + $layout = $this->getMockBuilder(TemplateLayout::class) + ->onlyMethods(['getAppNamefromPath']) + ->setConstructorArgs([TemplateResponse::RENDER_AS_ERROR]) + ->getMock(); + + self::invokePrivate(TemplateLayout::class, 'versionHash', ['version_hash']); + + $layout->expects(self::any()) + ->method('getAppNamefromPath') + ->willReturnCallback(fn ($appName) => match($appName) { + 'apps/shipped' => 'shippedApp', + 'other/app.css' => 'otherApp', + default => false, + }); + + $hash = self::invokePrivate($layout, 'getVersionHashSuffix', [$path, $file]); + self::assertEquals($expected, $hash); + } + + public static function dataVersionHash() { + return [ + 'no hash if in debug mode' => ['apps/shipped', 'style.css', true, true, ''], + 'only version hash if not installed' => ['apps/shipped', 'style.css', false, false, '?v=version_hash'], + 'version hash with cache buster if app not found' => ['unknown/path', '', true, false, '?v=version_hash-42'], + 'version hash with cache buster if neither path nor file provided' => [false, false, true, false, '?v=version_hash-42'], + 'app version hash if external app' => ['', 'other/app.css', true, false, '?v=' . substr(md5('other_2'), 0, 8) . '-42'], + 'app version and version hash if shipped app' => ['apps/shipped', 'style.css', true, false, '?v=' . substr(md5('shipped_1-version_hash'), 0, 8) . '-42'], + 'prefer path over file' => ['apps/shipped', 'other/app.css', true, false, '?v=' . substr(md5('shipped_1-version_hash'), 0, 8) . '-42'], + ]; + } + +}