Merge pull request #11112 from creative-commoners/pulls/4.13/cve-2023-49783

[CVE-2023-49783] Allow permission checks in BulkLoader

src/Dev/BulkLoader.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Dev;
 
+use SilverStripe\Control\HTTPResponse_Exception;
 use SilverStripe\Core\Environment;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\View\ViewableData;
@@ -21,6 +22,7 @@ use SilverStripe\View\ViewableData;
  */
 abstract class BulkLoader extends ViewableData
 {
+    private bool $checkPermissions = false;
 
     /**
      * Each row in the imported dataset should map to one instance
@@ -135,6 +137,23 @@ abstract class BulkLoader extends ViewableData
         parent::__construct();
     }
 
+    /**
+     * If true, this bulk loader will respect create/edit/delete permissions.
+     */
+    public function getCheckPermissions(): bool
+    {
+        return $this->checkPermissions;
+    }
+
+    /**
+     * Determine whether this bulk loader should respect create/edit/delete permissions.
+     */
+    public function setCheckPermissions(bool $value): self
+    {
+        $this->checkPermissions = $value;
+        return $this;
+    }
+
     /*
      * Load the given file via {@link self::processAll()} and {@link self::processRecord()}.
      * Optionally truncates (clear) the table before it imports.
@@ -148,6 +167,21 @@ abstract class BulkLoader extends ViewableData
 
         //get all instances of the to be imported data object
         if ($this->deleteExistingRecords) {
+            if ($this->getCheckPermissions()) {
+                // We need to check each record, in case there's some fancy conditional logic in the canDelete method.
+                // If we can't delete even a single record, we should bail because otherwise the result would not be
+                // what the user expects.
+                /** @var DataObject $record */
+                foreach (DataObject::get($this->objectClass) as $record) {
+                    if (!$record->canDelete()) {
+                        $type = $record->i18n_singular_name();
+                        throw new HTTPResponse_Exception(
+                            _t(__CLASS__ . '.CANNOT_DELETE', "Not allowed to delete '$type' records"),
+                            403
+                        );
+                    }
+                }
+            }
             DataObject::get($this->objectClass)->removeAll();
         }
 

src/Dev/CsvBulkLoader.php

@@ -5,6 +5,7 @@ namespace SilverStripe\Dev;
 use League\Csv\MapIterator;
 use League\Csv\Reader;
 use SilverStripe\Control\Director;
+use SilverStripe\Control\HTTPResponse_Exception;
 use SilverStripe\ORM\DataObject;
 
 /**
@@ -43,6 +44,10 @@ class CsvBulkLoader extends BulkLoader
      */
     public $hasHeaderRow = true;
 
+    public $duplicateChecks = [
+        'ID' => 'ID',
+    ];
+
     /**
      * Number of lines to split large CSV files into.
      *
@@ -136,6 +141,10 @@ class CsvBulkLoader extends BulkLoader
                 $this->processRecord($row, $this->columnMap, $result, $preview);
             }
         } catch (\Exception $e) {
+            if ($e instanceof HTTPResponse_Exception) {
+                throw $e;
+            }
+
             $failedMessage = sprintf("Failed to parse %s", $filepath);
             if (Director::isDev()) {
                 $failedMessage = sprintf($failedMessage . " because %s", $e->getMessage());
@@ -305,8 +314,29 @@ class CsvBulkLoader extends BulkLoader
 
         // find existing object, or create new one
         $existingObj = $this->findExistingObject($record, $columnMap);
+        $alreadyExists = (bool) $existingObj;
+
+        // If we can't edit the existing object, bail early.
+        if ($this->getCheckPermissions() && !$preview && $alreadyExists && !$existingObj->canEdit()) {
+            $type = $existingObj->i18n_singular_name();
+            throw new HTTPResponse_Exception(
+                _t(BulkLoader::class . '.CANNOT_EDIT', "Not allowed to edit '$type' records"),
+                403
+            );
+        }
+
         /** @var DataObject $obj */
-        $obj = ($existingObj) ? $existingObj : new $class();
+        $obj = $alreadyExists ? $existingObj : new $class();
+
+        // If we can't create a new record, bail out early.
+        if ($this->getCheckPermissions() && !$preview && !$alreadyExists && !$obj->canCreate()) {
+            $type = $obj->i18n_singular_name();
+            throw new HTTPResponse_Exception(
+                _t(BulkLoader::class . '.CANNOT_CREATE', "Not allowed to create '$type' records"),
+                403
+            );
+        }
+
         $schema = DataObject::getSchema();
 
         // first run: find/create any relations and store them on the object
