From 975b2f40d7f4f7f4ba82e2c4b78dad6bceb63ef4 Mon Sep 17 00:00:00 2001 From: Piotr Mrowczynski Date: Fri, 28 Jul 2017 17:19:34 +0200 Subject: [PATCH] Adjust integration tests for validation plugin --- .../Connector/Sabre/ValidateRequestPlugin.php | 37 ++++++++---- .../Sabre/ValidateRequestPluginTest.php | 18 +----- .../features/bootstrap/Checksums.php | 22 ++++--- .../integration/features/bootstrap/WebDav.php | 57 ++++++++++--------- tests/integration/features/checksums.feature | 25 +++----- .../webdav-related-new-endpoint.feature | 32 +++++++++++ 6 files changed, 109 insertions(+), 82 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/ValidateRequestPlugin.php b/apps/dav/lib/Connector/Sabre/ValidateRequestPlugin.php index 4cd82efa4221..f26b50c3d44a 100644 --- a/apps/dav/lib/Connector/Sabre/ValidateRequestPlugin.php +++ b/apps/dav/lib/Connector/Sabre/ValidateRequestPlugin.php @@ -21,16 +21,23 @@ namespace OCA\DAV\Connector\Sabre; -use OCP\IConfig; use Sabre\DAV\Exception\ServiceUnavailable; use Sabre\DAV\ServerPlugin; +/** + * Class ValidateRequestPlugin is a plugin which examines correctness of + * the webdav request, checking if accessing a service e.g. "dav" + * the request has proper headers/structure. + * + * @see https://github.com/owncloud/core/issues/28200 + * @package OCA\DAV\Connector\Sabre + */ class ValidateRequestPlugin extends ServerPlugin { /** * Reference to main server object * - * @var Server + * @var \Sabre\DAV\Server */ private $server; @@ -41,6 +48,17 @@ class ValidateRequestPlugin extends ServerPlugin { */ private $service; + /** + * This plugin ensures that all request directed to specific + * services (type $service as decided by resolveService($service) in remote.php) + * contain correct headers and their structure is correct + * + * Currently supported: + * 'webdav' + * 'dav' + * + * @var string $service + */ public function __construct($service) { $this->service = $service; } @@ -58,7 +76,7 @@ public function __construct($service) { */ public function initialize(\Sabre\DAV\Server $server) { $this->server = $server; - $this->server->on('beforeMethod', [$this, 'checkValidity'], 1); + $this->server->on('beforeMethod:PUT', [$this, 'checkValidityPut'], 100); } /** @@ -68,18 +86,13 @@ public function initialize(\Sabre\DAV\Server $server) { * @throws ServiceUnavailable * @return bool */ - public function checkValidity() { + public function checkValidityPut() { $request = $this->server->httpRequest; - $method = $request->getMethod(); // Verify if optional OC headers are routed in the proper endpoint - if ($method === 'PUT' - && $request->hasHeader('OC-Chunk-Offset') - && ($this->service != 'dav')) { - throw new ServiceUnavailable('Specified OC-Chunk-Offset header is allowed only in dav endpoint'); - } else if ($method === 'PUT' - && ($request->hasHeader('HTTP_OC_CHUNKED') || $request->hasHeader('Oc-Chunked')) - && ($this->service != 'webdav')) { + if (($request->hasHeader('HTTP_OC_CHUNKED') || $request->hasHeader('Oc-Chunked')) + && ($this->service !== 'webdav')) { + // Headers not allowed in new dav endpoint throw new ServiceUnavailable('Specified header (HTTP_OC_CHUNKED/OC-Chunked header) is allowed only in webdav endpoint'); } diff --git a/apps/dav/tests/unit/Connector/Sabre/ValidateRequestPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/ValidateRequestPluginTest.php index 932a92cdc8b0..964fda2bd6f9 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ValidateRequestPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ValidateRequestPluginTest.php @@ -43,20 +43,6 @@ public function setUp() { $this->server = new \Sabre\DAV\Server(); } - /** - * @expectedException \Sabre\DAV\Exception\ServiceUnavailable - * @expectedExceptionMessage Specified OC-Chunk-Offset header is allowed only in dav endpoint - */ - public function testNewChunkingInOldEndpoint() { - $headers['OC-Chunk-Offset'] = 0; - $this->plugin = new ValidateRequestPlugin('webdav'); - - $this->server->httpRequest = new \Sabre\HTTP\Request('PUT', 'http://server/root_url/remote.php/webdav/testchunkold.txt', $headers); - $this->plugin->initialize($this->server); - - $this->plugin->checkValidity(); - } - /** * @expectedException \Sabre\DAV\Exception\ServiceUnavailable * @expectedExceptionMessage Specified header (HTTP_OC_CHUNKED/OC-Chunked header) is allowed only in webdav endpoint @@ -68,7 +54,7 @@ public function testOldChunkingInNewEndpoint1() { $this->server->httpRequest = new \Sabre\HTTP\Request('PUT', 'http://server/root_url/remote.php/dav/files/user/testnew.txt', $headers); $this->plugin->initialize($this->server); - $this->plugin->checkValidity(); + $this->plugin->checkValidityPut(); } /** @@ -82,7 +68,7 @@ public function testOldChunkingInNewEndpoint2() { $this->server->httpRequest = new \Sabre\HTTP\Request('PUT', 'http://server/root_url/remote.php/dav/files/user/testnew.txt', $headers); $this->plugin->initialize($this->server); - $this->plugin->checkValidity(); + $this->plugin->checkValidityPut(); } } diff --git a/tests/integration/features/bootstrap/Checksums.php b/tests/integration/features/bootstrap/Checksums.php index 76f3b911ae26..41f92aeb5ac9 100644 --- a/tests/integration/features/bootstrap/Checksums.php +++ b/tests/integration/features/bootstrap/Checksums.php @@ -159,14 +159,18 @@ public function theOcChecksumHeaderShouldNotBeThere() * @param string $checksum */ public function userUploadsChunkFileOfWithToWithChecksum($user, $num, $total, $data, $destination, $checksum) { - $num -= 1; - $data = \GuzzleHttp\Stream\Stream::factory($data); - $file = $destination . '-chunking-42-' . $total . '-' . $num; - $this->response = $this->makeDavRequest($user, - 'PUT', - $file, - ['OC-Checksum' => $checksum, 'OC-Chunked' => '1'], - $data, - "files"); + try { + $num -= 1; + $data = \GuzzleHttp\Stream\Stream::factory($data); + $file = $destination . '-chunking-42-' . $total . '-' . $num; + $this->response = $this->makeDavRequest($user, + 'PUT', + $file, + ['OC-Checksum' => $checksum, 'OC-Chunked' => '1'], + $data, + "files"); + } catch (\GuzzleHttp\Exception\RequestException $ex) { + $this->response = $ex->getResponse(); + } } } diff --git a/tests/integration/features/bootstrap/WebDav.php b/tests/integration/features/bootstrap/WebDav.php index 10c67dda5d63..1cb375232c96 100644 --- a/tests/integration/features/bootstrap/WebDav.php +++ b/tests/integration/features/bootstrap/WebDav.php @@ -680,19 +680,6 @@ public function uploadWithAllMechanisms($user, $source, $destination, $overwrite } } if ($dav === 'new') { - // old chunking style applied to new endpoint 🙈 - if (!$overwriteMode) { - $suffix = '-' . $dav . 'dav-oldchunking'; - } - try { - // FIXME: prepending new dav path because the chunking utility functions are messed up - $this->userUploadsAFileToWithChunks($user, $source, '/files/' . $user . '/' . ltrim($destination, '/') . $suffix, 'old'); - $responses[] = $this->response; - } catch (ServerException $e) { - $responses[] = $e->getResponse(); - } - - // new chunking style applied to new endpoint if (!$overwriteMode) { $suffix = '-' . $dav . 'dav-newchunking'; } @@ -834,10 +821,14 @@ public function userCreatedAFolder($user, $destination){ */ public function userUploadsChunkedFile($user, $num, $total, $data, $destination) { - $num -= 1; - $data = \GuzzleHttp\Stream\Stream::factory($data); - $file = $destination . '-chunking-42-' . $total . '-' . $num; - $this->makeDavRequest($user, 'PUT', $file, ['OC-Chunked' => '1'], $data, "uploads"); + try { + $num -= 1; + $data = \GuzzleHttp\Stream\Stream::factory($data); + $file = $destination . '-chunking-42-' . $total . '-' . $num; + $this->makeDavRequest($user, 'PUT', $file, ['OC-Chunked' => '1'], $data, "uploads"); + } catch (\GuzzleHttp\Exception\RequestException $ex) { + $this->response = $ex->getResponse(); + } } /** @@ -845,8 +836,12 @@ public function userUploadsChunkedFile($user, $num, $total, $data, $destination) */ public function userCreatesANewChunkingUploadWithId($user, $id) { - $destination = '/uploads/'.$user.'/'.$id; - $this->makeDavRequest($user, 'MKCOL', $destination, [], null, "uploads"); + try { + $destination = '/uploads/'.$user.'/'.$id; + $this->makeDavRequest($user, 'MKCOL', $destination, [], null, "uploads"); + } catch (\GuzzleHttp\Exception\RequestException $ex) { + $this->response = $ex->getResponse(); + } } /** @@ -854,9 +849,13 @@ public function userCreatesANewChunkingUploadWithId($user, $id) */ public function userUploadsNewChunkFileOfWithToId($user, $num, $data, $id) { - $data = \GuzzleHttp\Stream\Stream::factory($data); - $destination = '/uploads/'. $user .'/'. $id .'/' . $num; - $this->makeDavRequest($user, 'PUT', $destination, [], $data, "uploads"); + try { + $data = \GuzzleHttp\Stream\Stream::factory($data); + $destination = '/uploads/'. $user .'/'. $id .'/' . $num; + $this->makeDavRequest($user, 'PUT', $destination, [], $data, "uploads"); + } catch (\GuzzleHttp\Exception\RequestException $ex) { + $this->response = $ex->getResponse(); + } } /** @@ -885,11 +884,15 @@ public function userUploadsNewChunkFileOfWithToIdWithChecksum($user, $num, $data */ public function userMovesNewChunkFileWithIdToMychunkedfile($user, $id, $dest) { - $source = '/uploads/' . $user . '/' . $id . '/.file'; - $destination = $this->baseUrlWithoutOCSAppendix() . $this->getDavFilesPath($user) . $dest; - $this->makeDavRequest($user, 'MOVE', $source, [ - 'Destination' => $destination - ], null, "uploads"); + try { + $source = '/uploads/' . $user . '/' . $id . '/.file'; + $destination = $this->baseUrlWithoutOCSAppendix() . $this->getDavFilesPath($user) . $dest; + $this->makeDavRequest($user, 'MOVE', $source, [ + 'Destination' => $destination + ], null, "uploads"); + } catch (\GuzzleHttp\Exception\RequestException $ex) { + $this->response = $ex->getResponse(); + } } diff --git a/tests/integration/features/checksums.feature b/tests/integration/features/checksums.feature index 1c178a3fea94..50618cd0b166 100644 --- a/tests/integration/features/checksums.feature +++ b/tests/integration/features/checksums.feature @@ -115,24 +115,6 @@ Feature: checksums And user "user0" downloads the file "/myChecksumFileCopy.txt" Then The header checksum should match "SHA1:3ee962b839762adb0ad8ba6023a4690be478de6f" - Scenario: Uploading a chunked file with checksum should return the checksum in the propfind using new dav path - Given using new dav path - And user "user0" exists - And user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" - And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" - And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" - When user "user0" request the checksum of "/myChecksumFile.txt" via propfind - Then The webdav checksum should match "SHA1:acfa6b1565f9710d4d497c6035d5c069bd35a8e8 MD5:45a72715acdd5019c5be30bdbb75233e ADLER32:1ecd03df" - - Scenario: Uploading a chunked file with checksum should return the checksum in the download header using new dav path - Given using new dav path - And user "user0" exists - And user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" - And user "user0" uploads chunk file "2" of "3" with "BBBBB" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" - And user "user0" uploads chunk file "3" of "3" with "CCCCC" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" - When user "user0" downloads the file "/myChecksumFile.txt" - Then The header checksum should match "SHA1:acfa6b1565f9710d4d497c6035d5c069bd35a8e8" - @local_storage Scenario: Downloading a file from local storage has correct checksum using new dav path Given using new dav path @@ -183,3 +165,10 @@ Feature: checksums Then The following headers should be set | OC-Checksum | SHA1:ce5582148c6f0c1282335b87df5ed4be4b781399 | + ## Validation Plugin or Old Endpoint Specific + + Scenario: Uploading a old method chunked file with checksum should fail using new dav path + Given using new dav path + And user "user0" exists + When user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChecksumFile.txt" with checksum "MD5:45a72715acdd5019c5be30bdbb75233e" + Then the HTTP status code should be "503" diff --git a/tests/integration/features/webdav-related-new-endpoint.feature b/tests/integration/features/webdav-related-new-endpoint.feature index df2510cca4ca..3c65b636b3f6 100644 --- a/tests/integration/features/webdav-related-new-endpoint.feature +++ b/tests/integration/features/webdav-related-new-endpoint.feature @@ -548,3 +548,35 @@ Feature: webdav-related-new-endpoint And as "user1" the folder "/folderB/ONE/TWO" exists And User "user1" checks id of file "/folderB/ONE" + ## Validation Plugin or Old Endpoint Specific + + Scenario: New chunked upload MKDIR using old dav path should fail + Given using old dav path + And user "user0" exists + When user "user0" creates a new chunking upload with id "chunking-42" + Then the HTTP status code should be "409" + + Scenario: New chunked upload PUT using old dav path should fail + Given using new dav path + And user "user0" exists + And user "user0" creates a new chunking upload with id "chunking-42" + When using old dav path + And user "user0" uploads new chunk file "1" with "AAAAA" to id "chunking-42" + Then the HTTP status code should be "404" + + Scenario: New chunked upload MOVE using old dav path should fail + 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 "2" with "BBBBB" to id "chunking-42" + And user "user0" uploads new chunk file "3" with "CCCCC" to id "chunking-42" + And user "user0" uploads new chunk file "1" with "AAAAA" to id "chunking-42" + When using old dav path + And user "user0" moves new chunk file with id "chunking-42" to "/myChunkedFile.txt" + Then the HTTP status code should be "404" + + Scenario: Upload to new dav path using old way should fail + Given using new dav path + And user "user0" exists + When user "user0" uploads chunk file "1" of "3" with "AAAAA" to "/myChunkedFile.txt" + Then the HTTP status code should be "503" \ No newline at end of file