Migrate app dir owner check to new SetupCheck API

apps/settings/composer/composer/autoload_classmap.php

@@ -76,6 +76,7 @@
     'OCA\\Settings\\Settings\\Personal\\Security\\TwoFactor' => $baseDir . '/../lib/Settings/Personal/Security/TwoFactor.php',
     'OCA\\Settings\\Settings\\Personal\\Security\\WebAuthn' => $baseDir . '/../lib/Settings/Personal/Security/WebAuthn.php',
     'OCA\\Settings\\Settings\\Personal\\ServerDevNotice' => $baseDir . '/../lib/Settings/Personal/ServerDevNotice.php',
+    'OCA\\Settings\\SetupChecks\\AppDirsWithDifferentOwner' => $baseDir . '/../lib/SetupChecks/AppDirsWithDifferentOwner.php',
     'OCA\\Settings\\SetupChecks\\BruteForceThrottler' => $baseDir . '/../lib/SetupChecks/BruteForceThrottler.php',
     'OCA\\Settings\\SetupChecks\\CheckUserCertificates' => $baseDir . '/../lib/SetupChecks/CheckUserCertificates.php',
     'OCA\\Settings\\SetupChecks\\DatabaseHasMissingColumns' => $baseDir . '/../lib/SetupChecks/DatabaseHasMissingColumns.php',

apps/settings/composer/composer/autoload_static.php

@@ -91,6 +91,7 @@ class ComposerStaticInitSettings
         'OCA\\Settings\\Settings\\Personal\\Security\\TwoFactor' => __DIR__ . '/..' . '/../lib/Settings/Personal/Security/TwoFactor.php',
         'OCA\\Settings\\Settings\\Personal\\Security\\WebAuthn' => __DIR__ . '/..' . '/../lib/Settings/Personal/Security/WebAuthn.php',
         'OCA\\Settings\\Settings\\Personal\\ServerDevNotice' => __DIR__ . '/..' . '/../lib/Settings/Personal/ServerDevNotice.php',
+        'OCA\\Settings\\SetupChecks\\AppDirsWithDifferentOwner' => __DIR__ . '/..' . '/../lib/SetupChecks/AppDirsWithDifferentOwner.php',
         'OCA\\Settings\\SetupChecks\\BruteForceThrottler' => __DIR__ . '/..' . '/../lib/SetupChecks/BruteForceThrottler.php',
         'OCA\\Settings\\SetupChecks\\CheckUserCertificates' => __DIR__ . '/..' . '/../lib/SetupChecks/CheckUserCertificates.php',
         'OCA\\Settings\\SetupChecks\\DatabaseHasMissingColumns' => __DIR__ . '/..' . '/../lib/SetupChecks/DatabaseHasMissingColumns.php',

apps/settings/lib/AppInfo/Application.php

@@ -48,6 +48,7 @@
 use OCA\Settings\Search\AppSearch;
 use OCA\Settings\Search\SectionSearch;
 use OCA\Settings\Search\UserSearch;
+use OCA\Settings\SetupChecks\AppDirsWithDifferentOwner;
 use OCA\Settings\SetupChecks\BruteForceThrottler;
 use OCA\Settings\SetupChecks\CheckUserCertificates;
 use OCA\Settings\SetupChecks\DatabaseHasMissingColumns;
@@ -164,6 +165,7 @@ public function register(IRegistrationContext $context): void {
 				Util::getDefaultEmailAddress('no-reply')
 			);
 		});
+		$context->registerSetupCheck(AppDirsWithDifferentOwner::class);
 		$context->registerSetupCheck(BruteForceThrottler::class);
 		$context->registerSetupCheck(CheckUserCertificates::class);
 		$context->registerSetupCheck(DatabaseHasMissingColumns::class);

apps/settings/lib/Controller/CheckSetupController.php

@@ -45,7 +45,6 @@
  */
 namespace OCA\Settings\Controller;
 
-use DirectoryIterator;
 use GuzzleHttp\Exception\ClientException;
 use OC\AppFramework\Http;
 use OC\IntegrityCheck\Checker;
@@ -343,53 +342,6 @@ private function isTemporaryDirectoryWritable(): bool {
 		return false;
 	}
 
