Merge pull request #7533 from nextcloud/oc-28545-handle-oc-total-length-in-new-chunking

[oc] Handle OC-Total-Length in new chunking
This commit is contained in:
Morris Jobke 2018-01-03 16:18:24 +01:00 committed by GitHub
commit 876238ce8b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 351 additions and 121 deletions

View File

@ -73,5 +73,7 @@ while(!feof($stream)) {
$destination = pathinfo($file, PATHINFO_BASENAME);
//echo "Moving $uploadUrl/.file to it's final destination $baseUri/files/$userName/$destination" . PHP_EOL;
request($client, 'MOVE', "$uploadUrl/.file", null, [
'Destination' => "$baseUri/files/$userName/$destination"
'Destination' => "$baseUri/files/$userName/$destination",
'OC-Total-Length' => filesize($file),
'X-OC-MTime' => filemtime($file)
]);

View File

@ -144,6 +144,7 @@ return array(
'OCA\\DAV\\SystemTag\\SystemTagsObjectTypeCollection' => $baseDir . '/../lib/SystemTag/SystemTagsObjectTypeCollection.php',
'OCA\\DAV\\SystemTag\\SystemTagsRelationsCollection' => $baseDir . '/../lib/SystemTag/SystemTagsRelationsCollection.php',
'OCA\\DAV\\Upload\\AssemblyStream' => $baseDir . '/../lib/Upload/AssemblyStream.php',
'OCA\\DAV\\Upload\\ChunkingPlugin' => $baseDir . '/../lib/Upload/ChunkingPlugin.php',
'OCA\\DAV\\Upload\\FutureFile' => $baseDir . '/../lib/Upload/FutureFile.php',
'OCA\\DAV\\Upload\\RootCollection' => $baseDir . '/../lib/Upload/RootCollection.php',
'OCA\\DAV\\Upload\\UploadFolder' => $baseDir . '/../lib/Upload/UploadFolder.php',

View File

@ -159,6 +159,7 @@ class ComposerStaticInitDAV
'OCA\\DAV\\SystemTag\\SystemTagsObjectTypeCollection' => __DIR__ . '/..' . '/../lib/SystemTag/SystemTagsObjectTypeCollection.php',
'OCA\\DAV\\SystemTag\\SystemTagsRelationsCollection' => __DIR__ . '/..' . '/../lib/SystemTag/SystemTagsRelationsCollection.php',
'OCA\\DAV\\Upload\\AssemblyStream' => __DIR__ . '/..' . '/../lib/Upload/AssemblyStream.php',
'OCA\\DAV\\Upload\\ChunkingPlugin' => __DIR__ . '/..' . '/../lib/Upload/ChunkingPlugin.php',
'OCA\\DAV\\Upload\\FutureFile' => __DIR__ . '/..' . '/../lib/Upload/FutureFile.php',
'OCA\\DAV\\Upload\\RootCollection' => __DIR__ . '/..' . '/../lib/Upload/RootCollection.php',
'OCA\\DAV\\Upload\\UploadFolder' => __DIR__ . '/..' . '/../lib/Upload/UploadFolder.php',

View File

@ -589,18 +589,6 @@ class File extends Node implements IFile {
throw new \Sabre\DAV\Exception($e->getMessage(), 0, $e);
}
private function sanitizeMtime($mtimeFromRequest) {
// In PHP 5.X "is_numeric" returns true for strings in hexadecimal
// notation. This is no longer the case in PHP 7.X, so this check
// ensures that strings with hexadecimal notations fail too in PHP 5.X.
$isHexadecimal = is_string($mtimeFromRequest) && preg_match('/^\s*0[xX]/', $mtimeFromRequest);
if ($isHexadecimal || !is_numeric($mtimeFromRequest)) {
throw new \InvalidArgumentException('X-OC-MTime header must be an integer (unix timestamp).');
}
return intval($mtimeFromRequest);
}
/**
* Get the checksum for this file
*

View File

@ -32,7 +32,7 @@
namespace OCA\DAV\Connector\Sabre;
use OC\Files\View;
use OC\AppFramework\Http\Request;
use OCP\Files\ForbiddenException;
use OCP\IPreview;
use Sabre\DAV\Exception\Forbidden;
@ -47,7 +47,6 @@ use \Sabre\HTTP\ResponseInterface;
use OCP\Files\StorageNotAvailableException;
use OCP\IConfig;
use OCP\IRequest;
use OCA\DAV\Upload\FutureFile;
class FilesPlugin extends ServerPlugin {
@ -90,11 +89,6 @@ class FilesPlugin extends ServerPlugin {
*/
private $isPublic;
/**
* @var View
*/
private $fileView;
/**
* @var bool
*/
@ -183,7 +177,6 @@ class FilesPlugin extends ServerPlugin {
}
});
$this->server->on('beforeMove', [$this, 'checkMove']);
$this->server->on('beforeMove', [$this, 'beforeMoveFutureFile']);
}
/**
@ -258,9 +251,9 @@ class FilesPlugin extends ServerPlugin {
$filename = $node->getName();
if ($this->request->isUserAgent(
[
\OC\AppFramework\Http\Request::USER_AGENT_IE,
\OC\AppFramework\Http\Request::USER_AGENT_ANDROID_MOBILE_CHROME,
\OC\AppFramework\Http\Request::USER_AGENT_FREEBOX,
Request::USER_AGENT_IE,
Request::USER_AGENT_ANDROID_MOBILE_CHROME,
Request::USER_AGENT_FREEBOX,
])) {
$response->addHeader('Content-Disposition', 'attachment; filename="' . rawurlencode($filename) . '"');
} else {
@ -461,43 +454,4 @@ class FilesPlugin extends ServerPlugin {
}
}
}
/**
* Move handler for future file.
*
* This overrides the default move behavior to prevent Sabre
* to delete the target file before moving. Because deleting would
* lose the file id and metadata.
*
* @param string $path source path
* @param string $destination destination path
* @return bool|void false to stop handling, void to skip this handler
*/
public function beforeMoveFutureFile($path, $destination) {
$sourceNode = $this->tree->getNodeForPath($path);
if (!$sourceNode instanceof FutureFile) {
// skip handling as the source is not a chunked FutureFile
return;
}
if (!$this->tree->nodeExists($destination)) {
// skip and let the default handler do its work
return;
}
// do a move manually, skipping Sabre's default "delete" for existing nodes
$this->tree->move($path, $destination);
// trigger all default events (copied from CorePlugin::move)
$this->server->emit('afterMove', [$path, $destination]);
$this->server->emit('afterUnbind', [$path]);
$this->server->emit('afterBind', [$destination]);
$response = $this->server->httpResponse;
$response->setHeader('Content-Length', '0');
$response->setStatus(204);
return false;
}
}

