Skip to content

Commit

Permalink
Merge pull request #42176 from nextcloud/feat/migrate-appdirowner-set…
Browse files Browse the repository at this point in the history
…upcheck

Migrate app dir owner check to new SetupCheck API
  • Loading branch information
come-nc authored Jan 9, 2024
2 parents dc2066b + d2dbe1c commit 012dcbb
Show file tree
Hide file tree
Showing 9 changed files with 208 additions and 185 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'] ?? '']));
}
}
}
58 changes: 0 additions & 58 deletions apps/settings/tests/Controller/CheckSetupControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
*/
namespace OCA\Settings\Tests\Controller;

use OC;
use OC\IntegrityCheck\Checker;
use OCA\Settings\Controller\CheckSetupController;
use OCP\AppFramework\Http;
Expand Down Expand Up @@ -140,7 +139,6 @@ protected function setUp(): void {
'getCurlVersion',
'isPhpOutdated',
'isPHPMailerUsed',
'getAppDirsWithDifferentOwner',
'isImagickEnabled',
'areWebauthnExtensionsEnabled',
'isMysqlUsedWithoutUTF8MB4',
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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')
Expand Down
Loading

0 comments on commit 012dcbb

Please sign in to comment.