Prevent invalid lock config

glpi-project · Apr 3, 2024 · 878bcc2 · 878bcc2
1 parent bf38adc
commit 878bcc2
Show file tree

Hide file tree

Showing 6 changed files with 97 additions and 4 deletions.
diff --git a/src/Config.php b/src/Config.php
@@ -298,6 +298,23 @@ public function prepareInputForUpdate($input)
             $input['glpinetwork_registration_key'] = trim($input['glpinetwork_registration_key']);
         }
 
+        // Prevent invalid profile to be set as the lock profile.
+        // User updating the config from GLPI's UI should not be able to send
+        // invalid values but API or manual HTTP requests might be invalid.
+        if (isset($input['lock_lockprofile_id'])) {
+            $profile = Profile::getById($input['lock_lockprofile_id']);
+
+            if (!$profile || $profile->fields['interface'] !== 'central') {
+                // Invalid profile
+                Session::addMessageAfterRedirect(
+                    __("The specified profile doesn't exist or is not allowed to access the central interface."),
+                    false,
+                    ERROR
+                );
+                unset($input['lock_lockprofile_id']);
+            }
+        }
+
         $this->setConfigurationValues('core', $input);
 
         return false;

diff --git a/src/Profile.php b/src/Profile.php
@@ -312,6 +312,8 @@ public function cleanDBonPurge()
 
     public function prepareInputForUpdate($input)
     {
+        /** @var array $CFG_GLPI */
+        global $CFG_GLPI;
 
         if (isset($input["_helpdesk_item_types"])) {
             if ((!isset($input["helpdesk_item_type"])) || (!is_array($input["helpdesk_item_type"]))) {
@@ -430,6 +432,21 @@ public function prepareInputForUpdate($input)
             unset($input['interface']);
         }
 
+        // If the profile is used as the "Profile to be used when locking items",
+        // it can't be set to the "helpdesk" interface.
+        if (
+            isset($input['interface'])
+            && $input['interface'] === "helpdesk"
+            && $this->fields['id'] === (int) $CFG_GLPI['lock_lockprofile_id']
+        ) {
+            Session::addMessageAfterRedirect(
+                __("This profile can't be moved to the simplified interface as it is used for locking items."),
+                false,
+                ERROR
+            );
+            unset($input['interface']);
+        }
+
         // KEEP AT THE END
         $this->profileRight = [];
         foreach (array_keys(ProfileRight::getAllPossibleRights()) as $right) {

diff --git a/templates/pages/setup/general/general_setup.html.twig b/templates/pages/setup/general/general_setup.html.twig
@@ -276,6 +276,9 @@
         inline_field_options|merge(lock_options)|merge({
             'width': 'auto',
             'rand': locks_rand,
+            'condition': {
+                'interface': 'central',
+            }
         })
     ) }}
 

diff --git a/tests/DbTestCase.php b/tests/DbTestCase.php
@@ -229,18 +229,19 @@ protected function createItem($itemtype, $input, $skip_fields = []): CommonDBTM
      *
      * @param string $itemtype
      * @param array $input
+     * @param array $skip_fields Fields that wont be checked after update
      */
-    protected function updateItem($itemtype, $id, $input)
+    protected function updateItem($itemtype, $id, $input, $skip_fields = [])
     {
         $item = new $itemtype();
         $input['id'] = $id;
         $input = Sanitizer::sanitize($input);
         $success = $item->update($input);
         $this->boolean($success)->isTrue();
 
-       // Remove special fields
-        $input = array_filter($input, function ($key) {
-            return strpos($key, '_') !== 0;
+        // Remove special fields
+        $input = array_filter($input, function ($key) use ($skip_fields) {
+            return !in_array($key, $skip_fields) && strpos($key, '_') !== 0;
         }, ARRAY_FILTER_USE_KEY);
 
         $this->checkInput($item, $id, $input);

diff --git a/tests/functional/Config.php b/tests/functional/Config.php
@@ -39,6 +39,7 @@
 use Glpi\Plugin\Hooks;
 use Log;
 use PHPMailer\PHPMailer\PHPMailer;
+use Profile;
 use Session;
 
 /* Test for inc/config.class.php */
@@ -931,4 +932,47 @@ public function testConfigLogNotEmpty()
         $total_number = countElementsInTable("glpi_logs", ['items_id' => $config_id, 'itemtype' => $itemtype]);
         $this->integer($total_number)->isGreaterThan(0);
     }
+
+    /**
+     * Test the `prepareInputForUpdate` method.
+     *
+     * @return void
+     */
+    public function testPrepareInputForUpdate(): void
+    {
+        /** @var array $CFG_GLPI */
+        global $CFG_GLPI;
+
+        $this->login();
+
+        // Ensure the profile used for locks is valid.
+        $config = new \Config();
+        $default_lock_profile = $CFG_GLPI['lock_lockprofile_id']; // 8
+
+        // Invalid profile 1: simplified interface
+        $config->prepareInputForUpdate([
+            'lock_lockprofile_id' => getItemByTypeName(Profile::class, "Self-Service", true),
+        ]);
+        $this->integer((int) $CFG_GLPI['lock_lockprofile_id'])->isEqualTo($default_lock_profile);
+        $this->hasSessionMessages(ERROR, [
+            "The specified profile doesn't exist or is not allowed to access the central interface."
+        ]);
+
+        // Invalid profile 2: doesn't exist
+        $config->prepareInputForUpdate([
+            'lock_lockprofile_id' => 674568,
+        ]);
+        $this->integer((int) $CFG_GLPI['lock_lockprofile_id'])->isEqualTo($default_lock_profile);
+        $this->hasSessionMessages(ERROR, [
+            "The specified profile doesn't exist or is not allowed to access the central interface."
+        ]);
+
+        // Valid profile
+        $super_admin = getItemByTypeName(Profile::class, "Super-Admin", true);
+        $this->integer((int) $CFG_GLPI['lock_lockprofile_id'])->isNotEqualTo($super_admin);
+        $config->prepareInputForUpdate([
+            'lock_lockprofile_id' => $super_admin,
+        ]);
+        $this->integer((int) $CFG_GLPI['lock_lockprofile_id'])->isEqualTo($super_admin);
+    }
 }
diff --git a/tests/functional/Profile.php b/tests/functional/Profile.php
@@ -249,6 +249,17 @@ public function testprepareInputForUpdate(): void
         $this->hasSessionMessages(ERROR, [
             "Can't remove update right on this profile as it is the only remaining profile with this right."
         ]);
+
+        // Try to change the interface of the lock profile
+        $readonly = getItemByTypeName('Profile', 'Read-Only');
+        $this->updateItem("Profile", $readonly->getID(), [
+            'interface' => 'helpdesk'
+        ], ['interface']); // Skip interface check as it should not be updated.
+        $readonly->getFromDB($readonly->fields['id']); // Reload data
+        $this->string($readonly->fields['interface'])->isEqualTo('central');
+        $this->hasSessionMessages(ERROR, [
+            "This profile can't be moved to the simplified interface as it is used for locking items."
+        ]);
     }
 
     /**