Fix: check read rights before displaying additional fields

CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed
 
 - Fix additional fields being saved on an item the user is not allowed to update
+- Fix additional fields being displayed for an item the user is not allowed to read
 - Fix invalid characters being kept in the generated field name
 - Fix missing right checks on the target item when displaying or saving additional fields values
 

ajax/container.php

@@ -45,18 +45,25 @@
     $items_id      = (int) $_GET['items_id'];
     $type          = $_GET['type'];
     $subtype       = $_GET['subtype'];
-    $input         = $_GET['input'];
+    $input         = is_array($_GET['input'] ?? null) ? $_GET['input'] : [];
+
+    if ($items_id > 0 && !PluginFieldsContainer::canReadTargetItem($itemtype, $items_id)) {
+        throw new AccessDeniedHttpException();
+    }
 
     $dbu = new DbUtils();
     $item = $dbu->getItemForItemtype($itemtype);
-    if ($items_id > 0) {
-        if (!$item->getFromDB($items_id)) {
-            throw new NotFoundHttpException();
-        }
 
+    if (!$item instanceof CommonDBTM) {
+        throw new NotFoundHttpException();
+    }
+
+    if ($items_id > 0) {
         if (!$item->can($items_id, READ)) {
             throw new AccessDeniedHttpException();
         }
+    } elseif (!$item->can(0, CREATE, $input)) {
+        throw new AccessDeniedHttpException();
     }
 
     $item->input = $input;

inc/container.class.php

@@ -1362,10 +1362,26 @@ public static function displayTabContentForItem(CommonGLPI $item, $tabnum = 1, $
      * @param integer $items_id Item id
      */
     public static function canUpdateTargetItem(string $itemtype, int $items_id): bool
+    {
+        return self::canTargetItem($itemtype, $items_id, UPDATE);
+    }
+
+    /**
+     * Check that current user is allowed to read the item the fields values are attached to
+     *
+     * @param string  $itemtype Item type
+     * @param integer $items_id Item id
+     */
+    public static function canReadTargetItem(string $itemtype, int $items_id): bool
+    {
+        return self::canTargetItem($itemtype, $items_id, READ);
+    }
+
+    private static function canTargetItem(string $itemtype, int $items_id, int $right): bool
     {
         $item = (new DbUtils())->getItemForItemtype($itemtype);
 
-        return $item instanceof CommonDBTM && $item->can($items_id, UPDATE);
+        return $item instanceof CommonDBTM && $item->can($items_id, $right);
     }
 
     /**

tests/Units/ContainerItemRightTest.php

@@ -35,10 +35,30 @@
 use Computer;
 use Entity;
 use Glpi\Tests\DbTestCase;
+use Glpi\Tests\GLPITestCase;
+use GlpiPlugin\Field\Tests\FieldTestTrait;
 use PluginFieldsContainer;
+use PluginFieldsField;
+use PluginFieldsProfile;
+
+require_once __DIR__ . '/../FieldTestCase.php';
 
 final class ContainerItemRightTest extends DbTestCase
 {
+    use FieldTestTrait;
+
+    public function setUp(): void
+    {
+        GLPITestCase::setUp();
+        $this->login();
+    }
+
+    public function tearDown(): void
+    {
+        $this->tearDownFieldTest();
+        GLPITestCase::tearDown();
+    }
+
     public function testCanUpdateTargetItemFollowsRightOnItem(): void
     {
         $this->login();
@@ -63,4 +83,96 @@ public function testCanUpdateTargetItemRejectsInvalidItemtype(): void
 
         $this->assertFalse(PluginFieldsContainer::canUpdateTargetItem('', 1));
     }
+
+    public function testCanReadTargetItemFollowsRightOnItem(): void
+    {
+        $this->login();
+        $root_entity_id = getItemByTypeName(Entity::class, '_test_root_entity', true);
+        $child_entity = $this->createItem(Entity::class, [
+            'name'         => 'Entity ' . $this->getUniqueString(),
+            'entities_id'  => $root_entity_id,
+        ]);
+        $computer = $this->createItem(Computer::class, [
+            'name'        => 'Computer ' . $this->getUniqueString(),
+            'entities_id' => $child_entity->getID(),
+        ]);
+
+        $this->assertTrue(PluginFieldsContainer::canReadTargetItem(Computer::class, $computer->getID()));
+
+        $this->setEntity($root_entity_id, false);
+
+        $this->assertFalse(PluginFieldsContainer::canReadTargetItem(Computer::class, $computer->getID()));
+    }
+
+    public function testCanReadTargetItemRejectsUnknownItem(): void
+    {
+        $this->login();
+
+        $this->assertFalse(PluginFieldsContainer::canReadTargetItem('', 1));
+        $this->assertFalse(PluginFieldsContainer::canReadTargetItem(Computer::class, 999999));
+    }
+
+    public function testShowDomContainerRendersReadOnlyFieldsWithoutUpdateRight(): void
+    {
+        $entity_id = getItemByTypeName(Entity::class, '_test_root_entity', true);
+        $this->setEntity($entity_id, true);
+
+        $container = $this->createFieldContainer([
+            'label'        => 'Dom container ' . $this->getUniqueString(),
+            'type'         => 'dom',
+            'itemtypes'    => [Computer::class],
+            'is_active'    => 1,
+            'entities_id'  => $entity_id,
+            'is_recursive' => 1,
+        ]);
+        $field = $this->createField([
+            'label'                                     => 'Dom field',
+            'type'                                      => 'text',
+            PluginFieldsContainer::getForeignKeyField() => $container->getID(),
+            'ranking'                                   => 1,
+            'is_active'                                 => 1,
+            'is_readonly'                               => 0,
+        ]);
+        $computer = $this->createItem(Computer::class, [
+            'name'        => 'Computer ' . $this->getUniqueString(),
+            'entities_id' => $entity_id,
+        ]);
+
+        $this->assertStringNotContainsString(
+            'readonly',
+            $this->renderDomContainer($container->getID(), $computer),
+        );
+
+        $this->setRightOnContainer($container->getID(), READ);
+
+        $this->assertStringContainsString(
+            'readonly',
+            $this->renderDomContainer($container->getID(), $computer),
+        );
+
+        $this->setRightOnContainer($container->getID(), 0);
+
+        $this->assertStringNotContainsString(
+            $field->fields['name'],
+            $this->renderDomContainer($container->getID(), $computer),
+        );
+    }
+
+    private function renderDomContainer(int $containers_id, Computer $computer): string
+    {
+        ob_start();
+        PluginFieldsField::showDomContainer($containers_id, $computer);
+
+        return (string) ob_get_clean();
+    }
+
+    private function setRightOnContainer(int $containers_id, int $right): void
+    {
+        $profile_right = new PluginFieldsProfile();
+        $this->assertTrue($profile_right->getFromDBByCrit([
+            'profiles_id'                 => $_SESSION['glpiactiveprofile']['id'],
+            'plugin_fields_containers_id' => $containers_id,
+        ]));
+        $this->updateItem(PluginFieldsProfile::class, $profile_right->getID(), ['right' => $right]);
+    }
 }