From 8c0ef4c4bda4836aa7f4f3b180d1d132c8ca9879 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 3 Mar 2016 20:58:18 +0100 Subject: [PATCH 1/3] Add sharePermissions webdav property This property can be queries by the clients so they know the max permissions they can use to share a file with. This will improve the UX. The oc:permissions proptery is not enough since mountpoints have different permissions (delete + move by default). By making it a new property the clients can just request it. On older servers it will just return a 404 for that property (and thus they know they have to fall back to their hacky work arounds). But if the property is returned the client can show proper info. * unit tests * intergration test --- apps/dav/lib/connector/sabre/filesplugin.php | 6 + apps/dav/lib/connector/sabre/node.php | 33 ++++++ apps/dav/tests/unit/connector/sabre/node.php | 61 ++++++++++ .../integration/features/bootstrap/WebDav.php | 14 +++ build/integration/features/sharing-v1.feature | 104 ++++++++++++++++++ 5 files changed, 218 insertions(+) diff --git a/apps/dav/lib/connector/sabre/filesplugin.php b/apps/dav/lib/connector/sabre/filesplugin.php index fb2af554c6..8b54291793 100644 --- a/apps/dav/lib/connector/sabre/filesplugin.php +++ b/apps/dav/lib/connector/sabre/filesplugin.php @@ -45,6 +45,7 @@ class FilesPlugin extends \Sabre\DAV\ServerPlugin { const FILEID_PROPERTYNAME = '{http://owncloud.org/ns}id'; const INTERNAL_FILEID_PROPERTYNAME = '{http://owncloud.org/ns}fileid'; const PERMISSIONS_PROPERTYNAME = '{http://owncloud.org/ns}permissions'; + const SHARE_PERMISSIONS_PROPERTYNAME = '{http://owncloud.org/ns}share-permissions'; const DOWNLOADURL_PROPERTYNAME = '{http://owncloud.org/ns}downloadURL'; const SIZE_PROPERTYNAME = '{http://owncloud.org/ns}size'; const GETETAG_PROPERTYNAME = '{DAV:}getetag'; @@ -116,6 +117,7 @@ class FilesPlugin extends \Sabre\DAV\ServerPlugin { $server->protectedProperties[] = self::FILEID_PROPERTYNAME; $server->protectedProperties[] = self::INTERNAL_FILEID_PROPERTYNAME; $server->protectedProperties[] = self::PERMISSIONS_PROPERTYNAME; + $server->protectedProperties[] = self::SHARE_PERMISSIONS_PROPERTYNAME; $server->protectedProperties[] = self::SIZE_PROPERTYNAME; $server->protectedProperties[] = self::DOWNLOADURL_PROPERTYNAME; $server->protectedProperties[] = self::OWNER_ID_PROPERTYNAME; @@ -245,6 +247,10 @@ class FilesPlugin extends \Sabre\DAV\ServerPlugin { return $perms; }); + $propFind->handle(self::SHARE_PERMISSIONS_PROPERTYNAME, function() use ($node) { + return $node->getSharePermissions(); + }); + $propFind->handle(self::GETETAG_PROPERTYNAME, function() use ($node) { return $node->getETag(); }); diff --git a/apps/dav/lib/connector/sabre/node.php b/apps/dav/lib/connector/sabre/node.php index 95a5f0a874..86e1844687 100644 --- a/apps/dav/lib/connector/sabre/node.php +++ b/apps/dav/lib/connector/sabre/node.php @@ -213,6 +213,39 @@ abstract class Node implements \Sabre\DAV\INode { return $this->info->getId(); } + /** + * @return int + */ + public function getSharePermissions() { + $storage = $this->info->getStorage(); + + $path = $this->info->getInternalPath(); + + if ($storage->instanceOfStorage('\OC\Files\Storage\Shared')) { + /** @var \OC\Files\Storage\Shared $storage */ + $permissions = (int)$storage->getShare()['permissions']; + } else { + $permissions = $storage->getPermissions($path); + } + + /* + * Without sharing permissions there are also no other permissions + */ + if (!($permissions & \OCP\Constants::PERMISSION_SHARE) || + !($permissions & \OCP\Constants::PERMISSION_READ)) { + return 0; + } + + /* + * Files can't have create or delete permissions + */ + if ($this->info->getType() === \OCP\Files\FileInfo::TYPE_FILE) { + $permissions &= ~(\OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_DELETE); + } + + return $permissions; + } + /** * @return string */ diff --git a/apps/dav/tests/unit/connector/sabre/node.php b/apps/dav/tests/unit/connector/sabre/node.php index 8c92c2f063..c9fe6e6027 100644 --- a/apps/dav/tests/unit/connector/sabre/node.php +++ b/apps/dav/tests/unit/connector/sabre/node.php @@ -63,4 +63,65 @@ class Node extends \Test\TestCase { $node = new \OCA\DAV\Connector\Sabre\File($view, $info); $this->assertEquals($expected, $node->getDavPermissions()); } + + public function sharePermissionsProvider() { + return [ + [\OCP\Files\FileInfo::TYPE_FILE, 1, 0], + [\OCP\Files\FileInfo::TYPE_FILE, 3, 0], + [\OCP\Files\FileInfo::TYPE_FILE, 5, 0], + [\OCP\Files\FileInfo::TYPE_FILE, 7, 0], + [\OCP\Files\FileInfo::TYPE_FILE, 9, 0], + [\OCP\Files\FileInfo::TYPE_FILE, 11, 0], + [\OCP\Files\FileInfo::TYPE_FILE, 13, 0], + [\OCP\Files\FileInfo::TYPE_FILE, 15, 0], + [\OCP\Files\FileInfo::TYPE_FILE, 17, 17], + [\OCP\Files\FileInfo::TYPE_FILE, 19, 19], + [\OCP\Files\FileInfo::TYPE_FILE, 21, 17], + [\OCP\Files\FileInfo::TYPE_FILE, 23, 19], + [\OCP\Files\FileInfo::TYPE_FILE, 25, 17], + [\OCP\Files\FileInfo::TYPE_FILE, 27, 19], + [\OCP\Files\FileInfo::TYPE_FILE, 29, 17], + [\OCP\Files\FileInfo::TYPE_FILE, 30, 0], + [\OCP\Files\FileInfo::TYPE_FILE, 31, 19], + [\OCP\Files\FileInfo::TYPE_FOLDER, 1, 0], + [\OCP\Files\FileInfo::TYPE_FOLDER, 3, 0], + [\OCP\Files\FileInfo::TYPE_FOLDER, 5, 0], + [\OCP\Files\FileInfo::TYPE_FOLDER, 7, 0], + [\OCP\Files\FileInfo::TYPE_FOLDER, 9, 0], + [\OCP\Files\FileInfo::TYPE_FOLDER, 11, 0], + [\OCP\Files\FileInfo::TYPE_FOLDER, 13, 0], + [\OCP\Files\FileInfo::TYPE_FOLDER, 15, 0], + [\OCP\Files\FileInfo::TYPE_FOLDER, 17, 17], + [\OCP\Files\FileInfo::TYPE_FOLDER, 19, 19], + [\OCP\Files\FileInfo::TYPE_FOLDER, 21, 21], + [\OCP\Files\FileInfo::TYPE_FOLDER, 23, 23], + [\OCP\Files\FileInfo::TYPE_FOLDER, 25, 25], + [\OCP\Files\FileInfo::TYPE_FOLDER, 27, 27], + [\OCP\Files\FileInfo::TYPE_FOLDER, 29, 29], + [\OCP\Files\FileInfo::TYPE_FOLDER, 30, 0], + [\OCP\Files\FileInfo::TYPE_FOLDER, 31, 31], + ]; + } + + /** + * @dataProvider sharePermissionsProvider + */ + public function testSharePermissions($type, $permissions, $expected) { + $storage = $this->getMock('\OCP\Files\Storage'); + + $storage->method('getPermissions')->willReturn($permissions); + + $info = $this->getMockBuilder('\OC\Files\FileInfo') + ->disableOriginalConstructor() + ->setMethods(array('getStorage', 'getType')) + ->getMock(); + + $info->method('getStorage')->willReturn($storage); + $info->method('getType')->willReturn($type); + + $view = $this->getMock('\OC\Files\View'); + + $node = new \OCA\DAV\Connector\Sabre\File($view, $info); + $this->assertEquals($expected, $node->getSharePermissions()); + } } diff --git a/build/integration/features/bootstrap/WebDav.php b/build/integration/features/bootstrap/WebDav.php index b56a1b7d2f..2ef5f252f1 100644 --- a/build/integration/features/bootstrap/WebDav.php +++ b/build/integration/features/bootstrap/WebDav.php @@ -315,6 +315,20 @@ trait WebDav { } } + /** + * @When User :user uploads file with content :content to :destination + */ + public function userUploadsAFileWithContentTo($user, $content, $destination) + { + $file = \GuzzleHttp\Stream\Stream::factory($content); + try { + $this->response = $this->makeDavRequest($user, "PUT", $destination, [], $file); + } catch (\GuzzleHttp\Exception\ServerException $e) { + // 4xx and 5xx responses cause an exception + $this->response = $e->getResponse(); + } + } + /** * @When User :user deletes file :file * @param string $user diff --git a/build/integration/features/sharing-v1.feature b/build/integration/features/sharing-v1.feature index ba535e83aa..b9d77120b9 100644 --- a/build/integration/features/sharing-v1.feature +++ b/build/integration/features/sharing-v1.feature @@ -566,3 +566,107 @@ Feature: sharing | path | welcome.txt | | shareType | 3 | Then share ids should match + + Scenario: Correct webdav share-permissions for owned file + Given user "user0" exists + And User "user0" uploads file with content "foo" to "/tmp.txt" + When as "user0" gets properties of folder "/tmp.txt" with + |{http://owncloud.org/ns}share-permissions| + Then the single response should contain a property "{http://owncloud.org/ns}share-permissions" with value "19" + + Scenario: Correct webdav share-permissions for received file with edit and reshare permissions + Given user "user0" exists + And user "user1" exists + And User "user0" uploads file with content "foo" to "/tmp.txt" + And file "tmp.txt" of user "user0" is shared with user "user1" + When as "user1" gets properties of folder "/tmp.txt" with + |{http://owncloud.org/ns}share-permissions| + Then the single response should contain a property "{http://owncloud.org/ns}share-permissions" with value "19" + + Scenario: Correct webdav share-permissions for received file with edit permissions but no reshare permissions + Given user "user0" exists + And user "user1" exists + And User "user0" uploads file with content "foo" to "/tmp.txt" + And file "tmp.txt" of user "user0" is shared with user "user1" + And As an "user0" + And Updating last share with + | permissions | 3 | + When as "user1" gets properties of folder "/tmp.txt" with + |{http://owncloud.org/ns}share-permissions| + Then the single response should contain a property "{http://owncloud.org/ns}share-permissions" with value "0" + + Scenario: Correct webdav share-permissions for received file with reshare permissions but no edit permissions + Given user "user0" exists + And user "user1" exists + And User "user0" uploads file with content "foo" to "/tmp.txt" + And file "tmp.txt" of user "user0" is shared with user "user1" + And As an "user0" + And Updating last share with + | permissions | 17 | + When as "user1" gets properties of folder "/tmp.txt" with + |{http://owncloud.org/ns}share-permissions| + Then the single response should contain a property "{http://owncloud.org/ns}share-permissions" with value "17" + + Scenario: Correct webdav share-permissions for owned folder + Given user "user0" exists + And user "user0" created a folder "/tmp" + When as "user0" gets properties of folder "/" with + |{http://owncloud.org/ns}share-permissions| + Then the single response should contain a property "{http://owncloud.org/ns}share-permissions" with value "31" + + Scenario: Correct webdav share-permissions for received folder with all permissions + Given user "user0" exists + And user "user1" exists + And user "user0" created a folder "/tmp" + And file "/tmp" of user "user0" is shared with user "user1" + When as "user1" gets properties of folder "/tmp" with + |{http://owncloud.org/ns}share-permissions| + Then the single response should contain a property "{http://owncloud.org/ns}share-permissions" with value "31" + + Scenario: Correct webdav share-permissions for received folder with all permissions but edit + Given user "user0" exists + And user "user1" exists + And user "user0" created a folder "/tmp" + And file "/tmp" of user "user0" is shared with user "user1" + And As an "user0" + And Updating last share with + | permissions | 29 | + When as "user1" gets properties of folder "/tmp" with + |{http://owncloud.org/ns}share-permissions| + Then the single response should contain a property "{http://owncloud.org/ns}share-permissions" with value "29" + + Scenario: Correct webdav share-permissions for received folder with all permissions but create + Given user "user0" exists + And user "user1" exists + And user "user0" created a folder "/tmp" + And file "/tmp" of user "user0" is shared with user "user1" + And As an "user0" + And Updating last share with + | permissions | 27 | + When as "user1" gets properties of folder "/tmp" with + |{http://owncloud.org/ns}share-permissions| + Then the single response should contain a property "{http://owncloud.org/ns}share-permissions" with value "27" + + Scenario: Correct webdav share-permissions for received folder with all permissions but delete + Given user "user0" exists + And user "user1" exists + And user "user0" created a folder "/tmp" + And file "/tmp" of user "user0" is shared with user "user1" + And As an "user0" + And Updating last share with + | permissions | 23 | + When as "user1" gets properties of folder "/tmp" with + |{http://owncloud.org/ns}share-permissions| + Then the single response should contain a property "{http://owncloud.org/ns}share-permissions" with value "23" + + Scenario: Correct webdav share-permissions for received folder with all permissions but share + Given user "user0" exists + And user "user1" exists + And user "user0" created a folder "/tmp" + And file "/tmp" of user "user0" is shared with user "user1" + And As an "user0" + And Updating last share with + | permissions | 15 | + When as "user1" gets properties of folder "/tmp" with + |{http://owncloud.org/ns}share-permissions| + Then the single response should contain a property "{http://owncloud.org/ns}share-permissions" with value "0" From 5334bcce6683efc2297b742517c98716dd9d3e43 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 31 Mar 2016 20:17:59 +0200 Subject: [PATCH 2/3] Correct share permissions for external storages --- apps/dav/lib/connector/sabre/node.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/apps/dav/lib/connector/sabre/node.php b/apps/dav/lib/connector/sabre/node.php index 86e1844687..4828cb611e 100644 --- a/apps/dav/lib/connector/sabre/node.php +++ b/apps/dav/lib/connector/sabre/node.php @@ -228,6 +228,15 @@ abstract class Node implements \Sabre\DAV\INode { $permissions = $storage->getPermissions($path); } + /* + * We can always share non moveable mount points with DELETE and UPDATE + * Eventually we need to do this properly + */ + $mountpoint = $this->info->getMountPoint(); + if (!($mountpoint instanceof MoveableMount)) { + $permissions |= \OCP\Constants::PERMISSION_DELETE | \OCP\Constants::PERMISSION_UPDATE; + } + /* * Without sharing permissions there are also no other permissions */ From 89478a0961c70f2b8f55b6ded9103b2cac4223a1 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 31 Mar 2016 21:25:23 +0200 Subject: [PATCH 3/3] Fix unit tests --- apps/dav/lib/connector/sabre/node.php | 10 +++++++++- apps/dav/tests/unit/connector/sabre/node.php | 7 +++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/apps/dav/lib/connector/sabre/node.php b/apps/dav/lib/connector/sabre/node.php index 4828cb611e..9867fe66cd 100644 --- a/apps/dav/lib/connector/sabre/node.php +++ b/apps/dav/lib/connector/sabre/node.php @@ -30,6 +30,7 @@ namespace OCA\DAV\Connector\Sabre; +use OC\Files\Mount\MoveableMount; use OCA\DAV\Connector\Sabre\Exception\InvalidPath; @@ -234,7 +235,14 @@ abstract class Node implements \Sabre\DAV\INode { */ $mountpoint = $this->info->getMountPoint(); if (!($mountpoint instanceof MoveableMount)) { - $permissions |= \OCP\Constants::PERMISSION_DELETE | \OCP\Constants::PERMISSION_UPDATE; + $mountpointpath = $mountpoint->getMountPoint(); + if (substr($mountpointpath, -1) === '/') { + $mountpointpath = substr($mountpointpath, 0, -1); + } + + if ($mountpointpath === $this->info->getPath()) { + $permissions |= \OCP\Constants::PERMISSION_DELETE | \OCP\Constants::PERMISSION_UPDATE; + } } /* diff --git a/apps/dav/tests/unit/connector/sabre/node.php b/apps/dav/tests/unit/connector/sabre/node.php index c9fe6e6027..cde8e746dc 100644 --- a/apps/dav/tests/unit/connector/sabre/node.php +++ b/apps/dav/tests/unit/connector/sabre/node.php @@ -108,16 +108,19 @@ class Node extends \Test\TestCase { */ public function testSharePermissions($type, $permissions, $expected) { $storage = $this->getMock('\OCP\Files\Storage'); - $storage->method('getPermissions')->willReturn($permissions); + $mountpoint = $this->getMock('\OCP\Files\Mount\IMountPoint'); + $mountpoint->method('getMountPoint')->willReturn('myPath'); + $info = $this->getMockBuilder('\OC\Files\FileInfo') ->disableOriginalConstructor() - ->setMethods(array('getStorage', 'getType')) + ->setMethods(['getStorage', 'getType', 'getMountPoint']) ->getMock(); $info->method('getStorage')->willReturn($storage); $info->method('getType')->willReturn($type); + $info->method('getMountPoint')->willReturn($mountpoint); $view = $this->getMock('\OC\Files\View');