From 5e1579b337c2fde62532f487eb563b13afc49596 Mon Sep 17 00:00:00 2001 From: Bernie Hamlin Date: Mon, 8 Apr 2024 10:40:16 +1200 Subject: [PATCH] Force live object on unserialise This corrects behaviour that potentially allows indexing of draft content. Queues are normally run via a "dev task" which is run in the DevelopmentAdmin context. This controller will set the reading mode to DRAFT. The upshot of this is that an Index job will unserialise a DataObject in DRAFT reading mode (saved as a Data Query param) and the subsequent `toArray` call could get draft content. This is often seen as a link indexed with `?stage=Stage` parameters. The impact is likely limited because an IndexJob is usually queued by a publish action and run shortly afterwards. In these cases it is unlikely that draft changes are present - but is isn't impossible. I checked the ReIndexJob run and it doesn't appear to have this issue because it sets the reading mode in the setup method. I have instead added this to the unserialise function as it seemed a safer method to ensure the reading mode was as expected. --- src/DataObject/DataObjectDocument.php | 28 +++++++++--- tests/DataObject/DataObjectDocumentTest.php | 48 ++++++++++++++++++++- tests/Fake/PageFake.php | 20 +++++++++ 3 files changed, 88 insertions(+), 8 deletions(-) create mode 100644 tests/Fake/PageFake.php diff --git a/src/DataObject/DataObjectDocument.php b/src/DataObject/DataObjectDocument.php index 31106aa..adae59f 100644 --- a/src/DataObject/DataObjectDocument.php +++ b/src/DataObject/DataObjectDocument.php @@ -36,7 +36,9 @@ use SilverStripe\SearchService\Service\Traits\ConfigurationAware; use SilverStripe\SearchService\Service\Traits\ServiceAware; use SilverStripe\Security\Member; +use SilverStripe\Versioned\ReadingMode; use SilverStripe\Versioned\Versioned; +use SilverStripe\Versioned\VersionedStateExtension; use SilverStripe\View\ViewableData; class DataObjectDocument implements @@ -600,16 +602,28 @@ public function __serialize(): array public function __unserialize(array $data): void { - $dataObject = DataObject::get_by_id($data['className'], $data['id']); + if (DataObject::has_extension($data['className'], Versioned::class)) { + // get data object in live mode always - needed as queue runners normally use a dev task to run + // so will have a DRAFT reading mode @see SilverStripe\Dev\DevelopmentAdmin + $dataObject = Versioned::withVersionedMode(function () use ($data): ?DataObject { + Versioned::set_stage(Versioned::LIVE); - if (!$dataObject && DataObject::has_extension($data['className'], Versioned::class) && $data['fallback']) { - // get the latest version - usually this is an object that has been deleted - $dataObject = Versioned::get_latest_version( - $data['className'], - $data['id'] - ); + return DataObject::get_by_id($data['className'], $data['id']); + }); + + if (!$dataObject && $data['fallback']) { + // get the latest version - usually this is an object that has been deleted + $dataObject = Versioned::get_latest_version( + $data['className'], + $data['id'] + ); + } + } else { + // un-versioned object so we don't need to worry about stages and fallbacks do not exist + $dataObject = DataObject::get_by_id($data['className'], $data['id']); } + if (!$dataObject) { throw new Exception(sprintf('DataObject %s : %s does not exist', $data['className'], $data['id'])); } diff --git a/tests/DataObject/DataObjectDocumentTest.php b/tests/DataObject/DataObjectDocumentTest.php index 8f9b5ed..8982a8f 100644 --- a/tests/DataObject/DataObjectDocumentTest.php +++ b/tests/DataObject/DataObjectDocumentTest.php @@ -16,10 +16,12 @@ use SilverStripe\SearchService\Tests\Fake\DataObjectFakeVersioned; use SilverStripe\SearchService\Tests\Fake\DataObjectSubclassFake; use SilverStripe\SearchService\Tests\Fake\ImageFake; +use SilverStripe\SearchService\Tests\Fake\PageFake; use SilverStripe\SearchService\Tests\Fake\TagFake; use SilverStripe\SearchService\Tests\SearchServiceTest; use SilverStripe\Security\Member; use SilverStripe\Subsites\Model\Subsite; +use SilverStripe\Versioned\Versioned; class DataObjectDocumentTest extends SearchServiceTest { @@ -612,7 +614,7 @@ public function testDeletedDataObject(): void $id = $dataObject->ID; $doc = DataObjectDocument::create($dataObject)->setShouldFallbackToLatestVersion(true); - $dataObject->delete(); + $dataObject->doArchive(); /** @var DataObjectDocument $serialDoc */ $serialDoc = unserialize(serialize($doc)); @@ -626,4 +628,48 @@ public function testDeletedDataObject(): void unserialize(serialize($doc)); } + public function testToArrayInQueueRun(): void + { + $config = $this->mockConfig(); + $config->set('getSearchableClasses', [ + PageFake::class, + ]); + $config->set('getFieldsForClass', [ + PageFake::class => [ + new Field('title'), + new Field('page_link', 'Link'), + ], + ]); + + Versioned::withVersionedMode(function (): void { + // Reading mode as if run by job - development Admin sets Draft reading mode + // usually via /dev/tasks/ProcessJobQueueTask + // @see SilverStripe\Dev\DevelopmentAdmin + Versioned::set_stage(Versioned::DRAFT); + + $dataObject = PageFake::create(); + $dataObject->Title = 'Published'; + $dataObject->write(); + $dataObject->publishRecursive(); + $dataObject->Title = 'Draft'; + $dataObject->write(); + + $doc = DataObjectDocument::create($dataObject); + + /** @var DataObjectDocument $serialDoc */ + $serialDoc = unserialize(serialize($doc)); + $indexData = $serialDoc->toArray(); + + $this->assertEqualsCanonicalizing( + [ + 'page_content' => '', + 'title' => 'Published', + 'page_link' => '/published', + ], + $indexData, + 'Potential draft content being indexed' + ); + }); + } + } diff --git a/tests/Fake/PageFake.php b/tests/Fake/PageFake.php new file mode 100644 index 0000000..d52ec64 --- /dev/null +++ b/tests/Fake/PageFake.php @@ -0,0 +1,20 @@ +