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 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
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 ('infinity' !== $moveInfo['depth']) {
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
11 changes: 7 additions & 4 deletions lib/DAV/ICopyTarget.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,14 @@ interface ICopyTarget extends ICollection
* is that the copy was successful.
* If you return false, sabre/dav will handle the copy itself.
*
* @param string $targetName new local file/collection name
* @param string $sourcePath Full path to source node
* @param INode $sourceNode Source node itself
* @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 @@
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 ('infinity' !== $depth && is_numeric($depth)) {
$depth = (int) $depth;
if ($depth < 0) {
throw new Exception\BadRequest('The HTTP Depth header may only be "infinity", 0 or a positiv number');

Check warning on line 734 in lib/DAV/Server.php

View check run for this annotation

Codecov / codecov/patch

lib/DAV/Server.php#L734

Added line #L734 was not covered by tests
}
}

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

// These are the three relevant properties we need to return
return [
'depth' => $depth,
'destination' => $destination,
'destinationExists' => (bool) $destinationNode,
'destinationNode' => $destinationNode,
Expand Down
43 changes: 29 additions & 14 deletions lib/DAV/Tree.php
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,13 @@
/**
* Copies a file from path to another.
*
* @param string $sourcePath The source location
* @param string $destinationPath The full destination 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 @@

$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 @@
$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 @@
$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 [];

Check warning on line 228 in lib/DAV/Tree.php

View check run for this annotation

Codecov / codecov/patch

lib/DAV/Tree.php#L228

Added line #L228 was not covered by tests
}
}

Expand Down Expand Up @@ -302,9 +310,10 @@
/**
* copyNode.
*
* @param string $destinationName
* @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 @@
$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 (0 !== $depth) {
// Adjust next depth for children (keep 'infinity' or decrease)
$depth = 'infinity' === $depth ? '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
54 changes: 54 additions & 0 deletions tests/Sabre/DAV/CorePluginTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,65 @@

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
Loading