View File

@ -165,6 +165,7 @@ abstract class Node implements \Sabre\DAV\INode {
* Even if the modification time is set to a custom value the access time is set to now.
*/
public function touch($mtime) {
$mtime = $this->sanitizeMtime($mtime);
$this->fileView->touch($this->path, $mtime);
$this->refreshInfo();
}
@ -358,4 +359,17 @@ abstract class Node implements \Sabre\DAV\INode {
public function getFileInfo() {
return $this->info;
}
protected function sanitizeMtime($mtimeFromRequest) {
// In PHP 5.X "is_numeric" returns true for strings in hexadecimal
// notation. This is no longer the case in PHP 7.X, so this check
// ensures that strings with hexadecimal notations fail too in PHP 5.X.
$isHexadecimal = is_string($mtimeFromRequest) && preg_match('/^\s*0[xX]/', $mtimeFromRequest);
if ($isHexadecimal || !is_numeric($mtimeFromRequest)) {
throw new \InvalidArgumentException('X-OC-MTime header must be an integer (unix timestamp).');
}
return intval($mtimeFromRequest);
}
}

View File

@ -55,6 +55,7 @@ use OCA\DAV\DAV\CustomPropertiesBackend;
use OCA\DAV\Connector\Sabre\QuotaPlugin;
use OCA\DAV\Files\BrowserErrorPagePlugin;
use OCA\DAV\SystemTag\SystemTagPlugin;
use OCA\DAV\Upload\ChunkingPlugin;
use OCP\IRequest;
use OCP\SabrePluginEvent;
use Sabre\CardDAV\VCFExportPlugin;
@ -171,6 +172,7 @@ class Server {
));
$this->server->addPlugin(new CopyEtagHeaderPlugin());
$this->server->addPlugin(new ChunkingPlugin());
// allow setup of additional plugins
$dispatcher->dispatch('OCA\DAV\Connector\Sabre::addPlugin', $event);

