Skip to content

Commit

Permalink
Migrate app dir owner check to SetupCheck API
Browse files Browse the repository at this point in the history
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
  • Loading branch information
come-nc committed Jan 15, 2024
1 parent 3de96ac commit 14d132a
Show file tree
Hide file tree
Showing 8 changed files with 108 additions and 128 deletions.
1 change: 1 addition & 0 deletions apps/settings/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
1 change: 1 addition & 0 deletions apps/settings/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
2 changes: 2 additions & 0 deletions apps/settings/lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
49 changes: 0 additions & 49 deletions apps/settings/lib/Controller/CheckSetupController.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
*/
namespace OCA\Settings\Controller;

use DirectoryIterator;
use GuzzleHttp\Exception\ClientException;
use OC\AppFramework\Http;
use OC\IntegrityCheck\Checker;
Expand Down Expand Up @@ -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')) {
Expand Down Expand Up @@ -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(),
Expand Down
104 changes: 104 additions & 0 deletions apps/settings/lib/SetupChecks/AppDirsWithDifferentOwner.php
Original file line number Diff line number Diff line change
@@ -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'] ?? '']));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,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,
Expand Down
15 changes: 0 additions & 15 deletions core/js/setupchecks.js
Original file line number Diff line number Diff line change
Expand Up @@ -271,21 +271,6 @@
})
}

if(data.appDirsWithDifferentOwner && data.appDirsWithDifferentOwner.length > 0) {
var appDirsWithDifferentOwner = data.appDirsWithDifferentOwner.reduce(
function(appDirsWithDifferentOwner, directory) {
return appDirsWithDifferentOwner + '<li>' + directory + '</li>';
},
''
);
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:')
+ '<ul>' + appDirsWithDifferentOwner + '</ul>',
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}.')
Expand Down

0 comments on commit 14d132a

Please sign in to comment.