From 4342cf4db8b18fe7a3cdb40240b4eb865ca1ee74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 14 Nov 2023 16:57:32 +0100 Subject: [PATCH 01/12] Migrate database pending bigint conversions check to new API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 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 | 56 ----------- .../DatabasePendingBigIntConversions.php | 99 +++++++++++++++++++ .../Controller/CheckSetupControllerTest.php | 29 ------ core/Command/Db/ConvertFilecacheBigInt.php | 8 +- core/js/setupchecks.js | 12 --- core/js/tests/specs/setupchecksSpec.js | 17 ---- 9 files changed, 108 insertions(+), 117 deletions(-) create mode 100644 apps/settings/lib/SetupChecks/DatabasePendingBigIntConversions.php diff --git a/apps/settings/composer/composer/autoload_classmap.php b/apps/settings/composer/composer/autoload_classmap.php index 687a97fe7679a..4ad4e2b652558 100644 --- a/apps/settings/composer/composer/autoload_classmap.php +++ b/apps/settings/composer/composer/autoload_classmap.php @@ -81,6 +81,7 @@ 'OCA\\Settings\\SetupChecks\\DatabaseHasMissingColumns' => $baseDir . '/../lib/SetupChecks/DatabaseHasMissingColumns.php', 'OCA\\Settings\\SetupChecks\\DatabaseHasMissingIndices' => $baseDir . '/../lib/SetupChecks/DatabaseHasMissingIndices.php', 'OCA\\Settings\\SetupChecks\\DatabaseHasMissingPrimaryKeys' => $baseDir . '/../lib/SetupChecks/DatabaseHasMissingPrimaryKeys.php', + 'OCA\\Settings\\SetupChecks\\DatabasePendingBigIntConversions' => $baseDir . '/../lib/SetupChecks/DatabasePendingBigIntConversions.php', 'OCA\\Settings\\SetupChecks\\DefaultPhoneRegionSet' => $baseDir . '/../lib/SetupChecks/DefaultPhoneRegionSet.php', 'OCA\\Settings\\SetupChecks\\EmailTestSuccessful' => $baseDir . '/../lib/SetupChecks/EmailTestSuccessful.php', 'OCA\\Settings\\SetupChecks\\FileLocking' => $baseDir . '/../lib/SetupChecks/FileLocking.php', diff --git a/apps/settings/composer/composer/autoload_static.php b/apps/settings/composer/composer/autoload_static.php index 3308b4a7eea29..dc4a2b21b3508 100644 --- a/apps/settings/composer/composer/autoload_static.php +++ b/apps/settings/composer/composer/autoload_static.php @@ -96,6 +96,7 @@ class ComposerStaticInitSettings 'OCA\\Settings\\SetupChecks\\DatabaseHasMissingColumns' => __DIR__ . '/..' . '/../lib/SetupChecks/DatabaseHasMissingColumns.php', 'OCA\\Settings\\SetupChecks\\DatabaseHasMissingIndices' => __DIR__ . '/..' . '/../lib/SetupChecks/DatabaseHasMissingIndices.php', 'OCA\\Settings\\SetupChecks\\DatabaseHasMissingPrimaryKeys' => __DIR__ . '/..' . '/../lib/SetupChecks/DatabaseHasMissingPrimaryKeys.php', + 'OCA\\Settings\\SetupChecks\\DatabasePendingBigIntConversions' => __DIR__ . '/..' . '/../lib/SetupChecks/DatabasePendingBigIntConversions.php', 'OCA\\Settings\\SetupChecks\\DefaultPhoneRegionSet' => __DIR__ . '/..' . '/../lib/SetupChecks/DefaultPhoneRegionSet.php', 'OCA\\Settings\\SetupChecks\\EmailTestSuccessful' => __DIR__ . '/..' . '/../lib/SetupChecks/EmailTestSuccessful.php', 'OCA\\Settings\\SetupChecks\\FileLocking' => __DIR__ . '/..' . '/../lib/SetupChecks/FileLocking.php', diff --git a/apps/settings/lib/AppInfo/Application.php b/apps/settings/lib/AppInfo/Application.php index 12660f42ecc41..0da2485aa00a7 100644 --- a/apps/settings/lib/AppInfo/Application.php +++ b/apps/settings/lib/AppInfo/Application.php @@ -53,6 +53,7 @@ use OCA\Settings\SetupChecks\DatabaseHasMissingColumns; use OCA\Settings\SetupChecks\DatabaseHasMissingIndices; use OCA\Settings\SetupChecks\DatabaseHasMissingPrimaryKeys; +use OCA\Settings\SetupChecks\DatabasePendingBigIntConversions; use OCA\Settings\SetupChecks\DefaultPhoneRegionSet; use OCA\Settings\SetupChecks\EmailTestSuccessful; use OCA\Settings\SetupChecks\FileLocking; @@ -167,6 +168,7 @@ public function register(IRegistrationContext $context): void { $context->registerSetupCheck(DatabaseHasMissingColumns::class); $context->registerSetupCheck(DatabaseHasMissingIndices::class); $context->registerSetupCheck(DatabaseHasMissingPrimaryKeys::class); + $context->registerSetupCheck(DatabasePendingBigIntConversions::class); $context->registerSetupCheck(DefaultPhoneRegionSet::class); $context->registerSetupCheck(EmailTestSuccessful::class); $context->registerSetupCheck(FileLocking::class); diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php index 7bb2c8319c2e3..07ab12426bb0d 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -50,8 +50,6 @@ use GuzzleHttp\Exception\ClientException; use OC; use OC\AppFramework\Http; -use OC\DB\Connection; -use OC\DB\SchemaWrapper; use OC\IntegrityCheck\Checker; use OCP\App\IAppManager; use OCP\AppFramework\Controller; @@ -59,12 +57,10 @@ use OCP\AppFramework\Http\DataDisplayResponse; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\RedirectResponse; -use OCP\DB\Types; use OCP\EventDispatcher\IEventDispatcher; use OCP\Http\Client\IClientService; use OCP\IConfig; use OCP\IDateTimeFormatter; -use OCP\IDBConnection; use OCP\IL10N; use OCP\IRequest; use OCP\IServerContainer; @@ -91,16 +87,12 @@ class CheckSetupController extends Controller { private $logger; /** @var IEventDispatcher */ private $dispatcher; - /** @var Connection */ - private $db; /** @var ILockingProvider */ private $lockingProvider; /** @var IDateTimeFormatter */ private $dateTimeFormatter; /** @var IniGetWrapper */ private $iniGetWrapper; - /** @var IDBConnection */ - private $connection; /** @var ITempManager */ private $tempManager; /** @var IManager */ @@ -120,11 +112,9 @@ public function __construct($AppName, Checker $checker, LoggerInterface $logger, IEventDispatcher $dispatcher, - Connection $db, ILockingProvider $lockingProvider, IDateTimeFormatter $dateTimeFormatter, IniGetWrapper $iniGetWrapper, - IDBConnection $connection, ITempManager $tempManager, IManager $manager, IAppManager $appManager, @@ -139,11 +129,9 @@ public function __construct($AppName, $this->checker = $checker; $this->logger = $logger; $this->dispatcher = $dispatcher; - $this->db = $db; $this->lockingProvider = $lockingProvider; $this->dateTimeFormatter = $dateTimeFormatter; $this->iniGetWrapper = $iniGetWrapper; - $this->connection = $connection; $this->tempManager = $tempManager; $this->manager = $manager; $this->appManager = $appManager; @@ -528,49 +516,6 @@ protected function isMysqlUsedWithoutUTF8MB4(): bool { return ($this->config->getSystemValue('dbtype', 'sqlite') === 'mysql') && ($this->config->getSystemValue('mysql.utf8mb4', false) === false); } - protected function hasBigIntConversionPendingColumns(): array { - // copy of ConvertFilecacheBigInt::getColumnsByTable() - $tables = [ - 'activity' => ['activity_id', 'object_id'], - 'activity_mq' => ['mail_id'], - 'authtoken' => ['id'], - 'bruteforce_attempts' => ['id'], - 'federated_reshares' => ['share_id'], - 'filecache' => ['fileid', 'storage', 'parent', 'mimetype', 'mimepart', 'mtime', 'storage_mtime'], - 'filecache_extended' => ['fileid'], - 'files_trash' => ['auto_id'], - 'file_locks' => ['id'], - 'file_metadata' => ['id'], - 'jobs' => ['id'], - 'mimetypes' => ['id'], - 'mounts' => ['id', 'storage_id', 'root_id', 'mount_id'], - 'share_external' => ['id', 'parent'], - 'storages' => ['numeric_id'], - ]; - - $schema = new SchemaWrapper($this->db); - $isSqlite = $this->connection->getDatabaseProvider() === IDBConnection::PLATFORM_SQLITE; - $pendingColumns = []; - - foreach ($tables as $tableName => $columns) { - if (!$schema->hasTable($tableName)) { - continue; - } - - $table = $schema->getTable($tableName); - foreach ($columns as $columnName) { - $column = $table->getColumn($columnName); - $isAutoIncrement = $column->getAutoincrement(); - $isAutoIncrementOnSqlite = $isSqlite && $isAutoIncrement; - if ($column->getType()->getName() !== Types::BIGINT && !$isAutoIncrementOnSqlite) { - $pendingColumns[] = $tableName . '.' . $columnName; - } - } - } - - return $pendingColumns; - } - protected function isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed(): bool { $objectStore = $this->config->getSystemValue('objectstore', null); $objectStoreMultibucket = $this->config->getSystemValue('objectstore_multibucket', null); @@ -634,7 +579,6 @@ public function check() { 'appDirsWithDifferentOwner' => $this->getAppDirsWithDifferentOwner(), 'isImagickEnabled' => $this->isImagickEnabled(), 'areWebauthnExtensionsEnabled' => $this->areWebauthnExtensionsEnabled(), - 'pendingBigIntConversionColumns' => $this->hasBigIntConversionPendingColumns(), 'isMysqlUsedWithoutUTF8MB4' => $this->isMysqlUsedWithoutUTF8MB4(), 'isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed' => $this->isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed(), 'reverseProxyGeneratedURL' => $this->urlGenerator->getAbsoluteURL('index.php'), diff --git a/apps/settings/lib/SetupChecks/DatabasePendingBigIntConversions.php b/apps/settings/lib/SetupChecks/DatabasePendingBigIntConversions.php new file mode 100644 index 0000000000000..57dcbd689fc74 --- /dev/null +++ b/apps/settings/lib/SetupChecks/DatabasePendingBigIntConversions.php @@ -0,0 +1,99 @@ + + * + * @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 OC\Core\Command\Db\ConvertFilecacheBigInt; +use OC\DB\Connection; +use OC\DB\SchemaWrapper; +use OCP\DB\Types; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\IDBConnection; +use OCP\IL10N; +use OCP\IURLGenerator; +use OCP\SetupCheck\ISetupCheck; +use OCP\SetupCheck\SetupResult; + +class DatabasePendingBigIntConversions implements ISetupCheck { + public function __construct( + private IL10N $l10n, + private IURLGenerator $urlGenerator, + private Connection $db, + private IEventDispatcher $dispatcher, + private IDBConnection $connection, + ) { + } + + public function getCategory(): string { + return 'database'; + } + + public function getName(): string { + return $this->l10n->t('Database pending bigint migrations'); + } + + protected function getBigIntConversionPendingColumns(): array { + $tables = ConvertFilecacheBigInt::getColumnsByTable(); + + $schema = new SchemaWrapper($this->db); + $isSqlite = $this->connection->getDatabaseProvider() === IDBConnection::PLATFORM_SQLITE; + $pendingColumns = []; + + foreach ($tables as $tableName => $columns) { + if (!$schema->hasTable($tableName)) { + continue; + } + + $table = $schema->getTable($tableName); + foreach ($columns as $columnName) { + $column = $table->getColumn($columnName); + $isAutoIncrement = $column->getAutoincrement(); + $isAutoIncrementOnSqlite = $isSqlite && $isAutoIncrement; + if ($column->getType()->getName() !== Types::BIGINT && !$isAutoIncrementOnSqlite) { + $pendingColumns[] = $tableName . '.' . $columnName; + } + } + } + + return $pendingColumns; + } + + public function run(): SetupResult { + $pendingColumns = $this->getBigIntConversionPendingColumns(); + if (empty($pendingColumns)) { + return SetupResult::success('None'); + } else { + $list = ''; + foreach ($pendingColumns as $pendingColumn) { + $list .= "\n$pendingColumn"; + } + $list .= "\n"; + return SetupResult::info( + $this->l10n->t('Some columns in the database are missing a conversion to big int. Due to the fact that changing column types on big tables could take some time they were not changed automatically. By running "occ db:convert-filecache-bigint" those pending changes could be applied manually. This operation needs to be made while the instance is offline.').$list, + $this->urlGenerator->linkToDocs('admin-bigint-conversion') + ); + } + } +} diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php index 65ed93523c741..bd3ed270cdd68 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -35,9 +35,7 @@ namespace OCA\Settings\Tests\Controller; use bantu\IniGetWrapper\IniGetWrapper; -use Doctrine\DBAL\Platforms\SqlitePlatform; use OC; -use OC\DB\Connection; use OC\IntegrityCheck\Checker; use OCA\Settings\Controller\CheckSetupController; use OCP\App\IAppManager; @@ -49,7 +47,6 @@ use OCP\Http\Client\IClientService; use OCP\IConfig; use OCP\IDateTimeFormatter; -use OCP\IDBConnection; use OCP\IL10N; use OCP\IRequest; use OCP\IServerContainer; @@ -88,16 +85,12 @@ class CheckSetupControllerTest extends TestCase { private $checker; /** @var IEventDispatcher|\PHPUnit\Framework\MockObject\MockObject */ private $dispatcher; - /** @var Connection|\PHPUnit\Framework\MockObject\MockObject */ - private $db; /** @var ILockingProvider|\PHPUnit\Framework\MockObject\MockObject */ private $lockingProvider; /** @var IDateTimeFormatter|\PHPUnit\Framework\MockObject\MockObject */ private $dateTimeFormatter; /** @var IniGetWrapper|\PHPUnit\Framework\MockObject\MockObject */ private $iniGetWrapper; - /** @var IDBConnection|\PHPUnit\Framework\MockObject\MockObject */ - private $connection; /** @var ITempManager|\PHPUnit\Framework\MockObject\MockObject */ private $tempManager; /** @var IManager|\PHPUnit\Framework\MockObject\MockObject */ @@ -138,13 +131,9 @@ protected function setUp(): void { $this->checker = $this->getMockBuilder('\OC\IntegrityCheck\Checker') ->disableOriginalConstructor()->getMock(); $this->logger = $this->getMockBuilder(LoggerInterface::class)->getMock(); - $this->db = $this->getMockBuilder(Connection::class) - ->disableOriginalConstructor()->getMock(); $this->lockingProvider = $this->getMockBuilder(ILockingProvider::class)->getMock(); $this->dateTimeFormatter = $this->getMockBuilder(IDateTimeFormatter::class)->getMock(); $this->iniGetWrapper = $this->getMockBuilder(IniGetWrapper::class)->getMock(); - $this->connection = $this->getMockBuilder(IDBConnection::class) - ->disableOriginalConstructor()->getMock(); $this->tempManager = $this->getMockBuilder(ITempManager::class)->getMock(); $this->notificationManager = $this->getMockBuilder(IManager::class)->getMock(); $this->appManager = $this->createMock(IAppManager::class); @@ -161,11 +150,9 @@ protected function setUp(): void { $this->checker, $this->logger, $this->dispatcher, - $this->db, $this->lockingProvider, $this->dateTimeFormatter, $this->iniGetWrapper, - $this->connection, $this->tempManager, $this->notificationManager, $this->appManager, @@ -183,7 +170,6 @@ protected function setUp(): void { 'getAppDirsWithDifferentOwner', 'isImagickEnabled', 'areWebauthnExtensionsEnabled', - 'hasBigIntConversionPendingColumns', 'isMysqlUsedWithoutUTF8MB4', 'isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed', ])->getMock(); @@ -262,11 +248,6 @@ public function testCheck() { ->method('areWebauthnExtensionsEnabled') ->willReturn(false); - $this->checkSetupController - ->expects($this->once()) - ->method('hasBigIntConversionPendingColumns') - ->willReturn([]); - $this->checkSetupController ->expects($this->once()) ->method('isMysqlUsedWithoutUTF8MB4') @@ -307,9 +288,6 @@ public function testCheck() { } return ''; }); - $sqlitePlatform = $this->getMockBuilder(SqlitePlatform::class)->getMock(); - $this->connection->method('getDatabasePlatform') - ->willReturn($sqlitePlatform); $expected = new DataResponse( [ @@ -332,7 +310,6 @@ public function testCheck() { 'appDirsWithDifferentOwner' => [], 'isImagickEnabled' => false, 'areWebauthnExtensionsEnabled' => false, - 'pendingBigIntConversionColumns' => [], 'isMysqlUsedWithoutUTF8MB4' => false, 'isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed' => true, 'reverseProxyGeneratedURL' => 'https://server/index.php', @@ -357,11 +334,9 @@ public function testGetCurlVersion() { $this->checker, $this->logger, $this->dispatcher, - $this->db, $this->lockingProvider, $this->dateTimeFormatter, $this->iniGetWrapper, - $this->connection, $this->tempManager, $this->notificationManager, $this->appManager, @@ -1083,11 +1058,9 @@ public function testIsMysqlUsedWithoutUTF8MB4(string $db, bool $useUTF8MB4, bool $this->checker, $this->logger, $this->dispatcher, - $this->db, $this->lockingProvider, $this->dateTimeFormatter, $this->iniGetWrapper, - $this->connection, $this->tempManager, $this->notificationManager, $this->appManager, @@ -1136,11 +1109,9 @@ public function testIsEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed(string $m $this->checker, $this->logger, $this->dispatcher, - $this->db, $this->lockingProvider, $this->dateTimeFormatter, $this->iniGetWrapper, - $this->connection, $this->tempManager, $this->notificationManager, $this->appManager, diff --git a/core/Command/Db/ConvertFilecacheBigInt.php b/core/Command/Db/ConvertFilecacheBigInt.php index d73058767f2bd..44cd81cd7ebfb 100644 --- a/core/Command/Db/ConvertFilecacheBigInt.php +++ b/core/Command/Db/ConvertFilecacheBigInt.php @@ -54,8 +54,10 @@ protected function configure() { ->setDescription('Convert the ID columns of the filecache to BigInt'); } - protected function getColumnsByTable() { - // also update in CheckSetupController::hasBigIntConversionPendingColumns() + /** + * @return array + */ + public static function getColumnsByTable(): array { return [ 'activity' => ['activity_id', 'object_id'], 'activity_mq' => ['mail_id'], @@ -80,7 +82,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $isSqlite = $this->connection->getDatabasePlatform() instanceof SqlitePlatform; $updates = []; - $tables = $this->getColumnsByTable(); + $tables = static::getColumnsByTable(); foreach ($tables as $tableName => $columns) { if (!$schema->hasTable($tableName)) { continue; diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js index 24fec85aeb3e0..c947f3c30e742 100644 --- a/core/js/setupchecks.js +++ b/core/js/setupchecks.js @@ -282,18 +282,6 @@ type: OC.SetupChecks.MESSAGE_TYPE_INFO }) } - if (data.pendingBigIntConversionColumns.length > 0) { - var listOfPendingBigIntConversionColumns = ""; - data.pendingBigIntConversionColumns.forEach(function(element){ - listOfPendingBigIntConversionColumns += '
  • ' + element + '
    • '; - }); - messages.push({ - msg: t('core', 'Some columns in the database are missing a conversion to big int. Due to the fact that changing column types on big tables could take some time they were not changed automatically. By running "occ db:convert-filecache-bigint" those pending changes could be applied manually. This operation needs to be made while the instance is offline. For further details read {linkstart}the documentation page about this ↗{linkend}.') - .replace('{linkstart}', '') - .replace('{linkend}', '') + '
      ' + listOfPendingBigIntConversionColumns + '
    ', - type: OC.SetupChecks.MESSAGE_TYPE_INFO - }) - } if (data.isSqliteUsed) { messages.push({ msg: t('core', 'SQLite is currently being used as the backend database. For larger installations we recommend that you switch to a different database backend.') + ' ' + t('core', 'This is particularly recommended when using the desktop client for file synchronisation.') + ' ' + diff --git a/core/js/tests/specs/setupchecksSpec.js b/core/js/tests/specs/setupchecksSpec.js index 9020a895af557..5184b3e8c0d80 100644 --- a/core/js/tests/specs/setupchecksSpec.js +++ b/core/js/tests/specs/setupchecksSpec.js @@ -236,7 +236,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -286,7 +285,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -336,7 +334,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -386,7 +383,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -436,7 +432,6 @@ describe('OC.SetupChecks tests', function() { ], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -485,7 +480,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -534,7 +528,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -614,7 +607,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -669,7 +661,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -717,7 +708,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: true, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -769,7 +759,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyDocs: 'https://docs.nextcloud.com/foo/bar.html', @@ -818,7 +807,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyDocs: 'https://docs.nextcloud.com/foo/bar.html', @@ -864,7 +852,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: false, reverseProxyGeneratedURL: 'https://server', @@ -913,7 +900,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: false, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -962,7 +948,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: false, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -1010,7 +995,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', @@ -1065,7 +1049,6 @@ describe('OC.SetupChecks tests', function() { appDirsWithDifferentOwner: [], isImagickEnabled: true, areWebauthnExtensionsEnabled: true, - pendingBigIntConversionColumns: [], isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, reverseProxyGeneratedURL: 'https://server', From 004d3c4bd0997ccf1c1a575ad43ae0763c9998ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 16 Nov 2023 11:55:01 +0100 Subject: [PATCH 02/12] Migrate away from deprecated doctrine/dbal getName function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../lib/SetupChecks/DatabasePendingBigIntConversions.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/settings/lib/SetupChecks/DatabasePendingBigIntConversions.php b/apps/settings/lib/SetupChecks/DatabasePendingBigIntConversions.php index 57dcbd689fc74..35143f74e9489 100644 --- a/apps/settings/lib/SetupChecks/DatabasePendingBigIntConversions.php +++ b/apps/settings/lib/SetupChecks/DatabasePendingBigIntConversions.php @@ -25,10 +25,10 @@ */ namespace OCA\Settings\SetupChecks; +use Doctrine\DBAL\Types\BigIntType; use OC\Core\Command\Db\ConvertFilecacheBigInt; use OC\DB\Connection; use OC\DB\SchemaWrapper; -use OCP\DB\Types; use OCP\EventDispatcher\IEventDispatcher; use OCP\IDBConnection; use OCP\IL10N; @@ -71,7 +71,7 @@ protected function getBigIntConversionPendingColumns(): array { $column = $table->getColumn($columnName); $isAutoIncrement = $column->getAutoincrement(); $isAutoIncrementOnSqlite = $isSqlite && $isAutoIncrement; - if ($column->getType()->getName() !== Types::BIGINT && !$isAutoIncrementOnSqlite) { + if (!($column->getType() instanceof BigIntType) && !$isAutoIncrementOnSqlite) { $pendingColumns[] = $tableName . '.' . $columnName; } } From ca63726a89a35438a1241a463aac220241851d30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 16 Nov 2023 11:48:04 +0100 Subject: [PATCH 03/12] Merge SQlite warning to existing SupportedDatabase setup check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../lib/Controller/CheckSetupController.php | 6 ---- .../lib/SetupChecks/SupportedDatabase.php | 7 +++- .../Controller/CheckSetupControllerTest.php | 6 ---- .../SetupChecks/SupportedDatabaseTest.php | 33 +++++++++++++++++-- core/js/setupchecks.js | 9 ----- 5 files changed, 36 insertions(+), 25 deletions(-) diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php index 07ab12426bb0d..14f7068ce59a1 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -401,10 +401,6 @@ protected function getOpcacheSetupRecommendations(): array { return $recommendations; } - protected function isSqliteUsed() { - return str_contains($this->config->getSystemValue('dbtype'), 'sqlite'); - } - protected function getSuggestedOverwriteCliURL(): string { $currentOverwriteCliUrl = $this->config->getSystemValue('overwrite.cli.url', ''); $suggestedOverwriteCliUrl = $this->request->getServerProtocol() . '://' . $this->request->getInsecureServerHost() . \OC::$WEBROOT; @@ -574,8 +570,6 @@ public function check() { 'codeIntegrityCheckerDocumentation' => $this->urlGenerator->linkToDocs('admin-code-integrity'), 'OpcacheSetupRecommendations' => $this->getOpcacheSetupRecommendations(), 'isSettimelimitAvailable' => $this->isSettimelimitAvailable(), - 'isSqliteUsed' => $this->isSqliteUsed(), - 'databaseConversionDocumentation' => $this->urlGenerator->linkToDocs('admin-db-conversion'), 'appDirsWithDifferentOwner' => $this->getAppDirsWithDifferentOwner(), 'isImagickEnabled' => $this->isImagickEnabled(), 'areWebauthnExtensionsEnabled' => $this->areWebauthnExtensionsEnabled(), diff --git a/apps/settings/lib/SetupChecks/SupportedDatabase.php b/apps/settings/lib/SetupChecks/SupportedDatabase.php index 7cb4820952fc0..5739bc17958f5 100644 --- a/apps/settings/lib/SetupChecks/SupportedDatabase.php +++ b/apps/settings/lib/SetupChecks/SupportedDatabase.php @@ -33,12 +33,14 @@ use Doctrine\DBAL\Platforms\SqlitePlatform; use OCP\IDBConnection; use OCP\IL10N; +use OCP\IURLGenerator; use OCP\SetupCheck\ISetupCheck; use OCP\SetupCheck\SetupResult; class SupportedDatabase implements ISetupCheck { public function __construct( private IL10N $l10n, + private IURLGenerator $urlGenerator, private IDBConnection $connection, ) { } @@ -81,7 +83,10 @@ public function run(): SetupResult { } elseif ($databasePlatform instanceof OraclePlatform) { $version = 'Oracle'; } elseif ($databasePlatform instanceof SqlitePlatform) { - $version = 'Sqlite'; + return SetupResult::warning( + $this->l10n->t('SQLite is currently being used as the backend database. For larger installations we recommend that you switch to a different database backend. This is particularly recommended when using the desktop client for file synchronisation. To migrate to another database use the command line tool: "occ db:convert-type".'), + $this->urlGenerator->linkToDocs('admin-db-conversion') + ); } else { return SetupResult::error($this->l10n->t('Unknown database platform')); } diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php index bd3ed270cdd68..a4cd6bb0a961e 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -165,7 +165,6 @@ protected function setUp(): void { 'getCurlVersion', 'isPhpOutdated', 'getOpcacheSetupRecommendations', - 'isSqliteUsed', 'isPHPMailerUsed', 'getAppDirsWithDifferentOwner', 'isImagickEnabled', @@ -213,9 +212,6 @@ public function testCheck() { ->expects($this->once()) ->method('getOpcacheSetupRecommendations') ->willReturn(['recommendation1', 'recommendation2']); - $this->checkSetupController - ->method('isSqliteUsed') - ->willReturn(false); $this->checkSetupController ->expects($this->once()) ->method('getSuggestedOverwriteCliURL') @@ -305,8 +301,6 @@ public function testCheck() { 'codeIntegrityCheckerDocumentation' => 'http://docs.example.org/server/go.php?to=admin-code-integrity', 'OpcacheSetupRecommendations' => ['recommendation1', 'recommendation2'], 'isSettimelimitAvailable' => true, - 'isSqliteUsed' => false, - 'databaseConversionDocumentation' => 'http://docs.example.org/server/go.php?to=admin-db-conversion', 'appDirsWithDifferentOwner' => [], 'isImagickEnabled' => false, 'areWebauthnExtensionsEnabled' => false, diff --git a/apps/settings/tests/SetupChecks/SupportedDatabaseTest.php b/apps/settings/tests/SetupChecks/SupportedDatabaseTest.php index 5521bec34b56a..aede25475c5f9 100644 --- a/apps/settings/tests/SetupChecks/SupportedDatabaseTest.php +++ b/apps/settings/tests/SetupChecks/SupportedDatabaseTest.php @@ -25,8 +25,11 @@ */ namespace OCA\Settings\Tests; +use Doctrine\DBAL\Platforms\SqlitePlatform; use OCA\Settings\SetupChecks\SupportedDatabase; +use OCP\IDBConnection; use OCP\IL10N; +use OCP\IUrlGenerator; use OCP\SetupCheck\SetupResult; use Test\TestCase; @@ -34,9 +37,33 @@ * @group DB */ class SupportedDatabaseTest extends TestCase { + private IL10N $l10n; + private IUrlGenerator $urlGenerator; + private IDBConnection $connection; + + private SupportedDatabase $check; + + protected function setUp(): void { + parent::setUp(); + + $this->l10n = $this->getMockBuilder(IL10N::class)->getMock(); + $this->urlGenerator = $this->getMockBuilder(IUrlGenerator::class)->getMock(); + $this->connection = \OCP\Server::get(IDBConnection::class); + + $this->check = new SupportedDatabase( + $this->l10n, + $this->urlGenerator, + \OCP\Server::get(IDBConnection::class) + ); + } + public function testPass(): void { - $l10n = $this->getMockBuilder(IL10N::class)->getMock(); - $check = new SupportedDatabase($l10n, \OC::$server->getDatabaseConnection()); - $this->assertEquals(SetupResult::SUCCESS, $check->run()->getSeverity()); + $platform = $this->connection->getDatabasePlatform(); + if ($platform instanceof SqlitePlatform) { + /** SQlite always gets a warning */ + $this->assertEquals(SetupResult::WARNING, $this->check->run()->getSeverity()); + } else { + $this->assertEquals(SetupResult::SUCCESS, $this->check->run()->getSeverity()); + } } } diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js index c947f3c30e742..e7ad920760eda 100644 --- a/core/js/setupchecks.js +++ b/core/js/setupchecks.js @@ -282,15 +282,6 @@ type: OC.SetupChecks.MESSAGE_TYPE_INFO }) } - if (data.isSqliteUsed) { - messages.push({ - msg: t('core', 'SQLite is currently being used as the backend database. For larger installations we recommend that you switch to a different database backend.') + ' ' + t('core', 'This is particularly recommended when using the desktop client for file synchronisation.') + ' ' + - t('core', 'To migrate to another database use the command line tool: "occ db:convert-type", or see the {linkstart}documentation ↗{linkend}.') - .replace('{linkstart}', '') - .replace('{linkend}', ''), - type: OC.SetupChecks.MESSAGE_TYPE_WARNING - }) - } if(data.appDirsWithDifferentOwner && data.appDirsWithDifferentOwner.length > 0) { var appDirsWithDifferentOwner = data.appDirsWithDifferentOwner.reduce( From 265f6dc1367915a19e195c035ef0e5e87f7308de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 16 Nov 2023 17:05:24 +0100 Subject: [PATCH 04/12] Migrate opcache check to new SetupCheck API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 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 | 79 ----------- .../lib/SetupChecks/PhpOpcacheSetup.php | 134 ++++++++++++++++++ .../Controller/CheckSetupControllerTest.php | 14 -- core/js/setupchecks.js | 12 -- core/js/tests/specs/setupchecksSpec.js | 63 -------- 8 files changed, 138 insertions(+), 168 deletions(-) create mode 100644 apps/settings/lib/SetupChecks/PhpOpcacheSetup.php diff --git a/apps/settings/composer/composer/autoload_classmap.php b/apps/settings/composer/composer/autoload_classmap.php index 4ad4e2b652558..b32aab9756955 100644 --- a/apps/settings/composer/composer/autoload_classmap.php +++ b/apps/settings/composer/composer/autoload_classmap.php @@ -95,6 +95,7 @@ 'OCA\\Settings\\SetupChecks\\PhpGetEnv' => $baseDir . '/../lib/SetupChecks/PhpGetEnv.php', 'OCA\\Settings\\SetupChecks\\PhpMemoryLimit' => $baseDir . '/../lib/SetupChecks/PhpMemoryLimit.php', 'OCA\\Settings\\SetupChecks\\PhpModules' => $baseDir . '/../lib/SetupChecks/PhpModules.php', + 'OCA\\Settings\\SetupChecks\\PhpOpcacheSetup' => $baseDir . '/../lib/SetupChecks/PhpOpcacheSetup.php', 'OCA\\Settings\\SetupChecks\\PhpOutdated' => $baseDir . '/../lib/SetupChecks/PhpOutdated.php', 'OCA\\Settings\\SetupChecks\\PhpOutputBuffering' => $baseDir . '/../lib/SetupChecks/PhpOutputBuffering.php', 'OCA\\Settings\\SetupChecks\\RandomnessSecure' => $baseDir . '/../lib/SetupChecks/RandomnessSecure.php', diff --git a/apps/settings/composer/composer/autoload_static.php b/apps/settings/composer/composer/autoload_static.php index dc4a2b21b3508..09e0d724e4ccb 100644 --- a/apps/settings/composer/composer/autoload_static.php +++ b/apps/settings/composer/composer/autoload_static.php @@ -110,6 +110,7 @@ class ComposerStaticInitSettings 'OCA\\Settings\\SetupChecks\\PhpGetEnv' => __DIR__ . '/..' . '/../lib/SetupChecks/PhpGetEnv.php', 'OCA\\Settings\\SetupChecks\\PhpMemoryLimit' => __DIR__ . '/..' . '/../lib/SetupChecks/PhpMemoryLimit.php', 'OCA\\Settings\\SetupChecks\\PhpModules' => __DIR__ . '/..' . '/../lib/SetupChecks/PhpModules.php', + 'OCA\\Settings\\SetupChecks\\PhpOpcacheSetup' => __DIR__ . '/..' . '/../lib/SetupChecks/PhpOpcacheSetup.php', 'OCA\\Settings\\SetupChecks\\PhpOutdated' => __DIR__ . '/..' . '/../lib/SetupChecks/PhpOutdated.php', 'OCA\\Settings\\SetupChecks\\PhpOutputBuffering' => __DIR__ . '/..' . '/../lib/SetupChecks/PhpOutputBuffering.php', 'OCA\\Settings\\SetupChecks\\RandomnessSecure' => __DIR__ . '/..' . '/../lib/SetupChecks/RandomnessSecure.php', diff --git a/apps/settings/lib/AppInfo/Application.php b/apps/settings/lib/AppInfo/Application.php index 0da2485aa00a7..7e963e355930b 100644 --- a/apps/settings/lib/AppInfo/Application.php +++ b/apps/settings/lib/AppInfo/Application.php @@ -67,6 +67,7 @@ use OCA\Settings\SetupChecks\PhpGetEnv; use OCA\Settings\SetupChecks\PhpMemoryLimit; use OCA\Settings\SetupChecks\PhpModules; +use OCA\Settings\SetupChecks\PhpOpcacheSetup; use OCA\Settings\SetupChecks\PhpOutdated; use OCA\Settings\SetupChecks\PhpOutputBuffering; use OCA\Settings\SetupChecks\RandomnessSecure; @@ -182,6 +183,7 @@ public function register(IRegistrationContext $context): void { $context->registerSetupCheck(PhpGetEnv::class); $context->registerSetupCheck(PhpMemoryLimit::class); $context->registerSetupCheck(PhpModules::class); + $context->registerSetupCheck(PhpOpcacheSetup::class); $context->registerSetupCheck(PhpOutdated::class); $context->registerSetupCheck(PhpOutputBuffering::class); $context->registerSetupCheck(RandomnessSecure::class); diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php index 14f7068ce59a1..0cb8c50d38728 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -45,7 +45,6 @@ */ namespace OCA\Settings\Controller; -use bantu\IniGetWrapper\IniGetWrapper; use DirectoryIterator; use GuzzleHttp\Exception\ClientException; use OC; @@ -91,8 +90,6 @@ class CheckSetupController extends Controller { private $lockingProvider; /** @var IDateTimeFormatter */ private $dateTimeFormatter; - /** @var IniGetWrapper */ - private $iniGetWrapper; /** @var ITempManager */ private $tempManager; /** @var IManager */ @@ -114,7 +111,6 @@ public function __construct($AppName, IEventDispatcher $dispatcher, ILockingProvider $lockingProvider, IDateTimeFormatter $dateTimeFormatter, - IniGetWrapper $iniGetWrapper, ITempManager $tempManager, IManager $manager, IAppManager $appManager, @@ -131,7 +127,6 @@ public function __construct($AppName, $this->dispatcher = $dispatcher; $this->lockingProvider = $lockingProvider; $this->dateTimeFormatter = $dateTimeFormatter; - $this->iniGetWrapper = $iniGetWrapper; $this->tempManager = $tempManager; $this->manager = $manager; $this->appManager = $appManager; @@ -328,79 +323,6 @@ public function getFailedIntegrityCheckFiles(): DataDisplayResponse { ); } - /** - * Checks whether a PHP OPcache is properly set up - * @return string[] The list of OPcache setup recommendations - */ - protected function getOpcacheSetupRecommendations(): array { - // If the module is not loaded, return directly to skip inapplicable checks - if (!extension_loaded('Zend OPcache')) { - return [$this->l10n->t('The PHP OPcache module is not loaded. For better performance it is recommended to load it into your PHP installation.')]; - } - - $recommendations = []; - - // Check whether Nextcloud is allowed to use the OPcache API - $isPermitted = true; - $permittedPath = $this->iniGetWrapper->getString('opcache.restrict_api'); - if (isset($permittedPath) && $permittedPath !== '' && !str_starts_with(\OC::$SERVERROOT, rtrim($permittedPath, '/'))) { - $isPermitted = false; - } - - if (!$this->iniGetWrapper->getBool('opcache.enable')) { - $recommendations[] = $this->l10n->t('OPcache is disabled. For better performance, it is recommended to apply opcache.enable=1 to your PHP configuration.'); - - // Check for saved comments only when OPcache is currently disabled. If it was enabled, opcache.save_comments=0 would break Nextcloud in the first place. - if (!$this->iniGetWrapper->getBool('opcache.save_comments')) { - $recommendations[] = $this->l10n->t('OPcache is configured to remove code comments. With OPcache enabled, opcache.save_comments=1 must be set for Nextcloud to function.'); - } - - if (!$isPermitted) { - $recommendations[] = $this->l10n->t('Nextcloud is not allowed to use the OPcache API. With OPcache enabled, it is highly recommended to include all Nextcloud directories with opcache.restrict_api or unset this setting to disable OPcache API restrictions, to prevent errors during Nextcloud core or app upgrades.'); - } - } elseif (!$isPermitted) { - $recommendations[] = $this->l10n->t('Nextcloud is not allowed to use the OPcache API. It is highly recommended to include all Nextcloud directories with opcache.restrict_api or unset this setting to disable OPcache API restrictions, to prevent errors during Nextcloud core or app upgrades.'); - } elseif ($this->iniGetWrapper->getBool('opcache.file_cache_only')) { - $recommendations[] = $this->l10n->t('The shared memory based OPcache is disabled. For better performance, it is recommended to apply opcache.file_cache_only=0 to your PHP configuration and use the file cache as second level cache only.'); - } else { - // Check whether opcache_get_status has been explicitly disabled an in case skip usage based checks - $disabledFunctions = $this->iniGetWrapper->getString('disable_functions'); - if (isset($disabledFunctions) && str_contains($disabledFunctions, 'opcache_get_status')) { - return []; - } - - $status = opcache_get_status(false); - - // Recommend to raise value, if more than 90% of max value is reached - if ( - empty($status['opcache_statistics']['max_cached_keys']) || - ($status['opcache_statistics']['num_cached_keys'] / $status['opcache_statistics']['max_cached_keys'] > 0.9) - ) { - $recommendations[] = $this->l10n->t('The maximum number of OPcache keys is nearly exceeded. To assure that all scripts can be kept in the cache, it is recommended to apply opcache.max_accelerated_files to your PHP configuration with a value higher than %s.', [($this->iniGetWrapper->getNumeric('opcache.max_accelerated_files') ?: 'currently')]); - } - - if ( - empty($status['memory_usage']['free_memory']) || - ($status['memory_usage']['used_memory'] / $status['memory_usage']['free_memory'] > 9) - ) { - $recommendations[] = $this->l10n->t('The OPcache buffer is nearly full. To assure that all scripts can be hold in cache, it is recommended to apply opcache.memory_consumption to your PHP configuration with a value higher than %s.', [($this->iniGetWrapper->getNumeric('opcache.memory_consumption') ?: 'currently')]); - } - - if ( - // Do not recommend to raise the interned strings buffer size above a quarter of the total OPcache size - ($this->iniGetWrapper->getNumeric('opcache.interned_strings_buffer') < $this->iniGetWrapper->getNumeric('opcache.memory_consumption') / 4) && - ( - empty($status['interned_strings_usage']['free_memory']) || - ($status['interned_strings_usage']['used_memory'] / $status['interned_strings_usage']['free_memory'] > 9) - ) - ) { - $recommendations[] = $this->l10n->t('The OPcache interned strings buffer is nearly full. To assure that repeating strings can be effectively cached, it is recommended to apply opcache.interned_strings_buffer to your PHP configuration with a value higher than %s.', [($this->iniGetWrapper->getNumeric('opcache.interned_strings_buffer') ?: 'currently')]); - } - } - - return $recommendations; - } - protected function getSuggestedOverwriteCliURL(): string { $currentOverwriteCliUrl = $this->config->getSystemValue('overwrite.cli.url', ''); $suggestedOverwriteCliUrl = $this->request->getServerProtocol() . '://' . $this->request->getInsecureServerHost() . \OC::$WEBROOT; @@ -568,7 +490,6 @@ public function check() { 'isCorrectMemcachedPHPModuleInstalled' => $this->isCorrectMemcachedPHPModuleInstalled(), 'hasPassedCodeIntegrityCheck' => $this->checker->hasPassedCheck(), 'codeIntegrityCheckerDocumentation' => $this->urlGenerator->linkToDocs('admin-code-integrity'), - 'OpcacheSetupRecommendations' => $this->getOpcacheSetupRecommendations(), 'isSettimelimitAvailable' => $this->isSettimelimitAvailable(), 'appDirsWithDifferentOwner' => $this->getAppDirsWithDifferentOwner(), 'isImagickEnabled' => $this->isImagickEnabled(), diff --git a/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php b/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php new file mode 100644 index 0000000000000..faf686e0dfbb5 --- /dev/null +++ b/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php @@ -0,0 +1,134 @@ + + * + * @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 bantu\IniGetWrapper\IniGetWrapper; +use OCP\IL10N; +use OCP\IURLGenerator; +use OCP\SetupCheck\ISetupCheck; +use OCP\SetupCheck\SetupResult; + +class PhpOpcacheSetup implements ISetupCheck { + public function __construct( + private IL10N $l10n, + private IURLGenerator $urlGenerator, + private IniGetWrapper $iniGetWrapper, + ) { + } + + public function getName(): string { + return $this->l10n->t('PHP opcache'); + } + + public function getCategory(): string { + return 'php'; + } + + /** + * Checks whether a PHP OPcache is properly set up + * @return string[] The list of OPcache setup recommendations + */ + protected function getOpcacheSetupRecommendations(): array { + // If the module is not loaded, return directly to skip inapplicable checks + if (!extension_loaded('Zend OPcache')) { + return [$this->l10n->t('The PHP OPcache module is not loaded. For better performance it is recommended to load it into your PHP installation.')]; + } + + $recommendations = []; + + // Check whether Nextcloud is allowed to use the OPcache API + $isPermitted = true; + $permittedPath = $this->iniGetWrapper->getString('opcache.restrict_api'); + if (isset($permittedPath) && $permittedPath !== '' && !str_starts_with(\OC::$SERVERROOT, rtrim($permittedPath, '/'))) { + $isPermitted = false; + } + + if (!$this->iniGetWrapper->getBool('opcache.enable')) { + $recommendations[] = $this->l10n->t('OPcache is disabled. For better performance, it is recommended to apply opcache.enable=1 to your PHP configuration.'); + + // Check for saved comments only when OPcache is currently disabled. If it was enabled, opcache.save_comments=0 would break Nextcloud in the first place. + if (!$this->iniGetWrapper->getBool('opcache.save_comments')) { + $recommendations[] = $this->l10n->t('OPcache is configured to remove code comments. With OPcache enabled, opcache.save_comments=1 must be set for Nextcloud to function.'); + } + + if (!$isPermitted) { + $recommendations[] = $this->l10n->t('Nextcloud is not allowed to use the OPcache API. With OPcache enabled, it is highly recommended to include all Nextcloud directories with opcache.restrict_api or unset this setting to disable OPcache API restrictions, to prevent errors during Nextcloud core or app upgrades.'); + } + } elseif (!$isPermitted) { + $recommendations[] = $this->l10n->t('Nextcloud is not allowed to use the OPcache API. It is highly recommended to include all Nextcloud directories with opcache.restrict_api or unset this setting to disable OPcache API restrictions, to prevent errors during Nextcloud core or app upgrades.'); + } elseif ($this->iniGetWrapper->getBool('opcache.file_cache_only')) { + $recommendations[] = $this->l10n->t('The shared memory based OPcache is disabled. For better performance, it is recommended to apply opcache.file_cache_only=0 to your PHP configuration and use the file cache as second level cache only.'); + } else { + // Check whether opcache_get_status has been explicitly disabled an in case skip usage based checks + $disabledFunctions = $this->iniGetWrapper->getString('disable_functions'); + if (isset($disabledFunctions) && str_contains($disabledFunctions, 'opcache_get_status')) { + return []; + } + + $status = opcache_get_status(false); + + // Recommend to raise value, if more than 90% of max value is reached + if ( + empty($status['opcache_statistics']['max_cached_keys']) || + ($status['opcache_statistics']['num_cached_keys'] / $status['opcache_statistics']['max_cached_keys'] > 0.9) + ) { + $recommendations[] = $this->l10n->t('The maximum number of OPcache keys is nearly exceeded. To assure that all scripts can be kept in the cache, it is recommended to apply opcache.max_accelerated_files to your PHP configuration with a value higher than %s.', [($this->iniGetWrapper->getNumeric('opcache.max_accelerated_files') ?: 'currently')]); + } + + if ( + empty($status['memory_usage']['free_memory']) || + ($status['memory_usage']['used_memory'] / $status['memory_usage']['free_memory'] > 9) + ) { + $recommendations[] = $this->l10n->t('The OPcache buffer is nearly full. To assure that all scripts can be hold in cache, it is recommended to apply opcache.memory_consumption to your PHP configuration with a value higher than %s.', [($this->iniGetWrapper->getNumeric('opcache.memory_consumption') ?: 'currently')]); + } + + if ( + // Do not recommend to raise the interned strings buffer size above a quarter of the total OPcache size + ($this->iniGetWrapper->getNumeric('opcache.interned_strings_buffer') < $this->iniGetWrapper->getNumeric('opcache.memory_consumption') / 4) && + ( + empty($status['interned_strings_usage']['free_memory']) || + ($status['interned_strings_usage']['used_memory'] / $status['interned_strings_usage']['free_memory'] > 9) + ) + ) { + $recommendations[] = $this->l10n->t('The OPcache interned strings buffer is nearly full. To assure that repeating strings can be effectively cached, it is recommended to apply opcache.interned_strings_buffer to your PHP configuration with a value higher than %s.', [($this->iniGetWrapper->getNumeric('opcache.interned_strings_buffer') ?: 'currently')]); + } + } + + return $recommendations; + } + + public function run(): SetupResult { + $recommendations = $this->getOpcacheSetupRecommendations(); + if (!empty($recommendations)) { + return SetupResult::error( + $this->l10n->t('The PHP OPcache module is not properly configured. %s.', implode("\n", $recommendations)), + $this->urlGenerator->linkToDocs('admin-php-opcache') + ); + } else { + return SetupResult::success($this->l10n->t('Correctly configured')); + } + } +} diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php index a4cd6bb0a961e..d732b615a0ea7 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -34,7 +34,6 @@ */ namespace OCA\Settings\Tests\Controller; -use bantu\IniGetWrapper\IniGetWrapper; use OC; use OC\IntegrityCheck\Checker; use OCA\Settings\Controller\CheckSetupController; @@ -89,8 +88,6 @@ class CheckSetupControllerTest extends TestCase { private $lockingProvider; /** @var IDateTimeFormatter|\PHPUnit\Framework\MockObject\MockObject */ private $dateTimeFormatter; - /** @var IniGetWrapper|\PHPUnit\Framework\MockObject\MockObject */ - private $iniGetWrapper; /** @var ITempManager|\PHPUnit\Framework\MockObject\MockObject */ private $tempManager; /** @var IManager|\PHPUnit\Framework\MockObject\MockObject */ @@ -133,7 +130,6 @@ protected function setUp(): void { $this->logger = $this->getMockBuilder(LoggerInterface::class)->getMock(); $this->lockingProvider = $this->getMockBuilder(ILockingProvider::class)->getMock(); $this->dateTimeFormatter = $this->getMockBuilder(IDateTimeFormatter::class)->getMock(); - $this->iniGetWrapper = $this->getMockBuilder(IniGetWrapper::class)->getMock(); $this->tempManager = $this->getMockBuilder(ITempManager::class)->getMock(); $this->notificationManager = $this->getMockBuilder(IManager::class)->getMock(); $this->appManager = $this->createMock(IAppManager::class); @@ -152,7 +148,6 @@ protected function setUp(): void { $this->dispatcher, $this->lockingProvider, $this->dateTimeFormatter, - $this->iniGetWrapper, $this->tempManager, $this->notificationManager, $this->appManager, @@ -164,7 +159,6 @@ protected function setUp(): void { 'getSuggestedOverwriteCliURL', 'getCurlVersion', 'isPhpOutdated', - 'getOpcacheSetupRecommendations', 'isPHPMailerUsed', 'getAppDirsWithDifferentOwner', 'isImagickEnabled', @@ -208,10 +202,6 @@ public function testCheck() { ->method('getHeader'); $this->clientService->expects($this->never()) ->method('newClient'); - $this->checkSetupController - ->expects($this->once()) - ->method('getOpcacheSetupRecommendations') - ->willReturn(['recommendation1', 'recommendation2']); $this->checkSetupController ->expects($this->once()) ->method('getSuggestedOverwriteCliURL') @@ -299,7 +289,6 @@ public function testCheck() { 'isCorrectMemcachedPHPModuleInstalled' => true, 'hasPassedCodeIntegrityCheck' => true, 'codeIntegrityCheckerDocumentation' => 'http://docs.example.org/server/go.php?to=admin-code-integrity', - 'OpcacheSetupRecommendations' => ['recommendation1', 'recommendation2'], 'isSettimelimitAvailable' => true, 'appDirsWithDifferentOwner' => [], 'isImagickEnabled' => false, @@ -330,7 +319,6 @@ public function testGetCurlVersion() { $this->dispatcher, $this->lockingProvider, $this->dateTimeFormatter, - $this->iniGetWrapper, $this->tempManager, $this->notificationManager, $this->appManager, @@ -1054,7 +1042,6 @@ public function testIsMysqlUsedWithoutUTF8MB4(string $db, bool $useUTF8MB4, bool $this->dispatcher, $this->lockingProvider, $this->dateTimeFormatter, - $this->iniGetWrapper, $this->tempManager, $this->notificationManager, $this->appManager, @@ -1105,7 +1092,6 @@ public function testIsEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed(string $m $this->dispatcher, $this->lockingProvider, $this->dateTimeFormatter, - $this->iniGetWrapper, $this->tempManager, $this->notificationManager, $this->appManager, diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js index e7ad920760eda..e85f32dae4ac6 100644 --- a/core/js/setupchecks.js +++ b/core/js/setupchecks.js @@ -240,18 +240,6 @@ type: OC.SetupChecks.MESSAGE_TYPE_ERROR }); } - if(data.OpcacheSetupRecommendations.length > 0) { - var listOfOPcacheRecommendations = ""; - data.OpcacheSetupRecommendations.forEach(function(element){ - listOfOPcacheRecommendations += '
  • ' + element + '
    • '; - }); - messages.push({ - msg: t('core', 'The PHP OPcache module is not properly configured. See the {linkstart}documentation ↗{linkend} for more information.') - .replace('{linkstart}', '') - .replace('{linkend}', '') + '
      ' + listOfOPcacheRecommendations + '
    ', - type: OC.SetupChecks.MESSAGE_TYPE_INFO - }); - } if(!data.isSettimelimitAvailable) { messages.push({ msg: t('core', 'The PHP function "set_time_limit" is not available. This could result in scripts being halted mid-execution, breaking your installation. Enabling this function is strongly recommended.'), diff --git a/core/js/tests/specs/setupchecksSpec.js b/core/js/tests/specs/setupchecksSpec.js index 5184b3e8c0d80..bb7d118fc52c5 100644 --- a/core/js/tests/specs/setupchecksSpec.js +++ b/core/js/tests/specs/setupchecksSpec.js @@ -227,7 +227,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -276,7 +275,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -325,7 +323,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -374,7 +371,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: false, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -421,7 +417,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -471,7 +466,6 @@ describe('OC.SetupChecks tests', function() { reverseProxyDocs: 'https://docs.nextcloud.com/foo/bar.html', isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: false, cronErrors: [], cronInfo: { @@ -519,7 +513,6 @@ describe('OC.SetupChecks tests', function() { reverseProxyDocs: 'https://docs.nextcloud.com/foo/bar.html', isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -598,7 +591,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -639,53 +631,6 @@ describe('OC.SetupChecks tests', function() { }); }); - it('should return an info if server has no proper opcache', 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, - OpcacheSetupRecommendations: ['recommendation1', 'recommendation2'], - isSettimelimitAvailable: true, - cronErrors: [], - cronInfo: { - diffInSeconds: 0 - }, - appDirsWithDifferentOwner: [], - 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: 'The PHP OPcache module is not properly configured. See the documentation ↗ for more information.
    • recommendation1
    • recommendation2
    ', - type: OC.SetupChecks.MESSAGE_TYPE_INFO - }]); - done(); - }); - }); - it('should return an error if the php version is no longer supported', function(done) { var async = OC.SetupChecks.checkSetup(); @@ -699,7 +644,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -750,7 +694,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -798,7 +741,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -843,7 +785,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -891,7 +832,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -939,7 +879,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -986,7 +925,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { @@ -1040,7 +978,6 @@ describe('OC.SetupChecks tests', function() { isFairUseOfFreePushService: true, isCorrectMemcachedPHPModuleInstalled: true, hasPassedCodeIntegrityCheck: true, - OpcacheSetupRecommendations: [], isSettimelimitAvailable: true, cronErrors: [], cronInfo: { From e9ed7e7f9efa774a60bf0c1e71eb9878c55444b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 16 Nov 2023 17:15:21 +0100 Subject: [PATCH 05/12] Remove unused properties from CheckSetupControllerTest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../lib/Controller/CheckSetupController.php | 23 +------------ .../Controller/CheckSetupControllerTest.php | 32 ------------------- 2 files changed, 1 insertion(+), 54 deletions(-) diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php index 0cb8c50d38728..b251adb0d84f8 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -47,25 +47,20 @@ use DirectoryIterator; use GuzzleHttp\Exception\ClientException; -use OC; use OC\AppFramework\Http; use OC\IntegrityCheck\Checker; -use OCP\App\IAppManager; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\IgnoreOpenAPI; use OCP\AppFramework\Http\DataDisplayResponse; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\RedirectResponse; -use OCP\EventDispatcher\IEventDispatcher; use OCP\Http\Client\IClientService; use OCP\IConfig; use OCP\IDateTimeFormatter; use OCP\IL10N; use OCP\IRequest; -use OCP\IServerContainer; use OCP\ITempManager; use OCP\IURLGenerator; -use OCP\Lock\ILockingProvider; use OCP\Notification\IManager; use OCP\SetupCheck\ISetupCheckManager; use Psr\Log\LoggerInterface; @@ -84,20 +79,12 @@ class CheckSetupController extends Controller { private $checker; /** @var LoggerInterface */ private $logger; - /** @var IEventDispatcher */ - private $dispatcher; - /** @var ILockingProvider */ - private $lockingProvider; /** @var IDateTimeFormatter */ private $dateTimeFormatter; /** @var ITempManager */ private $tempManager; /** @var IManager */ private $manager; - /** @var IAppManager */ - private $appManager; - /** @var IServerContainer */ - private $serverContainer; private ISetupCheckManager $setupCheckManager; public function __construct($AppName, @@ -108,13 +95,9 @@ public function __construct($AppName, IL10N $l10n, Checker $checker, LoggerInterface $logger, - IEventDispatcher $dispatcher, - ILockingProvider $lockingProvider, IDateTimeFormatter $dateTimeFormatter, ITempManager $tempManager, IManager $manager, - IAppManager $appManager, - IServerContainer $serverContainer, ISetupCheckManager $setupCheckManager, ) { parent::__construct($AppName, $request); @@ -124,13 +107,9 @@ public function __construct($AppName, $this->l10n = $l10n; $this->checker = $checker; $this->logger = $logger; - $this->dispatcher = $dispatcher; - $this->lockingProvider = $lockingProvider; $this->dateTimeFormatter = $dateTimeFormatter; $this->tempManager = $tempManager; $this->manager = $manager; - $this->appManager = $appManager; - $this->serverContainer = $serverContainer; $this->setupCheckManager = $setupCheckManager; } @@ -374,7 +353,7 @@ protected function getAppDirsWithDifferentOwner(): array { $currentUser = posix_getuid(); $appDirsWithDifferentOwner = [[]]; - foreach (OC::$APPSROOTS as $appRoot) { + foreach (\OC::$APPSROOTS as $appRoot) { if ($appRoot['writable'] === true) { $appDirsWithDifferentOwner[] = $this->getAppDirsWithDifferentOwnerForAppRoot($currentUser, $appRoot); } diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php index d732b615a0ea7..b9c2ddc8282ff 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -37,21 +37,17 @@ use OC; use OC\IntegrityCheck\Checker; use OCA\Settings\Controller\CheckSetupController; -use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataDisplayResponse; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\RedirectResponse; -use OCP\EventDispatcher\IEventDispatcher; use OCP\Http\Client\IClientService; use OCP\IConfig; use OCP\IDateTimeFormatter; use OCP\IL10N; use OCP\IRequest; -use OCP\IServerContainer; use OCP\ITempManager; use OCP\IURLGenerator; -use OCP\Lock\ILockingProvider; use OCP\Notification\IManager; use OCP\SetupCheck\ISetupCheckManager; use PHPUnit\Framework\MockObject\MockObject; @@ -82,20 +78,12 @@ class CheckSetupControllerTest extends TestCase { private $logger; /** @var Checker|\PHPUnit\Framework\MockObject\MockObject */ private $checker; - /** @var IEventDispatcher|\PHPUnit\Framework\MockObject\MockObject */ - private $dispatcher; - /** @var ILockingProvider|\PHPUnit\Framework\MockObject\MockObject */ - private $lockingProvider; /** @var IDateTimeFormatter|\PHPUnit\Framework\MockObject\MockObject */ private $dateTimeFormatter; /** @var ITempManager|\PHPUnit\Framework\MockObject\MockObject */ private $tempManager; /** @var IManager|\PHPUnit\Framework\MockObject\MockObject */ private $notificationManager; - /** @var IAppManager|MockObject */ - private $appManager; - /** @var IServerContainer|MockObject */ - private $serverContainer; /** @var ISetupCheckManager|MockObject */ private $setupCheckManager; @@ -124,16 +112,12 @@ protected function setUp(): void { ->willReturnCallback(function ($message, array $replace) { return vsprintf($message, $replace); }); - $this->dispatcher = $this->createMock(IEventDispatcher::class); $this->checker = $this->getMockBuilder('\OC\IntegrityCheck\Checker') ->disableOriginalConstructor()->getMock(); $this->logger = $this->getMockBuilder(LoggerInterface::class)->getMock(); - $this->lockingProvider = $this->getMockBuilder(ILockingProvider::class)->getMock(); $this->dateTimeFormatter = $this->getMockBuilder(IDateTimeFormatter::class)->getMock(); $this->tempManager = $this->getMockBuilder(ITempManager::class)->getMock(); $this->notificationManager = $this->getMockBuilder(IManager::class)->getMock(); - $this->appManager = $this->createMock(IAppManager::class); - $this->serverContainer = $this->createMock(IServerContainer::class); $this->setupCheckManager = $this->createMock(ISetupCheckManager::class); $this->checkSetupController = $this->getMockBuilder(CheckSetupController::class) ->setConstructorArgs([ @@ -145,13 +129,9 @@ protected function setUp(): void { $this->l10n, $this->checker, $this->logger, - $this->dispatcher, - $this->lockingProvider, $this->dateTimeFormatter, $this->tempManager, $this->notificationManager, - $this->appManager, - $this->serverContainer, $this->setupCheckManager, ]) ->setMethods([ @@ -316,13 +296,9 @@ public function testGetCurlVersion() { $this->l10n, $this->checker, $this->logger, - $this->dispatcher, - $this->lockingProvider, $this->dateTimeFormatter, $this->tempManager, $this->notificationManager, - $this->appManager, - $this->serverContainer, $this->setupCheckManager, ]) ->setMethods(null)->getMock(); @@ -1039,13 +1015,9 @@ public function testIsMysqlUsedWithoutUTF8MB4(string $db, bool $useUTF8MB4, bool $this->l10n, $this->checker, $this->logger, - $this->dispatcher, - $this->lockingProvider, $this->dateTimeFormatter, $this->tempManager, $this->notificationManager, - $this->appManager, - $this->serverContainer, $this->setupCheckManager, ); @@ -1089,13 +1061,9 @@ public function testIsEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed(string $m $this->l10n, $this->checker, $this->logger, - $this->dispatcher, - $this->lockingProvider, $this->dateTimeFormatter, $this->tempManager, $this->notificationManager, - $this->appManager, - $this->serverContainer, $this->setupCheckManager, ); From 26d1d84e0c45214370accd38f32323c73e98a29d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 12 Dec 2023 16:04:05 +0100 Subject: [PATCH 06/12] Remove unsupported tag and use quotes instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../settings/lib/SetupChecks/PhpOpcacheSetup.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php b/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php index faf686e0dfbb5..aa1f2367bf56e 100644 --- a/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php +++ b/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php @@ -67,20 +67,20 @@ protected function getOpcacheSetupRecommendations(): array { } if (!$this->iniGetWrapper->getBool('opcache.enable')) { - $recommendations[] = $this->l10n->t('OPcache is disabled. For better performance, it is recommended to apply opcache.enable=1 to your PHP configuration.'); + $recommendations[] = $this->l10n->t('OPcache is disabled. For better performance, it is recommended to apply "opcache.enable=1" to your PHP configuration.'); // Check for saved comments only when OPcache is currently disabled. If it was enabled, opcache.save_comments=0 would break Nextcloud in the first place. if (!$this->iniGetWrapper->getBool('opcache.save_comments')) { - $recommendations[] = $this->l10n->t('OPcache is configured to remove code comments. With OPcache enabled, opcache.save_comments=1 must be set for Nextcloud to function.'); + $recommendations[] = $this->l10n->t('OPcache is configured to remove code comments. With OPcache enabled, "opcache.save_comments=1" must be set for Nextcloud to function.'); } if (!$isPermitted) { - $recommendations[] = $this->l10n->t('Nextcloud is not allowed to use the OPcache API. With OPcache enabled, it is highly recommended to include all Nextcloud directories with opcache.restrict_api or unset this setting to disable OPcache API restrictions, to prevent errors during Nextcloud core or app upgrades.'); + $recommendations[] = $this->l10n->t('Nextcloud is not allowed to use the OPcache API. With OPcache enabled, it is highly recommended to include all Nextcloud directories with "opcache.restrict_api" or unset this setting to disable OPcache API restrictions, to prevent errors during Nextcloud core or app upgrades.'); } } elseif (!$isPermitted) { - $recommendations[] = $this->l10n->t('Nextcloud is not allowed to use the OPcache API. It is highly recommended to include all Nextcloud directories with opcache.restrict_api or unset this setting to disable OPcache API restrictions, to prevent errors during Nextcloud core or app upgrades.'); + $recommendations[] = $this->l10n->t('Nextcloud is not allowed to use the OPcache API. It is highly recommended to include all Nextcloud directories with "opcache.restrict_api" or unset this setting to disable OPcache API restrictions, to prevent errors during Nextcloud core or app upgrades.'); } elseif ($this->iniGetWrapper->getBool('opcache.file_cache_only')) { - $recommendations[] = $this->l10n->t('The shared memory based OPcache is disabled. For better performance, it is recommended to apply opcache.file_cache_only=0 to your PHP configuration and use the file cache as second level cache only.'); + $recommendations[] = $this->l10n->t('The shared memory based OPcache is disabled. For better performance, it is recommended to apply "opcache.file_cache_only=0" to your PHP configuration and use the file cache as second level cache only.'); } else { // Check whether opcache_get_status has been explicitly disabled an in case skip usage based checks $disabledFunctions = $this->iniGetWrapper->getString('disable_functions'); @@ -95,14 +95,14 @@ protected function getOpcacheSetupRecommendations(): array { empty($status['opcache_statistics']['max_cached_keys']) || ($status['opcache_statistics']['num_cached_keys'] / $status['opcache_statistics']['max_cached_keys'] > 0.9) ) { - $recommendations[] = $this->l10n->t('The maximum number of OPcache keys is nearly exceeded. To assure that all scripts can be kept in the cache, it is recommended to apply opcache.max_accelerated_files to your PHP configuration with a value higher than %s.', [($this->iniGetWrapper->getNumeric('opcache.max_accelerated_files') ?: 'currently')]); + $recommendations[] = $this->l10n->t('The maximum number of OPcache keys is nearly exceeded. To assure that all scripts can be kept in the cache, it is recommended to apply "opcache.max_accelerated_files" to your PHP configuration with a value higher than "%s".', [($this->iniGetWrapper->getNumeric('opcache.max_accelerated_files') ?: 'currently')]); } if ( empty($status['memory_usage']['free_memory']) || ($status['memory_usage']['used_memory'] / $status['memory_usage']['free_memory'] > 9) ) { - $recommendations[] = $this->l10n->t('The OPcache buffer is nearly full. To assure that all scripts can be hold in cache, it is recommended to apply opcache.memory_consumption to your PHP configuration with a value higher than %s.', [($this->iniGetWrapper->getNumeric('opcache.memory_consumption') ?: 'currently')]); + $recommendations[] = $this->l10n->t('The OPcache buffer is nearly full. To assure that all scripts can be hold in cache, it is recommended to apply "opcache.memory_consumption" to your PHP configuration with a value higher than "%s".', [($this->iniGetWrapper->getNumeric('opcache.memory_consumption') ?: 'currently')]); } if ( @@ -113,7 +113,7 @@ protected function getOpcacheSetupRecommendations(): array { ($status['interned_strings_usage']['used_memory'] / $status['interned_strings_usage']['free_memory'] > 9) ) ) { - $recommendations[] = $this->l10n->t('The OPcache interned strings buffer is nearly full. To assure that repeating strings can be effectively cached, it is recommended to apply opcache.interned_strings_buffer to your PHP configuration with a value higher than %s.', [($this->iniGetWrapper->getNumeric('opcache.interned_strings_buffer') ?: 'currently')]); + $recommendations[] = $this->l10n->t('The OPcache interned strings buffer is nearly full. To assure that repeating strings can be effectively cached, it is recommended to apply "opcache.interned_strings_buffer" to your PHP configuration with a value higher than "%s".', [($this->iniGetWrapper->getNumeric('opcache.interned_strings_buffer') ?: 'currently')]); } } From 4cfab5dc7cfcccb27ac8707656c92634ec6e41e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 12 Dec 2023 16:14:38 +0100 Subject: [PATCH 07/12] Fix small psalm issue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/settings/lib/SetupChecks/PhpOpcacheSetup.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php b/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php index aa1f2367bf56e..796f9c74df171 100644 --- a/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php +++ b/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php @@ -107,7 +107,7 @@ protected function getOpcacheSetupRecommendations(): array { if ( // Do not recommend to raise the interned strings buffer size above a quarter of the total OPcache size - ($this->iniGetWrapper->getNumeric('opcache.interned_strings_buffer') < $this->iniGetWrapper->getNumeric('opcache.memory_consumption') / 4) && + ($this->iniGetWrapper->getNumeric('opcache.interned_strings_buffer') ?? 0 < $this->iniGetWrapper->getNumeric('opcache.memory_consumption') / 4) && ( empty($status['interned_strings_usage']['free_memory']) || ($status['interned_strings_usage']['used_memory'] / $status['interned_strings_usage']['free_memory'] > 9) From 3de96ac17f9d9142616b88cb2e1353807a01e5a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 8 Jan 2024 15:07:31 +0100 Subject: [PATCH 08/12] Improve PHP opcache setup check and reduce level in some cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../lib/SetupChecks/PhpOpcacheSetup.php | 60 ++++++++++++------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php b/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php index 796f9c74df171..07e4b5134976c 100644 --- a/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php +++ b/apps/settings/lib/SetupChecks/PhpOpcacheSetup.php @@ -49,47 +49,44 @@ public function getCategory(): string { /** * Checks whether a PHP OPcache is properly set up - * @return string[] The list of OPcache setup recommendations + * @return array{'warning'|'error',list} The level and the list of OPcache setup recommendations */ protected function getOpcacheSetupRecommendations(): array { + $level = 'warning'; + // If the module is not loaded, return directly to skip inapplicable checks if (!extension_loaded('Zend OPcache')) { - return [$this->l10n->t('The PHP OPcache module is not loaded. For better performance it is recommended to load it into your PHP installation.')]; + return ['error',[$this->l10n->t('The PHP OPcache module is not loaded. For better performance it is recommended to load it into your PHP installation.')]]; } $recommendations = []; // Check whether Nextcloud is allowed to use the OPcache API $isPermitted = true; - $permittedPath = $this->iniGetWrapper->getString('opcache.restrict_api'); - if (isset($permittedPath) && $permittedPath !== '' && !str_starts_with(\OC::$SERVERROOT, rtrim($permittedPath, '/'))) { + $permittedPath = (string)$this->iniGetWrapper->getString('opcache.restrict_api'); + if ($permittedPath !== '' && !str_starts_with(\OC::$SERVERROOT, rtrim($permittedPath, '/'))) { $isPermitted = false; } if (!$this->iniGetWrapper->getBool('opcache.enable')) { $recommendations[] = $this->l10n->t('OPcache is disabled. For better performance, it is recommended to apply "opcache.enable=1" to your PHP configuration.'); - - // Check for saved comments only when OPcache is currently disabled. If it was enabled, opcache.save_comments=0 would break Nextcloud in the first place. - if (!$this->iniGetWrapper->getBool('opcache.save_comments')) { - $recommendations[] = $this->l10n->t('OPcache is configured to remove code comments. With OPcache enabled, "opcache.save_comments=1" must be set for Nextcloud to function.'); - } - - if (!$isPermitted) { - $recommendations[] = $this->l10n->t('Nextcloud is not allowed to use the OPcache API. With OPcache enabled, it is highly recommended to include all Nextcloud directories with "opcache.restrict_api" or unset this setting to disable OPcache API restrictions, to prevent errors during Nextcloud core or app upgrades.'); - } - } elseif (!$isPermitted) { - $recommendations[] = $this->l10n->t('Nextcloud is not allowed to use the OPcache API. It is highly recommended to include all Nextcloud directories with "opcache.restrict_api" or unset this setting to disable OPcache API restrictions, to prevent errors during Nextcloud core or app upgrades.'); + $level = 'error'; } elseif ($this->iniGetWrapper->getBool('opcache.file_cache_only')) { $recommendations[] = $this->l10n->t('The shared memory based OPcache is disabled. For better performance, it is recommended to apply "opcache.file_cache_only=0" to your PHP configuration and use the file cache as second level cache only.'); } else { // Check whether opcache_get_status has been explicitly disabled an in case skip usage based checks $disabledFunctions = $this->iniGetWrapper->getString('disable_functions'); if (isset($disabledFunctions) && str_contains($disabledFunctions, 'opcache_get_status')) { - return []; + return [$level, $recommendations]; } $status = opcache_get_status(false); + if ($status === false) { + $recommendations[] = $this->l10n->t('OPcache is not working as it should, opcache_get_status() returns false, please check configuration.'); + $level = 'error'; + } + // Recommend to raise value, if more than 90% of max value is reached if ( empty($status['opcache_statistics']['max_cached_keys']) || @@ -107,7 +104,7 @@ protected function getOpcacheSetupRecommendations(): array { if ( // Do not recommend to raise the interned strings buffer size above a quarter of the total OPcache size - ($this->iniGetWrapper->getNumeric('opcache.interned_strings_buffer') ?? 0 < $this->iniGetWrapper->getNumeric('opcache.memory_consumption') / 4) && + ($this->iniGetWrapper->getNumeric('opcache.interned_strings_buffer') ?? 0 < $this->iniGetWrapper->getNumeric('opcache.memory_consumption') ?? 0 / 4) && ( empty($status['interned_strings_usage']['free_memory']) || ($status['interned_strings_usage']['used_memory'] / $status['interned_strings_usage']['free_memory'] > 9) @@ -117,16 +114,33 @@ protected function getOpcacheSetupRecommendations(): array { } } - return $recommendations; + // Check for saved comments only when OPcache is currently disabled. If it was enabled, opcache.save_comments=0 would break Nextcloud in the first place. + if (!$this->iniGetWrapper->getBool('opcache.save_comments')) { + $recommendations[] = $this->l10n->t('OPcache is configured to remove code comments. With OPcache enabled, "opcache.save_comments=1" must be set for Nextcloud to function.'); + $level = 'error'; + } + + if (!$isPermitted) { + $recommendations[] = $this->l10n->t('Nextcloud is not allowed to use the OPcache API. With OPcache enabled, it is highly recommended to include all Nextcloud directories with "opcache.restrict_api" or unset this setting to disable OPcache API restrictions, to prevent errors during Nextcloud core or app upgrades.'); + $level = 'error'; + } + + return [$level, $recommendations]; } public function run(): SetupResult { - $recommendations = $this->getOpcacheSetupRecommendations(); + [$level,$recommendations] = $this->getOpcacheSetupRecommendations(); if (!empty($recommendations)) { - return SetupResult::error( - $this->l10n->t('The PHP OPcache module is not properly configured. %s.', implode("\n", $recommendations)), - $this->urlGenerator->linkToDocs('admin-php-opcache') - ); + return match($level) { + 'error' => SetupResult::error( + $this->l10n->t('The PHP OPcache module is not properly configured. %s.', implode("\n", $recommendations)), + $this->urlGenerator->linkToDocs('admin-php-opcache') + ), + default => SetupResult::warning( + $this->l10n->t('The PHP OPcache module is not properly configured. %s.', implode("\n", $recommendations)), + $this->urlGenerator->linkToDocs('admin-php-opcache') + ), + }; } else { return SetupResult::success($this->l10n->t('Correctly configured')); } From 14d132a4e6023e2d0c3a8e0d122aa794a14b01d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 12 Dec 2023 16:53:13 +0100 Subject: [PATCH 09/12] Migrate app dir owner check to SetupCheck API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 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 b32aab9756955..60717a4729449 100644 --- a/apps/settings/composer/composer/autoload_classmap.php +++ b/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', diff --git a/apps/settings/composer/composer/autoload_static.php b/apps/settings/composer/composer/autoload_static.php index 09e0d724e4ccb..7cd3892a6ec83 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 7e963e355930b..f3e2343e04886 100644 --- a/apps/settings/lib/AppInfo/Application.php +++ b/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); diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php index b251adb0d84f8..acd73479ea148 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 @@ 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(), diff --git a/apps/settings/lib/SetupChecks/AppDirsWithDifferentOwner.php b/apps/settings/lib/SetupChecks/AppDirsWithDifferentOwner.php new file mode 100644 index 0000000000000..3404e8dc8b315 --- /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 b9c2ddc8282ff..6336ca852d633 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -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, diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js index e85f32dae4ac6..4331e8e9cc929 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 bb7d118fc52c5..594b9ca39bc6b 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, From cf8c7e92702b14ce2ff8c57fd07d39cbb0ab17ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 9 Jan 2024 09:52:13 +0100 Subject: [PATCH 10/12] Migrate tests for AppDirsWithDifferentOwner setup check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../Controller/CheckSetupControllerTest.php | 57 ---------- .../AppDirsWithDifferentOwnerTest.php | 100 ++++++++++++++++++ 2 files changed, 100 insertions(+), 57 deletions(-) create mode 100644 apps/settings/tests/SetupChecks/AppDirsWithDifferentOwnerTest.php diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php index 6336ca852d633..488497956cebe 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/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') @@ -349,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') diff --git a/apps/settings/tests/SetupChecks/AppDirsWithDifferentOwnerTest.php b/apps/settings/tests/SetupChecks/AppDirsWithDifferentOwnerTest.php new file mode 100644 index 0000000000000..06a75225cb4d5 --- /dev/null +++ b/apps/settings/tests/SetupChecks/AppDirsWithDifferentOwnerTest.php @@ -0,0 +1,100 @@ + + * + * @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\Tests; + +use OCA\Settings\SetupChecks\AppDirsWithDifferentOwner; +use OCP\IL10N; +use Test\TestCase; + +class AppDirsWithDifferentOwnerTest extends TestCase { + private IL10N $l10n; + private AppDirsWithDifferentOwner $check; + + protected function setUp(): void { + parent::setUp(); + + $this->l10n = $this->getMockBuilder(IL10N::class) + ->disableOriginalConstructor()->getMock(); + $this->l10n->expects($this->any()) + ->method('t') + ->willReturnCallback(function ($message, array $replace) { + return vsprintf($message, $replace); + }); + $this->check = new AppDirsWithDifferentOwner( + $this->l10n, + ); + } + + /** + * 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->check, 'getAppDirsWithDifferentOwner', [posix_getuid()]) + ); + } + + /** + * 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->check, 'getAppDirsWithDifferentOwner', [posix_getuid()]) + ); + } +} From 17dd695e0eb89199f39f4c894af53e35edefce58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 12 Dec 2023 17:13:20 +0100 Subject: [PATCH 11/12] Migrate PHP imagick module check to new SetupCheck API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../lib/Controller/CheckSetupController.php | 15 ----- .../Controller/CheckSetupControllerTest.php | 8 --- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + apps/theming/lib/AppInfo/Application.php | 2 + .../lib/SetupChecks/PhpImagickModule.php | 63 +++++++++++++++++++ core/js/setupchecks.js | 15 ----- core/js/tests/specs/setupchecksSpec.js | 60 ------------------ 8 files changed, 67 insertions(+), 98 deletions(-) create mode 100644 apps/theming/lib/SetupChecks/PhpImagickModule.php diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php index acd73479ea148..c648e8af5fc36 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -342,15 +342,6 @@ private function isTemporaryDirectoryWritable(): bool { return false; } - protected function isImagickEnabled(): bool { - if ($this->config->getAppValue('theming', 'enabled', 'no') === 'yes') { - if (!extension_loaded('imagick')) { - return false; - } - } - return true; - } - protected function areWebauthnExtensionsEnabled(): bool { if (!extension_loaded('bcmath')) { return false; @@ -401,10 +392,6 @@ protected function isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed(): bool { return false; } - protected function imageMagickLacksSVGSupport(): bool { - return extension_loaded('imagick') && count(\Imagick::queryFormats('SVG')) === 0; - } - /** * @return DataResponse * @AuthorizedAdminSetting(settings=OCA\Settings\Settings\Admin\Overview) @@ -422,12 +409,10 @@ public function check() { 'hasPassedCodeIntegrityCheck' => $this->checker->hasPassedCheck(), 'codeIntegrityCheckerDocumentation' => $this->urlGenerator->linkToDocs('admin-code-integrity'), 'isSettimelimitAvailable' => $this->isSettimelimitAvailable(), - 'isImagickEnabled' => $this->isImagickEnabled(), 'areWebauthnExtensionsEnabled' => $this->areWebauthnExtensionsEnabled(), 'isMysqlUsedWithoutUTF8MB4' => $this->isMysqlUsedWithoutUTF8MB4(), 'isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed' => $this->isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed(), 'reverseProxyGeneratedURL' => $this->urlGenerator->getAbsoluteURL('index.php'), - 'imageMagickLacksSVGSupport' => $this->imageMagickLacksSVGSupport(), 'temporaryDirectoryWritable' => $this->isTemporaryDirectoryWritable(), 'generic' => $this->setupCheckManager->runAll(), ] diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php index 488497956cebe..c02bf66cf8719 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -139,7 +139,6 @@ protected function setUp(): void { 'getCurlVersion', 'isPhpOutdated', 'isPHPMailerUsed', - 'isImagickEnabled', 'areWebauthnExtensionsEnabled', 'isMysqlUsedWithoutUTF8MB4', 'isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed', @@ -197,11 +196,6 @@ public function testCheck() { ->method('hasPassedCheck') ->willReturn(true); - $this->checkSetupController - ->expects($this->once()) - ->method('isImagickEnabled') - ->willReturn(false); - $this->checkSetupController ->expects($this->once()) ->method('areWebauthnExtensionsEnabled') @@ -263,12 +257,10 @@ public function testCheck() { 'hasPassedCodeIntegrityCheck' => true, 'codeIntegrityCheckerDocumentation' => 'http://docs.example.org/server/go.php?to=admin-code-integrity', 'isSettimelimitAvailable' => true, - 'isImagickEnabled' => false, 'areWebauthnExtensionsEnabled' => false, 'isMysqlUsedWithoutUTF8MB4' => false, 'isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed' => true, 'reverseProxyGeneratedURL' => 'https://server/index.php', - 'imageMagickLacksSVGSupport' => false, 'isFairUseOfFreePushService' => false, 'temporaryDirectoryWritable' => false, 'generic' => [], diff --git a/apps/theming/composer/composer/autoload_classmap.php b/apps/theming/composer/composer/autoload_classmap.php index d60aeecd8cd02..b936682cd7ef2 100644 --- a/apps/theming/composer/composer/autoload_classmap.php +++ b/apps/theming/composer/composer/autoload_classmap.php @@ -31,6 +31,7 @@ 'OCA\\Theming\\Settings\\AdminSection' => $baseDir . '/../lib/Settings/AdminSection.php', 'OCA\\Theming\\Settings\\Personal' => $baseDir . '/../lib/Settings/Personal.php', 'OCA\\Theming\\Settings\\PersonalSection' => $baseDir . '/../lib/Settings/PersonalSection.php', + 'OCA\\Theming\\SetupChecks\\PhpImagickModule' => $baseDir . '/../lib/SetupChecks/PhpImagickModule.php', 'OCA\\Theming\\Themes\\CommonThemeTrait' => $baseDir . '/../lib/Themes/CommonThemeTrait.php', 'OCA\\Theming\\Themes\\DarkHighContrastTheme' => $baseDir . '/../lib/Themes/DarkHighContrastTheme.php', 'OCA\\Theming\\Themes\\DarkTheme' => $baseDir . '/../lib/Themes/DarkTheme.php', diff --git a/apps/theming/composer/composer/autoload_static.php b/apps/theming/composer/composer/autoload_static.php index bdf539bc59993..a872d8add67ff 100644 --- a/apps/theming/composer/composer/autoload_static.php +++ b/apps/theming/composer/composer/autoload_static.php @@ -46,6 +46,7 @@ class ComposerStaticInitTheming 'OCA\\Theming\\Settings\\AdminSection' => __DIR__ . '/..' . '/../lib/Settings/AdminSection.php', 'OCA\\Theming\\Settings\\Personal' => __DIR__ . '/..' . '/../lib/Settings/Personal.php', 'OCA\\Theming\\Settings\\PersonalSection' => __DIR__ . '/..' . '/../lib/Settings/PersonalSection.php', + 'OCA\\Theming\\SetupChecks\\PhpImagickModule' => __DIR__ . '/..' . '/../lib/SetupChecks/PhpImagickModule.php', 'OCA\\Theming\\Themes\\CommonThemeTrait' => __DIR__ . '/..' . '/../lib/Themes/CommonThemeTrait.php', 'OCA\\Theming\\Themes\\DarkHighContrastTheme' => __DIR__ . '/..' . '/../lib/Themes/DarkHighContrastTheme.php', 'OCA\\Theming\\Themes\\DarkTheme' => __DIR__ . '/..' . '/../lib/Themes/DarkTheme.php', diff --git a/apps/theming/lib/AppInfo/Application.php b/apps/theming/lib/AppInfo/Application.php index 43b854012f7d3..ec4c294087120 100644 --- a/apps/theming/lib/AppInfo/Application.php +++ b/apps/theming/lib/AppInfo/Application.php @@ -27,6 +27,7 @@ use OCA\Theming\Capabilities; use OCA\Theming\Listener\BeforePreferenceListener; use OCA\Theming\Listener\BeforeTemplateRenderedListener; +use OCA\Theming\SetupChecks\PhpImagickModule; use OCP\AppFramework\App; use OCP\AppFramework\Bootstrap\IBootContext; use OCP\AppFramework\Bootstrap\IBootstrap; @@ -49,6 +50,7 @@ public function register(IRegistrationContext $context): void { $context->registerEventListener(BeforeLoginTemplateRenderedEvent::class, BeforeTemplateRenderedListener::class); $context->registerEventListener(BeforePreferenceSetEvent::class, BeforePreferenceListener::class); $context->registerEventListener(BeforePreferenceDeletedEvent::class, BeforePreferenceListener::class); + $context->registerSetupCheck(PhpImagickModule::class); } public function boot(IBootContext $context): void { diff --git a/apps/theming/lib/SetupChecks/PhpImagickModule.php b/apps/theming/lib/SetupChecks/PhpImagickModule.php new file mode 100644 index 0000000000000..7d08aa8e954a3 --- /dev/null +++ b/apps/theming/lib/SetupChecks/PhpImagickModule.php @@ -0,0 +1,63 @@ + + * + * @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\Theming\SetupChecks; + +use OCP\IL10N; +use OCP\IURLGenerator; +use OCP\SetupCheck\ISetupCheck; +use OCP\SetupCheck\SetupResult; + +class PhpImagickModule implements ISetupCheck { + public function __construct( + private IL10N $l10n, + private IURLGenerator $urlGenerator, + ) { + } + + public function getName(): string { + return $this->l10n->t('PHP Imagick module'); + } + + public function getCategory(): string { + return 'php'; + } + + public function run(): SetupResult { + if (!extension_loaded('imagick')) { + return SetupResult::info( + $this->l10n->t('The PHP module "imagick" is not enabled although the theming app is. For favicon generation to work correctly, you need to install and enable this module.'), + $this->urlGenerator->linkToDocs('admin-php-modules') + ); + } elseif (count(\Imagick::queryFormats('SVG')) === 0) { + return SetupResult::info( + $this->l10n->t('The PHP module "imagick" in this instance has no SVG support. For better compatibility it is recommended to install it.'), + $this->urlGenerator->linkToDocs('admin-php-modules') + ); + } else { + return SetupResult::success(); + } + } +} diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js index 4331e8e9cc929..837482090b9e4 100644 --- a/core/js/setupchecks.js +++ b/core/js/setupchecks.js @@ -246,15 +246,6 @@ type: OC.SetupChecks.MESSAGE_TYPE_WARNING }); } - if (!data.isImagickEnabled) { - messages.push({ - msg: t( - 'core', - 'The PHP module "imagick" is not enabled although the theming app is. For favicon generation to work correctly, you need to install and enable this module.' - ), - type: OC.SetupChecks.MESSAGE_TYPE_INFO - }) - } if (!data.areWebauthnExtensionsEnabled) { messages.push({ msg: t( @@ -264,12 +255,6 @@ type: OC.SetupChecks.MESSAGE_TYPE_INFO }) } - if (data.imageMagickLacksSVGSupport) { - messages.push({ - msg: t('core', 'Module php-imagick in this instance has no SVG support. For better compatibility it is recommended to install it.'), - type: OC.SetupChecks.MESSAGE_TYPE_INFO - }) - } if (data.isMysqlUsedWithoutUTF8MB4) { messages.push({ diff --git a/core/js/tests/specs/setupchecksSpec.js b/core/js/tests/specs/setupchecksSpec.js index 594b9ca39bc6b..a407fbb145ad7 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 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, @@ -279,7 +278,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, @@ -326,7 +324,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, @@ -373,7 +370,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, @@ -419,7 +415,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, @@ -465,7 +460,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, @@ -542,7 +536,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, @@ -594,7 +587,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: true, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, @@ -643,7 +635,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, @@ -689,7 +680,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, @@ -732,7 +722,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: false, @@ -760,52 +749,6 @@ describe('OC.SetupChecks tests', function() { }); - it('should return an error if imagick is not enabled', 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 - }, - isImagickEnabled: false, - 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: 'The PHP module "imagick" is not enabled although the theming app is. For favicon generation to work correctly, you need to install and enable this module.', - type: OC.SetupChecks.MESSAGE_TYPE_INFO - }]); - done(); - }); - }); - - it('should return an error if gmp or bcmath are not enabled', function(done) { var async = OC.SetupChecks.checkSetup(); @@ -824,7 +767,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: false, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, @@ -869,7 +811,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, @@ -921,7 +862,6 @@ describe('OC.SetupChecks tests', function() { cronInfo: { diffInSeconds: 0 }, - isImagickEnabled: true, areWebauthnExtensionsEnabled: true, isMysqlUsedWithoutUTF8MB4: false, isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, From ed87e4b4ca417caa2d7cdd52b674322997d11149 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 15 Jan 2024 10:32:26 +0100 Subject: [PATCH 12/12] Fix AppDirsWithDifferentOwnerTest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../Controller/CheckSetupControllerTest.php | 20 ------------------- .../AppDirsWithDifferentOwnerTest.php | 20 +++++++++++++++++++ 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php index c02bf66cf8719..159e0a9358def 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -86,13 +86,6 @@ class CheckSetupControllerTest extends TestCase { /** @var ISetupCheckManager|MockObject */ private $setupCheckManager; - /** - * Holds a list of directories created during tests. - * - * @var array - */ - private $dirsToRemove = []; - protected function setUp(): void { parent::setUp(); @@ -145,19 +138,6 @@ protected function setUp(): void { ])->getMock(); } - /** - * Removes directories created during tests. - * - * @after - * @return void - */ - public function removeTestDirectories() { - foreach ($this->dirsToRemove as $dirToRemove) { - rmdir($dirToRemove); - } - $this->dirsToRemove = []; - } - public function testCheck() { $this->config->expects($this->any()) ->method('getAppValue') diff --git a/apps/settings/tests/SetupChecks/AppDirsWithDifferentOwnerTest.php b/apps/settings/tests/SetupChecks/AppDirsWithDifferentOwnerTest.php index 06a75225cb4d5..9216be3ace5e2 100644 --- a/apps/settings/tests/SetupChecks/AppDirsWithDifferentOwnerTest.php +++ b/apps/settings/tests/SetupChecks/AppDirsWithDifferentOwnerTest.php @@ -33,6 +33,13 @@ class AppDirsWithDifferentOwnerTest extends TestCase { private IL10N $l10n; private AppDirsWithDifferentOwner $check; + /** + * Holds a list of directories created during tests. + * + * @var array + */ + private $dirsToRemove = []; + protected function setUp(): void { parent::setUp(); @@ -97,4 +104,17 @@ public function testAppDirectoryOwnersNotWritable() { $this->invokePrivate($this->check, 'getAppDirsWithDifferentOwner', [posix_getuid()]) ); } + + /** + * Removes directories created during tests. + * + * @after + * @return void + */ + public function removeTestDirectories() { + foreach ($this->dirsToRemove as $dirToRemove) { + rmdir($dirToRemove); + } + $this->dirsToRemove = []; + } }