Merge pull request #34351 from owncloud/lock-prevent-public

Prevent LOCK/UNLOCK methods from public endpoint
owncloud · Feb 1, 2019 · 895f07d · 895f07d
2 parents ba74089 + 7a66731
commit 895f07d
Show file tree

Hide file tree

Showing 8 changed files with 59 additions and 327 deletions.
diff --git a/apps/dav/lib/Files/FileLocksBackend.php b/apps/dav/lib/Files/FileLocksBackend.php
@@ -30,6 +30,7 @@
 use Sabre\DAV\Tree;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCA\DAV\Connector\Sabre\ObjectTree;
+use OCA\DAV\Connector\Sabre\Exception\Forbidden;
 
 class FileLocksBackend implements BackendInterface {
 
@@ -40,13 +41,13 @@ class FileLocksBackend implements BackendInterface {
 	/** @var ITimeFactory */
 	private $timeFactory;
 	/** @var bool */
-	private $hideLockTokenInList;
+	private $isPublicEndpoint;
 
-	public function __construct($tree, $useV1, $timeFactory, $hideLockTokenInList = false) {
+	public function __construct($tree, $useV1, $timeFactory, $isPublicEndpoint = false) {
 		$this->tree = $tree;
 		$this->useV1 = $useV1;
 		$this->timeFactory = $timeFactory;
-		$this->hideLockTokenInList = $hideLockTokenInList;
+		$this->isPublicEndpoint = $isPublicEndpoint;
 	}
 
 	/**
@@ -134,7 +135,7 @@ public function getLocks($uri, $returnChildLocks) {
 				}
 			}
 
-			if (!$this->hideLockTokenInList) {
+			if (!$this->isPublicEndpoint) {
 				$lockInfo->token = $lock->getToken();
 				$lockInfo->owner = $lock->getOwner();
 			}
@@ -160,6 +161,9 @@ public function getLocks($uri, $returnChildLocks) {
 	 * @return bool
 	 */
 	public function lock($uri, Locks\LockInfo $lockInfo) {
+		if ($this->isPublicEndpoint) {
+			throw new Forbidden('Forbidden to lock from public endpoint');
+		}
 		try {
 			$node = $this->tree->getNodeForPath($uri);
 		} catch (NotFound $e) {
@@ -197,6 +201,9 @@ public function lock($uri, Locks\LockInfo $lockInfo) {
 	 * @return bool
 	 */
 	public function unlock($uri, Locks\LockInfo $lockInfo) {
+		if ($this->isPublicEndpoint) {
+			throw new Forbidden('Forbidden to unlock from public endpoint');
+		}
 		try {
 			$node = $this->tree->getNodeForPath($uri);
 		} catch (NotFound $e) {

diff --git a/apps/dav/tests/unit/Files/FileLocksBackendTest.php b/apps/dav/tests/unit/Files/FileLocksBackendTest.php
@@ -314,6 +314,50 @@ public function testLock() {
 		$this->assertTrue($this->plugin->lock('file-to-be-locked.txt', $lockInfo));
 	}
 
+	/**
+	 * @expectedException \OCA\DAV\Connector\Sabre\Exception\Forbidden
+	 */
+	public function testLockPublic() {
+		$lockInfo = new LockInfo();
+		$lockInfo->token = '123-456-7890';
+		$lockInfo->scope = LockInfo::SHARED;
+		$lockInfo->owner = 'Alice Wonder';
+		$lockInfo->timeout = 800;
+		$lockInfo->created = self::CREATION_TIME;
+
+		$this->storageOfFileToBeLocked
+			->expects($this->never())
+			->method('lockNodePersistent');
+
+		$timeFactory = $this->createMock(ITimeFactory::class);
+		$timeFactory->method('getTime')->willReturn(self::CURRENT_TIME);
+
+		$this->plugin = new FileLocksBackend($this->tree, true, $timeFactory, true);
+		$this->plugin->lock('file-to-be-locked.txt', $lockInfo);
+	}
+
+	/**
+	 * @expectedException \OCA\DAV\Connector\Sabre\Exception\Forbidden
+	 */
+	public function testUnlockPublic() {
+		$lockInfo = new LockInfo();
+		$lockInfo->token = '123-456-7890';
+		$lockInfo->scope = LockInfo::SHARED;
+		$lockInfo->owner = 'Alice Wonder';
+		$lockInfo->timeout = 800;
+		$lockInfo->created = self::CREATION_TIME;
+
+		$this->storageOfFileToBeLocked
+			->expects($this->never())
+			->method('lockNodePersistent');
+
+		$timeFactory = $this->createMock(ITimeFactory::class);
+		$timeFactory->method('getTime')->willReturn(self::CURRENT_TIME);
+
+		$this->plugin = new FileLocksBackend($this->tree, true, $timeFactory, true);
+		$this->plugin->unlock('file-to-be-locked.txt', $lockInfo);
+	}
+
 	public function testUnlock() {
 		$lockInfo = new LockInfo();
 		$lockInfo->token = '123-456-7890';

diff --git a/tests/acceptance/features/apiWebdavLocks/publicLink.feature b/tests/acceptance/features/apiWebdavLocks/publicLink.feature
@@ -67,11 +67,9 @@ Feature: persistent-locking in case of a public link
       | new      | shared     |
       | new      | exclusive  |
 
-  Scenario: public tries to lock a folder inside an exclusively locked folder
+  Scenario: Public locking is forbidden
     Given user "user0" has created a public link share of folder "PARENT" with change permission
-    And user "user0" has locked folder "PARENT" setting following properties
-      | lockscope | exclusive |
     When the public locks "/CHILD" in the last public shared folder using the WebDAV API setting following properties
-      | lockscope | shared |
-    Then the HTTP status code should be "423"
-    And the value of the item "//d:no-conflicting-lock/d:href" in the response should be ""
+      | lockscope | exclusive |
+    Then the HTTP status code should be "403"
+    And the value of the item "//s:message" in the response should be "Forbidden to lock from public endpoint"
diff --git a/tests/acceptance/features/apiWebdavLocks/publicLinkLockdiscovery.feature b/tests/acceptance/features/apiWebdavLocks/publicLinkLockdiscovery.feature
@@ -102,120 +102,3 @@ Feature: LOCKDISCOVERY for public links
       | shared     |
       | exclusive  |
 
-  Scenario Outline: lockdiscovery root of public link when root is locked by public
-    Given user "user0" has created a public link share of folder "PARENT" with change permission
-    And the public has locked the last public shared folder setting following properties
-      | lockscope | <lock-scope> |
-    When the public gets the following properties of entry "/" in the last created public link using the WebDAV API
-      | d:lockdiscovery |
-    Then the value of the item "//d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/$/"
-    And the value of the item "//d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:timeout" in the response should match "/Second-\d+/"
-    Examples:
-      | lock-scope |
-      | shared     |
-      | exclusive  |
-
-  Scenario Outline: lockdiscovery subfolder of public link when root is locked by public
-    Given user "user0" has created a public link share of folder "PARENT" with change permission
-    And the public has locked the last public shared folder setting following properties
-      | lockscope | <lock-scope> |
-    When the public gets the following properties of entry "/CHILD" in the last created public link using the WebDAV API
-      | d:lockdiscovery |
-    Then the value of the item "//d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/$/"
-    And the value of the item "//d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:timeout" in the response should match "/Second-\d+/"
-    Examples:
-      | lock-scope |
-      | shared     |
-      | exclusive  |
-
-  Scenario Outline: lockdiscovery subfolder of public link when subfolder is locked by public
-    Given user "user0" has created a public link share of folder "PARENT" with change permission
-    And the public has locked "CHILD" in the last public shared folder setting following properties
-      | lockscope | <lock-scope> |
-    When the public gets the following properties of entry "/CHILD" in the last created public link using the WebDAV API
-      | d:lockdiscovery |
-    Then the value of the item "//d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/CHILD$/"
-    And the value of the item "//d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:timeout" in the response should match "/Second-\d+/"
-    Examples:
-      | lock-scope |
-      | shared     |
-      | exclusive  |
-
-  Scenario: lockdiscovery root of public link when root is locked by public and user
-    Given user "user0" has created a public link share of folder "PARENT" with change permission
-    And user "user0" has locked folder "PARENT" setting following properties
-      | lockscope | shared |
-    And the public has locked the last public shared folder setting following properties
-      | lockscope | shared |
-    When the public gets the following properties of entry "/" in the last created public link using the WebDAV API
-      | d:lockdiscovery |
-    Then the value of the item "//d:activelock[1]/d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/$/"
-    And the value of the item "//d:activelock[2]/d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/$/"
-    And the value of the item "//d:activelock[1]/d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:activelock[2]/d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:activelock[1]/d:timeout" in the response should match "/Second-\d+/"
-    And the value of the item "//d:activelock[2]/d:timeout" in the response should match "/Second-\d+/"
-
-  Scenario: lockdiscovery subfolder of public link when root is locked by user and subfolder is locked by public
-    Given user "user0" has created a public link share of folder "PARENT" with change permission
-    And user "user0" has locked folder "PARENT" setting following properties
-      | lockscope | shared |
-    And the public has locked "CHILD" in the last public shared folder setting following properties
-      | lockscope | shared |
-    When the public gets the following properties of entry "/CHILD" in the last created public link using the WebDAV API
-      | d:lockdiscovery |
-    Then the value of the item "//d:activelock[1]/d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/$/"
-    And the value of the item "//d:activelock[2]/d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/CHILD$/"
-    And the value of the item "//d:activelock[1]/d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:activelock[2]/d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:activelock[1]/d:timeout" in the response should match "/Second-\d+/"
-    And the value of the item "//d:activelock[2]/d:timeout" in the response should match "/Second-\d+/"
-
-  Scenario: lockdiscovery root of public link when user has locked folder above public link and public has locked root of public link
-    Given user "user0" has created a public link share of folder "PARENT/CHILD" with change permission
-    And user "user0" has locked folder "PARENT" setting following properties
-      | lockscope | shared |
-    And the public has locked "/" in the last public shared folder setting following properties
-      | lockscope | shared |
-    When the public gets the following properties of entry "/" in the last created public link using the WebDAV API
-      | d:lockdiscovery |
-    Then the value of the item "//d:activelock[1]/d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/$/"
-    And the value of the item "//d:activelock[2]/d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/$/"
-    And the value of the item "//d:activelock[1]/d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:activelock[2]/d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:activelock[1]/d:timeout" in the response should match "/Second-\d+/"
-    And the value of the item "//d:activelock[2]/d:timeout" in the response should match "/Second-\d+/"
-
-  Scenario: lockdiscovery subfolder of public link when user has locked folder above public link and public has locked subfolder of public link
-    Given user "user0" has created a public link share of folder "PARENT/CHILD" with change permission
-    And user "user0" has created folder "PARENT/CHILD/GRANDCHILD"
-    And user "user0" has locked folder "PARENT" setting following properties
-      | lockscope | shared |
-    And the public has locked "/GRANDCHILD" in the last public shared folder setting following properties
-      | lockscope | shared |
-    When the public gets the following properties of entry "/GRANDCHILD" in the last created public link using the WebDAV API
-      | d:lockdiscovery |
-    Then the value of the item "//d:activelock[1]/d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/$/"
-    And the value of the item "//d:activelock[2]/d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/GRANDCHILD$/"
-    And the value of the item "//d:activelock[1]/d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:activelock[2]/d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:activelock[1]/d:timeout" in the response should match "/Second-\d+/"
-    And the value of the item "//d:activelock[2]/d:timeout" in the response should match "/Second-\d+/"
-
-  Scenario: lockdiscovery file in public link when user has locked folder above public link and public has locked file inside of public link
-    Given user "user0" has created a public link share of folder "PARENT/CHILD" with change permission
-    And user "user0" has locked folder "PARENT" setting following properties
-      | lockscope | shared |
-    And the public has locked "/child.txt" in the last public shared folder setting following properties
-      | lockscope | shared |
-    When the public gets the following properties of entry "/child.txt" in the last created public link using the WebDAV API
-      | d:lockdiscovery |
-    Then the value of the item "//d:activelock[1]/d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/$/"
-    And the value of the item "//d:activelock[2]/d:lockroot/d:href" in the response should match "/%base_path%\/public.php\/webdav\/child.txt$/"
-    And the value of the item "//d:activelock[1]/d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:activelock[2]/d:locktoken/d:href" in the response should be "opaquelocktoken:"
-    And the value of the item "//d:activelock[1]/d:timeout" in the response should match "/Second-\d+/"
-    And the value of the item "//d:activelock[2]/d:timeout" in the response should match "/Second-\d+/"
diff --git a/tests/acceptance/features/apiWebdavLocks/resharedShares.feature b/tests/acceptance/features/apiWebdavLocks/resharedShares.feature
@@ -106,23 +106,3 @@ Feature: lock should propagate correctly if a share is reshared
       | new      | shared     |
       | new      | exclusive  |
 
-  Scenario Outline: upload to a reshared share that was locked by the public
-    Given using <dav-path> DAV path
-    And user "user0" has shared folder "PARENT" with user "user1"
-    And user "user1" has shared folder "PARENT (2)" with user "user2"
-    And user "user2" has created a public link share of folder "PARENT (2)" with change permission
-    And the public locks "/CHILD" in the last public shared folder using the WebDAV API setting following properties
-      | lockscope | shared |
-    When user "user2" uploads file "filesForUpload/textfile.txt" to "/PARENT (2)/CHILD/textfile.txt" using the WebDAV API
-    Then the HTTP status code should be "423"
-    When user "user1" uploads file "filesForUpload/textfile.txt" to "/PARENT (2)/CHILD/textfile.txt" using the WebDAV API
-    Then the HTTP status code should be "423"
-    When user "user0" uploads file "filesForUpload/textfile.txt" to "/PARENT/CHILD/textfile.txt" using the WebDAV API
-    Then the HTTP status code should be "423"
-    And as "user0" file "/PARENT/textfile.txt" should not exist
-    Examples:
-      | dav-path | lock-scope |
-      | old      | shared     |
-      | old      | exclusive  |
-      | new      | shared     |
-      | new      | exclusive  |
diff --git a/tests/acceptance/features/apiWebdavLocks/setTimeout.feature b/tests/acceptance/features/apiWebdavLocks/setTimeout.feature
@@ -117,30 +117,3 @@ Feature: set timeouts of LOCKS
       | new      | second--1       | /Second-\d{5}$/ |
       | new      | second-0        | /Second-\d{4}$/ |
 
-  Scenario Outline: as public set timeout on folder as owner check it
-    Given using <dav-path> DAV path
-    And user "user0" has created a public link share of folder "PARENT"
-    When the public locks the last public shared folder using the WebDAV API setting following properties
-      | lockscope | shared    |
-      | timeout   | <timeout> |
-    And user "user0" gets the following properties of folder "PARENT" using the WebDAV API
-      | d:lockdiscovery |
-    Then the value of the item "//d:timeout" in the response should match "<result>"
-    When user "user0" gets the following properties of folder "PARENT/CHILD" using the WebDAV API
-      | d:lockdiscovery |
-    Then the value of the item "//d:timeout" in the response should match "<result>"
-    When user "user0" gets the following properties of folder "PARENT/parent.txt" using the WebDAV API
-      | d:lockdiscovery |
-    Then the value of the item "//d:timeout" in the response should match "<result>"
-    Examples:
-      | dav-path | timeout         | result          |
-      | old      | second-999      | /Second-\d{3}$/ |
-      | old      | second-99999999 | /Second-\d{5}$/ |
-      | old      | infinite        | /Second-\d{5}$/ |
-      | old      | second--1       | /Second-\d{5}$/ |
-      | old      | second-0        | /Second-\d{4}$/ |
-      | new      | second-999      | /Second-\d{3}$/ |
-      | new      | second-99999999 | /Second-\d{5}$/ |
-      | new      | infinite        | /Second-\d{5}$/ |
-      | new      | second--1       | /Second-\d{5}$/ |
-      | new      | second-0        | /Second-\d{4}$/ |