[stable10] Let CopyPlugin fall back to default impl if no ICopySource

apps/dav/lib/DAV/CopyPlugin.php

@@ -35,8 +35,12 @@
 
 /**
  * Class CopyPlugin - adds own implementation of the COPY method.
- * This is necessary because we don't want the target to be deleted before the move.
  *
+ * Invokes ICopySource->copy() if the source and destination types match.
+ * If the source doesn't implement ICopySource, fall back to the default behavior.
+ *
+ * Currently only used for versions.
+ * This is necessary because we don't want the target to be deleted before the move.
  * Deleting the target will kill the versions which is the wrong behavior.
  *
  * @package OCA\DAV\DAV
@@ -57,9 +61,6 @@ function initialize(Server $server) {
 	/**
 	 * WebDAV HTTP COPY method
 	 *
-	 * This method copies one uri to a different uri, and works much like the MOVE request
-	 * A lot of the actual request processing is done in getCopyMoveInfo
-	 *
 	 * @param RequestInterface $request
 	 * @param ResponseInterface $response
 	 * @return bool
@@ -71,24 +72,20 @@ function httpCopy(RequestInterface $request, ResponseInterface $response) {
 
 			$path = $request->getPath();
 
-			$copyInfo = $this->server->getCopyAndMoveInfo($request);
 			$sourceNode = $this->server->tree->getNodeForPath($path);
+			if (!$sourceNode instanceof ICopySource) {
+				return true;
+			}
+
+			$copyInfo = $this->server->getCopyAndMoveInfo($request);
 			$destinationNode = $copyInfo['destinationNode'];
 			if (!$copyInfo['destinationExists'] || !$destinationNode instanceof File || !$sourceNode instanceof IFile) {
 				return true;
 			}
 
 			if (!$this->server->emit('beforeBind', [$copyInfo['destination']])) return false;
 
-			$copySuccess = false;
-			if ($sourceNode instanceof ICopySource) {
-				$copySuccess = $sourceNode->copy($destinationNode->getFileInfo()->getPath());
-			}
-			if (!$copySuccess) {
-				$destinationNode->acquireLock(ILockingProvider::LOCK_SHARED);
-				$destinationNode->put($sourceNode->get());
-				$destinationNode->releaseLock(ILockingProvider::LOCK_SHARED);
-			}
+			$sourceNode->copy($destinationNode->getFileInfo()->getPath());
 
 			$this->server->emit('afterBind', [$copyInfo['destination']]);
 

apps/dav/tests/unit/DAV/CopyPluginTest.php

@@ -33,6 +33,9 @@
 use Sabre\HTTP\RequestInterface;
 use Sabre\HTTP\ResponseInterface;
 use Test\TestCase;
+use OCP\Files\FileInfo;
+use OCP\Files\ForbiddenException;
+use OCA\DAV\Files\ICopySource;
 
 class CopyPluginTest extends TestCase {
 
@@ -71,7 +74,7 @@ public function setUp() {
 	public function testCopyPluginReturnTrue($destinationExists, $destinationNode, $sourceNode) {
 
 		$this->tree->expects($this->once())->method('getNodeForPath')->willReturn($sourceNode);
-		$this->server->expects($this->once())->method('getCopyAndMoveInfo')->willReturn([
+		$this->server->expects($this->any())->method('getCopyAndMoveInfo')->willReturn([
 			'destinationExists' => $destinationExists,
 			'destinationNode' => $destinationNode
 		]);
@@ -82,16 +85,16 @@ public function testCopyPluginReturnTrue($destinationExists, $destinationNode, $
 
 	public function providesSourceAndDestinations() {
 		return [
-			'destination does not exist' => [false, null, null],
-			'destination is not a File' => [true, $this->createMock(Directory::class), $this->createMock(IFile::class)],
-			'source is not a IFile' => [true, $this->createMock(File::class), $this->createMock(ICollection::class)],
+			'source is not a ICopySource' => [true, null, $this->createMock(IFile::class)],
+			'destination does not exist' => [false, null, $this->createMock(ICopySource::class)],
+			'destination is not a File' => [true, $this->createMock(Directory::class), $this->createMock(ICopySource::class)],
 		];
 	}
 
 	public function testCopyPluginReturnFalse() {
 
 		$destinationNode = $this->createMock(File::class);
-		$sourceNode = $this->createMock(IFile::class);
+		$sourceNode = $this->createMock([IFile::class, ICopySource::class]);
 
 		$this->tree->expects($this->once())->method('getNodeForPath')->willReturn($sourceNode);
 		$this->server->expects($this->once())->method('getCopyAndMoveInfo')->willReturn([
@@ -104,9 +107,12 @@ public function testCopyPluginReturnFalse() {
 		$this->server->expects($this->exactly(2))->method('emit')->withConsecutive(
 			['beforeBind', ['destination.txt']], ['afterBind', ['destination.txt']])->willReturn(true);
 
-		// make sure the file content is actually copied over
-		$sourceNode->expects($this->once())->method('get')->willReturn('123456');
-		$destinationNode->expects($this->once())->method('put')->with('123456');
+		$fileInfo = $this->createMock(FileInfo::class);
+		$fileInfo->method('getPath')->willReturn('path/to/destination.txt');
+
+		$sourceNode->expects($this->once())->method('copy')->with('path/to/destination.txt');
+
+		$destinationNode->expects($this->once())->method('getFileInfo')->willReturn($fileInfo);
 
 		// make sure http status and content length are properly set
 		$this->response->expects($this->once())->method('setHeader')->with('Content-Length', '0');
@@ -115,4 +121,38 @@ public function testCopyPluginReturnFalse() {
 		$returnValue = $this->plugin->httpCopy($this->request, $this->response);
 		$this->assertFalse($returnValue);
 	}
+
+	/**
+	 * @expectedException OCA\DAV\Connector\Sabre\Exception\Forbidden
+	 * @expectedExceptionMessage Test exception
+	 */
+	public function testCopyPluginRethrowForbidden() {
+
+		$destinationNode = $this->createMock(File::class);
+		$sourceNode = $this->createMock([ICopySource::class, IFile::class]);
+
+		$this->tree->expects($this->once())->method('getNodeForPath')->willReturn($sourceNode);
+		$this->server->expects($this->once())->method('getCopyAndMoveInfo')->willReturn([
+			'destinationExists' => true,
+			'destinationNode' => $destinationNode,
+			'destination' => 'destination.txt'
+		]);
+
+		// make sure the plugin properly emits beforeBind and afterBind
+		$this->server->expects($this->once(2))
+			->method('emit')
+			->with('beforeBind', ['destination.txt'])
+			->willReturn(true);
+
+		$sourceNode->expects($this->once())
+			->method('copy')
+			->will($this->throwException(new ForbiddenException('Test exception', false)));
+
+		$fileInfo = $this->createMock(FileInfo::class);
+		$fileInfo->method('isDeletable')->willReturn(true);
+
+		$destinationNode->expects($this->once())->method('getFileInfo')->willReturn($fileInfo);
+
+		$this->plugin->httpCopy($this->request, $this->response);
+	}
 }