Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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
1 change: 1 addition & 0 deletions apps/dav/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@
'OCA\\DAV\\Connector\\Sabre\\Server' => $baseDir . '/../lib/Connector/Sabre/Server.php',
'OCA\\DAV\\Connector\\Sabre\\ServerFactory' => $baseDir . '/../lib/Connector/Sabre/ServerFactory.php',
'OCA\\DAV\\Connector\\Sabre\\ShareTypeList' => $baseDir . '/../lib/Connector/Sabre/ShareTypeList.php',
'OCA\\DAV\\Connector\\Sabre\\ShareeList' => $baseDir . '/../lib/Connector/Sabre/ShareeList.php',
'OCA\\DAV\\Connector\\Sabre\\SharesPlugin' => $baseDir . '/../lib/Connector/Sabre/SharesPlugin.php',
'OCA\\DAV\\Connector\\Sabre\\TagList' => $baseDir . '/../lib/Connector/Sabre/TagList.php',
'OCA\\DAV\\Connector\\Sabre\\TagsPlugin' => $baseDir . '/../lib/Connector/Sabre/TagsPlugin.php',
Expand Down
1 change: 1 addition & 0 deletions apps/dav/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ class ComposerStaticInitDAV
'OCA\\DAV\\Connector\\Sabre\\Server' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Server.php',
'OCA\\DAV\\Connector\\Sabre\\ServerFactory' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ServerFactory.php',
'OCA\\DAV\\Connector\\Sabre\\ShareTypeList' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ShareTypeList.php',
'OCA\\DAV\\Connector\\Sabre\\ShareeList' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ShareeList.php',
'OCA\\DAV\\Connector\\Sabre\\SharesPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/SharesPlugin.php',
'OCA\\DAV\\Connector\\Sabre\\TagList' => __DIR__ . '/..' . '/../lib/Connector/Sabre/TagList.php',
'OCA\\DAV\\Connector\\Sabre\\TagsPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/TagsPlugin.php',
Expand Down
61 changes: 61 additions & 0 deletions apps/dav/lib/Connector/Sabre/ShareeList.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
<?php
declare(strict_types=1);
/**
* @copyright Copyright (c) 2019, Roeland Jago Douma <[email protected]>
*
* @author Roeland Jago Douma <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program 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 program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\DAV\Connector\Sabre;

use OCP\Share\IShare;
use Sabre\Xml\Element;
use Sabre\Xml\Reader;
use Sabre\Xml\Writer;
use Sabre\Xml\XmlSerializable;

/**
* This property contains multiple "sharee" elements, each containing a share sharee
*/
class ShareeList implements XmlSerializable {
const NS_NEXTCLOUD = 'http://nextcloud.org/ns';

/** @var IShare[] */
private $shares;

public function __construct(array $shares) {
$this->shares = $shares;
}

/**
* The xmlSerialize metod is called during xml writing.
*
* @param Writer $writer
* @return void
*/
function xmlSerialize(Writer $writer) {
foreach ($this->shares as $share) {
$writer->startElement('{' . self::NS_NEXTCLOUD . '}sharee');
$writer->writeElement('{' . self::NS_NEXTCLOUD . '}id', $share->getSharedWith());
$writer->writeElement('{' . self::NS_NEXTCLOUD . '}display-name', $share->getSharedWithDisplayName());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$writer->writeElement('{' . self::NS_NEXTCLOUD . '}display-name', $share->getSharedWithDisplayName());
$writer->writeElement('{' . self::NS_NEXTCLOUD . '}display-name', $share->getSharedWithDisplayName() ?? $share->getSharedWith());

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote a test for android library and there the problem also exists.
It seems that always one of the display names is set to userId.

$writer->writeElement('{' . self::NS_NEXTCLOUD . '}type', $share->getShareType());
$writer->endElement();
}
}
}
95 changes: 47 additions & 48 deletions apps/dav/lib/Connector/Sabre/SharesPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@
class SharesPlugin extends \Sabre\DAV\ServerPlugin {

const NS_OWNCLOUD = 'http://owncloud.org/ns';
const NS_NEXTCLOUD = 'http://nextcloud.org/ns';
const SHARETYPES_PROPERTYNAME = '{http://owncloud.org/ns}share-types';
const SHAREES_PROPERTYNAME = '{http://nextcloud.org/ns}sharees';

/**
* Reference to main server object
Expand Down Expand Up @@ -66,10 +68,8 @@ class SharesPlugin extends \Sabre\DAV\ServerPlugin {
*/
private $userFolder;

/**
* @var IShare[]
*/
private $cachedShareTypes;
/** @var IShare[] */
private $cachedShares = [];

private $cachedFolders = [];

Expand All @@ -89,7 +89,6 @@ public function __construct(
$this->shareManager = $shareManager;
$this->userFolder = $userFolder;
$this->userId = $userSession->getUser()->getUID();
$this->cachedShareTypes = [];
}

/**
Expand All @@ -106,20 +105,14 @@ public function initialize(\Sabre\DAV\Server $server) {
$server->xml->namespacesMap[self::NS_OWNCLOUD] = 'oc';
$server->xml->elementMap[self::SHARETYPES_PROPERTYNAME] = ShareTypeList::class;
$server->protectedProperties[] = self::SHARETYPES_PROPERTYNAME;
$server->protectedProperties[] = self::SHAREES_PROPERTYNAME;

$this->server = $server;
$this->server->on('propFind', array($this, 'handleGetProperties'));
}

/**
* Return a list of share types for outgoing shares
*
* @param \OCP\Files\Node $node file node
*
* @return int[] array of share types
*/
private function getShareTypes(\OCP\Files\Node $node) {
$shareTypes = [];
private function getShare(\OCP\Files\Node $node): array {
$result = [];
$requestedShareTypes = [
\OCP\Share::SHARE_TYPE_USER,
\OCP\Share::SHARE_TYPE_GROUP,
Expand All @@ -138,32 +131,40 @@ private function getShareTypes(\OCP\Files\Node $node) {
false,
1
);
if (!empty($shares)) {
$shareTypes[] = $requestedShareType;
foreach ($shares as $share) {
$result[] = $share;
}
}
return $shareTypes;
return $result;
}

private function getSharesTypesInFolder(\OCP\Files\Folder $node) {
$shares = $this->shareManager->getSharesInFolder(
private function getSharesFolder(\OCP\Files\Folder $node): array {
return $this->shareManager->getSharesInFolder(
$this->userId,
$node,
true
);
}

$shareTypesByFileId = [];

foreach($shares as $fileId => $sharesForFile) {
$types = array_map(function(IShare $share) {
return $share->getShareType();
}, $sharesForFile);
$types = array_unique($types);
sort($types);
$shareTypesByFileId[$fileId] = $types;
private function getShares(\Sabre\DAV\INode $sabreNode): array {
if (isset($this->cachedShares[$sabreNode->getId()])) {
$shares = $this->cachedShares[$sabreNode->getId()];
} else {
list($parentPath,) = \Sabre\Uri\split($sabreNode->getPath());
if ($parentPath === '') {
$parentPath = '/';
}
// if we already cached the folder this file is in we know there are no shares for this file
if (array_search($parentPath, $this->cachedFolders) === false) {
$node = $this->userFolder->get($sabreNode->getPath());
$shares = $this->getShare($node);
$this->cachedShares[$sabreNode->getId()] = $shares;
} else {
return [];
}
}

return $shareTypesByFileId;
return $shares;
}

/**
Expand All @@ -183,36 +184,34 @@ public function handleGetProperties(
// need prefetch ?
if ($sabreNode instanceof \OCA\DAV\Connector\Sabre\Directory
&& $propFind->getDepth() !== 0
&& !is_null($propFind->getStatus(self::SHARETYPES_PROPERTYNAME))
&& (
!is_null($propFind->getStatus(self::SHARETYPES_PROPERTYNAME)) ||
!is_null($propFind->getStatus(self::SHAREES_PROPERTYNAME))
)
) {
$folderNode = $this->userFolder->get($sabreNode->getPath());

$childShares = $this->getSharesTypesInFolder($folderNode);
$this->cachedFolders[] = $sabreNode->getPath();
$this->cachedShareTypes[$folderNode->getId()] = $this->getShareTypes($folderNode);
$childShares = $this->getSharesFolder($folderNode);
foreach ($childShares as $id => $shares) {
$this->cachedShareTypes[$id] = $shares;
$this->cachedShares[$id] = $shares;
}
}

$propFind->handle(self::SHARETYPES_PROPERTYNAME, function () use ($sabreNode) {
if (isset($this->cachedShareTypes[$sabreNode->getId()])) {
$shareTypes = $this->cachedShareTypes[$sabreNode->getId()];
} else {
list($parentPath,) = \Sabre\Uri\split($sabreNode->getPath());
if ($parentPath === '') {
$parentPath = '/';
}
// if we already cached the folder this file is in we know there are no shares for this file
if (array_search($parentPath, $this->cachedFolders) === false) {
$node = $this->userFolder->get($sabreNode->getPath());
$shareTypes = $this->getShareTypes($node);
} else {
return [];
}
}
$shares = $this->getShares($sabreNode);

$shareTypes = array_unique(array_map(function(IShare $share) {
return $share->getShareType();
}, $shares));

return new ShareTypeList($shareTypes);
});

$propFind->handle(self::SHAREES_PROPERTYNAME, function() use ($sabreNode) {
$shares = $this->getShares($sabreNode);

return new ShareeList($shares);
});
}
}
100 changes: 37 additions & 63 deletions apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,28 +68,17 @@ class SharesPluginTest extends \Test\TestCase {
public function setUp() {
parent::setUp();
$this->server = new \Sabre\DAV\Server();
$this->tree = $this->getMockBuilder(Tree::class)
->disableOriginalConstructor()
->getMock();
$this->shareManager = $this->getMockBuilder(IManager::class)
->disableOriginalConstructor()
->getMock();
$user = $this->getMockBuilder(IUser::class)
->disableOriginalConstructor()
->getMock();
$this->tree = $this->createMock(Tree::class);
$this->shareManager = $this->createMock(IManager::class);
$user = $this->createMock(IUser::class);
$user->expects($this->once())
->method('getUID')
->will($this->returnValue('user1'));
$userSession = $this->getMockBuilder(IUserSession::class)
->disableOriginalConstructor()
->getMock();
$userSession = $this->createMock(IUserSession::class);
$userSession->expects($this->once())
->method('getUser')
->will($this->returnValue($user));

$this->userFolder = $this->getMockBuilder(Folder::class)
->disableOriginalConstructor()
->getMock();
$this->userFolder = $this->createMock(Folder::class);

$this->plugin = new \OCA\DAV\Connector\Sabre\SharesPlugin(
$this->tree,
Expand Down Expand Up @@ -135,7 +124,10 @@ public function testGetProperties($shareTypes) {
)
->will($this->returnCallback(function($userId, $requestedShareType, $node, $flag, $limit) use ($shareTypes){
if (in_array($requestedShareType, $shareTypes)) {
return ['dummyshare'];
$share = $this->createMock(IShare::class);
$share->method('getShareType')
->willReturn($requestedShareType);
return [$share];
}
return [];
}));
Expand All @@ -162,30 +154,18 @@ public function testGetProperties($shareTypes) {
* @dataProvider sharesGetPropertiesDataProvider
*/
public function testPreloadThenGetProperties($shareTypes) {
$sabreNode1 = $this->getMockBuilder(File::class)
->disableOriginalConstructor()
->getMock();
$sabreNode1->expects($this->any())
->method('getId')
->will($this->returnValue(111));
$sabreNode1->expects($this->any())
->method('getPath');
$sabreNode2 = $this->getMockBuilder(File::class)
->disableOriginalConstructor()
->getMock();
$sabreNode2->expects($this->any())
->method('getId')
->will($this->returnValue(222));
$sabreNode2->expects($this->any())
->method('getPath')
->will($this->returnValue('/subdir/foo'));
$sabreNode1 = $this->createMock(File::class);
$sabreNode1->method('getId')
->willReturn(111);
$sabreNode2 = $this->createMock(File::class);
$sabreNode2->method('getId')
->willReturn(222);
$sabreNode2->method('getPath')
->willReturn('/subdir/foo');

$sabreNode = $this->getMockBuilder(Directory::class)
->disableOriginalConstructor()
->getMock();
$sabreNode->expects($this->any())
->method('getId')
->will($this->returnValue(123));
$sabreNode = $this->createMock(Directory::class);
$sabreNode->method('getId')
->willReturn(123);
// never, because we use getDirectoryListing from the Node API instead
$sabreNode->expects($this->never())
->method('getChildren');
Expand All @@ -194,29 +174,19 @@ public function testPreloadThenGetProperties($shareTypes) {
->will($this->returnValue('/subdir'));

// node API nodes
$node = $this->getMockBuilder(Folder::class)
->disableOriginalConstructor()
->getMock();
$node->expects($this->any())
->method('getId')
->will($this->returnValue(123));
$node1 = $this->getMockBuilder(File::class)
->disableOriginalConstructor()
->getMock();
$node1->expects($this->any())
->method('getId')
->will($this->returnValue(111));
$node2 = $this->getMockBuilder(File::class)
->disableOriginalConstructor()
->getMock();
$node2->expects($this->any())
->method('getId')
->will($this->returnValue(222));
$node = $this->createMock(Folder::class);
$node->method('getId')
->willReturn(123);
$node1 = $this->createMock(File::class);
$node1->method('getId')
->willReturn(111);
$node2 = $this->createMock(File::class);
$node2->method('getId')
->willReturn(222);

$this->userFolder->expects($this->once())
->method('get')
$this->userFolder->method('get')
->with('/subdir')
->will($this->returnValue($node));
->willReturn($node);

$dummyShares = array_map(function($type) {
$share = $this->getMockBuilder(IShare::class)->getMock();
Expand All @@ -235,9 +205,13 @@ public function testPreloadThenGetProperties($shareTypes) {
$this->equalTo(false),
$this->equalTo(1)
)
->will($this->returnCallback(function($userId, $requestedShareType, $node, $flag, $limit) use ($shareTypes){
->will($this->returnCallback(function($userId, $requestedShareType, $node, $flag, $limit) use ($shareTypes, $dummyShares){
if ($node->getId() === 111 && in_array($requestedShareType, $shareTypes)) {
return ['dummyshare'];
foreach ($dummyShares as $dummyShare) {
if ($dummyShare->getShareType() === $requestedShareType) {
return [$dummyShare];
}
}
}

return [];
Expand Down