From e667b28298add5b6ecc6a3956bfb93cb4a047097 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 14 Nov 2016 18:29:11 +0100 Subject: [PATCH] Fix files node API failed rename/copy Whenever a rename or copy operation failed on the view, we must throw an exception instead of just ignoring. Signed-off-by: Vincent Petry --- lib/private/Files/Node/File.php | 56 ++++------------------ lib/private/Files/Node/Folder.php | 55 ++++------------------ lib/private/Files/Node/Node.php | 78 ++++++++++++++++++++++++------- tests/lib/Files/Node/FileTest.php | 3 -- tests/lib/Files/Node/NodeTest.php | 47 ++++++++++++++++++- 5 files changed, 127 insertions(+), 112 deletions(-) diff --git a/lib/private/Files/Node/File.php b/lib/private/Files/Node/File.php index c4430b9181..4bfa5d583f 100644 --- a/lib/private/Files/Node/File.php +++ b/lib/private/Files/Node/File.php @@ -29,6 +29,16 @@ namespace OC\Files\Node; use OCP\Files\NotPermittedException; class File extends Node implements \OCP\Files\File { + /** + * Creates a Folder that represents a non-existing path + * + * @param string $path path + * @return string non-existing node class + */ + protected function createNonExistingNode($path) { + return new NonExistingFile($this->root, $this->view, $path); + } + /** * @return string * @throws \OCP\Files\NotPermittedException @@ -113,52 +123,6 @@ class File extends Node implements \OCP\Files\File { } } - /** - * @param string $targetPath - * @throws \OCP\Files\NotPermittedException - * @return \OC\Files\Node\Node - */ - public function copy($targetPath) { - $targetPath = $this->normalizePath($targetPath); - $parent = $this->root->get(dirname($targetPath)); - if ($parent instanceof Folder and $this->isValidPath($targetPath) and $parent->isCreatable()) { - $nonExisting = new NonExistingFile($this->root, $this->view, $targetPath); - $this->root->emit('\OC\Files', 'preCopy', array($this, $nonExisting)); - $this->root->emit('\OC\Files', 'preWrite', array($nonExisting)); - $this->view->copy($this->path, $targetPath); - $targetNode = $this->root->get($targetPath); - $this->root->emit('\OC\Files', 'postCopy', array($this, $targetNode)); - $this->root->emit('\OC\Files', 'postWrite', array($targetNode)); - return $targetNode; - } else { - throw new NotPermittedException(); - } - } - - /** - * @param string $targetPath - * @throws \OCP\Files\NotPermittedException - * @return \OC\Files\Node\Node - */ - public function move($targetPath) { - $targetPath = $this->normalizePath($targetPath); - $parent = $this->root->get(dirname($targetPath)); - if ($parent instanceof Folder and $this->isValidPath($targetPath) and $parent->isCreatable()) { - $nonExisting = new NonExistingFile($this->root, $this->view, $targetPath); - $this->root->emit('\OC\Files', 'preRename', array($this, $nonExisting)); - $this->root->emit('\OC\Files', 'preWrite', array($nonExisting)); - $this->view->rename($this->path, $targetPath); - $targetNode = $this->root->get($targetPath); - $this->root->emit('\OC\Files', 'postRename', array($this, $targetNode)); - $this->root->emit('\OC\Files', 'postWrite', array($targetNode)); - $this->path = $targetPath; - $this->fileInfo = null; - return $targetNode; - } else { - throw new NotPermittedException(); - } - } - /** * @param string $type * @param bool $raw diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php index 288a02ef20..fd907f708f 100644 --- a/lib/private/Files/Node/Folder.php +++ b/lib/private/Files/Node/Folder.php @@ -35,6 +35,16 @@ use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; class Folder extends Node implements \OCP\Files\Folder { + /** + * Creates a Folder that represents a non-existing path + * + * @param string $path path + * @return string non-existing node class + */ + protected function createNonExistingNode($path) { + return new NonExistingFolder($this->root, $this->view, $path); + } + /** * @param string $path path relative to the folder * @return string @@ -325,51 +335,6 @@ class Folder extends Node implements \OCP\Files\Folder { } } - /** - * @param string $targetPath - * @throws \OCP\Files\NotPermittedException - * @return \OC\Files\Node\Node - */ - public function copy($targetPath) { - $targetPath = $this->normalizePath($targetPath); - $parent = $this->root->get(dirname($targetPath)); - if ($parent instanceof Folder and $this->isValidPath($targetPath) and $parent->isCreatable()) { - $nonExisting = new NonExistingFolder($this->root, $this->view, $targetPath); - $this->root->emit('\OC\Files', 'preCopy', array($this, $nonExisting)); - $this->root->emit('\OC\Files', 'preWrite', array($nonExisting)); - $this->view->copy($this->path, $targetPath); - $targetNode = $this->root->get($targetPath); - $this->root->emit('\OC\Files', 'postCopy', array($this, $targetNode)); - $this->root->emit('\OC\Files', 'postWrite', array($targetNode)); - return $targetNode; - } else { - throw new NotPermittedException('No permission to copy to path'); - } - } - - /** - * @param string $targetPath - * @throws \OCP\Files\NotPermittedException - * @return \OC\Files\Node\Node - */ - public function move($targetPath) { - $targetPath = $this->normalizePath($targetPath); - $parent = $this->root->get(dirname($targetPath)); - if ($parent instanceof Folder and $this->isValidPath($targetPath) and $parent->isCreatable()) { - $nonExisting = new NonExistingFolder($this->root, $this->view, $targetPath); - $this->root->emit('\OC\Files', 'preRename', array($this, $nonExisting)); - $this->root->emit('\OC\Files', 'preWrite', array($nonExisting)); - $this->view->rename($this->path, $targetPath); - $targetNode = $this->root->get($targetPath); - $this->root->emit('\OC\Files', 'postRename', array($this, $targetNode)); - $this->root->emit('\OC\Files', 'postWrite', array($targetNode)); - $this->path = $targetPath; - return $targetNode; - } else { - throw new NotPermittedException('No permission to move to path'); - } - } - /** * Add a suffix to the name in case the file exists * diff --git a/lib/private/Files/Node/Node.php b/lib/private/Files/Node/Node.php index afda495cb2..e00debe690 100644 --- a/lib/private/Files/Node/Node.php +++ b/lib/private/Files/Node/Node.php @@ -33,6 +33,7 @@ use OCP\Files\InvalidPathException; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; +// FIXME: this class really should be abstract class Node implements \OCP\Files\Node { /** * @var \OC\Files\View $view @@ -67,6 +68,16 @@ class Node implements \OCP\Files\Node { $this->fileInfo = $fileInfo; } + /** + * Creates a Node of the same type that represents a non-existing path + * + * @param string $path path + * @return string non-existing node class + */ + protected function createNonExistingNode($path) { + throw new \Exception('Must be implemented by subclasses'); + } + /** * Returns the matching file info * @@ -106,27 +117,10 @@ class Node implements \OCP\Files\Node { return ($this->getPermissions() & $permissions) === $permissions; } - /** - * @param string $targetPath - * @throws \OCP\Files\NotPermittedException - * @return \OC\Files\Node\Node - */ - public function move($targetPath) { - return; - } - public function delete() { return; } - /** - * @param string $targetPath - * @return \OC\Files\Node\Node - */ - public function copy($targetPath) { - return; - } - /** * @param int $mtime * @throws \OCP\Files\NotPermittedException @@ -381,4 +375,54 @@ class Node implements \OCP\Files\Node { public function unlock($type) { $this->view->unlockFile($this->path, $type); } + + /** + * @param string $targetPath + * @throws \OCP\Files\NotPermittedException if copy not allowed or failed + * @return \OC\Files\Node\Node + */ + public function copy($targetPath) { + $targetPath = $this->normalizePath($targetPath); + $parent = $this->root->get(dirname($targetPath)); + if ($parent instanceof Folder and $this->isValidPath($targetPath) and $parent->isCreatable()) { + $nonExisting = $this->createNonExistingNode($targetPath); + $this->root->emit('\OC\Files', 'preCopy', [$this, $nonExisting]); + $this->root->emit('\OC\Files', 'preWrite', [$nonExisting]); + if (!$this->view->copy($this->path, $targetPath)) { + throw new NotPermittedException('Could not copy ' . $this->path . ' to ' . $targetPath); + } + $targetNode = $this->root->get($targetPath); + $this->root->emit('\OC\Files', 'postCopy', [$this, $targetNode]); + $this->root->emit('\OC\Files', 'postWrite', [$targetNode]); + return $targetNode; + } else { + throw new NotPermittedException('No permission to copy to path ' . $targetPath); + } + } + + /** + * @param string $targetPath + * @throws \OCP\Files\NotPermittedException if move not allowed or failed + * @return \OC\Files\Node\Node + */ + public function move($targetPath) { + $targetPath = $this->normalizePath($targetPath); + $parent = $this->root->get(dirname($targetPath)); + if ($parent instanceof Folder and $this->isValidPath($targetPath) and $parent->isCreatable()) { + $nonExisting = $this->createNonExistingNode($targetPath); + $this->root->emit('\OC\Files', 'preRename', [$this, $nonExisting]); + $this->root->emit('\OC\Files', 'preWrite', [$nonExisting]); + if (!$this->view->rename($this->path, $targetPath)) { + throw new NotPermittedException('Could not move ' . $this->path . ' to ' . $targetPath); + } + $targetNode = $this->root->get($targetPath); + $this->root->emit('\OC\Files', 'postRename', [$this, $targetNode]); + $this->root->emit('\OC\Files', 'postWrite', [$targetNode]); + $this->path = $targetPath; + return $targetNode; + } else { + throw new NotPermittedException('No permission to move to path ' . $targetPath); + } + } + } diff --git a/tests/lib/Files/Node/FileTest.php b/tests/lib/Files/Node/FileTest.php index 87402cd5f5..a17cc1d1a3 100644 --- a/tests/lib/Files/Node/FileTest.php +++ b/tests/lib/Files/Node/FileTest.php @@ -16,9 +16,6 @@ namespace Test\Files\Node; * @package Test\Files\Node */ class FileTest extends NodeTest { - - public $viewDeleteMethod = 'unlink'; - protected function createTestNode($root, $view, $path) { return new \OC\Files\Node\File($root, $view, $path); } diff --git a/tests/lib/Files/Node/NodeTest.php b/tests/lib/Files/Node/NodeTest.php index ce7250dc06..03f9118d9e 100644 --- a/tests/lib/Files/Node/NodeTest.php +++ b/tests/lib/Files/Node/NodeTest.php @@ -614,7 +614,6 @@ abstract class NodeTest extends \Test\TestCase { ->method('rename'); $node = $this->createTestNode($this->root, $this->view, '/bar/foo'); - $parentNode = new \OC\Files\Node\Folder($this->root, $this->view, '/bar'); $this->root->expects($this->once()) ->method('get') @@ -641,4 +640,50 @@ abstract class NodeTest extends \Test\TestCase { $node->move('/bar/asd'); } + + /** + * @expectedException \OCP\Files\NotPermittedException + */ + public function testMoveFailed() { + $this->view->expects($this->any()) + ->method('rename') + ->with('/bar/foo', '/bar/asd') + ->will($this->returnValue(false)); + + $this->view->expects($this->any()) + ->method('getFileInfo') + ->will($this->returnValue($this->getFileInfo(['permissions' => \OCP\Constants::PERMISSION_ALL, 'fileid' => 1]))); + + $node = $this->createTestNode($this->root, $this->view, '/bar/foo'); + $parentNode = new \OC\Files\Node\Folder($this->root, $this->view, '/bar'); + + $this->root->expects($this->any()) + ->method('get') + ->will($this->returnValueMap([['/bar', $parentNode], ['/bar/asd', $node]])); + + $node->move('/bar/asd'); + } + + /** + * @expectedException \OCP\Files\NotPermittedException + */ + public function testCopyFailed() { + $this->view->expects($this->any()) + ->method('copy') + ->with('/bar/foo', '/bar/asd') + ->will($this->returnValue(false)); + + $this->view->expects($this->any()) + ->method('getFileInfo') + ->will($this->returnValue($this->getFileInfo(['permissions' => \OCP\Constants::PERMISSION_ALL, 'fileid' => 1]))); + + $node = $this->createTestNode($this->root, $this->view, '/bar/foo'); + $parentNode = new \OC\Files\Node\Folder($this->root, $this->view, '/bar'); + + $this->root->expects($this->any()) + ->method('get') + ->will($this->returnValueMap([['/bar', $parentNode], ['/bar/asd', $node]])); + + $node->copy('/bar/asd'); + } }