From 7ed583cb8ee64f77696b0e23f79d8d1b4038bcbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 12 Sep 2024 16:17:19 +0200 Subject: [PATCH 1/3] chore: Migrate cleanAppId and getAppPath calls to IAppManager from OC_App MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../lib/Controller/AppConfigController.php | 7 +++-- .../Controller/AppConfigControllerTest.php | 27 ++++++++-------- .../lib/Controller/AppSettingsController.php | 11 +++---- apps/theming/tests/UtilTest.php | 31 +++++-------------- core/Command/L10n/CreateJs.php | 7 ++++- lib/private/App/AppManager.php | 9 ++++-- .../TwoFactorAuth/ProviderLoader.php | 30 +++++++----------- lib/private/L10N/Factory.php | 2 +- lib/private/Route/Router.php | 26 ++++++++++------ lib/private/legacy/OC_App.php | 3 +- lib/public/App/IAppManager.php | 16 ++++++++-- remote.php | 19 ++++++------ tests/apps.php | 11 +++++-- tests/lib/App/AppManagerTest.php | 4 +-- tests/lib/InfoXmlTest.php | 12 +++++-- 15 files changed, 115 insertions(+), 100 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/AppConfigController.php b/apps/provisioning_api/lib/Controller/AppConfigController.php index 5061a0ffc8ffa..bb161f6cb8c8a 100644 --- a/apps/provisioning_api/lib/Controller/AppConfigController.php +++ b/apps/provisioning_api/lib/Controller/AppConfigController.php @@ -10,6 +10,7 @@ use OC\AppConfig; use OC\AppFramework\Middleware\Security\Exceptions\NotAdminException; +use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; @@ -35,6 +36,7 @@ public function __construct( private IL10N $l10n, private IGroupManager $groupManager, private IManager $settingManager, + private IAppManager $appManager, ) { parent::__construct($appName, $request); } @@ -171,11 +173,10 @@ public function deleteKey(string $app, string $key): DataResponse { } /** - * @param string $app * @throws \InvalidArgumentException */ - protected function verifyAppId(string $app) { - if (\OC_App::cleanAppId($app) !== $app) { + protected function verifyAppId(string $app): void { + if ($this->appManager->cleanAppId($app) !== $app) { throw new \InvalidArgumentException('Invalid app id given'); } } diff --git a/apps/provisioning_api/tests/Controller/AppConfigControllerTest.php b/apps/provisioning_api/tests/Controller/AppConfigControllerTest.php index 0bdee39cebc00..23a1f2ee195ff 100644 --- a/apps/provisioning_api/tests/Controller/AppConfigControllerTest.php +++ b/apps/provisioning_api/tests/Controller/AppConfigControllerTest.php @@ -7,6 +7,7 @@ use OC\AppConfig; use OCA\Provisioning_API\Controller\AppConfigController; +use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; use OCP\Exceptions\AppConfigUnknownKeyException; @@ -17,6 +18,7 @@ use OCP\IUser; use OCP\IUserSession; use OCP\Settings\IManager; +use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; use function json_decode; use function json_encode; @@ -28,16 +30,12 @@ */ class AppConfigControllerTest extends TestCase { - /** @var IAppConfig|\PHPUnit\Framework\MockObject\MockObject */ - private $appConfig; - /** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */ - private $userSession; - /** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */ - private $l10n; - /** @var IManager|\PHPUnit\Framework\MockObject\MockObject */ - private $settingManager; - /** @var IGroupManager|\PHPUnit\Framework\MockObject\MockObject */ - private $groupManager; + private IAppConfig&MockObject $appConfig; + private IUserSession&MockObject $userSession; + private IL10N&MockObject $l10n; + private IManager&MockObject $settingManager; + private IGroupManager&MockObject $groupManager; + private IAppManager $appManager; protected function setUp(): void { parent::setUp(); @@ -45,8 +43,9 @@ protected function setUp(): void { $this->appConfig = $this->createMock(AppConfig::class); $this->userSession = $this->createMock(IUserSession::class); $this->l10n = $this->createMock(IL10N::class); - $this->groupManager = $this->createMock(IGroupManager::class); $this->settingManager = $this->createMock(IManager::class); + $this->groupManager = $this->createMock(IGroupManager::class); + $this->appManager = \OCP\Server::get(IAppManager::class); } /** @@ -64,7 +63,8 @@ protected function getInstance(array $methods = []) { $this->userSession, $this->l10n, $this->groupManager, - $this->settingManager + $this->settingManager, + $this->appManager, ); } else { return $this->getMockBuilder(AppConfigController::class) @@ -75,7 +75,8 @@ protected function getInstance(array $methods = []) { $this->userSession, $this->l10n, $this->groupManager, - $this->settingManager + $this->settingManager, + $this->appManager, ]) ->setMethods($methods) ->getMock(); diff --git a/apps/settings/lib/Controller/AppSettingsController.php b/apps/settings/lib/Controller/AppSettingsController.php index 760584888c015..2c682ac460035 100644 --- a/apps/settings/lib/Controller/AppSettingsController.php +++ b/apps/settings/lib/Controller/AppSettingsController.php @@ -14,7 +14,6 @@ use OC\App\DependencyAnalyzer; use OC\App\Platform; use OC\Installer; -use OC_App; use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; use OCP\AppFramework\Controller; @@ -479,7 +478,7 @@ public function enableApps(array $appIds, array $groups = []): JSONResponse { $updateRequired = false; foreach ($appIds as $appId) { - $appId = OC_App::cleanAppId($appId); + $appId = $this->appManager->cleanAppId($appId); // Check if app is already downloaded /** @var Installer $installer */ @@ -537,7 +536,7 @@ public function disableApp(string $appId): JSONResponse { public function disableApps(array $appIds): JSONResponse { try { foreach ($appIds as $appId) { - $appId = OC_App::cleanAppId($appId); + $appId = $this->appManager->cleanAppId($appId); $this->appManager->disableApp($appId); } return new JSONResponse([]); @@ -553,7 +552,7 @@ public function disableApps(array $appIds): JSONResponse { */ #[PasswordConfirmationRequired] public function uninstallApp(string $appId): JSONResponse { - $appId = OC_App::cleanAppId($appId); + $appId = $this->appManager->cleanAppId($appId); $result = $this->installer->removeApp($appId); if ($result !== false) { $this->appManager->clearAppsCache(); @@ -567,7 +566,7 @@ public function uninstallApp(string $appId): JSONResponse { * @return JSONResponse */ public function updateApp(string $appId): JSONResponse { - $appId = OC_App::cleanAppId($appId); + $appId = $this->appManager->cleanAppId($appId); $this->config->setSystemValue('maintenance', true); try { @@ -594,7 +593,7 @@ private function sortApps($a, $b) { } public function force(string $appId): JSONResponse { - $appId = OC_App::cleanAppId($appId); + $appId = $this->appManager->cleanAppId($appId); $this->appManager->ignoreNextcloudRequirementForApp($appId); return new JSONResponse(); } diff --git a/apps/theming/tests/UtilTest.php b/apps/theming/tests/UtilTest.php index 4c9f9f10c974d..474daab603091 100644 --- a/apps/theming/tests/UtilTest.php +++ b/apps/theming/tests/UtilTest.php @@ -18,22 +18,17 @@ class UtilTest extends TestCase { - /** @var Util */ - protected $util; - /** @var IConfig|MockObject */ - protected $config; - /** @var IAppData|MockObject */ - protected $appData; - /** @var IAppManager|MockObject */ - protected $appManager; - /** @var ImageManager|MockObject */ - protected $imageManager; + protected Util $util; + protected IConfig&MockObject $config; + protected IAppData&MockObject $appData; + protected IAppManager $appManager; + protected ImageManager&MockObject $imageManager; protected function setUp(): void { parent::setUp(); $this->config = $this->createMock(IConfig::class); $this->appData = $this->createMock(IAppData::class); - $this->appManager = $this->createMock(IAppManager::class); + $this->appManager = \OCP\Server::get(IAppManager::class); $this->imageManager = $this->createMock(ImageManager::class); $this->util = new Util($this->config, $this->appManager, $this->appData, $this->imageManager); } @@ -152,19 +147,15 @@ public function testGetAppIcon($app, $expected) { ->method('getFolder') ->with('global/images') ->willThrowException(new NotFoundException()); - $this->appManager->expects($this->once()) - ->method('getAppPath') - ->with($app) - ->willReturn(\OC_App::getAppPath($app)); $icon = $this->util->getAppIcon($app); $this->assertEquals($expected, $icon); } public function dataGetAppIcon() { return [ - ['user_ldap', \OC_App::getAppPath('user_ldap') . '/img/app.svg'], + ['user_ldap', \OCP\Server::get(IAppManager::class)->getAppPath('user_ldap') . '/img/app.svg'], ['noapplikethis', \OC::$SERVERROOT . '/core/img/logo/logo.svg'], - ['comments', \OC_App::getAppPath('comments') . '/img/comments.svg'], + ['comments', \OCP\Server::get(IAppManager::class)->getAppPath('comments') . '/img/comments.svg'], ]; } @@ -187,12 +178,6 @@ public function testGetAppIconThemed() { * @dataProvider dataGetAppImage */ public function testGetAppImage($app, $image, $expected) { - if ($app !== 'core') { - $this->appManager->expects($this->once()) - ->method('getAppPath') - ->with($app) - ->willReturn(\OC_App::getAppPath($app)); - } $this->assertEquals($expected, $this->util->getAppImage($app, $image)); } diff --git a/core/Command/L10n/CreateJs.php b/core/Command/L10n/CreateJs.php index 517016b7e1668..27d537b093ec8 100644 --- a/core/Command/L10n/CreateJs.php +++ b/core/Command/L10n/CreateJs.php @@ -11,6 +11,7 @@ use DirectoryIterator; +use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; use Stecman\Component\Symfony\Console\BashCompletion\Completion\CompletionAwareInterface; use Stecman\Component\Symfony\Console\BashCompletion\CompletionContext; @@ -150,7 +151,11 @@ public function completeArgumentValues($argumentName, CompletionContext $context return \OC_App::getAllApps(); } elseif ($argumentName === 'lang') { $appName = $context->getWordAtIndex($context->getWordIndex() - 1); - return $this->getAllLanguages(\OC_App::getAppPath($appName)); + try { + return $this->getAllLanguages($this->appManager->getAppPath($appName)); + } catch(AppPathNotFoundException) { + return []; + } } return []; } diff --git a/lib/private/App/AppManager.php b/lib/private/App/AppManager.php index fe2f7b74b2221..974545cfe92d2 100644 --- a/lib/private/App/AppManager.php +++ b/lib/private/App/AppManager.php @@ -647,11 +647,9 @@ public function disableApp($appId, $automaticDisabled = false) { /** * Get the directory for the given app. * - * @param string $appId - * @return string * @throws AppPathNotFoundException if app folder can't be found */ - public function getAppPath($appId) { + public function getAppPath(string $appId): string { $appPath = \OC_App::getAppPath($appId); if ($appPath === false) { throw new AppPathNotFoundException('Could not find path for ' . $appId); @@ -877,4 +875,9 @@ public function isBackendRequired(string $backend): bool { return false; } + + public function cleanAppId(string $app): string { + // FIXME should list allowed characters instead + return str_replace(['<', '>', '"', "'", '\0', '/', '\\', '..'], '', $app); + } } diff --git a/lib/private/Authentication/TwoFactorAuth/ProviderLoader.php b/lib/private/Authentication/TwoFactorAuth/ProviderLoader.php index b9a0a97bec442..7e674a01dd860 100644 --- a/lib/private/Authentication/TwoFactorAuth/ProviderLoader.php +++ b/lib/private/Authentication/TwoFactorAuth/ProviderLoader.php @@ -9,8 +9,7 @@ namespace OC\Authentication\TwoFactorAuth; use Exception; -use OC; -use OC_App; +use OC\AppFramework\Bootstrap\Coordinator; use OCP\App\IAppManager; use OCP\AppFramework\QueryException; use OCP\Authentication\TwoFactorAuth\IProvider; @@ -19,15 +18,10 @@ class ProviderLoader { public const BACKUP_CODES_APP_ID = 'twofactor_backupcodes'; - /** @var IAppManager */ - private $appManager; - - /** @var OC\AppFramework\Bootstrap\Coordinator */ - private $coordinator; - - public function __construct(IAppManager $appManager, OC\AppFramework\Bootstrap\Coordinator $coordinator) { - $this->appManager = $appManager; - $this->coordinator = $coordinator; + public function __construct( + private IAppManager $appManager, + private Coordinator $coordinator, + ) { } /** @@ -58,12 +52,12 @@ public function getProviders(IUser $user): array { } } - $registeredProviders = $this->coordinator->getRegistrationContext()->getTwoFactorProviders(); + $registeredProviders = $this->coordinator->getRegistrationContext()?->getTwoFactorProviders() ?? []; foreach ($registeredProviders as $provider) { try { $this->loadTwoFactorApp($provider->getAppId()); - $provider = \OCP\Server::get($provider->getService()); - $providers[$provider->getId()] = $provider; + $providerInstance = \OCP\Server::get($provider->getService()); + $providers[$providerInstance->getId()] = $providerInstance; } catch (QueryException $exc) { // Provider class can not be resolved throw new Exception('Could not load two-factor auth provider ' . $provider->getService()); @@ -75,12 +69,10 @@ public function getProviders(IUser $user): array { /** * Load an app by ID if it has not been loaded yet - * - * @param string $appId */ - protected function loadTwoFactorApp(string $appId) { - if (!OC_App::isAppLoaded($appId)) { - OC_App::loadApp($appId); + protected function loadTwoFactorApp(string $appId): void { + if (!$this->appManager->isAppLoaded($appId)) { + $this->appManager->loadApp($appId); } } } diff --git a/lib/private/L10N/Factory.php b/lib/private/L10N/Factory.php index 6b6dc5d3b4084..fc76a15b07be9 100644 --- a/lib/private/L10N/Factory.php +++ b/lib/private/L10N/Factory.php @@ -81,7 +81,7 @@ public function __construct( */ public function get($app, $lang = null, $locale = null) { return new LazyL10N(function () use ($app, $lang, $locale) { - $app = \OC_App::cleanAppId($app); + $app = $this->appManager->cleanAppId($app); if ($lang !== null) { $lang = str_replace(['\0', '/', '\\', '..'], '', $lang); } diff --git a/lib/private/Route/Router.php b/lib/private/Route/Router.php index 646d1d4e6ed6e..ba369eecac07e 100644 --- a/lib/private/Route/Router.php +++ b/lib/private/Route/Router.php @@ -104,7 +104,7 @@ public function getRoutingFiles() { */ public function loadRoutes($app = null) { if (is_string($app)) { - $app = \OC_App::cleanAppId($app); + $app = $this->appManager->cleanAppId($app); } $requestedApp = $app; @@ -123,11 +123,15 @@ public function loadRoutes($app = null) { if (isset($this->loadedApps[$app])) { return; } - $appPath = \OC_App::getAppPath($app); - $file = $appPath . '/appinfo/routes.php'; - if ($appPath !== false && file_exists($file)) { - $routingFiles = [$app => $file]; - } else { + try { + $appPath = $this->appManager->getAppPath($app); + $file = $appPath . '/appinfo/routes.php'; + if (file_exists($file)) { + $routingFiles = [$app => $file]; + } else { + $routingFiles = []; + } + } catch (AppPathNotFoundException) { $routingFiles = []; } @@ -238,14 +242,14 @@ public function findMatchingRoute(string $url): array { // empty string / 'apps' / $app / rest of the route [, , $app,] = explode('/', $url, 4); - $app = \OC_App::cleanAppId($app); + $app = $this->appManager->cleanAppId($app); \OC::$REQUESTEDAPP = $app; $this->loadRoutes($app); } elseif (str_starts_with($url, '/ocsapp/apps/')) { // empty string / 'ocsapp' / 'apps' / $app / rest of the route [, , , $app,] = explode('/', $url, 5); - $app = \OC_App::cleanAppId($app); + $app = $this->appManager->cleanAppId($app); \OC::$REQUESTEDAPP = $app; $this->loadRoutes($app); } elseif (str_starts_with($url, '/settings/')) { @@ -433,7 +437,11 @@ private function getAttributeRoutes(string $app): array { $appControllerPath = __DIR__ . '/../../../core/Controller'; $appNameSpace = 'OC\\Core'; } else { - $appControllerPath = \OC_App::getAppPath($app) . '/lib/Controller'; + try { + $appControllerPath = $this->appManager->getAppPath($app) . '/lib/Controller'; + } catch (AppPathNotFoundException) { + return []; + } $appNameSpace = App::buildAppNamespace($app); } diff --git a/lib/private/legacy/OC_App.php b/lib/private/legacy/OC_App.php index f48f4fd0b9892..d72d5fe8522cb 100644 --- a/lib/private/legacy/OC_App.php +++ b/lib/private/legacy/OC_App.php @@ -45,8 +45,7 @@ class OC_App { * @psalm-taint-escape html * @psalm-taint-escape has_quotes * - * @param string $app AppId that needs to be cleaned - * @return string + * @deprecated 31.0.0 use IAppManager::cleanAppId */ public static function cleanAppId(string $app): string { return str_replace(['<', '>', '"', "'", '\0', '/', '\\', '..'], '', $app); diff --git a/lib/public/App/IAppManager.php b/lib/public/App/IAppManager.php index b735b0d7c642b..68c3cc771f450 100644 --- a/lib/public/App/IAppManager.php +++ b/lib/public/App/IAppManager.php @@ -141,12 +141,10 @@ public function disableApp($appId, $automaticDisabled = false); /** * Get the directory for the given app. * - * @param string $appId - * @return string * @since 11.0.0 * @throws AppPathNotFoundException */ - public function getAppPath($appId); + public function getAppPath(string $appId): string; /** * Get the web path for the given app. @@ -282,4 +280,16 @@ public function setDefaultApps(array $defaultApps): void; * @since 30.0.0 */ public function isBackendRequired(string $backend): bool; + + /** + * Clean the appId from forbidden characters + * + * @psalm-taint-escape file + * @psalm-taint-escape include + * @psalm-taint-escape html + * @psalm-taint-escape has_quotes + * + * @since 31.0.0 + */ + public function cleanAppId(string $app): string; } diff --git a/remote.php b/remote.php index 1cdb74c4139a9..9b56e6c97f95e 100644 --- a/remote.php +++ b/remote.php @@ -8,6 +8,7 @@ require_once __DIR__ . '/lib/versioncheck.php'; use OCA\DAV\Connector\Sabre\ExceptionLoggerPlugin; +use OCP\App\IAppManager; use Psr\Log\LoggerInterface; use Sabre\DAV\Exception\ServiceUnavailable; use Sabre\DAV\Server; @@ -20,10 +21,7 @@ class RemoteException extends \Exception { } -/** - * @param Exception|Error $e - */ -function handleException($e) { +function handleException(Exception|Error $e): void { try { $request = \OC::$server->getRequest(); // in case the request content type is text/xml - we assume it's a WebDAV request @@ -126,20 +124,21 @@ function resolveService($service) { // Load all required applications \OC::$REQUESTEDAPP = $app; - OC_App::loadApps(['authentication']); - OC_App::loadApps(['extended_authentication']); - OC_App::loadApps(['filesystem', 'logging']); + $appManager = \OCP\Server::get(IAppManager::class); + $appManager->loadApps(['authentication']); + $appManager->loadApps(['extended_authentication']); + $appManager->loadApps(['filesystem', 'logging']); switch ($app) { case 'core': $file = OC::$SERVERROOT .'/'. $file; break; default: - if (!\OC::$server->getAppManager()->isInstalled($app)) { + if (!$appManager->isInstalled($app)) { throw new RemoteException('App not installed: ' . $app); } - OC_App::loadApp($app); - $file = OC_App::getAppPath($app) .'/'. $parts[1]; + $appManager->loadApp($app); + $file = $appManager->getAppPath($app) .'/'. ($parts[1] ?? ''); break; } $baseuri = OC::$WEBROOT . '/remote.php/'.$service.'/'; diff --git a/tests/apps.php b/tests/apps.php index c01e364c0a8f2..c2015fecda96a 100644 --- a/tests/apps.php +++ b/tests/apps.php @@ -44,10 +44,15 @@ function getSubclasses($parentClassName): array { } $apps = OC_App::getEnabledApps(); +$appManager = \OCP\Server::get(\OCP\App\IAppManager::class); foreach ($apps as $app) { - $dir = OC_App::getAppPath($app); - if (is_dir($dir . '/tests')) { - loadDirectory($dir . '/tests'); + try { + $dir = $appManager->getAppPath($app); + if (is_dir($dir . '/tests')) { + loadDirectory($dir . '/tests'); + } + } catch (\OCP\App\AppPathNotFoundException) { + /* ignore */ } } diff --git a/tests/lib/App/AppManagerTest.php b/tests/lib/App/AppManagerTest.php index 82a3f0d204517..ac470c0033520 100644 --- a/tests/lib/App/AppManagerTest.php +++ b/tests/lib/App/AppManagerTest.php @@ -341,7 +341,7 @@ public function testEnableAppForGroupsAllowedTypes(array $appInfo) { $manager->expects($this->once()) ->method('getAppPath') ->with('test') - ->willReturn(null); + ->willReturn(''); $manager->expects($this->once()) ->method('getAppInfo') @@ -402,7 +402,7 @@ public function testEnableAppForGroupsForbiddenTypes($type) { $manager->expects($this->once()) ->method('getAppPath') ->with('test') - ->willReturn(null); + ->willReturn(''); $manager->expects($this->once()) ->method('getAppInfo') diff --git a/tests/lib/InfoXmlTest.php b/tests/lib/InfoXmlTest.php index 702eca4c0ce1a..1527e363bd715 100644 --- a/tests/lib/InfoXmlTest.php +++ b/tests/lib/InfoXmlTest.php @@ -7,6 +7,7 @@ namespace Test; use OCP\App\IAppManager; +use OCP\Server; /** * Class InfoXmlTest @@ -15,6 +16,13 @@ * @package Test */ class InfoXmlTest extends TestCase { + private IAppManager $appManager; + + protected function setUp(): void { + parent::setUp(); + $this->appManager = Server::get(IAppManager::class); + } + public function dataApps() { return [ ['admin_audit'], @@ -45,8 +53,8 @@ public function dataApps() { * @param string $app */ public function testClasses($app) { - $appInfo = \OCP\Server::get(IAppManager::class)->getAppInfo($app); - $appPath = \OC_App::getAppPath($app); + $appInfo = $this->appManager->getAppInfo($app); + $appPath = $this->appManager->getAppPath($app); \OC_App::registerAutoloading($app, $appPath); //Add the appcontainer From 76f2bc0bfc86a9d1ed34d37c574c7e7a327c0fab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 12 Sep 2024 16:38:35 +0200 Subject: [PATCH 2/3] fix: Replace OC_App::getAllApps with a method in AppManager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- core/Command/App/Enable.php | 2 +- core/Command/App/GetPath.php | 2 +- core/Command/App/Update.php | 2 +- .../Command/Db/Migrations/GenerateCommand.php | 13 +++----- .../Db/Migrations/GenerateMetadataCommand.php | 2 +- core/Command/L10n/CreateJs.php | 2 +- lib/private/App/AppManager.php | 31 +++++++++++++++++++ lib/private/IntegrityCheck/Checker.php | 4 +-- .../IntegrityCheck/Helpers/AppLocator.php | 9 ------ lib/private/Server.php | 2 +- lib/private/legacy/OC_App.php | 28 +++-------------- lib/public/App/IAppManager.php | 8 +++++ tests/lib/IntegrityCheck/CheckerTest.php | 4 +-- .../IntegrityCheck/Helpers/AppLocatorTest.php | 4 --- 14 files changed, 58 insertions(+), 55 deletions(-) diff --git a/core/Command/App/Enable.php b/core/Command/App/Enable.php index b351a14c39e53..5366230b84158 100644 --- a/core/Command/App/Enable.php +++ b/core/Command/App/Enable.php @@ -145,7 +145,7 @@ public function completeOptionValues($optionName, CompletionContext $context): a */ public function completeArgumentValues($argumentName, CompletionContext $context): array { if ($argumentName === 'app-id') { - $allApps = \OC_App::getAllApps(); + $allApps = $this->appManager->getAllAppsInAppsFolders(); return array_diff($allApps, \OC_App::getEnabledApps(true, true)); } return []; diff --git a/core/Command/App/GetPath.php b/core/Command/App/GetPath.php index 442af2f35705b..3ba4ed7781b1b 100644 --- a/core/Command/App/GetPath.php +++ b/core/Command/App/GetPath.php @@ -63,7 +63,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int */ public function completeArgumentValues($argumentName, CompletionContext $context): array { if ($argumentName === 'app') { - return \OC_App::getAllApps(); + return $this->appManager->getAllAppsInAppsFolders(); } return []; } diff --git a/core/Command/App/Update.php b/core/Command/App/Update.php index b4018c94b7969..b2d02e222de61 100644 --- a/core/Command/App/Update.php +++ b/core/Command/App/Update.php @@ -69,7 +69,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int return 1; } } elseif ($input->getOption('all') || $input->getOption('showonly')) { - $apps = \OC_App::getAllApps(); + $apps = $this->manager->getAllAppsInAppsFolders(); } else { $output->writeln('Please specify an app to update or "--all" to update all updatable apps"'); return 1; diff --git a/core/Command/Db/Migrations/GenerateCommand.php b/core/Command/Db/Migrations/GenerateCommand.php index 62d5ad201ede2..cd92dc5acd60d 100644 --- a/core/Command/Db/Migrations/GenerateCommand.php +++ b/core/Command/Db/Migrations/GenerateCommand.php @@ -71,13 +71,10 @@ public function postSchemaChange(IOutput $output, Closure $schemaClosure, array } '; - protected Connection $connection; - protected IAppManager $appManager; - - public function __construct(Connection $connection, IAppManager $appManager) { - $this->connection = $connection; - $this->appManager = $appManager; - + public function __construct( + protected Connection $connection, + protected IAppManager $appManager, + ) { parent::__construct(); } @@ -155,7 +152,7 @@ public function completeOptionValues($optionName, CompletionContext $context) { */ public function completeArgumentValues($argumentName, CompletionContext $context) { if ($argumentName === 'app') { - $allApps = \OC_App::getAllApps(); + $allApps = $this->appManager->getAllAppsInAppsFolders(); return array_diff($allApps, \OC_App::getEnabledApps(true, true)); } diff --git a/core/Command/Db/Migrations/GenerateMetadataCommand.php b/core/Command/Db/Migrations/GenerateMetadataCommand.php index 55a2a6aedab91..ae83f92b29e8c 100644 --- a/core/Command/Db/Migrations/GenerateMetadataCommand.php +++ b/core/Command/Db/Migrations/GenerateMetadataCommand.php @@ -64,7 +64,7 @@ private function extractMigrationMetadataFromCore(): array { * @throws \Exception */ private function extractMigrationMetadataFromApps(): array { - $allApps = \OC_App::getAllApps(); + $allApps = $this->appManager->getAllAppsInAppsFolders(); $metadata = []; foreach ($allApps as $appId) { // We need to load app before being able to extract Migrations diff --git a/core/Command/L10n/CreateJs.php b/core/Command/L10n/CreateJs.php index 27d537b093ec8..7d45fbe91f891 100644 --- a/core/Command/L10n/CreateJs.php +++ b/core/Command/L10n/CreateJs.php @@ -148,7 +148,7 @@ public function completeOptionValues($optionName, CompletionContext $context) { */ public function completeArgumentValues($argumentName, CompletionContext $context) { if ($argumentName === 'app') { - return \OC_App::getAllApps(); + return $this->appManager->getAllAppsInAppsFolders(); } elseif ($argumentName === 'lang') { $appName = $context->getWordAtIndex($context->getWordIndex() - 1); try { diff --git a/lib/private/App/AppManager.php b/lib/private/App/AppManager.php index 974545cfe92d2..4ffddef98c324 100644 --- a/lib/private/App/AppManager.php +++ b/lib/private/App/AppManager.php @@ -155,6 +155,37 @@ public function getInstalledApps() { return array_keys($this->getInstalledAppsValues()); } + /** + * Get a list of all apps in the apps folder + * + * @return list an array of app names (string IDs) + */ + public function getAllAppsInAppsFolders(): array { + $apps = []; + + foreach (\OC::$APPSROOTS as $apps_dir) { + if (!is_readable($apps_dir['path'])) { + $this->logger->warning('unable to read app folder : ' . $apps_dir['path'], ['app' => 'core']); + continue; + } + $dh = opendir($apps_dir['path']); + + if (is_resource($dh)) { + while (($file = readdir($dh)) !== false) { + if ( + $file[0] != '.' && + is_dir($apps_dir['path'] . '/' . $file) && + is_file($apps_dir['path'] . '/' . $file . '/appinfo/info.xml') + ) { + $apps[] = $file; + } + } + } + } + + return array_values(array_unique($apps)); + } + /** * List all apps enabled for a user * diff --git a/lib/private/IntegrityCheck/Checker.php b/lib/private/IntegrityCheck/Checker.php index d38ccf497f4c1..6443c43d210b4 100644 --- a/lib/private/IntegrityCheck/Checker.php +++ b/lib/private/IntegrityCheck/Checker.php @@ -46,7 +46,7 @@ public function __construct( private ?IConfig $config, private ?IAppConfig $appConfig, ICacheFactory $cacheFactory, - private ?IAppManager $appManager, + private IAppManager $appManager, private IMimeTypeDetector $mimeTypeDetector, ) { $this->cache = $cacheFactory->createDistributed(self::CACHE_KEY); @@ -536,7 +536,7 @@ public function verifyCoreSignature(): array { public function runInstanceVerification() { $this->cleanResults(); $this->verifyCoreSignature(); - $appIds = $this->appLocator->getAllApps(); + $appIds = $this->appManager->getAllAppsInAppsFolders(); foreach ($appIds as $appId) { // If an application is shipped a valid signature is required $isShipped = $this->appManager->isShipped($appId); diff --git a/lib/private/IntegrityCheck/Helpers/AppLocator.php b/lib/private/IntegrityCheck/Helpers/AppLocator.php index 3da5cc1322796..148a3aeda76a9 100644 --- a/lib/private/IntegrityCheck/Helpers/AppLocator.php +++ b/lib/private/IntegrityCheck/Helpers/AppLocator.php @@ -30,13 +30,4 @@ public function getAppPath(string $appId): string { } return $path; } - - /** - * Providers \OC_App::getAllApps() - * - * @return array - */ - public function getAllApps(): array { - return \OC_App::getAllApps(); - } } diff --git a/lib/private/Server.php b/lib/private/Server.php index b0ccb0f4b4d8c..c514a4b93ff0a 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -954,10 +954,10 @@ public function __construct($webRoot, \OC\Config $config) { if (\OC::$server->get(SystemConfig::class)->getValue('installed', false)) { $config = $c->get(\OCP\IConfig::class); $appConfig = $c->get(\OCP\IAppConfig::class); - $appManager = $c->get(IAppManager::class); } else { $config = $appConfig = $appManager = null; } + $appManager = $c->get(IAppManager::class); return new Checker( new EnvironmentHelper(), diff --git a/lib/private/legacy/OC_App.php b/lib/private/legacy/OC_App.php index d72d5fe8522cb..ef84d35d7bca3 100644 --- a/lib/private/legacy/OC_App.php +++ b/lib/private/legacy/OC_App.php @@ -468,30 +468,10 @@ public static function getAlternativeLogIns(): array { * get a list of all apps in the apps folder * * @return string[] an array of app names (string IDs) - * @todo: change the name of this method to getInstalledApps, which is more accurate + * @deprecated 31.0.0 Use IAppManager::getAllAppsInAppsFolders instead */ public static function getAllApps(): array { - $apps = []; - - foreach (OC::$APPSROOTS as $apps_dir) { - if (!is_readable($apps_dir['path'])) { - \OCP\Server::get(LoggerInterface::class)->warning('unable to read app folder : ' . $apps_dir['path'], ['app' => 'core']); - continue; - } - $dh = opendir($apps_dir['path']); - - if (is_resource($dh)) { - while (($file = readdir($dh)) !== false) { - if ($file[0] != '.' and is_dir($apps_dir['path'] . '/' . $file) and is_file($apps_dir['path'] . '/' . $file . '/appinfo/info.xml')) { - $apps[] = $file; - } - } - } - } - - $apps = array_unique($apps); - - return $apps; + return \OCP\Server::get(IAppManager::class)->getAllAppsInAppsFolders(); } /** @@ -512,9 +492,9 @@ public function getSupportedApps(): array { * @return array */ public function listAllApps(): array { - $installedApps = OC_App::getAllApps(); - $appManager = \OC::$server->getAppManager(); + + $installedApps = $appManager->getAllAppsInAppsFolders(); //we don't want to show configuration for these $blacklist = $appManager->getAlwaysEnabledApps(); $appList = []; diff --git a/lib/public/App/IAppManager.php b/lib/public/App/IAppManager.php index 68c3cc771f450..580288fbff7d5 100644 --- a/lib/public/App/IAppManager.php +++ b/lib/public/App/IAppManager.php @@ -292,4 +292,12 @@ public function isBackendRequired(string $backend): bool; * @since 31.0.0 */ public function cleanAppId(string $app): string; + + /** + * Get a list of all apps in the apps folder + * + * @return list an array of app names (string IDs) + * @since 31.0.0 + */ + public function getAllAppsInAppsFolders(): array; } diff --git a/tests/lib/IntegrityCheck/CheckerTest.php b/tests/lib/IntegrityCheck/CheckerTest.php index bf4ea16f564c7..05607f8113ce4 100644 --- a/tests/lib/IntegrityCheck/CheckerTest.php +++ b/tests/lib/IntegrityCheck/CheckerTest.php @@ -1030,9 +1030,9 @@ public function testRunInstanceVerification() { $this->checker ->expects($this->once()) ->method('verifyCoreSignature'); - $this->appLocator + $this->appManager ->expects($this->once()) - ->method('getAllApps') + ->method('getAllAppsInAppsFolders') ->willReturn([ 'files', 'calendar', diff --git a/tests/lib/IntegrityCheck/Helpers/AppLocatorTest.php b/tests/lib/IntegrityCheck/Helpers/AppLocatorTest.php index a86507f6bb2bc..d99a42e0d6389 100644 --- a/tests/lib/IntegrityCheck/Helpers/AppLocatorTest.php +++ b/tests/lib/IntegrityCheck/Helpers/AppLocatorTest.php @@ -30,8 +30,4 @@ public function testGetAppPathNotExistentApp() { $this->locator->getAppPath('aTotallyNotExistingApp'); } - - public function testGetAllApps() { - $this->assertSame(\OC_App::getAllApps(), $this->locator->getAllApps()); - } } From 7a16d01ea79bf8effedd5e0e9ac7c2e4b4ea41f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Fri, 13 Sep 2024 10:26:36 +0200 Subject: [PATCH 3/3] chore(tests): Fix Router test by mocking AppManager methods correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- tests/lib/Route/RouterTest.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/lib/Route/RouterTest.php b/tests/lib/Route/RouterTest.php index 713d90d3c203d..8647bb0f2f6bd 100644 --- a/tests/lib/Route/RouterTest.php +++ b/tests/lib/Route/RouterTest.php @@ -13,6 +13,7 @@ use OCP\Diagnostics\IEventLogger; use OCP\IConfig; use OCP\IRequest; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -26,6 +27,8 @@ */ class RouterTest extends TestCase { private Router $router; + private IAppManager&MockObject $appManager; + protected function setUp(): void { parent::setUp(); /** @var LoggerInterface $logger */ @@ -36,13 +39,16 @@ function (string $message, array $data) { $this->fail('Unexpected info log: '.(string)($data['exception'] ?? $message)); } ); + + $this->appManager = $this->createMock(IAppManager::class); + $this->router = new Router( $logger, $this->createMock(IRequest::class), $this->createMock(IConfig::class), $this->createMock(IEventLogger::class), $this->createMock(ContainerInterface::class), - $this->createMock(IAppManager::class), + $this->appManager, ); } @@ -51,6 +57,12 @@ public function testHeartbeat(): void { } public function testGenerateConsecutively(): void { + $this->appManager->expects(self::atLeastOnce()) + ->method('cleanAppId') + ->willReturnArgument(0); + $this->appManager->expects(self::atLeastOnce()) + ->method('getAppPath') + ->willReturnCallback(fn (string $appid): string => \OC::$SERVERROOT . '/apps/' . $appid); $this->assertEquals('/index.php/apps/files/', $this->router->generate('files.view.index'));