From 3d4c4f19b4061be862ada764f453a3fdd035e1fb Mon Sep 17 00:00:00 2001 From: =?utf8?q?C=C3=B4me=20Chilliet?= Date: Tue, 12 Dec 2023 16:53:13 +0100 Subject: [PATCH] Migrate app dir owner check to SetupCheck API MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + apps/settings/lib/AppInfo/Application.php | 2 + .../lib/Controller/CheckSetupController.php | 49 --------- .../SetupChecks/AppDirsWithDifferentOwner.php | 104 ++++++++++++++++++ .../Controller/CheckSetupControllerTest.php | 1 - core/js/setupchecks.js | 15 --- core/js/tests/specs/setupchecksSpec.js | 63 ----------- 8 files changed, 108 insertions(+), 128 deletions(-) create mode 100644 apps/settings/lib/SetupChecks/AppDirsWithDifferentOwner.php 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 @@ + + * + * @author Côme Chilliet + * + * @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 . + * + */ +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, diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js index e85f32dae4a..4331e8e9cc9 100644 --- a/core/js/setupchecks.js +++ b/core/js/setupchecks.js @@ -271,21 +271,6 @@ }) } - if(data.appDirsWithDifferentOwner && data.appDirsWithDifferentOwner.length > 0) { - var appDirsWithDifferentOwner = data.appDirsWithDifferentOwner.reduce( - function(appDirsWithDifferentOwner, directory) { - return appDirsWithDifferentOwner + '
  • ' + directory + '
    • '; - }, - '' - ); - messages.push({ - msg: t('core', '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:') - + '
      ' + appDirsWithDifferentOwner + '
    ', - type: OC.SetupChecks.MESSAGE_TYPE_WARNING - }); - } if (data.isMysqlUsedWithoutUTF8MB4) { messages.push({ msg: t('core', 'MySQL is used as database but does not support 4-byte characters. To be able to handle 4-byte characters (like emojis) without issues in filenames or comments for example it is recommended to enable the 4-byte support in MySQL. For further details read {linkstart}the documentation page about this ↗{linkend}.') diff --git a/core/js/tests/specs/setupchecksSpec.js b/core/js/tests/specs/setupchecksSpec.js index bb7d118fc52..594b9ca39bc 100644 --- a/core/js/tests/specs/setupchecksSpec.js +++ b/core/js/tests/specs/setupchecksSpec.js @@ -232,7 +232,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, @@ -280,7 +279,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, @@ -328,7 +326,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, @@ -376,7 +373,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, @@ -404,54 +400,6 @@ describe('OC.SetupChecks tests', function() { }); }); - it('should return a warning if there are app directories with wrong permissions', function(done) { - var async = OC.SetupChecks.checkSetup(); - - suite.server.requests[0].respond( - 200, - { - 'Content-Type': 'application/json', - }, - JSON.stringify({ - suggestedOverwriteCliURL: '', - isFairUseOfFreePushService: true, - isCorrectMemcachedPHPModuleInstalled: true, - hasPassedCodeIntegrityCheck: true, - isSettimelimitAvailable: true, - cronErrors: [], - cronInfo: { - diffInSeconds: 0 - }, - appDirsWithDifferentOwner: [ - '/some/path' - ], - isImagickEnabled: true, - areWebauthnExtensionsEnabled: true, - isMysqlUsedWithoutUTF8MB4: false, - isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, - reverseProxyGeneratedURL: 'https://server', - temporaryDirectoryWritable: true, - generic: { - network: { - "Internet connectivity": { - severity: "success", - description: null, - linkToDoc: null - } - }, - }, - }) - ); - - async.done(function( data, s, x ){ - expect(data).toEqual([{ - msg: '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:
    • /some/path
    ', - type: OC.SetupChecks.MESSAGE_TYPE_WARNING - }]); - done(); - }); - }); - it('should return an error if set_time_limit is unavailable', function(done) { var async = OC.SetupChecks.checkSetup(); @@ -471,7 +419,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, @@ -518,7 +465,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, @@ -596,7 +542,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, @@ -649,7 +594,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: true, @@ -699,7 +643,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, @@ -746,7 +689,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, @@ -790,7 +732,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, @@ -837,7 +778,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: false, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, @@ -884,7 +824,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: false, isMysqlUsedWithoutUTF8MB4: false, @@ -930,7 +869,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, @@ -983,7 +921,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, -- 2.39.5