From 33c19642a2d4c178c4c97bd517fac36761ac7978 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Anne?= Date: Mon, 18 Dec 2023 17:03:58 +0100 Subject: [PATCH] Prevent useless optional/out of context requirement checks --- .../Requirement/AbstractRequirement.php | 63 +++++++++++++++---- .../DataDirectoriesProtectedPath.php | 10 +-- src/System/Requirement/DbConfiguration.php | 5 +- src/System/Requirement/DbEngine.php | 5 +- src/System/Requirement/DbTimezones.php | 9 ++- .../Requirement/DirectoriesWriteAccess.php | 4 +- .../Requirement/DirectoryWriteAccess.php | 40 ++++++------ src/System/Requirement/Extension.php | 9 ++- src/System/Requirement/ExtensionConstant.php | 9 ++- src/System/Requirement/ExtensionGroup.php | 9 ++- .../Requirement/InstallationNotOverriden.php | 11 +++- src/System/Requirement/IntegerSize.php | 8 ++- src/System/Requirement/LogsWriteAccess.php | 5 +- src/System/Requirement/MemoryLimit.php | 5 +- .../Requirement/PhpSupportedVersion.php | 10 +-- src/System/Requirement/PhpVersion.php | 5 +- src/System/Requirement/ProtectedWebAccess.php | 10 ++- src/System/Requirement/SafeDocumentRoot.php | 16 ++--- src/System/Requirement/SeLinux.php | 9 ++- .../Requirement/SessionsConfiguration.php | 4 +- .../SessionsSecurityConfiguration.php | 12 ++-- src/System/RequirementsList.php | 4 +- 22 files changed, 177 insertions(+), 85 deletions(-) diff --git a/src/System/Requirement/AbstractRequirement.php b/src/System/Requirement/AbstractRequirement.php index 740de68e5f6..964065da452 100644 --- a/src/System/Requirement/AbstractRequirement.php +++ b/src/System/Requirement/AbstractRequirement.php @@ -50,35 +50,35 @@ abstract class AbstractRequirement implements RequirementInterface /** * Flag that indicates if requirement is considered as optional. * - * @var bool + * @var bool|null */ - protected $optional = false; + protected $optional; /** * Flag that indicates if requirement is recommended for security reasons. * - * @var bool + * @var bool|null */ - protected bool $recommended_for_security = false; + protected ?bool $recommended_for_security; /** * Flag that indicates if requirement is considered as out of context. * - * @var bool + * @var bool|null */ - protected $out_of_context = false; + protected $out_of_context; /** * Requirement title. * - * @var string + * @var string|null */ protected $title; /** * Requirement description. * - * @var string + * @var string|null */ protected $description; @@ -96,6 +96,20 @@ abstract class AbstractRequirement implements RequirementInterface */ protected $validation_messages = []; + public function __construct( + ?string $title, + ?string $description = null, + ?bool $optional = false, + ?bool $recommended_for_security = false, + ?bool $out_of_context = false + ) { + $this->title = $title; + $this->description = $description; + $this->optional = $optional; + $this->recommended_for_security = $recommended_for_security; + $this->out_of_context = $out_of_context; + } + /** * Check requirement. * @@ -121,13 +135,23 @@ private function doCheck() public function getTitle(): string { + if ($this->title !== null) { + // No need to run checks if variable is defined by constructor. + return $this->title; + } + $this->doCheck(); - return $this->title; + return $this->title ?? ''; } public function getDescription(): ?string { + if ($this->description !== null) { + // No need to run checks if variable is defined by constructor. + return $this->description; + } + $this->doCheck(); return $this->description; @@ -149,23 +173,38 @@ public function isMissing(): bool public function isOptional(): bool { + if ($this->optional !== null) { + // No need to run checks if variable is defined by constructor. + return $this->optional; + } + $this->doCheck(); - return $this->optional; + return $this->optional ?? false; } public function isRecommendedForSecurity(): bool { + if ($this->recommended_for_security !== null) { + // No need to run checks if variable is defined by constructor. + return $this->recommended_for_security; + } + $this->doCheck(); - return $this->recommended_for_security; + return $this->recommended_for_security ?? false; } public function isOutOfContext(): bool { + if ($this->out_of_context !== null) { + // No need to run checks if variable is defined by constructor. + return $this->out_of_context; + } + $this->doCheck(); - return $this->out_of_context; + return $this->out_of_context ?? false; } public function isValidated(): bool diff --git a/src/System/Requirement/DataDirectoriesProtectedPath.php b/src/System/Requirement/DataDirectoriesProtectedPath.php index 6d29bb50d40..506966680c6 100644 --- a/src/System/Requirement/DataDirectoriesProtectedPath.php +++ b/src/System/Requirement/DataDirectoriesProtectedPath.php @@ -71,10 +71,12 @@ public function __construct( string $var_root_constant = 'GLPI_VAR_DIR', string $glpi_root_directory = GLPI_ROOT ) { - $this->title = __('Safe path for data directories'); - $this->description = __('GLPI data directories should be placed outside web root directory. It can be achieved by redefining corresponding constants. See installation documentation for more details.'); - $this->optional = true; - $this->recommended_for_security = true; + parent::__construct( + __('Safe path for data directories'), + __('GLPI data directories should be placed outside web root directory. It can be achieved by redefining corresponding constants. See installation documentation for more details.'), + true, + true + ); $this->directories_constants = $directories_constants; $this->var_root_constant = $var_root_constant; diff --git a/src/System/Requirement/DbConfiguration.php b/src/System/Requirement/DbConfiguration.php index 7ed9611cc9e..82c121543b7 100644 --- a/src/System/Requirement/DbConfiguration.php +++ b/src/System/Requirement/DbConfiguration.php @@ -51,7 +51,10 @@ class DbConfiguration extends AbstractRequirement public function __construct(DBmysql $db) { - $this->title = __('DB configuration'); + parent::__construct( + __('DB configuration') + ); + $this->db = $db; } diff --git a/src/System/Requirement/DbEngine.php b/src/System/Requirement/DbEngine.php index 6fa934e3a26..13ce90e0434 100644 --- a/src/System/Requirement/DbEngine.php +++ b/src/System/Requirement/DbEngine.php @@ -49,7 +49,10 @@ class DbEngine extends AbstractRequirement public function __construct(\DBmysql $db) { - $this->title = __('DB engine version'); + parent::__construct( + __('DB engine version') + ); + $this->db = $db; } diff --git a/src/System/Requirement/DbTimezones.php b/src/System/Requirement/DbTimezones.php index f90c8e24501..76c2a32574d 100644 --- a/src/System/Requirement/DbTimezones.php +++ b/src/System/Requirement/DbTimezones.php @@ -49,10 +49,13 @@ class DbTimezones extends AbstractRequirement public function __construct(\DBmysql $db) { - $this->title = __('DB timezone data'); - $this->description = __('Enable usage of timezones.'); + parent::__construct( + __('DB timezone data'), + __('Enable usage of timezones.'), + true + ); + $this->db = $db; - $this->optional = true; } protected function check() diff --git a/src/System/Requirement/DirectoriesWriteAccess.php b/src/System/Requirement/DirectoriesWriteAccess.php index c44a7251b95..287c4ddf7d4 100644 --- a/src/System/Requirement/DirectoriesWriteAccess.php +++ b/src/System/Requirement/DirectoriesWriteAccess.php @@ -54,9 +54,9 @@ class DirectoriesWriteAccess extends AbstractRequirement */ public function __construct(string $title, array $paths, bool $optional = false) { - $this->title = $title; + parent::__construct($title, null, $optional); + $this->paths = $paths; - $this->optional = $optional; } protected function check() diff --git a/src/System/Requirement/DirectoryWriteAccess.php b/src/System/Requirement/DirectoryWriteAccess.php index f02bb7133af..56d43f8f3f6 100644 --- a/src/System/Requirement/DirectoryWriteAccess.php +++ b/src/System/Requirement/DirectoryWriteAccess.php @@ -54,57 +54,57 @@ class DirectoryWriteAccess extends AbstractRequirement */ public function __construct(string $path, bool $optional = false, ?string $description = null) { - $this->path = $path; - $this->optional = $optional; - $this->description = $description; - - switch (realpath($this->path)) { + switch (realpath($path)) { case realpath(GLPI_CACHE_DIR): - $this->title = __('Permissions for cache files'); + $title = __('Permissions for cache files'); break; case realpath(GLPI_CONFIG_DIR): - $this->title = __('Permissions for setting files'); + $title = __('Permissions for setting files'); break; case realpath(GLPI_CRON_DIR): - $this->title = __('Permissions for automatic actions files'); + $title = __('Permissions for automatic actions files'); break; case realpath(GLPI_DOC_DIR): - $this->title = __('Permissions for document files'); + $title = __('Permissions for document files'); break; case realpath(GLPI_DUMP_DIR): - $this->title = __('Permissions for dump files'); + $title = __('Permissions for dump files'); break; case realpath(GLPI_GRAPH_DIR): - $this->title = __('Permissions for graphic files'); + $title = __('Permissions for graphic files'); break; case realpath(GLPI_LOCK_DIR): - $this->title = __('Permissions for lock files'); + $title = __('Permissions for lock files'); break; case realpath(GLPI_MARKETPLACE_DIR): - $this->title = __('Permissions for marketplace directory'); + $title = __('Permissions for marketplace directory'); break; case realpath(GLPI_PLUGIN_DOC_DIR): - $this->title = __('Permissions for plugins document files'); + $title = __('Permissions for plugins document files'); break; case realpath(GLPI_PICTURE_DIR): - $this->title = __('Permissions for pictures files'); + $title = __('Permissions for pictures files'); break; case realpath(GLPI_RSS_DIR): - $this->title = __('Permissions for rss files'); + $title = __('Permissions for rss files'); break; case realpath(GLPI_SESSION_DIR): - $this->title = __('Permissions for session files'); + $title = __('Permissions for session files'); break; case realpath(GLPI_TMP_DIR): - $this->title = __('Permissions for temporary files'); + $title = __('Permissions for temporary files'); break; case realpath(GLPI_UPLOAD_DIR): - $this->title = __('Permissions for upload files'); + $title = __('Permissions for upload files'); break; default: - $this->title = sprintf(__('Permissions for directory %s'), $this->path); + $title = sprintf(__('Permissions for directory %s'), $path); break; } + + parent::__construct($title, $description, $optional); + + $this->path = $path; } protected function check() diff --git a/src/System/Requirement/Extension.php b/src/System/Requirement/Extension.php index cb071c63d8c..1b70ddd1b75 100644 --- a/src/System/Requirement/Extension.php +++ b/src/System/Requirement/Extension.php @@ -54,10 +54,13 @@ class Extension extends AbstractRequirement */ public function __construct(string $name, bool $optional = false, ?string $description = null) { - $this->title = sprintf(__('%s extension'), $name); + parent::__construct( + sprintf(__('%s extension'), $name), + $description, + $optional + ); + $this->name = $name; - $this->optional = $optional; - $this->description = $description; } protected function check() diff --git a/src/System/Requirement/ExtensionConstant.php b/src/System/Requirement/ExtensionConstant.php index 74c9c27416f..b7b0628a8cc 100644 --- a/src/System/Requirement/ExtensionConstant.php +++ b/src/System/Requirement/ExtensionConstant.php @@ -55,10 +55,13 @@ class ExtensionConstant extends AbstractRequirement */ public function __construct(string $title, string $name, bool $optional = false, string $description = '') { - $this->title = $title; + parent::__construct( + $title, + $description, + $optional + ); + $this->name = $name; - $this->optional = $optional; - $this->description = $description; } protected function check() diff --git a/src/System/Requirement/ExtensionGroup.php b/src/System/Requirement/ExtensionGroup.php index edc626fc9a3..11d69481687 100644 --- a/src/System/Requirement/ExtensionGroup.php +++ b/src/System/Requirement/ExtensionGroup.php @@ -55,10 +55,13 @@ class ExtensionGroup extends AbstractRequirement */ public function __construct(string $title, array $extensions, bool $optional = false, ?string $description = null) { - $this->title = $title; + parent::__construct( + $title, + $description, + $optional + ); + $this->extensions = $extensions; - $this->optional = $optional; - $this->description = $description; } protected function check() diff --git a/src/System/Requirement/InstallationNotOverriden.php b/src/System/Requirement/InstallationNotOverriden.php index 27cb9efb253..a5928cc3d0b 100644 --- a/src/System/Requirement/InstallationNotOverriden.php +++ b/src/System/Requirement/InstallationNotOverriden.php @@ -58,11 +58,16 @@ final class InstallationNotOverriden extends AbstractRequirement public function __construct(?DBmysql $db, string $version_dir = GLPI_ROOT . '/version') { + parent::__construct( + __('Previous GLPI version files detection'), + __('The presence of source files from previous versions of GLPI can lead to security issues or bugs.'), + false, + false, + null // $out_of_context will be computed on check + ); + $this->db = $db; $this->version_dir = $version_dir; - - $this->title = __('Previous GLPI version files detection'); - $this->description = __('The presence of source files from previous versions of GLPI can lead to security issues or bugs.'); } protected function check() diff --git a/src/System/Requirement/IntegerSize.php b/src/System/Requirement/IntegerSize.php index 25157fe2329..8974a98e0be 100644 --- a/src/System/Requirement/IntegerSize.php +++ b/src/System/Requirement/IntegerSize.php @@ -39,9 +39,11 @@ final class IntegerSize extends AbstractRequirement { public function __construct() { - $this->title = __('PHP maximal integer size'); - $this->description = __('Support of 64 bits integers is required for IP addresses related operations (network inventory, API clients IP filtering, ...).'); - $this->optional = true; + parent::__construct( + __('PHP maximal integer size'), + __('Support of 64 bits integers is required for IP addresses related operations (network inventory, API clients IP filtering, ...).'), + true + ); } protected function check() diff --git a/src/System/Requirement/LogsWriteAccess.php b/src/System/Requirement/LogsWriteAccess.php index 6281d96724e..08da0da9002 100644 --- a/src/System/Requirement/LogsWriteAccess.php +++ b/src/System/Requirement/LogsWriteAccess.php @@ -55,8 +55,11 @@ class LogsWriteAccess extends AbstractRequirement */ public function __construct(LoggerInterface $logger) { + parent::__construct( + __('Permissions for log files') + ); + $this->logger = $logger; - $this->title = __('Permissions for log files'); } protected function check() diff --git a/src/System/Requirement/MemoryLimit.php b/src/System/Requirement/MemoryLimit.php index f621d22ff22..c0523fb9b76 100644 --- a/src/System/Requirement/MemoryLimit.php +++ b/src/System/Requirement/MemoryLimit.php @@ -52,7 +52,10 @@ class MemoryLimit extends AbstractRequirement */ public function __construct(int $min) { - $this->title = __('Allocated memory'); + parent::__construct( + __('Allocated memory') + ); + $this->min = $min; } diff --git a/src/System/Requirement/PhpSupportedVersion.php b/src/System/Requirement/PhpSupportedVersion.php index 6bdae1e2ee4..e4a98c98263 100644 --- a/src/System/Requirement/PhpSupportedVersion.php +++ b/src/System/Requirement/PhpSupportedVersion.php @@ -50,10 +50,12 @@ class PhpSupportedVersion extends AbstractRequirement public function __construct() { - $this->title = __('PHP maintained version'); - $this->description = __('A PHP version maintained by the PHP community should be used to get the benefits of PHP security and bug fixes.'); - $this->optional = true; - $this->recommended_for_security = true; + parent::__construct( + __('PHP maintained version'), + __('A PHP version maintained by the PHP community should be used to get the benefits of PHP security and bug fixes.'), + true, + true + ); } protected function check() diff --git a/src/System/Requirement/PhpVersion.php b/src/System/Requirement/PhpVersion.php index 7b7a386b9f6..a4f1e573dbb 100644 --- a/src/System/Requirement/PhpVersion.php +++ b/src/System/Requirement/PhpVersion.php @@ -62,7 +62,10 @@ class PhpVersion extends AbstractRequirement */ public function __construct(string $min_version, string $max_version) { - $this->title = __('PHP Parser'); + parent::__construct( + __('PHP Parser') + ); + $this->min_version = $min_version; $this->max_version = $max_version; } diff --git a/src/System/Requirement/ProtectedWebAccess.php b/src/System/Requirement/ProtectedWebAccess.php index 073b3e9bbda..498b6cad2bc 100644 --- a/src/System/Requirement/ProtectedWebAccess.php +++ b/src/System/Requirement/ProtectedWebAccess.php @@ -54,9 +54,13 @@ class ProtectedWebAccess extends AbstractRequirement */ public function __construct(array $directories) { - $this->title = __('Protected access to files directory'); - $this->description = __('Web access to GLPI var directories should be disabled to prevent unauthorized access to them.'); - $this->optional = true; + parent::__construct( + __('Protected access to files directory'), + __('Web access to GLPI var directories should be disabled to prevent unauthorized access to them.'), + true, + false, + null // $out_of_context will be computed on check + ); $this->directories = $directories; } diff --git a/src/System/Requirement/SafeDocumentRoot.php b/src/System/Requirement/SafeDocumentRoot.php index ba774d6bb20..7f8f67fe1b0 100644 --- a/src/System/Requirement/SafeDocumentRoot.php +++ b/src/System/Requirement/SafeDocumentRoot.php @@ -42,19 +42,21 @@ final class SafeDocumentRoot extends AbstractRequirement { public function __construct() { - $this->title = __('Safe configuration of web root directory'); - $this->description = sprintf( - __('Web server root directory should be `%s` to ensure non-public files cannot be accessed.'), - realpath(GLPI_ROOT) . DIRECTORY_SEPARATOR . 'public' + parent::__construct( + __('Safe configuration of web root directory'), + sprintf( + __('Web server root directory should be `%s` to ensure non-public files cannot be accessed.'), + realpath(GLPI_ROOT) . DIRECTORY_SEPARATOR . 'public' + ), + true, + true, + isCommandLine() // out of context when tested from CLI ); - $this->optional = true; - $this->recommended_for_security = true; } protected function check() { if (isCommandLine()) { - $this->out_of_context = true; $this->validated = false; $this->validation_messages[] = __('Checking web server root directory configuration cannot be done on CLI context.'); return; diff --git a/src/System/Requirement/SeLinux.php b/src/System/Requirement/SeLinux.php index 3ea4c42ebc0..4a72e5024a5 100644 --- a/src/System/Requirement/SeLinux.php +++ b/src/System/Requirement/SeLinux.php @@ -42,8 +42,13 @@ class SeLinux extends AbstractRequirement { public function __construct() { - $this->title = __('SELinux configuration'); - $this->optional = true; + parent::__construct( + __('SELinux configuration'), + null, + true, + false, + null, // $out_of_context will be computed on check + ); } protected function check() diff --git a/src/System/Requirement/SessionsConfiguration.php b/src/System/Requirement/SessionsConfiguration.php index ed218d7b982..8a9767ca06a 100644 --- a/src/System/Requirement/SessionsConfiguration.php +++ b/src/System/Requirement/SessionsConfiguration.php @@ -42,7 +42,9 @@ class SessionsConfiguration extends AbstractRequirement { public function __construct() { - $this->title = __('Sessions configuration'); + parent::__construct( + __('Sessions configuration') + ); } protected function check() diff --git a/src/System/Requirement/SessionsSecurityConfiguration.php b/src/System/Requirement/SessionsSecurityConfiguration.php index df4938f0a16..e12e27a7bd9 100644 --- a/src/System/Requirement/SessionsSecurityConfiguration.php +++ b/src/System/Requirement/SessionsSecurityConfiguration.php @@ -42,10 +42,13 @@ class SessionsSecurityConfiguration extends AbstractRequirement { public function __construct() { - $this->title = __('Security configuration for sessions'); - $this->description = __('Ensure security is enforced on session cookies.'); - $this->optional = true; - $this->recommended_for_security = true; + parent::__construct( + __('Security configuration for sessions'), + __('Ensure security is enforced on session cookies.'), + true, + true, + isCommandLine() // out of context when tested from CLI + ); } protected function check() @@ -61,7 +64,6 @@ protected function check() if ($is_cli) { $this->validation_messages[] = __('Checking the session cookie configuration of the web server cannot be done in the CLI context.'); $this->validation_messages[] = __('You should apply the following recommendations for configuring the web server.'); - $this->out_of_context = true; } $cookie_secure_ko = $is_https_request && !$cookie_secure; if ($is_cli || $cookie_secure_ko) { diff --git a/src/System/RequirementsList.php b/src/System/RequirementsList.php index 63981c4230b..d90436ec83b 100644 --- a/src/System/RequirementsList.php +++ b/src/System/RequirementsList.php @@ -71,7 +71,7 @@ public function getIterator(): Traversable public function hasMissingMandatoryRequirements() { foreach ($this->requirements as $requirement) { - if (!$requirement->isOutOfContext() && $requirement->isMissing() && !$requirement->isOptional()) { + if (!$requirement->isOptional() && !$requirement->isOutOfContext() && $requirement->isMissing()) { return true; } } @@ -86,7 +86,7 @@ public function hasMissingMandatoryRequirements() public function hasMissingOptionalRequirements() { foreach ($this->requirements as $requirement) { - if (!$requirement->isOutOfContext() && $requirement->isMissing() && $requirement->isOptional()) { + if ($requirement->isOptional() && !$requirement->isOutOfContext() && $requirement->isMissing()) { return true; } }