@@ -331,9 +361,17 @@ class CsvBulkLoader extends BulkLoader
                 }
                 if (!$relationObj || !$relationObj->exists()) {
                     $relationClass = $schema->hasOneComponent(get_class($obj), $relationName);
+                    /** @var DataObject $relationObj */
                     $relationObj = new $relationClass();
                     //write if we aren't previewing
                     if (!$preview) {
+                        if ($this->getCheckPermissions() && !$relationObj->canCreate()) {
+                            $type = $relationObj->i18n_singular_name();
+                            throw new HTTPResponse_Exception(
+                                _t(BulkLoader::class . '.CANNOT_CREATE', "Not allowed to create '$type' records"),
+                                403
+                            );
+                        }
                         $relationObj->write();
                     }
                 }
@@ -349,6 +387,13 @@ class CsvBulkLoader extends BulkLoader
                 // always gives us an component (either empty or existing)
                 $relationObj = $obj->getComponent($relationName);
                 if (!$preview) {
+                    if ($this->getCheckPermissions() && !$relationObj->canEdit()) {
+                        $type = $relationObj->i18n_singular_name();
+                        throw new HTTPResponse_Exception(
+                            _t(BulkLoader::class . '.CANNOT_EDIT', "Not allowed to edit '$type' records"),
+                            403
+                        );
+                    }
                     $relationObj->write();
                 }
                 $obj->{"{$relationName}ID"} = $relationObj->ID;

src/Security/GroupCsvBulkLoader.php

@@ -12,6 +12,7 @@ class GroupCsvBulkLoader extends CsvBulkLoader
 {
 
     public $duplicateChecks = [
+        'ID' => 'ID',
         'Code' => 'Code',
     ];
 

src/Security/MemberCsvBulkLoader.php

@@ -33,6 +33,7 @@ class MemberCsvBulkLoader extends CsvBulkLoader
      * @var array
      */
     public $duplicateChecks = [
+        'ID' => 'ID',
         'Email' => 'Email',
     ];
 

tests/php/Dev/CsvBulkLoaderTest.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\Dev\Tests;
 
 use League\Csv\Writer;
+use SilverStripe\Control\HTTPResponse_Exception;
 use SilverStripe\Dev\Tests\CsvBulkLoaderTest\CustomLoader;
 use SilverStripe\Dev\Tests\CsvBulkLoaderTest\Player;
 use SilverStripe\Dev\Tests\CsvBulkLoaderTest\PlayerContract;
@@ -12,6 +13,10 @@ use SilverStripe\ORM\FieldType\DBField;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Dev\CsvBulkLoader;
 use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Dev\Tests\CsvBulkLoaderTest\CanModifyModel;
+use SilverStripe\Dev\Tests\CsvBulkLoaderTest\CantCreateModel;
+use SilverStripe\Dev\Tests\CsvBulkLoaderTest\CantDeleteModel;
+use SilverStripe\Dev\Tests\CsvBulkLoaderTest\CantEditModel;
 
 class CsvBulkLoaderTest extends SapphireTest
 {
@@ -22,6 +27,10 @@ class CsvBulkLoaderTest extends SapphireTest
         Team::class,
         Player::class,
         PlayerContract::class,
+        CanModifyModel::class,
+        CantCreateModel::class,
+        CantEditModel::class,
+        CantDeleteModel::class,
     ];
 
     /**
@@ -337,4 +346,65 @@ class CsvBulkLoaderTest extends SapphireTest
 
         $this->assertCount(10, $results);
     }
+
+    /**
+     * @dataProvider provideCheckPermissions
+     */
+    public function testCheckPermissions(string $class, string $file, bool $respectPerms, string $exceptionMessage)
+    {
+        $loader = new CsvBulkLoader($class);
+        $loader->setCheckPermissions($respectPerms);
+        // Don't delete CantEditModel records, 'cause we need to explicitly edit them
+        $loader->deleteExistingRecords = $class !== CantEditModel::class;
+        // We can't rely on IDs in unit tests, so use Title as the unique field
+        $loader->duplicateChecks['Title'] = 'Title';
+
+        if ($exceptionMessage) {
+            $this->expectException(HTTPResponse_Exception::class);
+            $this->expectExceptionMessage($exceptionMessage);
+        }
+
+        $results = $loader->load($this->csvPath . $file);
+
+        // If there's no permission exception, we should get some valid results.
+        if (!$exceptionMessage) {
+            $this->assertCount(3, $results);
+        }
+    }
+
+    public function provideCheckPermissions()
+    {
+        $scenarios = [
+            'Has all permissions' => [
+                'class' => CanModifyModel::class,
+                'file' => 'PermissionCheck.csv',
+                'respectPerms' => true,
+                'exceptionMessage' => '',
+            ],
+            'No create permissions' => [
+                'class' => CantCreateModel::class,
+                'file' => 'PermissionCheck.csv',
+                'respectPerms' => true,
+                'exceptionMessage' => "Not allowed to create 'Cant Create Model' records",
+            ],
+            'No edit permissions' => [
+                'class' => CantEditModel::class,
+                'file' => 'PermissionCheck.csv',
+                'respectPerms' => true,
+                'exceptionMessage' => "Not allowed to edit 'Cant Edit Model' records",
+            ],
+            'No delete permissions' => [
+                'class' => CantDeleteModel::class,
+                'file' => 'PermissionCheck.csv',
+                'respectPerms' => true,
+                'exceptionMessage' => "Not allowed to delete 'Cant Delete Model' records",
+            ],
+        ];
+        foreach ($scenarios as $name => $scenario) {
+            $scenario['respectPerms'] = false;
+            $scenario['exceptionMessage'] = '';
+            $scenarios[$name . ' but no perm checks'] = $scenario;
+        }
+        return $scenarios;
+    }
 }

