fix: Shipped apps should include the Nextcloud version in the cache busterfix/shipped-app-version

Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de>
Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

3 files changed, 142 insertions, 52 deletions

diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml
index 16fe161a9ba..1a9b15c00cd 100644
--- a/build/psalm-baseline.xml
+++ b/build/psalm-baseline.xml
@@ -2693,12 +2693,8 @@
       <code><![CDATA[string]]></code>
     </InvalidParamDefault>
     <InvalidScalarArgument>
-      <code><![CDATA[$appName]]></code>
-      <code><![CDATA[$appName]]></code>
+      <code><![CDATA[$appId]]></code>
     </InvalidScalarArgument>
-    <UndefinedInterfaceMethod>
-      <code><![CDATA[getInitialStates]]></code>
-    </UndefinedInterfaceMethod>
   </file>
   <file src="lib/private/URLGenerator.php">
     <InvalidReturnStatement>
diff --git a/lib/private/TemplateLayout.php b/lib/private/TemplateLayout.php
index 60c7526435e..2bc9ff5a2af 100644
--- a/lib/private/TemplateLayout.php
+++ b/lib/private/TemplateLayout.php
@@ -31,6 +31,8 @@ use OCP\Util;
 
 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 @@ class TemplateLayout extends \OC_Template {
 			}
 			// 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 @@ class TemplateLayout extends \OC_Template {
 			$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 @@ class TemplateLayout extends \OC_Template {
 				$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 @@ class TemplateLayout extends \OC_Template {
 		$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 @@ class TemplateLayout extends \OC_Template {
 				\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 @@ class TemplateLayout extends \OC_Template {
 			// 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 00000000000..405f1df7330
--- /dev/null
+++ b/tests/lib/TemplateLayoutTest.php
@@ -0,0 +1,84 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace Test;
+
+use OC\InitialStateService;
+use OC\TemplateLayout;
+use OCP\App\IAppManager;
+use OCP\AppFramework\Http\TemplateResponse;
+use OCP\IConfig;
+
+class TemplateLayoutTest extends \Test\TestCase {
+
+
+	/** @dataProvider dataVersionHash */
+	public function testVersionHash($path, $file, $installed, $debug, $expected): void {
+		$appManager = $this->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'],
+		];
+	}
+
+}