Merge pull request #50407 from nextcloud/backport/50398/stable31

[stable31] fix: Correctly return app id and app version for `core` styles and images

Author: AndyScherzinger

build/psalm-baseline.xml

@@ -2576,15 +2576,6 @@
       <code><![CDATA[$script]]></code>
     </ParamNameMismatch>
   </file>
-  <file src="lib/private/TemplateLayout.php">
-    <InvalidParamDefault>
-      <code><![CDATA[string]]></code>
-      <code><![CDATA[string]]></code>
-    </InvalidParamDefault>
-    <InvalidScalarArgument>
-      <code><![CDATA[$appId]]></code>
-    </InvalidScalarArgument>
-  </file>
   <file src="lib/private/URLGenerator.php">
     <InvalidReturnStatement>
       <code><![CDATA[$path]]></code>

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];
 	}

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);

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
@@ -349,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;
 		}
 
@@ -376,14 +376,16 @@ 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)) {
 			$pathParts = explode('/', $path);
 			if ($pathParts[0] === 'css') {
 				// This is a scss request
 				return $pathParts[1];
+			} elseif ($pathParts[0] === 'core') {
+				return 'core';
 			}
 			return end($pathParts);
 		}

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'),
+		);
+	}
+
 }

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),
 		));
 	}