force admin to make a choice about inventory auth (#23571)

orthagh · web-flow · commit dc8ffc7b53a9 · 2026-04-13T10:01:37.000+02:00
diff --git a/src/Glpi/Agent/Communication/AbstractRequest.php b/src/Glpi/Agent/Communication/AbstractRequest.php
@@ -49,6 +49,7 @@
 use League\OAuth2\Server\Exception\OAuthServerException;
 use RuntimeException;
 use Safe\Exceptions\SimplexmlException;
+use Safe\Exceptions\UrlException;
 use Toolbox;
 use UnexpectedValueException;
 
@@ -228,55 +229,62 @@ public function handleRequest(mixed $data): bool
         $guess_mode = ($base_mode === null);
         $this->setMode(self::JSON_MODE);
 
-        $auth_required = false;
         if (!$this->isLocal()) {
             $auth_required = Config::getConfigurationValue('inventory', 'auth_required');
-        }
-        if ($auth_required === Conf::CLIENT_CREDENTIALS) {
-            $request = new Request('POST', $_SERVER['REQUEST_URI'], $this->headers->getHeaders());
-            try {
-                $client = Server::validateAccessToken($request);
-                if (!in_array('inventory', $client['scopes'], true)) {
-                    $this->addError('Access denied. Agent must authenticate using client credentials and have the "inventory" OAuth scope', 401);
-                    return false;
-                }
-            } catch (OAuth2KeyException $e) {
-                ErrorHandler::logCaughtException($e);
-                $this->addError($e->getMessage());
-                return false;
-            } catch (OAuthServerException) {
-                $this->addError('Authorization header required to send an inventory', 401);
-                return false;
-            }
-        }
 
-        if ($auth_required === Conf::BASIC_AUTH) {
-            $authorization_header = $this->headers->getHeader('Authorization');
-            if (is_null($authorization_header)) {
-                $this->headers->setHeader("www-authenticate", 'Basic realm="basic"');
-                $this->addError('Authorization header required to send an inventory', 401);
-                return false;
-            } else {
-                $allowed = false;
-                // if Authorization start with 'Basic'
-                if (preg_match('/^Basic\s+(.*)$/i', $authorization_header, $matches)) {
-                    $inventory_login = Config::getConfigurationValue('inventory', 'basic_auth_login');
-                    $inventory_password = (new GLPIKey())
-                        ->decrypt(Config::getConfigurationValue('inventory', 'basic_auth_password'));
-                    $agent_credential = base64_decode($matches[1]);
-                    [$agent_login, $agent_password] = explode(':', $agent_credential, 2);
-                    if (
-                        $inventory_login == $agent_login
-                        && $inventory_password == $agent_password
-                    ) {
-                        $allowed = true;
+            if ($auth_required === Conf::CLIENT_CREDENTIALS) {
+                $request = new Request('POST', $_SERVER['REQUEST_URI'], $this->headers->getHeaders());
+                try {
+                    $client = Server::validateAccessToken($request);
+                    if (!in_array('inventory', $client['scopes'], true)) {
+                        $this->addError('Access denied. Agent must authenticate using client credentials and have the "inventory" OAuth scope', 401);
+                        return false;
                     }
+                } catch (OAuth2KeyException $e) {
+                    ErrorHandler::logCaughtException($e);
+                    $this->addError($e->getMessage());
+                    return false;
+                } catch (OAuthServerException) {
+                    $this->addError('Authorization header required to send an inventory', 401);
+                    return false;
                 }
-                if (!$allowed) {
+            } elseif ($auth_required === Conf::BASIC_AUTH) {
+                $authorization_header = $this->headers->getHeader('Authorization');
+                if (is_null($authorization_header)) {
                     $this->headers->setHeader("www-authenticate", 'Basic realm="basic"');
-                    $this->addError('Access denied. Wrong login or password for basic authentication.', 401);
+                    $this->addError('Authorization header required to send an inventory', 401);
                     return false;
+                } else {
+                    $allowed = false;
+                    // if Authorization start with 'Basic'
+                    if (preg_match('/^Basic\s+(.*)$/i', $authorization_header, $matches)) {
+                        $inventory_login = Config::getConfigurationValue('inventory', 'basic_auth_login');
+                        $inventory_password = (new GLPIKey())
+                            ->decrypt(Config::getConfigurationValue('inventory', 'basic_auth_password'));
+                        try {
+                            $agent_credential = base64_decode($matches[1], true);
+                            if (str_contains($agent_credential, ':')) {
+                                [$agent_login, $agent_password] = explode(':', $agent_credential, 2);
+                                if (
+                                    $inventory_login == $agent_login
+                                    && $inventory_password == $agent_password
+                                ) {
+                                    $allowed = true;
+                                }
+                            }
+                        } catch (UrlException) {
+                            // malformed base64 — leave $allowed as false
+                        }
+                    }
+                    if (!$allowed) {
+                        $this->headers->setHeader("www-authenticate", 'Basic realm="basic"');
+                        $this->addError('Access denied. Wrong login or password for basic authentication.', 401);
+                        return false;
+                    }
                 }
+            } elseif ($auth_required !== Conf::NO_AUTH) {
+                $this->addError('Server configuration error: invalid inventory authentication setting. Please configure the Inventory → Authorization header in GLPI.', 503);
+                return false;
             }
         }
 
diff --git a/src/Glpi/Inventory/Conf.php b/src/Glpi/Inventory/Conf.php
@@ -72,6 +72,7 @@
 use Monitor;
 use NetworkPort;
 use NetworkPortType;
+use OAuthClient;
 use Peripheral;
 use Plugin;
 use Printer;
@@ -140,6 +141,8 @@ class Conf extends CommonGLPI
 
     public const STALE_AGENT_ACTION_TRASHBIN = 2;
 
+    public const NO_AUTH = 'none';
+
     public const CLIENT_CREDENTIALS = 'client_credentials';
 
     public const BASIC_AUTH = 'basic_auth';
@@ -396,18 +399,50 @@ public function showConfigForm()
             echo "</tr>";
             echo "<tr class='tab_bg_1'>";
             echo "<td>";
-            echo "<i class='ti ti-cloud-lock me-2'></i>";
-            echo "<label for='auth'>" . __s('Authorization header') . "</label>";
+            $auth_rand = mt_rand();
+            echo "<i class='ti ti-shield me-2'></i>";
+            echo "<label for='dropdown_auth_required{$auth_rand}'>" . __s('Authorization header') . "</label>";
+            echo "<span class='required'>*</span>";
             echo "</td>";
-            echo "<td>";
+            echo "<td colspan='3'>";
             Dropdown::showFromArray('auth_required', [
-                'none' => __('None'),
+                '' => Dropdown::EMPTY_VALUE,
                 self::CLIENT_CREDENTIALS => __s('OAuth - Client credentials'),
                 self::BASIC_AUTH => __s('Basic Authentication'),
+                self::NO_AUTH => __('None (not recommended)'),
             ], [
-                'value' => $config['auth_required'] ?? 'none',
+                'value' => $config['auth_required'] ?? '',
+                'required' => true,
+                'rand' => $auth_rand,
             ]);
+            echo "<div id='oauth_client_hint_row' class='mt-1'>";
+            echo "<div class='alert alert-info d-inline-flex align-items-center mb-0 py-2' role='note'>";
+            echo "<i class='ti ti-info-circle flex-shrink-0 me-1'></i>";
+            echo "<span>";
+            echo __s('Using OAuth Client Credentials requires a registered OAuth client with the "inventory" scope.');
+            echo '<br>';
+            echo sprintf(
+                __s('The generated client ID and secret must then be set in the GLPI Agent configuration using the %s and %s parameters.'),
+                '<code>oauth-client-id</code>',
+                '<code>oauth-client-secret</code>'
+            );
+            echo ' <a href="' . htmlescape(OAuthClient::getFormURL()) . '">';
+            echo __s('Add an OAuth client');
+            echo ' <i class="ti ti-external-link ms-1"></i>';
+            echo '</a>';
+            echo "</span>";
+            echo "</div>";
+            echo "</div>";
+            echo "<div id='no_auth_warning_row' class='mt-1'>";
+            echo "<div class='alert alert-warning d-inline-flex align-items-center mb-0 py-2' role='alert'>";
+            echo "<i class='ti ti-alert-triangle flex-shrink-0 me-1'></i>";
+            echo "<span>";
+            echo __s('Not using any authentication on inventory is not recommended and poses a security risk. Any agent will be able to send inventory data without verification.');
+            echo "</span>";
+            echo "</div>";
+            echo "</div>";
             echo "</td></tr>";
+
             echo "<tr class='tab_bg_1' id='basic_auth_login_row'>";
             echo "<td>";
             echo "<i class='ti ti-abc me-2'></i>";
@@ -437,13 +472,14 @@ public function showConfigForm()
             echo "</tr>";
             echo Html::scriptBlock("
                 function toggleDisplayLoginInputs(select) {
-                    let displayedInputs = false;
                     const selectedValue = $(select).val();
-                    if (selectedValue == '" . self::BASIC_AUTH . "') {
-                        displayedInputs = true;
-                    }
-                    $('#basic_auth_login_row').toggle(displayedInputs);
-                    $('#basic_auth_password_row').toggle(displayedInputs);
+                    const isBasicAuth = selectedValue == '" . self::BASIC_AUTH . "';
+                    const isOAuth = selectedValue == '" . self::CLIENT_CREDENTIALS . "';
+                    const isNoAuth = selectedValue == '" . self::NO_AUTH . "';
+                    $('#basic_auth_login_row').toggle(isBasicAuth);
+                    $('#basic_auth_password_row').toggle(isBasicAuth);
+                    $('#oauth_client_hint_row').toggle(isOAuth);
+                    $('#no_auth_warning_row').toggle(isNoAuth);
                 }
 
                 const selectAuthHeader = $(`select[name='auth_required']`);
@@ -1146,6 +1182,24 @@ public function saveConf(array $values)
             $values['stale_agents_status_condition'] = ['all'];
         }
 
+        $enabled_inventory = (int) ($values['enabled_inventory'] ?? $defaults['enabled_inventory']) === 1;
+        if ($enabled_inventory) {
+            $allowed_auth_required = [
+                self::CLIENT_CREDENTIALS,
+                self::BASIC_AUTH,
+                self::NO_AUTH,
+            ];
+            $auth_required = $values['auth_required'] ?? null;
+            if (!is_string($auth_required) || !in_array($auth_required, $allowed_auth_required, true)) {
+                Session::addMessageAfterRedirect(
+                    __s('Inventory is enabled. Please select a valid authorization header method.'),
+                    false,
+                    ERROR
+                );
+                return false;
+            }
+        }
+
         if (isset($values['auth_required']) && $values['auth_required'] === Conf::BASIC_AUTH) {
             if (
                 !empty($values['basic_auth_password'])
@@ -1306,7 +1360,7 @@ public static function getDefaults(): array
             'stale_agents_status'            => 0,
             'stale_agents_status_condition'  => exportArrayToDB(['all']),
             'import_env'                     => 0,
-            'auth_required'                  => 'none',
+            'auth_required'                  => '',
             'basic_auth_login'               => '',
             'basic_auth_password'            => '',
         ];
diff --git a/tests/functional/Glpi/Inventory/ConfTest.php b/tests/functional/Glpi/Inventory/ConfTest.php
@@ -34,12 +34,13 @@
 
 namespace tests\units\Glpi\Inventory;
 
+use Config;
 use Glpi\Inventory\Conf;
-use Glpi\Tests\GLPITestCase;
+use Glpi\Tests\DbTestCase;
 use PHPUnit\Framework\Attributes\DataProvider;
 use Psr\Log\LogLevel;
 
-class ConfTest extends GLPITestCase
+class ConfTest extends DbTestCase
 {
     public function testKnownInventoryExtensions()
     {
@@ -84,6 +85,7 @@ public static function confProvider(): array
     {
         $provider = [];
         $defaults = Conf::getDefaults();
+        $defaults['auth_required'] = Conf::NO_AUTH;
         foreach ($defaults as $key => $value) {
             $provider[] = [
                 'key'    => $key,
@@ -107,4 +109,34 @@ public function testErrorGetter()
         $this->hasPhpLogRecordThatContains('Property doesNotExists does not exists!', LogLevel::WARNING);
         $this->hasSessionMessages(WARNING, ['Property doesNotExists does not exists!']);
     }
+
+    public static function invalidAuthRequiredProvider(): iterable
+    {
+        yield 'empty string' => [''];
+        yield 'null value' => [null];
+        yield 'unexpected value' => ['unexpected_value'];
+    }
+
+    #[DataProvider('invalidAuthRequiredProvider')]
+    public function testSaveConfRejectsInvalidAuthRequiredWhenInventoryIsEnabled(mixed $auth_required): void
+    {
+        $this->login();
+
+        Config::setConfigurationValues('inventory', [
+            'enabled_inventory' => 1,
+            'auth_required'     => Conf::NO_AUTH,
+        ]);
+
+        $conf = new Conf();
+        $result = $conf->saveConf([
+            'enabled_inventory' => 1,
+            'auth_required'     => $auth_required,
+        ]);
+
+        $this->assertFalse($result);
+        $this->hasSessionMessages(ERROR, [
+            'Inventory is enabled. Please select a valid authorization header method.',
+        ]);
+        $this->assertSame(Conf::NO_AUTH, Config::getConfigurationValue('inventory', 'auth_required'));
+    }
 }
diff --git a/tests/src/autoload/functions.php b/tests/src/autoload/functions.php
@@ -40,6 +40,7 @@
 use Glpi\Form\Section;
 use Glpi\Helpdesk\HelpdeskTranslation;
 use Glpi\Helpdesk\Tile\GlpiPageTile;
+use Glpi\Inventory\Conf;
 use Glpi\Socket;
 
 function loadDataset()
@@ -953,6 +954,8 @@ function loadDataset()
         Search::$search = [];
         Config::setConfigurationValues('phpunit', ['dataset' => $data['_version']]);
     }
+
+    Config::setConfigurationValues('inventory', ['auth_required' => Conf::NO_AUTH]);
     $DB->commit();
 
     $_SESSION = $session_bak; // Unset force session variables
diff --git a/tests/web/Glpi/Inventory/RequestTest.php b/tests/web/Glpi/Inventory/RequestTest.php
