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

This commit is contained in:
Guy Sartorelli 2023-12-06 16:00:14 +13:00
parent b979ce5896
commit 4b1b487041
No known key found for this signature in database
GPG Key ID: F313E3B9504D496A
11 changed files with 260 additions and 1 deletions

View File

@ -2,6 +2,7 @@
namespace SilverStripe\Dev; namespace SilverStripe\Dev;
use SilverStripe\Control\HTTPResponse_Exception;
use SilverStripe\Core\Environment; use SilverStripe\Core\Environment;
use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataObject;
use SilverStripe\View\ViewableData; use SilverStripe\View\ViewableData;
@ -21,6 +22,7 @@ use SilverStripe\View\ViewableData;
*/ */
abstract class BulkLoader extends ViewableData abstract class BulkLoader extends ViewableData
{ {
private bool $checkPermissions = false;
/** /**
* Each row in the imported dataset should map to one instance * Each row in the imported dataset should map to one instance
@ -135,6 +137,23 @@ abstract class BulkLoader extends ViewableData
parent::__construct(); 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()}. * Load the given file via {@link self::processAll()} and {@link self::processRecord()}.
* Optionally truncates (clear) the table before it imports. * 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 //get all instances of the to be imported data object
if ($this->deleteExistingRecords) { 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(); DataObject::get($this->objectClass)->removeAll();
} }

View File

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

View File

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

View File

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

View File

@ -3,6 +3,7 @@
namespace SilverStripe\Dev\Tests; namespace SilverStripe\Dev\Tests;
use League\Csv\Writer; use League\Csv\Writer;
use SilverStripe\Control\HTTPResponse_Exception;
use SilverStripe\Dev\Tests\CsvBulkLoaderTest\CustomLoader; use SilverStripe\Dev\Tests\CsvBulkLoaderTest\CustomLoader;
use SilverStripe\Dev\Tests\CsvBulkLoaderTest\Player; use SilverStripe\Dev\Tests\CsvBulkLoaderTest\Player;
use SilverStripe\Dev\Tests\CsvBulkLoaderTest\PlayerContract; use SilverStripe\Dev\Tests\CsvBulkLoaderTest\PlayerContract;
@ -12,6 +13,10 @@ use SilverStripe\ORM\FieldType\DBField;
use SilverStripe\Core\Config\Config; use SilverStripe\Core\Config\Config;
use SilverStripe\Dev\CsvBulkLoader; use SilverStripe\Dev\CsvBulkLoader;
use SilverStripe\Dev\SapphireTest; 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 class CsvBulkLoaderTest extends SapphireTest
{ {
@ -22,6 +27,10 @@ class CsvBulkLoaderTest extends SapphireTest
Team::class, Team::class,
Player::class, Player::class,
PlayerContract::class, PlayerContract::class,
CanModifyModel::class,
CantCreateModel::class,
CantEditModel::class,
CantDeleteModel::class,
]; ];
/** /**
@ -337,4 +346,65 @@ class CsvBulkLoaderTest extends SapphireTest
$this->assertCount(10, $results); $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;
}
} }

View File

@ -1,3 +1,19 @@
SilverStripe\Dev\Tests\CsvBulkLoaderTest\Team: SilverStripe\Dev\Tests\CsvBulkLoaderTest\Team:
team1: team1:
Title: My Team 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

View File

@ -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;
}
}

View File

@ -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;
}
}

View File

@ -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;
}
}

View File

@ -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;
}
}

View File

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