View File

@ -0,0 +1,106 @@
<?php
/**
* @author Thomas Müller <thomas.mueller@tmit.eu>
*
* @copyright Copyright (c) 2017, ownCloud GmbH
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/
namespace OCA\DAV\Upload;
use OCA\DAV\Connector\Sabre\File;
use Sabre\DAV\Exception\BadRequest;
use Sabre\DAV\Server;
use Sabre\DAV\ServerPlugin;
class ChunkingPlugin extends ServerPlugin {
/** @var Server */
private $server;
/** @var FutureFile */
private $sourceNode;
/**
* @inheritdoc
*/
function initialize(Server $server) {
$server->on('beforeMove', [$this, 'beforeMove']);
$this->server = $server;
}
/**
* @param string $sourcePath source path
* @param string $destination destination path
*/
function beforeMove($sourcePath, $destination) {
$this->sourceNode = $this->server->tree->getNodeForPath($sourcePath);
if (!$this->sourceNode instanceof FutureFile) {
// skip handling as the source is not a chunked FutureFile
return;
}
$this->verifySize();
return $this->performMove($sourcePath, $destination);
}
/**
* Move handler for future file.
*
* This overrides the default move behavior to prevent Sabre
* to delete the target file before moving. Because deleting would
* lose the file id and metadata.
*
* @param string $path source path
* @param string $destination destination path
* @return bool|void false to stop handling, void to skip this handler
*/
public function performMove($path, $destination) {
if (!$this->server->tree->nodeExists($destination)) {
// skip and let the default handler do its work
return;
}
// do a move manually, skipping Sabre's default "delete" for existing nodes
$this->server->tree->move($path, $destination);
// trigger all default events (copied from CorePlugin::move)
$this->server->emit('afterMove', [$path, $destination]);
$this->server->emit('afterUnbind', [$path]);
$this->server->emit('afterBind', [$destination]);
$response = $this->server->httpResponse;
$response->setHeader('Content-Length', '0');
$response->setStatus(204);
return false;
}
/**
* @throws BadRequest
*/
private function verifySize() {
$expectedSize = $this->server->httpRequest->getHeader('OC-Total-Length');
if ($expectedSize === null) {
return;
}
$actualSize = $this->sourceNode->getSize();
if ((int)$expectedSize !== $actualSize) {
throw new BadRequest("Chunks on server do not sum up to $expectedSize but to $actualSize bytes");
}
}
}

View File

