Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion lib/private/Files/Storage/Common.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@
use OC\Files\Cache\Updater;
use OC\Files\Filesystem;
use OC\Files\Cache\Watcher;
use OC\Files\Storage\Wrapper\Jail;
use OC\Files\Storage\Wrapper\Wrapper;
use OCP\Files\EmptyFileNameException;
use OCP\Files\FileNameTooLongException;
use OCP\Files\InvalidCharacterInPathException;
Expand Down Expand Up @@ -635,14 +637,40 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
return (bool)$result;
}

/**
* Check if a storage is the same as the current one, including wrapped storages
*
* @param IStorage $storage
* @return bool
*/
private function isSameStorage(IStorage $storage): bool {
while ($storage->instanceOfStorage(Wrapper::class)) {
/**
* @var Wrapper $sourceStorage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @var Wrapper $sourceStorage
* @var Wrapper

*/
$storage = $storage->getWrapperStorage();
}

return $storage === $this;
}

/**
* @param IStorage $sourceStorage
* @param string $sourceInternalPath
* @param string $targetInternalPath
* @return bool
*/
public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
if ($sourceStorage === $this) {
if ($this->isSameStorage($sourceStorage)) {
// resolve any jailed paths
while ($sourceStorage->instanceOfStorage(Jail::class)) {
/**
* @var Jail $sourceStorage
*/
$sourceInternalPath = $sourceStorage->getUnjailedPath($sourceInternalPath);
$sourceStorage = $sourceStorage->getUnjailedStorage();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do this? It is not used

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is, in the next loop iteration

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right it loops... 🤦‍♂️

Copy link
Contributor

@kesselb kesselb Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same .... Pretty nice the while we loop the storage wrappers up thing.

}

return $this->rename($sourceInternalPath, $targetInternalPath);
}

Expand Down
8 changes: 8 additions & 0 deletions lib/private/Files/Storage/Wrapper/Jail.php
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,14 @@ public function getUnjailedPath($path) {
}
}

/**
* This is separate from Wrapper::getWrapperStorage so we can get the jailed storage consistently even if the jail is inside another wrapper
*/
public function getUnjailedStorage() {
return $this->storage;
}


public function getJailedPath($path) {
$root = rtrim($this->rootPath, '/') . '/';

Expand Down
117 changes: 97 additions & 20 deletions tests/lib/Files/Storage/CommonTest.php
Original file line number Diff line number Diff line change
@@ -1,27 +1,31 @@
<?php
/**
* ownCloud
*
* @author Robin Appelman
* @copyright 2012 Robin Appelman [email protected]
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE
* License as published by the Free Software Foundation; either
* version 3 of the License, or any later version.
*
* This library 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 along with this library. If not, see <http://www.gnu.org/licenses/>.
*
*/
* ownCloud
*
* @author Robin Appelman
* @copyright 2012 Robin Appelman [email protected]
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE
* License as published by the Free Software Foundation; either
* version 3 of the License, or any later version.
*
* This library 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 along with this library. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace Test\Files\Storage;

use OC\Files\Storage\Wrapper\Jail;
use OC\Files\Storage\Wrapper\Wrapper;
use PHPUnit\Framework\MockObject\MockObject;

/**
* Class CommonTest
*
Expand All @@ -34,15 +38,88 @@ class CommonTest extends Storage {
* @var string tmpDir
*/
private $tmpDir;

protected function setUp() {
parent::setUp();

$this->tmpDir = \OC::$server->getTempManager()->getTemporaryFolder();
$this->instance=new \OC\Files\Storage\CommonTest(array('datadir'=>$this->tmpDir));
$this->instance = new \OC\Files\Storage\CommonTest(['datadir' => $this->tmpDir]);
}

protected function tearDown() {
\OC_Helper::rmdirr($this->tmpDir);
parent::tearDown();
}

public function testMoveFromStorageWrapped() {
/** @var \OC\Files\Storage\CommonTest|MockObject $instance */
$instance = $this->getMockBuilder(\OC\Files\Storage\CommonTest::class)
->setMethods(['copyFromStorage', 'rmdir', 'unlink'])
->setConstructorArgs([['datadir' => $this->tmpDir]])
->getMock();
$instance->method('copyFromStorage')
->willThrowException(new \Exception('copy'));

$source = new Wrapper([
'storage' => $instance,
]);

$instance->file_put_contents('foo.txt', 'bar');
$instance->moveFromStorage($source, 'foo.txt', 'bar.txt');
$this->assertTrue($instance->file_exists('bar.txt'));
}

public function testMoveFromStorageJailed() {
/** @var \OC\Files\Storage\CommonTest|MockObject $instance */
$instance = $this->getMockBuilder(\OC\Files\Storage\CommonTest::class)
->setMethods(['copyFromStorage', 'rmdir', 'unlink'])
->setConstructorArgs([['datadir' => $this->tmpDir]])
->getMock();
$instance->method('copyFromStorage')
->willThrowException(new \Exception('copy'));

$source = new Jail([
'storage' => $instance,
'root' => 'foo'
]);
$source = new Wrapper([
'storage' => $source
]);

$instance->mkdir('foo');
$instance->file_put_contents('foo/foo.txt', 'bar');
$instance->moveFromStorage($source, 'foo.txt', 'bar.txt');
$this->assertTrue($instance->file_exists('bar.txt'));
}

public function testMoveFromStorageNestedJail() {
/** @var \OC\Files\Storage\CommonTest|MockObject $instance */
$instance = $this->getMockBuilder(\OC\Files\Storage\CommonTest::class)
->setMethods(['copyFromStorage', 'rmdir', 'unlink'])
->setConstructorArgs([['datadir' => $this->tmpDir]])
->getMock();
$instance->method('copyFromStorage')
->willThrowException(new \Exception('copy'));

$source = new Jail([
'storage' => $instance,
'root' => 'foo'
]);
$source = new Wrapper([
'storage' => $source
]);
$source = new Jail([
'storage' => $source,
'root' => 'bar'
]);
$source = new Wrapper([
'storage' => $source
]);

$instance->mkdir('foo');
$instance->mkdir('foo/bar');
$instance->file_put_contents('foo/bar/foo.txt', 'bar');
$instance->moveFromStorage($source, 'foo.txt', 'bar.txt');
$this->assertTrue($instance->file_exists('bar.txt'));
}
}