tests/php/Dev/CsvBulkLoaderTest.yml

@@ -1,3 +1,19 @@
 SilverStripe\Dev\Tests\CsvBulkLoaderTest\Team:
    team1:
       Title: My Team
+
+SilverStripe\Dev\Tests\CsvBulkLoaderTest\CantDeleteModel:
+  model1:
+    Title: Record 1
+  model2:
+    Title: Record 2
+  model3:
+    Title: Record 3
+
+SilverStripe\Dev\Tests\CsvBulkLoaderTest\CantEditModel:
+  model1:
+    Title: Record 1
+  model2:
+    Title: Record 2
+  model3:
+    Title: Record 3

tests/php/Dev/CsvBulkLoaderTest/CanModifyModel.php

@@ -0,0 +1,31 @@
+<?php
+
+namespace SilverStripe\Dev\Tests\CsvBulkLoaderTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+
+class CanModifyModel extends DataObject implements TestOnly
+{
+    private static $table_name = 'CsvBulkLoaderTest_CanModifyModel';
+
+    private static array $db = [
+        'Title' => 'Varchar',
+        'AnotherField' => 'Varchar',
+    ];
+
+    public function canCreate($member = null, $context = [])
+    {
+        return true;
+    }
+
+    public function canEdit($member = null)
+    {
+        return true;
+    }
+
+    public function canDelete($member = null)
+    {
+        return true;
+    }
+}

tests/php/Dev/CsvBulkLoaderTest/CantCreateModel.php

@@ -0,0 +1,13 @@
+<?php
+
+namespace SilverStripe\Dev\Tests\CsvBulkLoaderTest;
+
+use SilverStripe\Dev\TestOnly;
+
+class CantCreateModel extends CanModifyModel implements TestOnly
+{
+    public function canCreate($member = null, $context = [])
+    {
+        return false;
+    }
+}

tests/php/Dev/CsvBulkLoaderTest/CantDeleteModel.php

@@ -0,0 +1,31 @@
+<?php
+
+namespace SilverStripe\Dev\Tests\CsvBulkLoaderTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+
+class CantDeleteModel extends DataObject implements TestOnly
+{
+    private static $table_name = 'CsvBulkLoaderTest_CantDeleteModel';
+
+    private static array $db = [
+        'Title' => 'Varchar',
+        'AnotherField' => 'Varchar',
+    ];
+
+    public function canCreate($member = null, $context = [])
+    {
+        return true;
+    }
+
+    public function canEdit($member = null)
+    {
+        return true;
+    }
+
+    public function canDelete($member = null)
+    {
+        return false;
+    }
+}

tests/php/Dev/CsvBulkLoaderTest/CantEditModel.php

@@ -0,0 +1,13 @@
+<?php
+
+namespace SilverStripe\Dev\Tests\CsvBulkLoaderTest;
+
+use SilverStripe\Dev\TestOnly;
+
+class CantEditModel extends CanModifyModel implements TestOnly
+{
+    public function canEdit($member = null)
+    {
+        return false;
+    }
+}

tests/php/Dev/CsvBulkLoaderTest/csv/PermissionCheck.csv

@@ -0,0 +1,4 @@
+Title, AnotherField
+Record 1, value 1
+Record 2, value 2
+Record 3, value 3