@ -78,6 +78,9 @@ class FileTest extends \Test\TestCase {
parent::tearDown();
}
/**
* @return \PHPUnit_Framework_MockObject_MockObject | Storage
*/
private function getMockStorage() {
$storage = $this->getMockBuilder(Storage::class)
->disableOriginalConstructor()
@ -165,6 +168,7 @@ class FileTest extends \Test\TestCase {
->setConstructorArgs([['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]])
->getMock();
\OC\Files\Filesystem::mount($storage, [], $this->user . '/');
/** @var View | \PHPUnit_Framework_MockObject_MockObject $view */
$view = $this->getMockBuilder(View::class)
->setMethods(['getRelativePath', 'resolvePath'])
->getMock();

View File

@ -43,8 +43,6 @@ use Sabre\DAV\Tree;
use Sabre\HTTP\RequestInterface;
use Sabre\HTTP\ResponseInterface;
use Test\TestCase;
use OCA\DAV\Upload\FutureFile;
use OCA\DAV\Connector\Sabre\Directory;
use OCP\Files\FileInfo;
/**
@ -600,59 +598,4 @@ class FilesPluginTest extends TestCase {
$this->assertEquals("false", $propFind->get(self::HAS_PREVIEW_PROPERTYNAME));
}
public function testBeforeMoveFutureFileSkip() {
$node = $this->createMock(Directory::class);
$this->tree->expects($this->any())
->method('getNodeForPath')
->with('source')
->will($this->returnValue($node));
$this->server->httpResponse->expects($this->never())
->method('setStatus');
$this->assertNull($this->plugin->beforeMoveFutureFile('source', 'target'));
}
public function testBeforeMoveFutureFileSkipNonExisting() {
$sourceNode = $this->createMock(FutureFile::class);
$this->tree->expects($this->any())
->method('getNodeForPath')
->with('source')
->will($this->returnValue($sourceNode));
$this->tree->expects($this->any())
->method('nodeExists')
->with('target')
->will($this->returnValue(false));
$this->server->httpResponse->expects($this->never())
->method('setStatus');
$this->assertNull($this->plugin->beforeMoveFutureFile('source', 'target'));
}
public function testBeforeMoveFutureFileMoveIt() {
$sourceNode = $this->createMock(FutureFile::class);
$this->tree->expects($this->any())
->method('getNodeForPath')
->with('source')
->will($this->returnValue($sourceNode));
$this->tree->expects($this->any())
->method('nodeExists')
->with('target')
->will($this->returnValue(true));
$this->tree->expects($this->once())
->method('move')
->with('source', 'target');
$this->server->httpResponse->expects($this->once())
->method('setHeader')
->with('Content-Length', '0');
$this->server->httpResponse->expects($this->once())
->method('setStatus')
->with(204);
$this->assertFalse($this->plugin->beforeMoveFutureFile('source', 'target'));
}
}

View File

@ -0,0 +1,167 @@
<?php
/**
* @author Thomas Müller <thomas.mueller@tmit.eu>
*
* @copyright Copyright (c) 2017, ownCloud GmbH
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/
namespace OCA\DAV\Tests\unit\Upload;
use OCA\DAV\Upload\ChunkingPlugin;
use Sabre\HTTP\RequestInterface;
use Sabre\HTTP\ResponseInterface;
use Test\TestCase;
use OCA\DAV\Upload\FutureFile;
use OCA\DAV\Connector\Sabre\Directory;
class ChunkingPluginTest extends TestCase {
/**
* @var \Sabre\DAV\Server | \PHPUnit_Framework_MockObject_MockObject
*/
private $server;
/**
* @var \Sabre\DAV\Tree | \PHPUnit_Framework_MockObject_MockObject
*/
private $tree;
/**
* @var ChunkingPlugin
*/
private $plugin;
/** @var RequestInterface | \PHPUnit_Framework_MockObject_MockObject */
private $request;
/** @var ResponseInterface | \PHPUnit_Framework_MockObject_MockObject */
private $response;
public function setUp() {
parent::setUp();
$this->server = $this->getMockBuilder('\Sabre\DAV\Server')
->disableOriginalConstructor()
->getMock();
$this->tree = $this->getMockBuilder('\Sabre\DAV\Tree')
->disableOriginalConstructor()
->getMock();
$this->server->tree = $this->tree;
$this->plugin = new ChunkingPlugin();
$this->request = $this->createMock(RequestInterface::class);
$this->response = $this->createMock(ResponseInterface::class);
$this->server->httpRequest = $this->request;
$this->server->httpResponse = $this->response;
$this->plugin->initialize($this->server);
}
public function testBeforeMoveFutureFileSkip() {
$node = $this->createMock(Directory::class);
$this->tree->expects($this->any())
->method('getNodeForPath')
->with('source')
->will($this->returnValue($node));
$this->response->expects($this->never())
->method('setStatus');
$this->assertNull($this->plugin->beforeMove('source', 'target'));
}
public function testBeforeMoveFutureFileSkipNonExisting() {
$sourceNode = $this->createMock(FutureFile::class);
$sourceNode->expects($this->once())
->method('getSize')
->willReturn(4);
$this->tree->expects($this->any())
->method('getNodeForPath')
->with('source')
->will($this->returnValue($sourceNode));
$this->tree->expects($this->any())
->method('nodeExists')
->with('target')
->will($this->returnValue(false));
$this->response->expects($this->never())
->method('setStatus');
$this->request->expects($this->once())
->method('getHeader')
->with('OC-Total-Length')
->willReturn(4);
$this->assertNull($this->plugin->beforeMove('source', 'target'));
}
public function testBeforeMoveFutureFileMoveIt() {
$sourceNode = $this->createMock(FutureFile::class);
$sourceNode->expects($this->once())
->method('getSize')
->willReturn(4);
$this->tree->expects($this->any())
->method('getNodeForPath')
->with('source')
->will($this->returnValue($sourceNode));
$this->tree->expects($this->any())
->method('nodeExists')
->with('target')
->will($this->returnValue(true));
$this->tree->expects($this->once())
->method('move')
->with('source', 'target');
$this->response->expects($this->once())
->method('setHeader')
->with('Content-Length', '0');
$this->response->expects($this->once())
->method('setStatus')
->with(204);
$this->request->expects($this->once())
->method('getHeader')
->with('OC-Total-Length')
->willReturn('4');
$this->assertFalse($this->plugin->beforeMove('source', 'target'));
}
/**
* @expectedException \Sabre\DAV\Exception\BadRequest
* @expectedExceptionMessage Chunks on server do not sum up to 4 but to 3 bytes
*/
public function testBeforeMoveSizeIsWrong() {
$sourceNode = $this->createMock(FutureFile::class);
$sourceNode->expects($this->once())
->method('getSize')
->willReturn(3);
$this->tree->expects($this->any())
->method('getNodeForPath')
->with('source')
->will($this->returnValue($sourceNode));
$this->request->expects($this->once())
->method('getHeader')
->with('OC-Total-Length')
->willReturn('4');
$this->assertFalse($this->plugin->beforeMove('source', 'target'));
}
}

View File

@ -260,11 +260,22 @@ OC.FileUpload.prototype = {
}
var uid = OC.getCurrentUser().uid;
var mtime = this.getFile().lastModified;
var size = this.getFile().size;
var headers = {};
if (mtime) {
headers['X-OC-Mtime'] = mtime / 1000;
}
if (size) {
headers['OC-Total-Length'] = size;
}
return this.uploader.davClient.move(
'uploads/' + encodeURIComponent(uid) + '/' + encodeURIComponent(this.getId()) + '/.file',
'files/' + encodeURIComponent(uid) + '/' + OC.joinPaths(this.getFullPath(), this.getFileName()),
true,
{'X-OC-Mtime': this.getFile().lastModified / 1000}
headers
);
},

View File

@ -595,6 +595,23 @@ trait WebDav {
], null, "uploads");
}
/**
* @Then user :user moves new chunk file with id :id to :dest with size :size
*/
public function userMovesNewChunkFileWithIdToMychunkedfileWithSize($user, $id, $dest, $size)
{
$source = '/uploads/' . $user . '/' . $id . '/.file';
$destination = substr($this->baseUrl, 0, -4) . $this->getDavFilesPath($user) . $dest;
try {
$this->response = $this->makeDavRequest($user, 'MOVE', $source, [
'Destination' => $destination,
'OC-Total-Length' => $size
], null, "uploads");
} catch(\GuzzleHttp\Exception\BadResponseException $ex) {
$this->response = $ex->getResponse();
}
}
/**
* @Given /^Downloading file "([^"]*)" as "([^"]*)"$/

View File

@ -578,3 +578,23 @@ Feature: webdav-related
And user "user0" created a folder "/testshare"
When User "user0" moves folder "/testshare" to "/hola%5Chola"
Then the HTTP status code should be "400"
Scenario: Upload file via new chunking endpoint with wrong size header
Given using new dav path
And user "user0" exists
And user "user0" creates a new chunking upload with id "chunking-42"
And user "user0" uploads new chunk file "1" with "AAAAA" to id "chunking-42"
And user "user0" uploads new chunk file "2" with "BBBBB" to id "chunking-42"
And user "user0" uploads new chunk file "3" with "CCCCC" to id "chunking-42"
When user "user0" moves new chunk file with id "chunking-42" to "/myChunkedFile.txt" with size 5
Then the HTTP status code should be "400"
Scenario: Upload file via new chunking endpoint with correct size header
Given using new dav path
And user "user0" exists
And user "user0" creates a new chunking upload with id "chunking-42"
And user "user0" uploads new chunk file "1" with "AAAAA" to id "chunking-42"
And user "user0" uploads new chunk file "2" with "BBBBB" to id "chunking-42"
And user "user0" uploads new chunk file "3" with "CCCCC" to id "chunking-42"
When user "user0" moves new chunk file with id "chunking-42" to "/myChunkedFile.txt" with size 15
Then the HTTP status code should be "201"