-	/**
-	 * Iterates through the configured app roots and
-	 * tests if the subdirectories are owned by the same user than the current user.
-	 *
-	 * @return array
-	 */
-	protected function getAppDirsWithDifferentOwner(): array {
-		$currentUser = posix_getuid();
-		$appDirsWithDifferentOwner = [[]];
-
-		foreach (\OC::$APPSROOTS as $appRoot) {
-			if ($appRoot['writable'] === true) {
-				$appDirsWithDifferentOwner[] = $this->getAppDirsWithDifferentOwnerForAppRoot($currentUser, $appRoot);
-			}
-		}
-
-		$appDirsWithDifferentOwner = array_merge(...$appDirsWithDifferentOwner);
-		sort($appDirsWithDifferentOwner);
-
-		return $appDirsWithDifferentOwner;
-	}
-
-	/**
-	 * Tests if the directories for one apps directory are writable by the current user.
-	 *
-	 * @param int $currentUser The current user
-	 * @param array $appRoot The app root config
-	 * @return string[] The none writable directory paths inside the app root
-	 */
-	private function getAppDirsWithDifferentOwnerForAppRoot(int $currentUser, array $appRoot): array {
-		$appDirsWithDifferentOwner = [];
-		$appsPath = $appRoot['path'];
-		$appsDir = new DirectoryIterator($appRoot['path']);
-
-		foreach ($appsDir as $fileInfo) {
-			if ($fileInfo->isDir() && !$fileInfo->isDot()) {
-				$absAppPath = $appsPath . DIRECTORY_SEPARATOR . $fileInfo->getFilename();
-				$appDirUser = fileowner($absAppPath);
-				if ($appDirUser !== $currentUser) {
-					$appDirsWithDifferentOwner[] = $absAppPath;
-				}
-			}
-		}
-
-		return $appDirsWithDifferentOwner;
-	}
-
 	protected function isImagickEnabled(): bool {
 		if ($this->config->getAppValue('theming', 'enabled', 'no') === 'yes') {
 			if (!extension_loaded('imagick')) {
@@ -470,7 +422,6 @@ public function check() {
 				'hasPassedCodeIntegrityCheck' => $this->checker->hasPassedCheck(),
 				'codeIntegrityCheckerDocumentation' => $this->urlGenerator->linkToDocs('admin-code-integrity'),
 				'isSettimelimitAvailable' => $this->isSettimelimitAvailable(),
-				'appDirsWithDifferentOwner' => $this->getAppDirsWithDifferentOwner(),
 				'isImagickEnabled' => $this->isImagickEnabled(),
 				'areWebauthnExtensionsEnabled' => $this->areWebauthnExtensionsEnabled(),
 				'isMysqlUsedWithoutUTF8MB4' => $this->isMysqlUsedWithoutUTF8MB4(),

apps/settings/lib/SetupChecks/AppDirsWithDifferentOwner.php

@@ -0,0 +1,104 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright Copyright (c) 2023 Côme Chilliet <come.chilliet@nextcloud.com>
+ *
+ * @author Côme Chilliet <come.chilliet@nextcloud.com>
+ *
+ * @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 <http://www.gnu.org/licenses/>.
+ *
+ */
+namespace OCA\Settings\SetupChecks;
+
+use OCP\IL10N;
+use OCP\SetupCheck\ISetupCheck;
+use OCP\SetupCheck\SetupResult;
+
+class AppDirsWithDifferentOwner implements ISetupCheck {
+	public function __construct(
+		private IL10N $l10n,
+	) {
+	}
+
+	public function getName(): string {
+		return $this->l10n->t('App directories owner');
+	}
+
+	public function getCategory(): string {
+		return 'security';
+	}
+
+	/**
+	 * Iterates through the configured app roots and
+	 * tests if the subdirectories are owned by the same user than the current user.
+	 *
+	 * @return string[]
+	 */
+	private function getAppDirsWithDifferentOwner(int $currentUser): array {
+		$appDirsWithDifferentOwner = [[]];
+
+		foreach (\OC::$APPSROOTS as $appRoot) {
+			if ($appRoot['writable'] === true) {
+				$appDirsWithDifferentOwner[] = $this->getAppDirsWithDifferentOwnerForAppRoot($currentUser, $appRoot);
+			}
+		}
+
+		$appDirsWithDifferentOwner = array_merge(...$appDirsWithDifferentOwner);
+		sort($appDirsWithDifferentOwner);
+
+		return $appDirsWithDifferentOwner;
+	}
+
+	/**
+	 * Tests if the directories for one apps directory are writable by the current user.
+	 *
+	 * @param int $currentUser The current user
+	 * @param array $appRoot The app root config
+	 * @return string[] The none writable directory paths inside the app root
+	 */
+	private function getAppDirsWithDifferentOwnerForAppRoot(int $currentUser, array $appRoot): array {
+		$appDirsWithDifferentOwner = [];
+		$appsPath = $appRoot['path'];
+		$appsDir = new \DirectoryIterator($appRoot['path']);
+
+		foreach ($appsDir as $fileInfo) {
+			if ($fileInfo->isDir() && !$fileInfo->isDot()) {
+				$absAppPath = $appsPath . DIRECTORY_SEPARATOR . $fileInfo->getFilename();
+				$appDirUser = fileowner($absAppPath);
+				if ($appDirUser !== $currentUser) {
+					$appDirsWithDifferentOwner[] = $absAppPath;
+				}
+			}
+		}
+
+		return $appDirsWithDifferentOwner;
+	}
+
+	public function run(): SetupResult {
+		$currentUser = posix_getuid();
+		$currentUserInfos = posix_getpwuid($currentUser) ?: [];
+		$appDirsWithDifferentOwner = $this->getAppDirsWithDifferentOwner($currentUser);
+		if (count($appDirsWithDifferentOwner) > 0) {
+			return SetupResult::warning(
+				$this->l10n->t("Some app directories are owned by a different user than the web server one. This may be the case if apps have been installed manually. Check the permissions of the following app directories:\n%s", implode("\n", $appDirsWithDifferentOwner))
+			);
+		} else {
+			return SetupResult::success($this->l10n->t('App directories have the correct owner "%s"', [$currentUserInfos['name'] ?? '']));
+		}
+	}
+}

apps/settings/tests/Controller/CheckSetupControllerTest.php

@@ -34,7 +34,6 @@
  */
 namespace OCA\Settings\Tests\Controller;
 
-use OC;
 use OC\IntegrityCheck\Checker;
 use OCA\Settings\Controller\CheckSetupController;
 use OCP\AppFramework\Http;
@@ -140,7 +139,6 @@ protected function setUp(): void {
 				'getCurlVersion',
 				'isPhpOutdated',
 				'isPHPMailerUsed',
-				'getAppDirsWithDifferentOwner',
 				'isImagickEnabled',
 				'areWebauthnExtensionsEnabled',
 				'isMysqlUsedWithoutUTF8MB4',
@@ -199,11 +197,6 @@ public function testCheck() {
 			->method('hasPassedCheck')
 			->willReturn(true);
 
-		$this->checkSetupController
-			->expects($this->once())
-			->method('getAppDirsWithDifferentOwner')
-			->willReturn([]);
-
 		$this->checkSetupController
 			->expects($this->once())
 			->method('isImagickEnabled')
@@ -270,7 +263,6 @@ public function testCheck() {
 				'hasPassedCodeIntegrityCheck' => true,
 				'codeIntegrityCheckerDocumentation' => 'http://docs.example.org/server/go.php?to=admin-code-integrity',
 				'isSettimelimitAvailable' => true,
-				'appDirsWithDifferentOwner' => [],
 				'isImagickEnabled' => false,
 				'areWebauthnExtensionsEnabled' => false,
 				'isMysqlUsedWithoutUTF8MB4' => false,
@@ -350,56 +342,6 @@ public function testIsUsedTlsLibOutdatedWithMatchingOpenSslVersion1() {
 		$this->assertSame('', $this->invokePrivate($this->checkSetupController, 'isUsedTlsLibOutdated'));
 	}
 
-	/**
-	 * Setups a temp directory and some subdirectories.
-	 * Then calls the 'getAppDirsWithDifferentOwner' method.
-	 * The result is expected to be empty since
-	 * there are no directories with different owners than the current user.
-	 *
-	 * @return void
-	 */
-	public function testAppDirectoryOwnersOk() {
-		$tempDir = tempnam(sys_get_temp_dir(), 'apps') . 'dir';
-		mkdir($tempDir);
-		mkdir($tempDir . DIRECTORY_SEPARATOR . 'app1');
-		mkdir($tempDir . DIRECTORY_SEPARATOR . 'app2');
-		$this->dirsToRemove[] = $tempDir . DIRECTORY_SEPARATOR . 'app1';
-		$this->dirsToRemove[] = $tempDir . DIRECTORY_SEPARATOR . 'app2';
-		$this->dirsToRemove[] = $tempDir;
-		OC::$APPSROOTS = [
-			[
-				'path' => $tempDir,
-				'url' => '/apps',
-				'writable' => true,
-			],
-		];
-		$this->assertSame(
-			[],
-			$this->invokePrivate($this->checkSetupController, 'getAppDirsWithDifferentOwner')
-		);
-	}
-
-	/**
-	 * Calls the check for a none existing app root that is marked as not writable.
-	 * It's expected that no error happens since the check shouldn't apply.
-	 *
-	 * @return void
-	 */
-	public function testAppDirectoryOwnersNotWritable() {
-		$tempDir = tempnam(sys_get_temp_dir(), 'apps') . 'dir';
-		OC::$APPSROOTS = [
-			[
-				'path' => $tempDir,
-				'url' => '/apps',
-				'writable' => false,
-			],
-		];
-		$this->assertSame(
-			[],
-			$this->invokePrivate($this->checkSetupController, 'getAppDirsWithDifferentOwner')
-		);
-	}
-
 	public function testIsBuggyNss400() {
 		$this->config->expects($this->any())
 			->method('getSystemValue')