From b475d9c373087c540ea360fdac5b5479262f5b94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Anne?= Date: Mon, 28 Oct 2024 10:05:09 +0100 Subject: [PATCH] Increase default firewall strategy for plugin legacy scripts --- dependency_injection/services.php | 8 - phpunit/functional/Glpi/Http/FirewallTest.php | 256 ++++++++++++++++-- src/Glpi/Http/Firewall.php | 178 ++++++------ src/Glpi/Http/FirewallInterface.php | 54 ---- src/Glpi/Http/FirewallStrategyListener.php | 4 +- 5 files changed, 333 insertions(+), 167 deletions(-) delete mode 100644 src/Glpi/Http/FirewallInterface.php diff --git a/dependency_injection/services.php b/dependency_injection/services.php index 31a3133cc5f..670e807bbc8 100644 --- a/dependency_injection/services.php +++ b/dependency_injection/services.php @@ -35,8 +35,6 @@ namespace Symfony\Component\DependencyInjection\Loader\Configurator; use Glpi\DependencyInjection\PublicService; -use Glpi\Http\Firewall; -use Glpi\Http\FirewallInterface; use Glpi\Log\LegacyGlobalLogger; return static function (ContainerConfigurator $container): void { @@ -61,12 +59,6 @@ $services->load('Glpi\Http\\', $projectDir . '/src/Glpi/Http'); $services->load('Glpi\DependencyInjection\\', $projectDir . '/src/Glpi/DependencyInjection'); - $services->set(Firewall::class) - ->factory([Firewall::class, 'createDefault']) - ->tag('proxy', ['interface' => FirewallInterface::class]) - ->lazy() - ; - /** * Override Symfony's logger. * @see \Symfony\Component\HttpKernel\DependencyInjection\LoggerPass diff --git a/phpunit/functional/Glpi/Http/FirewallTest.php b/phpunit/functional/Glpi/Http/FirewallTest.php index f131da4df87..48ada4fda7f 100644 --- a/phpunit/functional/Glpi/Http/FirewallTest.php +++ b/phpunit/functional/Glpi/Http/FirewallTest.php @@ -34,12 +34,17 @@ namespace tests\units\Glpi\Http; -use org\bovigo\vfs\vfsStream; +use Glpi\Exception\Http\AccessDeniedHttpException; +use Glpi\Exception\SessionExpiredException; use Glpi\Http\Firewall; +use KnowbaseItem; +use org\bovigo\vfs\vfsStream; +use PHPUnit\Framework\Attributes\DataProvider; +use Symfony\Component\HttpFoundation\Request; -class FirewallTest extends \GLPITestCase +class FirewallTest extends \DbTestCase { - public function testComputeFallbackStrategy() + public function testComputeFallbackStrategy(): void { vfsStream::setup( 'glpi', @@ -113,7 +118,7 @@ public function testComputeFallbackStrategy() ); $default_for_core_legacy = 'authenticated'; - $default_for_plugins_legacy = 'no_check'; + $default_for_plugins_legacy = 'authenticated'; $default_for_symfony_routes = 'central_access'; $default_mapping = [ @@ -137,26 +142,17 @@ public function testComputeFallbackStrategy() '/myplugindir/pluginb/Route/To/Something' => $default_for_symfony_routes, ]; - foreach ($default_mapping as $path => $expected_strategy) { - $this->dotestComputeFallbackStrategy( - root_doc: '', - path: $path, - expected_strategy: $expected_strategy, - ); - $this->dotestComputeFallbackStrategy( - root_doc: '/glpi', - path: '/glpi' . $path, - expected_strategy: $expected_strategy, - ); - $this->dotestComputeFallbackStrategy( - root_doc: '/path/to/app', - path: '/path/to/app' . $path, - expected_strategy: $expected_strategy, - ); - } - - // Hardcoded strategies foreach (['', '/glpi', '/path/to/app'] as $root_doc) { + // Default strategies + foreach ($default_mapping as $path => $expected_strategy) { + $this->dotestComputeFallbackStrategy( + root_doc: $root_doc, + path: $root_doc . $path, + expected_strategy: $expected_strategy, + ); + } + + // Hardcoded strategies // `/front/central.php` has a specific strategy only if some get parameters are defined $this->dotestComputeFallbackStrategy( root_doc: $root_doc, @@ -220,6 +216,47 @@ public function testComputeFallbackStrategy() expected_strategy: 'no_check', ); } + + // Specific strategies defined by plugins + Firewall::addPluginStrategyForLegacyScripts('myplugin', '#^.*/foo.php#', 'faq_access'); + $this->dotestComputeFallbackStrategy( + root_doc: $root_doc, + path: $root_doc . '/marketplace/myplugin/ajax/foo.php', + expected_strategy: 'faq_access', + ); + $this->dotestComputeFallbackStrategy( + root_doc: $root_doc, + path: $root_doc . '/marketplace/myplugin/front/foo.php', + expected_strategy: 'faq_access', + ); + $this->dotestComputeFallbackStrategy( + root_doc: $root_doc, + path: $root_doc . '/marketplace/myplugin/front/dir/bar.php', + expected_strategy: $default_for_plugins_legacy, // does not match the pattern + ); + Firewall::addPluginStrategyForLegacyScripts('myplugin', '#^/front/dir/#', 'helpdesk_access'); + $this->dotestComputeFallbackStrategy( + root_doc: $root_doc, + path: $root_doc . '/marketplace/myplugin/ajax/foo.php', + expected_strategy: 'faq_access', + ); + $this->dotestComputeFallbackStrategy( + root_doc: $root_doc, + path: $root_doc . '/marketplace/myplugin/front/foo.php', + expected_strategy: 'faq_access', + ); + $this->dotestComputeFallbackStrategy( + root_doc: $root_doc, + path: $root_doc . '/marketplace/myplugin/front/dir/bar.php', + expected_strategy: 'helpdesk_access', + ); + Firewall::addPluginStrategyForLegacyScripts('myplugin', '#^/PluginRoute$#', 'helpdesk_access'); + $this->dotestComputeFallbackStrategy( + root_doc: $root_doc, + path: $root_doc . '/marketplace/myplugin/PluginRoute', + expected_strategy: $default_for_symfony_routes, // fallback strategies MUST NOT apply to symfony routes + ); + Firewall::resetPluginsStrategies(); } } @@ -227,15 +264,182 @@ private function dotestComputeFallbackStrategy( string $root_doc, string $path, string $expected_strategy - ) { + ): void { $instance = new Firewall( - $root_doc, vfsStream::url('glpi'), [vfsStream::url('glpi/myplugindir'), vfsStream::url('glpi/marketplace')] ); + + $request = new Request(); + $request->server->set('SCRIPT_FILENAME', $root_doc . '/index.php'); + $request->server->set('SCRIPT_NAME', $root_doc . '/index.php'); + $request->server->set('REQUEST_URI', $path); + $this->assertEquals( $expected_strategy, - $instance->computeFallbackStrategy($path) + $instance->computeFallbackStrategy($request), + $path ); } + + public static function provideStrategy(): iterable + { + yield ['strategy' => Firewall::STRATEGY_AUTHENTICATED]; + yield ['strategy' => Firewall::STRATEGY_CENTRAL_ACCESS]; + yield ['strategy' => Firewall::STRATEGY_FAQ_ACCESS]; + yield ['strategy' => Firewall::STRATEGY_HELPDESK_ACCESS]; + } + + #[DataProvider('provideStrategy')] + public function testApplyStrategyWhenLoggedOut(string $strategy): void + { + $this->expectException(SessionExpiredException::class); + + $instance = new Firewall(); + $instance->applyStrategy($strategy); + } + + #[DataProvider('provideStrategy')] + public function testApplyStrategyWhenSessionIsCorrupted(string $strategy): void + { + $this->login(); + + $_SESSION = []; + + $this->expectException(SessionExpiredException::class); + + $instance = new Firewall(); + $instance->applyStrategy($strategy); + } + + public static function provideStrategyResults(): iterable + { + $central_users = [ + TU_USER => TU_PASS, + 'glpi' => 'glpi', + 'tech' => 'tech', + 'normal' => 'normal', + ]; + + foreach ($central_users as $login => $pass) { + yield [ + 'strategy' => Firewall::STRATEGY_AUTHENTICATED, + 'credentials' => [$login, $pass], + 'exception' => null, + ]; + yield [ + 'strategy' => Firewall::STRATEGY_CENTRAL_ACCESS, + 'credentials' => [$login, $pass], + 'exception' => null, + ]; + yield [ + 'strategy' => Firewall::STRATEGY_FAQ_ACCESS, + 'credentials' => [$login, $pass], + 'exception' => null, + ]; + yield [ + 'strategy' => Firewall::STRATEGY_HELPDESK_ACCESS, + 'credentials' => [$login, $pass], + 'exception' => new AccessDeniedHttpException('The current profile does not use the simplified interface'), + ]; + } + + $helpdesk_users = [ + 'post-only' => 'postonly', + ]; + foreach ($helpdesk_users as $login => $pass) { + yield [ + 'strategy' => Firewall::STRATEGY_AUTHENTICATED, + 'credentials' => [$login, $pass], + 'exception' => null, + ]; + yield [ + 'strategy' => Firewall::STRATEGY_CENTRAL_ACCESS, + 'credentials' => [$login, $pass], + 'exception' => new AccessDeniedHttpException('The current profile does not use the standard interface'), + ]; + yield [ + 'strategy' => Firewall::STRATEGY_FAQ_ACCESS, + 'credentials' => [$login, $pass], + 'exception' => null, + ]; + yield [ + 'strategy' => Firewall::STRATEGY_HELPDESK_ACCESS, + 'credentials' => [$login, $pass], + 'exception' => null, + ]; + } + } + + #[DataProvider('provideStrategyResults')] + public function testApplyStrategyWithUser(string $strategy, array $credentials, ?\Throwable $exception): void + { + $this->login(...$credentials); + + if ($exception !== null) { + $this->expectExceptionObject($exception); + } + + $instance = new Firewall(); + $instance->applyStrategy($strategy); + } + + public static function provideFaqAccessStrategyResults(): iterable + { + yield [ + 'use_public_faq' => false, + 'knowbase_rights' => KnowbaseItem::READFAQ, + 'exception' => null, + ]; + + yield [ + 'use_public_faq' => false, + 'knowbase_rights' => READ, + 'exception' => null, + ]; + + yield [ + 'use_public_faq' => false, + 'knowbase_rights' => 0, + 'exception' => new AccessDeniedHttpException('Missing FAQ right'), + ]; + + yield [ + 'use_public_faq' => true, + 'knowbase_rights' => KnowbaseItem::READFAQ, + 'exception' => null, + ]; + + yield [ + 'use_public_faq' => true, + 'knowbase_rights' => READ, + 'exception' => null, + ]; + + yield [ + 'use_public_faq' => true, + 'knowbase_rights' => 0, + 'exception' => null, + ]; + } + + #[DataProvider('provideFaqAccessStrategyResults')] + public function testApplyStrategyFaqAccess(bool $use_public_faq, int $knowbase_rights, ?\Throwable $exception): void + { + /** @var array $CFG_GLPI */ + global $CFG_GLPI; + + $this->login(); + + $CFG_GLPI['use_public_faq'] = $use_public_faq; + + $_SESSION['glpiactiveprofile']['knowbase'] = $knowbase_rights; + + if ($exception !== null) { + $this->expectExceptionObject($exception); + } + + $instance = new Firewall(); + $instance->applyStrategy(Firewall::STRATEGY_FAQ_ACCESS); + } } diff --git a/src/Glpi/Http/Firewall.php b/src/Glpi/Http/Firewall.php index a4de696d4e9..447aec61b9c 100644 --- a/src/Glpi/Http/Firewall.php +++ b/src/Glpi/Http/Firewall.php @@ -35,11 +35,12 @@ namespace Glpi\Http; use Session; +use Symfony\Component\HttpFoundation\Request; /** * @since 10.0.10 */ -final class Firewall implements FirewallInterface +final class Firewall { /** * Nothing to check. Entrypoint accepts anonymous access. @@ -67,32 +68,17 @@ final class Firewall implements FirewallInterface public const STRATEGY_FAQ_ACCESS = 'faq_access'; /** - * Default strategy to apply (except for legacy scripts). + * Fallback strategy to apply (except for legacy scripts). */ - private const STRATEGY_DEFAULT = self::STRATEGY_CENTRAL_ACCESS; + private const FALLBACK_STRATEGY = self::STRATEGY_CENTRAL_ACCESS; /** - * Security strategy to apply by default on core legacy scripts. + * Fallback strategy to apply to legacy scripts. */ - private const STRATEGY_DEFAULT_FOR_CORE_LEGACY = self::STRATEGY_AUTHENTICATED; - - /** - * Security strategy to apply by default on plugins legacy scripts. - * - * @TODO In GLPI 11.0, raise default level to `self::STRATEGY_AUTHENTICATED`. - * It requires to give to plugins the ability to define a specific strategy for legacy files. - */ - private const STRATEGY_DEFAULT_FOR_PLUGINS_LEGACY = self::STRATEGY_NO_CHECK; - - /** - * GLPI URLs path prefix. - * @var string - */ - private string $path_prefix; + private const FALLBACK_STRATEGY_FOR_LEGACY_SCRIPTS = self::STRATEGY_AUTHENTICATED; /** * GLPI root directory. - * @var string */ private string $root_dir; @@ -103,27 +89,52 @@ final class Firewall implements FirewallInterface private array $plugins_dirs; /** - * @param string $path_prefix GLPI URLs path prefix - * @param ?string $root_dir GLPI root directory on filesystem - * @param ?array $plugins_dirs GLPI plugins root directories on filesystem + * Registered plugins strategies for legacy scripts. + * + * @phpstan-var array> + */ + private static array $plugins_legacy_scripts_strategies = []; + + /** + * @param ?string $root_dir GLPI root directory on filesystem + * @param ?array $plugins_dirs GLPI plugins root directories on filesystem */ - public function __construct(string $path_prefix, ?string $root_dir = null, ?array $plugins_dirs = null) + public function __construct(?string $root_dir = null, ?array $plugins_dirs = null) { - $this->path_prefix = $path_prefix; $this->root_dir = $root_dir ?? \GLPI_ROOT; $this->plugins_dirs = $plugins_dirs ?? \PLUGINS_DIRECTORIES; } - public static function createDefault(): self - { - /** - * @var array $CFG_GLPI - */ - global $CFG_GLPI; + /** + * Add a security strategy for specific plugin legacy scripts. + * + * @param string $plugin_key The plugin key. + * @param string $pattern The resource pattern, relative to the plugin root URI (e.g. `#^/front/api.php/#`). + * @param string $strategy The strategy to apply. + * + * @phpstan-param self::STRATEGY_* $strategy + */ + public static function addPluginStrategyForLegacyScripts( + string $plugin_key, + string $pattern, + string $strategy + ): void { + self::$plugins_legacy_scripts_strategies[$plugin_key][$pattern] = $strategy; + } - return new Firewall($CFG_GLPI['root_doc']); + /** + * Reset strategies for all plugins. + */ + public static function resetPluginsStrategies(): void + { + self::$plugins_legacy_scripts_strategies = []; } + /** + * Apply the firewall strategy. + * + * @phpstan-param self::STRATEGY_* $strategy + */ public function applyStrategy(string $strategy): void { switch ($strategy) { @@ -147,61 +158,39 @@ public function applyStrategy(string $strategy): void } } - public function computeFallbackStrategy(string $path): string + /** + * Compute the fallback strategy for given path. + */ + public function computeFallbackStrategy(Request $request): string { - $unprefixed_path = preg_replace('/^' . preg_quote($this->path_prefix, '/') . '/', '', $path); - - // Check if endpoint is a plugin endpoint - $is_plugin_endpoint = false; - foreach ($this->plugins_dirs as $plugins_dir) { - $relative_path = preg_replace( - '/^' . preg_quote($this->normalizePath($this->root_dir), '/') . '/', - '', - $this->normalizePath($plugins_dir) - ); + $unprefixed_path = preg_replace( + '/^' . preg_quote($request->getBasePath(), '/') . '/', + '', + $request->getPathInfo() + ); - if (preg_match('/^' . preg_quote($relative_path, '/') . '\//', $unprefixed_path) === 1) { - $is_plugin_endpoint = true; - break; - } - } - - // Legacy script - if (file_exists($this->root_dir . $unprefixed_path)) { - return $is_plugin_endpoint - ? self::STRATEGY_DEFAULT_FOR_PLUGINS_LEGACY - : $this->computeStrategyForCoreLegacyScript($unprefixed_path); + $path_matches = []; + $plugin_path_pattern = '#^/(plugins|marketplace)/(?[^/]+)(?/.+)$#'; + if (preg_match($plugin_path_pattern, $unprefixed_path, $path_matches) === 1) { + return $this->computeFallbackStrategyForPlugin( + $path_matches['plugin_key'], + $path_matches['plugin_resource'] + ); } - // Modern controllers - return self::STRATEGY_DEFAULT; + return $this->computeFallbackStrategyForCore($unprefixed_path); } /** - * Normalize a path, to make comparisons and relative paths computation easier. - * - * @param string $path - * @return string + * Compute the fallback strategy for GLPI resources. */ - private function normalizePath(string $path): string + private function computeFallbackStrategyForCore(string $path): string { - $realpath = realpath($path); - if ($realpath !== false) { - // Use realpath if possible. - // As `realpath()` will return `false` on streams, we cannot always use it, or we will not be able to do unit tests on this method. - $path = $realpath; + if (!file_exists($this->root_dir . $path)) { + // Modern controllers + return self::FALLBACK_STRATEGY; } - // Normalize all directory separators to `/`. - $path = preg_replace('/\\\/', '/', $path); - return $path; - } - - /** - * Compute the strategy for GLPI legacy scripts. - */ - private function computeStrategyForCoreLegacyScript(string $path): string - { if (isset($_GET["embed"], $_GET["dashboard"]) && str_starts_with($path, '/front/central.php')) { // Allow anonymous access for embed dashboards. return 'no_check'; @@ -239,6 +228,41 @@ private function computeStrategyForCoreLegacyScript(string $path): string } } - return self::STRATEGY_DEFAULT_FOR_CORE_LEGACY; + return self::FALLBACK_STRATEGY_FOR_LEGACY_SCRIPTS; + } + + /** + * Compute the fallback strategy for plugins resources. + */ + private function computeFallbackStrategyForPlugin(string $plugin_key, string $plugin_resource): string + { + // Check if the file exists to apply the strategies related to legacyy scripts + foreach ($this->plugins_dirs as $plugin_dir) { + $expected_filenames = [ + $plugin_dir . '/' . $plugin_key . $plugin_resource, + ]; + $resource_matches = []; + if (\preg_match('#^(?.+\.php)(/.*)$#', $plugin_resource, $resource_matches)) { + // /front/api.php/path/to/endpoint -> /front/api.php + $expected_filenames[] = $plugin_dir . '/' . $plugin_key . $resource_matches['filename']; + } + + foreach ($expected_filenames as $expected_filename) { + if (\file_exists($expected_filename)) { + // Try to match a registered strategy + $strategies = self::$plugins_legacy_scripts_strategies[$plugin_key] ?? []; + foreach ($strategies as $pattern => $strategy) { + if (preg_match($pattern, $plugin_resource) === 1) { + return $strategy; + } + } + + return self::FALLBACK_STRATEGY_FOR_LEGACY_SCRIPTS; + } + } + } + + // Modern controllers + return self::FALLBACK_STRATEGY; } } diff --git a/src/Glpi/Http/FirewallInterface.php b/src/Glpi/Http/FirewallInterface.php deleted file mode 100644 index da807566cae..00000000000 --- a/src/Glpi/Http/FirewallInterface.php +++ /dev/null @@ -1,54 +0,0 @@ -. - * - * --------------------------------------------------------------------- - */ - -namespace Glpi\Http; - -interface FirewallInterface -{ - /** - * Apply the firewall strategy. - * - * @param string $strategy - * @return void - */ - public function applyStrategy(string $strategy): void; - - /** - * Compute the fallback strategy for given path. - * - * @param string $path - * @return string - */ - public function computeFallbackStrategy(string $path): string; -} diff --git a/src/Glpi/Http/FirewallStrategyListener.php b/src/Glpi/Http/FirewallStrategyListener.php index e57d81f9177..807379a20bd 100644 --- a/src/Glpi/Http/FirewallStrategyListener.php +++ b/src/Glpi/Http/FirewallStrategyListener.php @@ -41,7 +41,7 @@ final readonly class FirewallStrategyListener implements EventSubscriberInterface { - public function __construct(private FirewallInterface $firewall) + public function __construct(private Firewall $firewall) { } @@ -66,7 +66,7 @@ public function onKernelController(ControllerEvent $event): void } elseif ($number_of_attributes === 1) { $strategy = current($attributes)->strategy; } elseif ($event->isMainRequest()) { - $strategy = $this->firewall->computeFallbackStrategy($event->getRequest()->getPathInfo()); + $strategy = $this->firewall->computeFallbackStrategy($event->getRequest()); } if ($strategy !== null) {