From e241d2631672836d7a512cd890b14d7523fbc756 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 8 Dec 2015 11:32:18 +0100 Subject: [PATCH 1/4] Compute share permissions in the view The share permissions are now computed in the View/FileInfo instead of storing them directly/permanently on the storage --- apps/files/tests/controller/apicontrollertest.php | 9 ++++++--- lib/private/files/fileinfo.php | 8 +++++++- lib/private/files/storage/common.php | 4 ---- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/apps/files/tests/controller/apicontrollertest.php b/apps/files/tests/controller/apicontrollertest.php index fb728d5eff..bc66e4641b 100644 --- a/apps/files/tests/controller/apicontrollertest.php +++ b/apps/files/tests/controller/apicontrollertest.php @@ -92,6 +92,7 @@ class ApiControllerTest extends TestCase { [ 'mtime' => 55, 'mimetype' => 'application/pdf', + 'permissions' => 31, 'size' => 1234, 'etag' => 'MyEtag', ], @@ -111,7 +112,7 @@ class ApiControllerTest extends TestCase { 'parentId' => null, 'mtime' => 55000, 'name' => 'root.txt', - 'permissions' => null, + 'permissions' => 31, 'mimetype' => 'application/pdf', 'size' => 1234, 'type' => 'file', @@ -139,6 +140,7 @@ class ApiControllerTest extends TestCase { [ 'mtime' => 55, 'mimetype' => 'application/pdf', + 'permissions' => 31, 'size' => 1234, 'etag' => 'MyEtag', ], @@ -155,6 +157,7 @@ class ApiControllerTest extends TestCase { [ 'mtime' => 999, 'mimetype' => 'application/binary', + 'permissions' => 31, 'size' => 9876, 'etag' => 'SubEtag', ], @@ -174,7 +177,7 @@ class ApiControllerTest extends TestCase { 'parentId' => null, 'mtime' => 55000, 'name' => 'root.txt', - 'permissions' => null, + 'permissions' => 31, 'mimetype' => 'application/pdf', 'size' => 1234, 'type' => 'file', @@ -191,7 +194,7 @@ class ApiControllerTest extends TestCase { 'parentId' => null, 'mtime' => 999000, 'name' => 'root.txt', - 'permissions' => null, + 'permissions' => 31, 'mimetype' => 'application/binary', 'size' => 9876, 'type' => 'file', diff --git a/lib/private/files/fileinfo.php b/lib/private/files/fileinfo.php index 5b5e869700..0525c25960 100644 --- a/lib/private/files/fileinfo.php +++ b/lib/private/files/fileinfo.php @@ -100,6 +100,8 @@ class FileInfo implements \OCP\Files\FileInfo, \ArrayAccess { return $this->getType(); } else if ($offset === 'etag') { return $this->getEtag(); + } elseif ($offset === 'permissions') { + return $this->getPermissions(); } elseif (isset($this->data[$offset])) { return $this->data[$offset]; } else { @@ -193,7 +195,11 @@ class FileInfo implements \OCP\Files\FileInfo, \ArrayAccess { * @return int */ public function getPermissions() { - return $this->data['permissions']; + $perms = $this->data['permissions']; + if (\OCP\Util::isSharingDisabledForUser()) { + $perms = $perms & ~\OCP\Constants::PERMISSION_SHARE; + } + return $perms; } /** diff --git a/lib/private/files/storage/common.php b/lib/private/files/storage/common.php index 0cd67e343f..b06543d0a6 100644 --- a/lib/private/files/storage/common.php +++ b/lib/private/files/storage/common.php @@ -141,10 +141,6 @@ abstract class Common implements Storage { } public function isSharable($path) { - if (\OCP\Util::isSharingDisabledForUser()) { - return false; - } - return $this->isReadable($path); } From 6e4006d1397817f83c2dd3cafc4d697ac1e6a2a7 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 8 Dec 2015 13:02:57 +0100 Subject: [PATCH 2/4] Add reshare permission checks Added in isSharable() in incoming remote share. Added in isSharable() in regular incoming share. Added in FileInfo to make sure the proper attributes are returned to the clients. --- apps/files_sharing/lib/external/storage.php | 8 ++++++++ apps/files_sharing/lib/sharedstorage.php | 2 +- lib/private/files/fileinfo.php | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/apps/files_sharing/lib/external/storage.php b/apps/files_sharing/lib/external/storage.php index 2a0d827e06..36ff4f0c22 100644 --- a/apps/files_sharing/lib/external/storage.php +++ b/apps/files_sharing/lib/external/storage.php @@ -265,4 +265,12 @@ class Storage extends DAV implements ISharedStorage { list(, $remote) = explode('://', $this->remote, 2); return $this->remoteUser . '@' . $remote; } + + public function isSharable($path) { + if (\OCP\Util::isSharingDisabledForUser() || !\OC\Share\Share::isResharingAllowed()) { + return false; + } + return ($this->getPermissions($path) & \OCP\Constants::PERMISSION_SHARE); + } + } diff --git a/apps/files_sharing/lib/sharedstorage.php b/apps/files_sharing/lib/sharedstorage.php index cda3f564d5..38f79762dc 100644 --- a/apps/files_sharing/lib/sharedstorage.php +++ b/apps/files_sharing/lib/sharedstorage.php @@ -257,7 +257,7 @@ class Shared extends \OC\Files\Storage\Common implements ISharedStorage { } public function isSharable($path) { - if (\OCP\Util::isSharingDisabledForUser()) { + if (\OCP\Util::isSharingDisabledForUser() || !\OC\Share\Share::isResharingAllowed()) { return false; } return ($this->getPermissions($path) & \OCP\Constants::PERMISSION_SHARE); diff --git a/lib/private/files/fileinfo.php b/lib/private/files/fileinfo.php index 0525c25960..5ed65cd379 100644 --- a/lib/private/files/fileinfo.php +++ b/lib/private/files/fileinfo.php @@ -196,7 +196,7 @@ class FileInfo implements \OCP\Files\FileInfo, \ArrayAccess { */ public function getPermissions() { $perms = $this->data['permissions']; - if (\OCP\Util::isSharingDisabledForUser()) { + if (\OCP\Util::isSharingDisabledForUser() || ($this->isShared() && !\OC\Share\Share::isResharingAllowed())) { $perms = $perms & ~\OCP\Constants::PERMISSION_SHARE; } return $perms; From 28e9bc115615aab6c951fb846b8bd28aa517331e Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 8 Dec 2015 16:33:39 +0100 Subject: [PATCH 3/4] Fix more unit tests to pass a mock storage instead of null to FileInfo --- apps/dav/tests/unit/connector/sabre/file.php | 44 ++++++++++++-------- tests/lib/files/node/file.php | 10 ++++- tests/lib/files/node/folder.php | 10 ++++- tests/lib/files/node/node.php | 10 ++++- 4 files changed, 53 insertions(+), 21 deletions(-) diff --git a/apps/dav/tests/unit/connector/sabre/file.php b/apps/dav/tests/unit/connector/sabre/file.php index 2a6cf46ef1..ad4c1d29ed 100644 --- a/apps/dav/tests/unit/connector/sabre/file.php +++ b/apps/dav/tests/unit/connector/sabre/file.php @@ -48,6 +48,14 @@ class File extends \Test\TestCase { parent::tearDown(); } + private function getMockStorage() { + $storage = $this->getMock('\OCP\Files\Storage'); + $storage->expects($this->any()) + ->method('getId') + ->will($this->returnValue('home::someuser')); + return $storage; + } + /** * @param string $string */ @@ -149,7 +157,7 @@ class File extends \Test\TestCase { ->method('getRelativePath') ->will($this->returnArgument(0)); - $info = new \OC\Files\FileInfo('/test.txt', null, null, array( + $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL ), null); @@ -209,7 +217,7 @@ class File extends \Test\TestCase { $_SERVER['HTTP_OC_CHUNKED'] = true; - $info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-0', null, null, [ + $info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-0', $this->getMockStorage(), null, [ 'permissions' => \OCP\Constants::PERMISSION_ALL ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -219,7 +227,7 @@ class File extends \Test\TestCase { $this->assertNull($file->put('test data one')); $file->releaseLock(ILockingProvider::LOCK_SHARED); - $info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-1', null, null, [ + $info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-1', $this->getMockStorage(), null, [ 'permissions' => \OCP\Constants::PERMISSION_ALL ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -261,7 +269,7 @@ class File extends \Test\TestCase { $info = new \OC\Files\FileInfo( $viewRoot . '/' . ltrim($path, '/'), - null, + $this->getMockStorage(), null, ['permissions' => \OCP\Constants::PERMISSION_ALL], null @@ -450,7 +458,7 @@ class File extends \Test\TestCase { $_SERVER['CONTENT_LENGTH'] = 123456; $_SERVER['REQUEST_METHOD'] = 'PUT'; - $info = new \OC\Files\FileInfo('/test.txt', null, null, array( + $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL ), null); @@ -483,7 +491,7 @@ class File extends \Test\TestCase { // simulate situation where the target file is locked $view->lockFile('/test.txt', ILockingProvider::LOCK_EXCLUSIVE); - $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt', null, null, array( + $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt', $this->getMockStorage(), null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL ), null); @@ -518,7 +526,7 @@ class File extends \Test\TestCase { $_SERVER['HTTP_OC_CHUNKED'] = true; - $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-0', null, null, [ + $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-0', $this->getMockStorage(), null, [ 'permissions' => \OCP\Constants::PERMISSION_ALL ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -526,7 +534,7 @@ class File extends \Test\TestCase { $this->assertNull($file->put('test data one')); $file->releaseLock(ILockingProvider::LOCK_SHARED); - $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-1', null, null, [ + $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-1', $this->getMockStorage(), null, [ 'permissions' => \OCP\Constants::PERMISSION_ALL ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -555,7 +563,7 @@ class File extends \Test\TestCase { ->method('getRelativePath') ->will($this->returnArgument(0)); - $info = new \OC\Files\FileInfo('/*', null, null, array( + $info = new \OC\Files\FileInfo('/*', $this->getMockStorage(), null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL ), null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -591,7 +599,7 @@ class File extends \Test\TestCase { ->method('getRelativePath') ->will($this->returnArgument(0)); - $info = new \OC\Files\FileInfo('/*', null, null, array( + $info = new \OC\Files\FileInfo('/*', $this->getMockStorage(), null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL ), null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -618,7 +626,7 @@ class File extends \Test\TestCase { $_SERVER['CONTENT_LENGTH'] = 12345; $_SERVER['REQUEST_METHOD'] = 'PUT'; - $info = new \OC\Files\FileInfo('/test.txt', null, null, array( + $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL ), null); @@ -654,7 +662,7 @@ class File extends \Test\TestCase { ->method('unlink') ->will($this->returnValue(true)); - $info = new \OC\Files\FileInfo('/test.txt', null, null, array( + $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL ), null); @@ -672,7 +680,7 @@ class File extends \Test\TestCase { $view = $this->getMock('\OC\Files\View', array()); - $info = new \OC\Files\FileInfo('/test.txt', null, null, array( + $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, array( 'permissions' => 0 ), null); @@ -695,7 +703,7 @@ class File extends \Test\TestCase { ->method('unlink') ->will($this->returnValue(false)); - $info = new \OC\Files\FileInfo('/test.txt', null, null, array( + $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL ), null); @@ -718,7 +726,7 @@ class File extends \Test\TestCase { ->method('unlink') ->willThrowException(new ForbiddenException('', true)); - $info = new \OC\Files\FileInfo('/test.txt', null, null, array( + $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL ), null); @@ -753,7 +761,7 @@ class File extends \Test\TestCase { $path = 'test-locking.txt'; $info = new \OC\Files\FileInfo( '/' . $this->user . '/files/' . $path, - null, + $this->getMockStorage(), null, ['permissions' => \OCP\Constants::PERMISSION_ALL], null @@ -865,7 +873,7 @@ class File extends \Test\TestCase { ->method('fopen') ->will($this->returnValue(false)); - $info = new \OC\Files\FileInfo('/test.txt', null, null, array( + $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL ), null); @@ -883,7 +891,7 @@ class File extends \Test\TestCase { ->method('fopen') ->willThrowException(new ForbiddenException('', true)); - $info = new \OC\Files\FileInfo('/test.txt', null, null, array( + $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, array( 'permissions' => \OCP\Constants::PERMISSION_ALL ), null); diff --git a/tests/lib/files/node/file.php b/tests/lib/files/node/file.php index d0072949c7..ccc777c499 100644 --- a/tests/lib/files/node/file.php +++ b/tests/lib/files/node/file.php @@ -21,8 +21,16 @@ class File extends \Test\TestCase { $this->user = new \OC\User\User('', new \Test\Util\User\Dummy); } + protected function getMockStorage() { + $storage = $this->getMock('\OCP\Files\Storage'); + $storage->expects($this->any()) + ->method('getId') + ->will($this->returnValue('home::someuser')); + return $storage; + } + protected function getFileInfo($data) { - return new FileInfo('', null, '', $data, null); + return new FileInfo('', $this->getMockStorage(), '', $data, null); } public function testDelete() { diff --git a/tests/lib/files/node/folder.php b/tests/lib/files/node/folder.php index d95e1b5d2b..09bf32561e 100644 --- a/tests/lib/files/node/folder.php +++ b/tests/lib/files/node/folder.php @@ -31,8 +31,16 @@ class Folder extends \Test\TestCase { $this->user = new \OC\User\User('', new \Test\Util\User\Dummy); } + protected function getMockStorage() { + $storage = $this->getMock('\OCP\Files\Storage'); + $storage->expects($this->any()) + ->method('getId') + ->will($this->returnValue('home::someuser')); + return $storage; + } + protected function getFileInfo($data) { - return new FileInfo('', null, '', $data, null); + return new FileInfo('', $this->getMockStorage(), '', $data, null); } public function testDelete() { diff --git a/tests/lib/files/node/node.php b/tests/lib/files/node/node.php index afcf4cbaba..a1693b034f 100644 --- a/tests/lib/files/node/node.php +++ b/tests/lib/files/node/node.php @@ -18,8 +18,16 @@ class Node extends \Test\TestCase { $this->user = new \OC\User\User('', new \Test\Util\User\Dummy); } + protected function getMockStorage() { + $storage = $this->getMock('\OCP\Files\Storage'); + $storage->expects($this->any()) + ->method('getId') + ->will($this->returnValue('home::someuser')); + return $storage; + } + protected function getFileInfo($data) { - return new FileInfo('', null, '', $data, null); + return new FileInfo('', $this->getMockStorage(), '', $data, null); } public function testStat() { From d0cca6c3aded2aaa35e5b2caab46ff49676eecbd Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 8 Dec 2015 16:48:33 +0100 Subject: [PATCH 4/4] Add explicit check for groups excluded from sharing Since isSharable() doesn't do the check for groups excluded from sharing, adding an explicit check in the sharing code. --- lib/private/share/share.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/share/share.php b/lib/private/share/share.php index 8899df2563..e62bdebc08 100644 --- a/lib/private/share/share.php +++ b/lib/private/share/share.php @@ -635,7 +635,7 @@ class Share extends Constants { throw new \Exception($message_t); } // verify that the user has share permission - if (!\OC\Files\Filesystem::isSharable($path)) { + if (!\OC\Files\Filesystem::isSharable($path) || \OCP\Util::isSharingDisabledForUser()) { $message = 'You are not allowed to share %s'; $message_t = $l->t('You are not allowed to share %s', [$path]); \OCP\Util::writeLog('OCP\Share', sprintf($message, $path), \OCP\Util::DEBUG);