diff --git a/src/Dev/BulkLoader.php b/src/Dev/BulkLoader.php index 43961e286..ebc53db85 100644 --- a/src/Dev/BulkLoader.php +++ b/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(); } diff --git a/src/Dev/CsvBulkLoader.php b/src/Dev/CsvBulkLoader.php index 2c82a03bd..14a19d4c5 100644 --- a/src/Dev/CsvBulkLoader.php +++ b/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; diff --git a/src/Security/GroupCsvBulkLoader.php b/src/Security/GroupCsvBulkLoader.php index 40db7d469..50b56bd1f 100644 --- a/src/Security/GroupCsvBulkLoader.php +++ b/src/Security/GroupCsvBulkLoader.php @@ -12,6 +12,7 @@ class GroupCsvBulkLoader extends CsvBulkLoader { public $duplicateChecks = [ + 'ID' => 'ID', 'Code' => 'Code', ]; diff --git a/src/Security/MemberCsvBulkLoader.php b/src/Security/MemberCsvBulkLoader.php index 94db9a7ae..b9202a820 100644 --- a/src/Security/MemberCsvBulkLoader.php +++ b/src/Security/MemberCsvBulkLoader.php @@ -33,6 +33,7 @@ class MemberCsvBulkLoader extends CsvBulkLoader * @var array */ public $duplicateChecks = [ + 'ID' => 'ID', 'Email' => 'Email', ]; diff --git a/tests/php/Dev/CsvBulkLoaderTest.php b/tests/php/Dev/CsvBulkLoaderTest.php index c6c65e5e0..8e51a5c82 100644 --- a/tests/php/Dev/CsvBulkLoaderTest.php +++ b/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; + } } diff --git a/tests/php/Dev/CsvBulkLoaderTest.yml b/tests/php/Dev/CsvBulkLoaderTest.yml index 0decfd674..4d86a7bd6 100644 --- a/tests/php/Dev/CsvBulkLoaderTest.yml +++ b/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 diff --git a/tests/php/Dev/CsvBulkLoaderTest/CanModifyModel.php b/tests/php/Dev/CsvBulkLoaderTest/CanModifyModel.php new file mode 100644 index 000000000..5ae9add25 --- /dev/null +++ b/tests/php/Dev/CsvBulkLoaderTest/CanModifyModel.php @@ -0,0 +1,31 @@ + '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; + } +} diff --git a/tests/php/Dev/CsvBulkLoaderTest/CantCreateModel.php b/tests/php/Dev/CsvBulkLoaderTest/CantCreateModel.php new file mode 100644 index 000000000..fec9cff57 --- /dev/null +++ b/tests/php/Dev/CsvBulkLoaderTest/CantCreateModel.php @@ -0,0 +1,13 @@ + '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; + } +} diff --git a/tests/php/Dev/CsvBulkLoaderTest/CantEditModel.php b/tests/php/Dev/CsvBulkLoaderTest/CantEditModel.php new file mode 100644 index 000000000..5281c7042 --- /dev/null +++ b/tests/php/Dev/CsvBulkLoaderTest/CantEditModel.php @@ -0,0 +1,13 @@ +