Migrate app dir owner check to SetupCheck API

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>

6 files changed, 108 insertions, 50 deletions

diff --git a/apps/settings/composer/composer/autoload_classmap.php b/apps/settings/composer/composer/autoload_classmap.php
index b32aab97569..60717a47294 100644
--- a/apps/settings/composer/composer/autoload_classmap.php
+++ b/apps/settings/composer/composer/autoload_classmap.php
@@ -76,6 +76,7 @@ return array(
     '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',
diff --git a/apps/settings/composer/composer/autoload_static.php b/apps/settings/composer/composer/autoload_static.php
index 09e0d724e4c..7cd3892a6ec 100644
--- a/apps/settings/composer/composer/autoload_static.php
+++ b/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',
diff --git a/apps/settings/lib/AppInfo/Application.php b/apps/settings/lib/AppInfo/Application.php
index 7e963e35593..f3e2343e048 100644
--- a/apps/settings/lib/AppInfo/Application.php
+++ b/apps/settings/lib/AppInfo/Application.php
@@ -48,6 +48,7 @@ use OCA\Settings\Middleware\SubadminMiddleware;
 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 @@ class Application extends App implements IBootstrap {
 				Util::getDefaultEmailAddress('no-reply')
 			);
 		});
+		$context->registerSetupCheck(AppDirsWithDifferentOwner::class);
 		$context->registerSetupCheck(BruteForceThrottler::class);
 		$context->registerSetupCheck(CheckUserCertificates::class);
 		$context->registerSetupCheck(DatabaseHasMissingColumns::class);
diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php
index b251adb0d84..acd73479ea1 100644
--- a/apps/settings/lib/Controller/CheckSetupController.php
+++ b/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 @@ Raw output
 		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 @@ Raw output
 				'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(),
diff --git a/apps/settings/lib/SetupChecks/AppDirsWithDifferentOwner.php b/apps/settings/lib/SetupChecks/AppDirsWithDifferentOwner.php
new file mode 100644
index 00000000000..3404e8dc8b3
--- /dev/null
+++ b/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'] ?? '']));
+		}
+	}
+}
diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php
index b9c2ddc8282..6336ca852d6 100644
--- a/apps/settings/tests/Controller/CheckSetupControllerTest.php
+++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php
@@ -270,7 +270,6 @@ class CheckSetupControllerTest extends TestCase {
 				'hasPassedCodeIntegrityCheck' => true,
 				'codeIntegrityCheckerDocumentation' => 'http://docs.example.org/server/go.php?to=admin-code-integrity',
 				'isSettimelimitAvailable' => true,
-				'appDirsWithDifferentOwner' => [],
 				'isImagickEnabled' => false,
 				'areWebauthnExtensionsEnabled' => false,
 				'isMysqlUsedWithoutUTF8MB4' => false,