Skip to content

Commit 71da519

Browse files
chfsxchfsx
committed
[FIX] ResourceStorage: collection upload reported success for files it did not store and ignored the upload policy limit
1 parent 5eff303 commit 71da519

3 files changed

Lines changed: 233 additions & 13 deletions

File tree

‎components/ILIAS/ResourceStorage/classes/Collections/View/UploadStorer.php‎

Lines changed: 64 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
namespace ILIAS\components\ResourceStorage\Collections\View;
2222

2323
use ILIAS\FileUpload\DTO\UploadResult;
24+
use ILIAS\Filesystem\Stream\FileStream;
2425
use ILIAS\ResourceStorage\Collection\Collections;
2526
use ILIAS\ResourceStorage\Collection\ResourceCollection;
2627
use ILIAS\ResourceStorage\Identification\ResourceIdentification;
@@ -56,11 +57,7 @@ public function store(
5657
OnDuplicate $on_duplicate,
5758
UploadResult $result
5859
): ?ResourceIdentification {
59-
// an existing resource with the same name is only relevant if duplicates
60-
// are not simply allowed
61-
$existing_rid = $on_duplicate === OnDuplicate::ALLOW
62-
? null
63-
: $this->collections->findIdentificationByNameIn($collection, $result->getName());
60+
$existing_rid = $this->findResourceToWriteTo($collection, $on_duplicate, $result->getName());
6461

6562
if ($existing_rid === null) {
6663
// no name clash (or duplicates allowed): store as a new, separate resource
@@ -86,4 +83,66 @@ public function store(
8683
// OnDuplicate::ALLOW never reaches this point ($existing_rid is null above)
8784
return $existing_rid;
8885
}
86+
87+
/**
88+
* The twin of store() for an upload that did not arrive as one request and
89+
* therefore has no UploadResult: a chunked upload is reassembled into a file
90+
* of its own and handed over as a stream. The duplicate behaviour is the
91+
* same, only the way the resource is written differs.
92+
*
93+
* @param string $file_name the name the file was uploaded under - it decides
94+
* what counts as a duplicate, and the stream alone does not carry it
95+
* @return ResourceIdentification|null the identification of the affected
96+
* resource, or null if the upload was rejected (OnDuplicate::REJECT)
97+
* and therefore not stored.
98+
*/
99+
public function storeStream(
100+
ResourceCollection $collection,
101+
ResourceStakeholder $stakeholder,
102+
OnDuplicate $on_duplicate,
103+
FileStream $stream,
104+
string $file_name
105+
): ?ResourceIdentification {
106+
$existing_rid = $this->findResourceToWriteTo($collection, $on_duplicate, $file_name);
107+
108+
if ($existing_rid === null) {
109+
// no name clash (or duplicates allowed): store as a new, separate resource
110+
$rid = $this->manage->stream($stream, $stakeholder, $file_name);
111+
$collection->add($rid);
112+
return $rid;
113+
}
114+
115+
switch ($on_duplicate) {
116+
case OnDuplicate::REJECT:
117+
// leave the existing resource untouched, do not store the upload
118+
return null;
119+
case OnDuplicate::REPLACE:
120+
// overwrite with a new revision and drop all previous revisions
121+
$this->manage->replaceWithStream($existing_rid, $stream, $stakeholder, $file_name);
122+
return $existing_rid;
123+
case OnDuplicate::APPEND_REVISION:
124+
// overwrite by appending a new revision while keeping the previous ones as history
125+
$this->manage->appendNewRevisionFromStream($existing_rid, $stream, $stakeholder, $file_name);
126+
return $existing_rid;
127+
}
128+
129+
// OnDuplicate::ALLOW never reaches this point ($existing_rid is null above)
130+
return $existing_rid;
131+
}
132+
133+
/**
134+
* @return ResourceIdentification|null the resource already holding that name,
135+
* or null if there is none - ALLOW never dedupes and must not even ask.
136+
*/
137+
private function findResourceToWriteTo(
138+
ResourceCollection $collection,
139+
OnDuplicate $on_duplicate,
140+
string $file_name
141+
): ?ResourceIdentification {
142+
// an existing resource with the same name is only relevant if duplicates
143+
// are not simply allowed
144+
return $on_duplicate === OnDuplicate::ALLOW
145+
? null
146+
: $this->collections->findIdentificationByNameIn($collection, $file_name);
147+
}
89148
}

‎components/ILIAS/ResourceStorage/classes/Collections/class.ilResourceCollectionGUI.php‎

Lines changed: 74 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
use ILIAS\components\ResourceStorage\Collections\View\ViewFactory;
2828
use ILIAS\components\ResourceStorage\Collections\DataProvider\TableDataProvider;
2929
use ILIAS\components\ResourceStorage\BinToHexSerializer;
30+
use ILIAS\components\ResourceStorage\ChunkedUploadBuffer;
3031
use ILIAS\components\ResourceStorage\Collections\View\ActionBuilder;
3132
use ILIAS\components\ResourceStorage\Collections\View\ViewControlBuilder;
3233
use ILIAS\components\ResourceStorage\Collections\View\UploadBuilder;
@@ -42,6 +43,7 @@
4243
class ilResourceCollectionGUI implements UploadHandler
4344
{
4445
use BinToHexSerializer;
46+
use ChunkedUploadBuffer;
4547

4648
public const P_RESOURCE_ID = 'resource_id';
4749
public const P_RESOURCE_IDS = 'resource_ids';
@@ -89,6 +91,7 @@ final public function __construct(
8991
$this->upload = $DIC->upload();
9092
$this->archive = $DIC->archives();
9193
$this->preview_definition = new PreviewDefinition();
94+
$this->initChunkedUploadBuffer();
9295

9396
$this->view_request = new Request(
9497
$DIC->ctrl(),
@@ -216,23 +219,46 @@ public function upload(): void
216219
$this->abortWithPermissionDenied();
217220
return;
218221
}
222+
$this->readChunkInformation();
219223
$this->upload->process();
220224
if (!$this->upload->hasUploads()) {
221225
return;
222226
}
227+
228+
$this->sendHandlerResult(
229+
$this->isChunkedUpload() ? $this->storeChunk() : $this->storeUploads()
230+
);
231+
}
232+
233+
/**
234+
* Stores every file of the request in the collection. A request can carry
235+
* more than one file, and the outcome has to cover all of them: reporting
236+
* success unconditionally hid both files rejected by a pre-processor and
237+
* files dropped because a resource of that name already existed.
238+
*/
239+
private function storeUploads(): BasicHandlerResult
240+
{
223241
$collection = $this->view_request->getCollection();
224242
$stakeholder = $this->view_configuration->getStakeholder();
225243
$on_duplicate = $this->view_request->getOnDuplicate();
226244
$storer = new UploadStorer($this->irss->manage(), $this->irss->collection());
245+
246+
$results = $this->upload->getResults();
247+
$stored_all = $results !== [];
248+
$message = '';
227249
$rid = null;
228-
foreach ($this->upload->getResults() as $result) {
250+
251+
foreach ($results as $result) {
229252
if (!$result->isOK()) {
253+
$stored_all = false;
254+
$message = $result->getStatus()->getMessage();
230255
continue;
231256
}
232257

233258
$stored_rid = $storer->store($collection, $stakeholder, $on_duplicate, $result);
234259
if ($stored_rid === null) {
235260
// the upload was rejected (OnDuplicate::REJECT), nothing was stored
261+
$stored_all = false;
236262
continue;
237263
}
238264
$rid = $stored_rid;
@@ -244,13 +270,53 @@ public function upload(): void
244270
);
245271
}
246272
$this->irss->collection()->store($collection);
247-
$upload_result = new BasicHandlerResult(
248-
self::P_RESOURCE_ID,
249-
BasicHandlerResult::STATUS_OK,
250-
$rid?->serialize() ?? '',
251-
''
273+
274+
return $stored_all
275+
? new BasicHandlerResult(self::P_RESOURCE_ID, BasicHandlerResult::STATUS_OK, $rid?->serialize() ?? '', '')
276+
: $this->failedResult($message);
277+
}
278+
279+
/**
280+
* A reassembled upload has no UploadResult to hand to the storer, so it is
281+
* stored from a stream instead - the duplicate handling is the same.
282+
*/
283+
private function storeChunk(): BasicHandlerResult
284+
{
285+
return $this->assembleChunk(
286+
$this->upload->getResults(),
287+
function (string $assembled_path, string $file_name): ?string {
288+
$collection = $this->view_request->getCollection();
289+
$storer = new UploadStorer($this->irss->manage(), $this->irss->collection());
290+
291+
$stream = Streams::ofResource(fopen($assembled_path, 'rb'));
292+
try {
293+
$rid = $storer->storeStream(
294+
$collection,
295+
$this->view_configuration->getStakeholder(),
296+
$this->view_request->getOnDuplicate(),
297+
$stream,
298+
$file_name
299+
);
300+
} finally {
301+
$stream->close();
302+
}
303+
304+
if ($rid === null) {
305+
// the upload was rejected (OnDuplicate::REJECT), nothing was stored
306+
return null;
307+
}
308+
309+
$this->irss->flavours()->ensure($rid, $this->preview_definition);
310+
$this->irss->collection()->store($collection);
311+
312+
return $rid->serialize();
313+
}
252314
);
253-
$response = $this->http->response()->withBody(Streams::ofString(json_encode($upload_result)));
315+
}
316+
317+
private function sendHandlerResult(BasicHandlerResult $result): never
318+
{
319+
$response = $this->http->response()->withBody(Streams::ofString(json_encode($result)));
254320
$this->http->saveResponse($response);
255321
$this->http->sendResponse();
256322
$this->http->close();
@@ -524,6 +590,6 @@ public function getInfoResult(string $identifier): ?FileInfoResult
524590

525591
public function supportsChunkedUploads(): bool
526592
{
527-
return false;
593+
return true;
528594
}
529595
}

‎components/ILIAS/ResourceStorage/tests/Collections/View/UploadStorerTest.php‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
use ILIAS\FileUpload\Collection\EntryLockingStringMap;
2424
use ILIAS\FileUpload\DTO\ProcessingStatus;
2525
use ILIAS\FileUpload\DTO\UploadResult;
26+
use ILIAS\Filesystem\Stream\FileStream;
2627
use ILIAS\ResourceStorage\Collection\Collections;
2728
use ILIAS\ResourceStorage\Collection\ResourceCollection;
2829
use ILIAS\ResourceStorage\Identification\ResourceIdentification;
@@ -47,6 +48,7 @@ final class UploadStorerTest extends TestCase
4748
private ResourceCollection&MockObject $collection;
4849
private ResourceStakeholder&MockObject $stakeholder;
4950
private UploadResult $result;
51+
private FileStream&MockObject $stream;
5052
private UploadStorer $storer;
5153

5254
protected function setUp(): void
@@ -64,6 +66,7 @@ protected function setUp(): void
6466
new ProcessingStatus(ProcessingStatus::OK, 'ok'),
6567
'dummy/path'
6668
);
69+
$this->stream = $this->createMock(FileStream::class);
6770
$this->storer = new UploadStorer($this->manage, $this->collections);
6871
}
6972

@@ -195,6 +198,98 @@ public function testAppendRevisionStoresNewResourceWhenNoNameClash(): void
195198
);
196199
}
197200

