Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Handle Depth header for COPY as this is required by RFC #1495

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
11 changes: 8 additions & 3 deletions lib/DAV/CorePlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,11 @@ public function httpMove(RequestInterface $request, ResponseInterface $response)

$moveInfo = $this->server->getCopyAndMoveInfo($request);

// MOVE does only allow "infinity" every other header value is considered invalid
if ($moveInfo['depth'] !== 'infinity') {
throw new BadRequest('The HTTP Depth header must only contain "infinity" for MOVE');
}

if ($moveInfo['destinationExists']) {
if (!$this->server->emit('beforeUnbind', [$moveInfo['destination']])) {
return false;
Expand Down Expand Up @@ -645,7 +650,7 @@ public function httpCopy(RequestInterface $request, ResponseInterface $response)
if (!$this->server->emit('beforeBind', [$copyInfo['destination']])) {
return false;
}
if (!$this->server->emit('beforeCopy', [$path, $copyInfo['destination']])) {
if (!$this->server->emit('beforeCopy', [$path, $copyInfo['destination'], $copyInfo['depth']])) {
return false;
}

Expand All @@ -656,8 +661,8 @@ public function httpCopy(RequestInterface $request, ResponseInterface $response)
$this->server->tree->delete($copyInfo['destination']);
}

$this->server->tree->copy($path, $copyInfo['destination']);
$this->server->emit('afterCopy', [$path, $copyInfo['destination']]);
$this->server->tree->copy($path, $copyInfo['destination'], $copyInfo['depth']);
$this->server->emit('afterCopy', [$path, $copyInfo['destination'], $copyInfo['depth']]);
$this->server->emit('afterBind', [$copyInfo['destination']]);

// If a resource was overwritten we should send a 204, otherwise a 201
Expand Down
5 changes: 4 additions & 1 deletion lib/DAV/ICopyTarget.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,11 @@ interface ICopyTarget extends ICollection
* @param string $targetName new local file/collection name
* @param string $sourcePath Full path to source node
* @param INode $sourceNode Source node itself
* @param string|int $depth How many level of children to copy.
* The value can be 'infinity' or a positiv number including zero.
* Zero means to only copy a shallow collection with props but without children.
*
* @return bool
*/
public function copyInto($targetName, $sourcePath, INode $sourceNode);
public function copyInto($targetName, $sourcePath, INode $sourceNode, $depth);
}
14 changes: 11 additions & 3 deletions lib/DAV/Server.php
Original file line number Diff line number Diff line change
Expand Up @@ -725,10 +725,17 @@ public function getCopyAndMoveInfo(RequestInterface $request)
throw new Exception\BadRequest('The destination header was not supplied');
}
$destination = $this->calculateUri($request->getHeader('Destination'));
$overwrite = $request->getHeader('Overwrite');
if (!$overwrite) {
$overwrite = 'T';

// Depth of inifinty is valid for MOVE and COPY. If it is not set the RFC requires to act like it was 'infinity'.
$depth = strtolower($request->getHeader('Depth') ?? 'infinity');
if ($depth !== 'infinity' && is_numeric($depth)) {
Copy link
Member

Choose a reason for hiding this comment

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

In core plugin above it is stated that only infinity is allowed. Adding this logic here can reduce the necessary code changes a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DeepDiver1975 yes thats for MOVE, but for COPY we need to support also numeric values.

$depth = (int)$depth;
if ($depth < 0) {
throw new Exception\BadRequest('The HTTP Depth header may only be "infinity", 0 or a positiv number');
}
}

$overwrite = $request->getHeader('Overwrite') ?? 'T';
if ('T' == strtoupper($overwrite)) {
$overwrite = true;
} elseif ('F' == strtoupper($overwrite)) {
Expand Down Expand Up @@ -773,6 +780,7 @@ public function getCopyAndMoveInfo(RequestInterface $request)

// These are the three relevant properties we need to return
return [
'depth' => $depth,
'destination' => $destination,
'destinationExists' => (bool) $destinationNode,
'destinationNode' => $destinationNode,
Expand Down
37 changes: 26 additions & 11 deletions lib/DAV/Tree.php
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,11 @@ public function nodeExists($path)
*
* @param string $sourcePath The source location
* @param string $destinationPath The full destination path
* @param int|string $depth How much levle of children to copy.
* The value can be 'infinity' or a positiv integer, including zero.
* Zero means only copy the collection without children but with its properties.
*/
public function copy($sourcePath, $destinationPath)
public function copy($sourcePath, $destinationPath, $depth = 'infinity')
{
$sourceNode = $this->getNodeForPath($sourcePath);

Expand All @@ -147,8 +150,8 @@ public function copy($sourcePath, $destinationPath)

$destinationParent = $this->getNodeForPath($destinationDir);
// Check if the target can handle the copy itself. If not, we do it ourselves.
if (!$destinationParent instanceof ICopyTarget || !$destinationParent->copyInto($destinationName, $sourcePath, $sourceNode)) {
$this->copyNode($sourceNode, $destinationParent, $destinationName);
if (!$destinationParent instanceof ICopyTarget || !$destinationParent->copyInto($destinationName, $sourcePath, $sourceNode, $depth)) {
$this->copyNode($sourceNode, $destinationParent, $destinationName, $depth);
}

$this->markDirty($destinationDir);
Expand Down Expand Up @@ -178,7 +181,8 @@ public function move($sourcePath, $destinationPath)
$moveSuccess = $newParentNode->moveInto($destinationName, $sourcePath, $sourceNode);
}
if (!$moveSuccess) {
$this->copy($sourcePath, $destinationPath);
// Move is a copy with depth = infinity and deleting the source afterwards
$this->copy($sourcePath, $destinationPath, 'infinity');
$this->getNodeForPath($sourcePath)->delete();
}
}
Expand Down Expand Up @@ -215,9 +219,13 @@ public function getChildren($path)
$basePath .= '/';
}

foreach ($node->getChildren() as $child) {
$this->cache[$basePath.$child->getName()] = $child;
yield $child;
if ($node instanceof ICollection) {
foreach ($node->getChildren() as $child) {
$this->cache[$basePath.$child->getName()] = $child;
yield $child;
}
} else {
yield from [];
}
}

Expand Down Expand Up @@ -303,8 +311,9 @@ public function getMultipleNodes($paths)
* copyNode.
*
* @param string $destinationName
* @param int|string $depth How many children of the node to copy
*/
protected function copyNode(INode $source, ICollection $destinationParent, $destinationName = null)
protected function copyNode(INode $source, ICollection $destinationParent, ?string $destinationName = null, $depth = 'infinity')
{
if ('' === (string) $destinationName) {
$destinationName = $source->getName();
Expand All @@ -326,10 +335,16 @@ protected function copyNode(INode $source, ICollection $destinationParent, $dest
$destination = $destinationParent->getChild($destinationName);
} elseif ($source instanceof ICollection) {
$destinationParent->createDirectory($destinationName);

$destination = $destinationParent->getChild($destinationName);
foreach ($source->getChildren() as $child) {
$this->copyNode($child, $destination);

// Copy children if depth is not zero
if ($depth !== 0) {
// Adjust next depth for children (keep 'infinity' or decrease)
$depth = $depth === 'infinity' ? 'infinity' : $depth - 1;
$destination = $destinationParent->getChild($destinationName);
foreach ($source->getChildren() as $child) {
$this->copyNode($child, $destination, null, $depth);
}
}
}
if ($source instanceof IProperties && $destination instanceof IProperties) {
Expand Down
50 changes: 50 additions & 0 deletions tests/Sabre/DAV/CorePluginTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,61 @@

namespace Sabre\DAV;

use PHPUnit\Framework\MockObject\MockObject;
use Sabre\DAV\Exception\BadRequest;
use Sabre\HTTP;

class CorePluginTest extends \PHPUnit\Framework\TestCase
{
public function testGetInfo()
{
$corePlugin = new CorePlugin();
self::assertEquals('core', $corePlugin->getPluginInfo()['name']);
}

public function moveInvalidDepthHeaderProvider() {
return [
[0],
[1],
];
}

/**
* MOVE does only allow "infinity" every other header value is considered invalid
* @dataProvider moveInvalidDepthHeaderProvider
*/
public function testMoveWithInvalidDepth($depthHeader) {
$request = new HTTP\Request('MOVE', '/path/');
$response = new HTTP\Response();

/** @var Server|MockObject */
$server = $this->getMockBuilder(Server::class)->getMock();
$corePlugin = new CorePlugin();
$corePlugin->initialize($server);

$server->expects($this->once())
->method('getCopyAndMoveInfo')
->willReturn(['depth' => $depthHeader]);

$this->expectException(BadRequest::class);
$corePlugin->httpMove($request, $response);
}

/**
* MOVE does only allow "infinity" every other header value is considered invalid
*/
public function testMoveSupportsDepth() {
$request = new HTTP\Request('MOVE', '/path/');
$response = new HTTP\Response();

/** @var Server|MockObject */
$server = $this->getMockBuilder(Server::class)->getMock();
$corePlugin = new CorePlugin();
$corePlugin->initialize($server);

$server->expects($this->once())
->method('getCopyAndMoveInfo')
->willReturn(['depth' => 'infinity', 'destinationExists' => true, 'destination' => 'dst']);
$corePlugin->httpMove($request, $response);
}
}
5 changes: 4 additions & 1 deletion tests/Sabre/DAV/FSExt/ServerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,10 @@ public function testCopy()
{
mkdir($this->tempDir.'/testcol');

$request = new HTTP\Request('COPY', '/test.txt', ['Destination' => '/testcol/test2.txt']);
$request = new HTTP\Request('COPY', '/test.txt', [
'Destination' => '/testcol/test2.txt',
'Depth' => 'infinity',
]);
$this->server->httpRequest = ($request);
$this->server->exec();

Expand Down
48 changes: 45 additions & 3 deletions tests/Sabre/DAV/HttpCopyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,29 @@ class HttpCopyTest extends AbstractDAVServerTestCase
*/
public function setUpTree()
{
$this->tree = new Mock\Collection('root', [
$propsCollection = new Mock\PropertiesCollection('propscoll', [
'file3' => 'content3',
'file4' => 'content4',
], [
'my-prop' => 'my-value',
]);
$propsCollection->failMode = 'updatepropstrue';
$this->tree = new Mock\PropertiesCollection('root', [
'file1' => 'content1',
'file2' => 'content2',
'coll1' => [
'coll1' => new Mock\Collection('coll1', [
'file3' => 'content3',
'file4' => 'content4',
],
]),
'propscoll' => $propsCollection,
]);
}

public function testCopyFile()
{
$request = new HTTP\Request('COPY', '/file1', [
'Destination' => '/file5',
'Depth' => 'infinity',
]);
$response = $this->request($request);
self::assertEquals(201, $response->getStatus());
Expand All @@ -54,6 +63,7 @@ public function testCopyFileToExisting()
{
$request = new HTTP\Request('COPY', '/file1', [
'Destination' => '/file2',
'Depth' => 'infinity',
]);
$response = $this->request($request);
self::assertEquals(204, $response->getStatus());
Expand All @@ -64,6 +74,7 @@ public function testCopyFileToExistingOverwriteT()
{
$request = new HTTP\Request('COPY', '/file1', [
'Destination' => '/file2',
'Depth' => 'infinity',
'Overwrite' => 'T',
]);
$response = $this->request($request);
Expand All @@ -75,6 +86,7 @@ public function testCopyFileToExistingOverwriteBadValue()
{
$request = new HTTP\Request('COPY', '/file1', [
'Destination' => '/file2',
'Depth' => 'infinity',
'Overwrite' => 'B',
]);
$response = $this->request($request);
Expand All @@ -85,6 +97,7 @@ public function testCopyFileNonExistantParent()
{
$request = new HTTP\Request('COPY', '/file1', [
'Destination' => '/notfound/file2',
'Depth' => 'infinity',
]);
$response = $this->request($request);
self::assertEquals(409, $response->getStatus());
Expand All @@ -94,6 +107,7 @@ public function testCopyFileToExistingOverwriteF()
{
$request = new HTTP\Request('COPY', '/file1', [
'Destination' => '/file2',
'Depth' => 'infinity',
'Overwrite' => 'F',
]);
$response = $this->request($request);
Expand All @@ -110,6 +124,7 @@ public function testCopyFileToExistinBlockedCreateDestination()
});
$request = new HTTP\Request('COPY', '/file1', [
'Destination' => '/file2',
'Depth' => 'infinity',
'Overwrite' => 'T',
]);
$response = $this->request($request);
Expand All @@ -122,16 +137,39 @@ public function testCopyColl()
{
$request = new HTTP\Request('COPY', '/coll1', [
'Destination' => '/coll2',
'Depth' => 'infinity',
]);
$response = $this->request($request);
self::assertEquals(201, $response->getStatus());
self::assertEquals('content3', $this->tree->getChild('coll2')->getChild('file3')->get());
}

public function testShallowCopyColl()
{
// Ensure proppatches are applied
$this->tree->failMode = 'updatepropstrue';
$request = new HTTP\Request('COPY', '/propscoll', [
'Destination' => '/shallow-coll',
'Depth' => '0',
]);
$response = $this->request($request);
// reset
$this->tree->failMode = false;

self::assertEquals(201, $response->getStatus());
// The copied collection exists
self::assertEquals(true, $this->tree->childExists('shallow-coll'));
// But it does not contain children
self::assertEquals([], $this->tree->getChild('shallow-coll')->getChildren());
// But the properties are preserved
self::assertEquals(['my-prop' => 'my-value'], $this->tree->getChild('shallow-coll')->getProperties([]));
}

public function testCopyCollToSelf()
{
$request = new HTTP\Request('COPY', '/coll1', [
'Destination' => '/coll1',
'Depth' => 'infinity',
]);
$response = $this->request($request);
self::assertEquals(403, $response->getStatus());
Expand All @@ -141,6 +179,7 @@ public function testCopyCollToExisting()
{
$request = new HTTP\Request('COPY', '/coll1', [
'Destination' => '/file2',
'Depth' => 'infinity',
]);
$response = $this->request($request);
self::assertEquals(204, $response->getStatus());
Expand All @@ -151,6 +190,7 @@ public function testCopyCollToExistingOverwriteT()
{
$request = new HTTP\Request('COPY', '/coll1', [
'Destination' => '/file2',
'Depth' => 'infinity',
'Overwrite' => 'T',
]);
$response = $this->request($request);
Expand All @@ -162,6 +202,7 @@ public function testCopyCollToExistingOverwriteF()
{
$request = new HTTP\Request('COPY', '/coll1', [
'Destination' => '/file2',
'Depth' => 'infinity',
'Overwrite' => 'F',
]);
$response = $this->request($request);
Expand All @@ -173,6 +214,7 @@ public function testCopyCollIntoSubtree()
{
$request = new HTTP\Request('COPY', '/coll1', [
'Destination' => '/coll1/subcol',
'Depth' => 'infinity',
]);
$response = $this->request($request);
self::assertEquals(409, $response->getStatus());
Expand Down
2 changes: 2 additions & 0 deletions tests/Sabre/DAV/Locks/PluginTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,7 @@ public function testLockCopyLockSource()

$request = new HTTP\Request('COPY', '/dir/child.txt', [
'Destination' => '/dir/child2.txt',
'Depth' => 'infinity',
]);

$this->server->httpRequest = $request;
Expand Down Expand Up @@ -619,6 +620,7 @@ public function testLockCopyLockDestination()

$request = new HTTP\Request('COPY', '/dir/child.txt', [
'Destination' => '/dir/child2.txt',
'Depth' => 'infinity',
]);
$this->server->httpRequest = $request;
$this->server->exec();
Expand Down
Loading
Loading