201+
// storeStream(): the twin used for a reassembled chunked upload. It has to
202+
// reach the same decision as store() in every mode, only writing the resource
203+
// from a stream instead of from an UploadResult.
204+
205+
public function testStreamAllowStoresNewResourceWithoutLookupEvenWhenNameExists(): void
206+
{
207+
$new_rid = new ResourceIdentification('new');
208+
209+
$this->collections->expects($this->never())->method('findIdentificationByNameIn');
210+
$this->manage->expects($this->never())->method('replaceWithStream');
211+
$this->manage->expects($this->never())->method('appendNewRevisionFromStream');
212+
213+
$this->manage->expects($this->once())
214+
->method('stream')
215+
->with($this->stream, $this->stakeholder, self::FILE_NAME)
216+
->willReturn($new_rid);
217+
$this->collection->expects($this->once())->method('add')->with($new_rid);
218+
219+
$this->assertSame($new_rid, $this->storeStream(OnDuplicate::ALLOW));
220+
}
221+
222+
public function testStreamRejectLeavesExistingResourceUntouchedAndStoresNothing(): void
223+
{
224+
$this->givenExistingResource(new ResourceIdentification('existing'));
225+
226+
$this->manage->expects($this->never())->method('stream');
227+
$this->manage->expects($this->never())->method('replaceWithStream');
228+
$this->manage->expects($this->never())->method('appendNewRevisionFromStream');
229+
$this->collection->expects($this->never())->method('add');
230+
231+
$this->assertNull($this->storeStream(OnDuplicate::REJECT));
232+
}
233+
234+
public function testStreamReplaceOverwritesExistingResource(): void
235+
{
236+
$existing_rid = new ResourceIdentification('existing');
237+
$this->givenExistingResource($existing_rid);
238+
239+
$this->manage->expects($this->never())->method('stream');
240+
$this->manage->expects($this->never())->method('appendNewRevisionFromStream');
241+
$this->collection->expects($this->never())->method('add');
242+
$this->manage->expects($this->once())
243+
->method('replaceWithStream')
244+
->with($existing_rid, $this->stream, $this->stakeholder, self::FILE_NAME)
245+
->willReturn($this->createStub(Revision::class));
246+
247+
$this->assertSame($existing_rid, $this->storeStream(OnDuplicate::REPLACE));
248+
}
249+
250+
public function testStreamAppendRevisionAddsRevisionToExistingResource(): void
251+
{
252+
$existing_rid = new ResourceIdentification('existing');
253+
$this->givenExistingResource($existing_rid);
254+
255+
$this->manage->expects($this->never())->method('stream');
256+
$this->manage->expects($this->never())->method('replaceWithStream');
257+
$this->collection->expects($this->never())->method('add');
258+
$this->manage->expects($this->once())
259+
->method('appendNewRevisionFromStream')
260+
->with($existing_rid, $this->stream, $this->stakeholder, self::FILE_NAME)
261+
->willReturn($this->createStub(Revision::class));
262+
263+
$this->assertSame($existing_rid, $this->storeStream(OnDuplicate::APPEND_REVISION));
264+
}
265+
266+
public function testStreamStoresNewResourceWhenNoNameClash(): void
267+
{
268+
foreach ([OnDuplicate::REJECT, OnDuplicate::REPLACE, OnDuplicate::APPEND_REVISION] as $on_duplicate) {
269+
$this->setUp();
270+
$new_rid = new ResourceIdentification('new');
271+
$this->givenNoExistingResource();
272+
273+
$this->manage->expects($this->never())->method('replaceWithStream');
274+
$this->manage->expects($this->never())->method('appendNewRevisionFromStream');
275+
$this->manage->expects($this->once())->method('stream')->willReturn($new_rid);
276+
$this->collection->expects($this->once())->method('add')->with($new_rid);
277+
278+
$this->assertSame($new_rid, $this->storeStream($on_duplicate), $on_duplicate->name);
279+
}
280+
}
281+
282+
private function storeStream(OnDuplicate $on_duplicate): ?ResourceIdentification
283+
{
284+
return $this->storer->storeStream(
285+
$this->collection,
286+
$this->stakeholder,
287+
$on_duplicate,
288+
$this->stream,
289+
self::FILE_NAME
290+
);
291+
}
292+
198293
private function givenExistingResource(ResourceIdentification $existing_rid): void
199294
{
200295
$this->collections->method('findIdentificationByNameIn')

0 commit comments

Comments
 (0)