Merge pull request #3595 from nextcloud/dav-sharing-changes

Restrict DAV share handling to the owner only

5 files changed, 85 insertions, 62 deletions

diff --git a/apps/dav/lib/CalDAV/Calendar.php b/apps/dav/lib/CalDAV/Calendar.php
index d1eff1aeaa3..a216e4e078b 100644
--- a/apps/dav/lib/CalDAV/Calendar.php
+++ b/apps/dav/lib/CalDAV/Calendar.php
@@ -30,6 +30,12 @@ use Sabre\DAV\Exception\Forbidden;
 use Sabre\DAV\Exception\NotFound;
 use Sabre\DAV\PropPatch;
 
+/**
+ * Class Calendar
+ *
+ * @package OCA\DAV\CalDAV
+ * @property BackendInterface|CalDavBackend $caldavBackend
+ */
 class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 
 	public function __construct(BackendInterface $caldavBackend, $calendarInfo, IL10N $l10n) {
@@ -61,11 +67,13 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 	 * @param array $add
 	 * @param array $remove
 	 * @return void
+	 * @throws Forbidden
 	 */
-	function updateShares(array $add, array $remove) {
-		/** @var CalDavBackend $calDavBackend */
-		$calDavBackend = $this->caldavBackend;
-		$calDavBackend->updateShares($this, $add, $remove);
+	public function updateShares(array $add, array $remove) {
+		if ($this->isShared()) {
+			throw new Forbidden();
+		}
+		$this->caldavBackend->updateShares($this, $add, $remove);
 	}
 
 	/**
@@ -80,10 +88,11 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 	 *
 	 * @return array
 	 */
-	function getShares() {
-		/** @var CalDavBackend $calDavBackend */
-		$calDavBackend = $this->caldavBackend;
-		return $calDavBackend->getShares($this->getResourceId());
+	public function getShares() {
+		if ($this->isShared()) {
+			return [];
+		}
+		return $this->caldavBackend->getShares($this->getResourceId());
 	}
 
 	/**
@@ -100,7 +109,7 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 		return $this->calendarInfo['principaluri'];
 	}
 
-	function getACL() {
+	public function getACL() {
 		$acl =  [
 			[
 				'privilege' => '{DAV:}read',
@@ -136,27 +145,29 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 			];
 		}
 
-		/** @var CalDavBackend $calDavBackend */
-		$calDavBackend = $this->caldavBackend;
-		return $calDavBackend->applyShareAcl($this->getResourceId(), $acl);
+		if ($this->isShared()) {
+			return $acl;
+		}
+
+		return $this->caldavBackend->applyShareAcl($this->getResourceId(), $acl);
 	}
 
-	function getChildACL() {
+	public function getChildACL() {
 		return $this->getACL();
 	}
 
-	function getOwner() {
+	public function getOwner() {
 		if (isset($this->calendarInfo['{http://owncloud.org/ns}owner-principal'])) {
 			return $this->calendarInfo['{http://owncloud.org/ns}owner-principal'];
 		}
 		return parent::getOwner();
 	}
 
-	function delete() {
+	public function delete() {
 		if (isset($this->calendarInfo['{http://owncloud.org/ns}owner-principal']) &&
 			$this->calendarInfo['{http://owncloud.org/ns}owner-principal'] !== $this->calendarInfo['principaluri']) {
 			$principal = 'principal:' . parent::getOwner();
-			$shares = $this->getShares();
+			$shares = $this->caldavBackend->getShares($this->getResourceId());
 			$shares = array_filter($shares, function($share) use ($principal){
 				return $share['href'] === $principal;
 			});
@@ -164,9 +175,7 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 				throw new Forbidden();
 			}
 
-			/** @var CalDavBackend $calDavBackend */
-			$calDavBackend = $this->caldavBackend;
-			$calDavBackend->updateShares($this, [], [
+			$this->caldavBackend->updateShares($this, [], [
 				'href' => $principal
 			]);
 			return;
@@ -174,7 +183,7 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 		parent::delete();
 	}
 
-	function propPatch(PropPatch $propPatch) {
+	public function propPatch(PropPatch $propPatch) {
 		// parent::propPatch will only update calendars table
 		// if calendar is shared, changes have to be made to the properties table
 		if (!$this->isShared()) {
@@ -182,7 +191,7 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 		}
 	}
 
-	function getChild($name) {
+	public function getChild($name) {
 
 		$obj = $this->caldavBackend->getCalendarObject($this->calendarInfo['id'], $name);
 
@@ -190,7 +199,7 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 			throw new NotFound('Calendar object not found');
 		}
 
-		if ($this->isShared() && $obj['classification'] === CalDavBackend::CLASSIFICATION_PRIVATE) {
+		if ($obj['classification'] === CalDavBackend::CLASSIFICATION_PRIVATE && $this->isShared()) {
 			throw new NotFound('Calendar object not found');
 		}
 
@@ -200,12 +209,12 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 
 	}
 
-	function getChildren() {
+	public function getChildren() {
 
 		$objs = $this->caldavBackend->getCalendarObjects($this->calendarInfo['id']);
 		$children = [];
 		foreach ($objs as $obj) {
-			if ($this->isShared() && $obj['classification'] === CalDavBackend::CLASSIFICATION_PRIVATE) {
+			if ($obj['classification'] === CalDavBackend::CLASSIFICATION_PRIVATE && $this->isShared()) {
 				continue;
 			}
 			$obj['acl'] = $this->getChildACL();
@@ -215,12 +224,12 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 
 	}
 
-	function getMultipleChildren(array $paths) {
+	public function getMultipleChildren(array $paths) {
 
 		$objs = $this->caldavBackend->getMultipleCalendarObjects($this->calendarInfo['id'], $paths);
 		$children = [];
 		foreach ($objs as $obj) {
-			if ($this->isShared() && $obj['classification'] === CalDavBackend::CLASSIFICATION_PRIVATE) {
+			if ($obj['classification'] === CalDavBackend::CLASSIFICATION_PRIVATE && $this->isShared()) {
 				continue;
 			}
 			$obj['acl'] = $this->getChildACL();
@@ -230,19 +239,19 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 
 	}
 
-	function childExists($name) {
+	public function childExists($name) {
 		$obj = $this->caldavBackend->getCalendarObject($this->calendarInfo['id'], $name);
 		if (!$obj) {
 			return false;
 		}
-		if ($this->isShared() && $obj['classification'] === CalDavBackend::CLASSIFICATION_PRIVATE) {
+		if ($obj['classification'] === CalDavBackend::CLASSIFICATION_PRIVATE && $this->isShared()) {
 			return false;
 		}
 
 		return true;
 	}
 
-	function calendarQuery(array $filters) {
+	public function calendarQuery(array $filters) {
 
 		$uris = $this->caldavBackend->calendarQuery($this->calendarInfo['id'], $filters);
 		if ($this->isShared()) {
@@ -258,7 +267,7 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 	 * @param boolean $value
 	 * @return string|null
 	 */
-	function setPublishStatus($value) {
+	public function setPublishStatus($value) {
 		$publicUri = $this->caldavBackend->setPublishStatus($value, $this);
 		$this->calendarInfo['publicuri'] = $publicUri;
 		return $publicUri;
@@ -267,7 +276,7 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 	/**
 	 * @return mixed $value
 	 */
-	function getPublishStatus() {
+	public function getPublishStatus() {
 		return $this->caldavBackend->getPublishStatus($this);
 	}
 
diff --git a/apps/dav/lib/CardDAV/AddressBook.php b/apps/dav/lib/CardDAV/AddressBook.php
index 1c13ac00aec..eb5bebaa2ee 100644
--- a/apps/dav/lib/CardDAV/AddressBook.php
+++ b/apps/dav/lib/CardDAV/AddressBook.php
@@ -29,6 +29,12 @@ use Sabre\DAV\Exception\Forbidden;
 use Sabre\DAV\Exception\NotFound;
 use Sabre\DAV\PropPatch;
 
+/**
+ * Class AddressBook
+ *
+ * @package OCA\DAV\CardDAV
+ * @property BackendInterface|CardDavBackend $carddavBackend
+ */
 class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable {
 
 	/**
@@ -41,8 +47,8 @@ class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable {
 	public function __construct(BackendInterface $carddavBackend, array $addressBookInfo, IL10N $l10n) {
 		parent::__construct($carddavBackend, $addressBookInfo);
 
-		if ($this->getName() === CardDavBackend::PERSONAL_ADDRESSBOOK_URI &&
-			$this->addressBookInfo['{DAV:}displayname'] === CardDavBackend::PERSONAL_ADDRESSBOOK_NAME) {
+		if ($this->addressBookInfo['{DAV:}displayname'] === CardDavBackend::PERSONAL_ADDRESSBOOK_NAME &&
+			$this->getName() === CardDavBackend::PERSONAL_ADDRESSBOOK_URI) {
 			$this->addressBookInfo['{DAV:}displayname'] = $l10n->t('Contacts');
 		}
 	}
@@ -64,11 +70,13 @@ class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable {
 	 * @param array $add
 	 * @param array $remove
 	 * @return void
+	 * @throws Forbidden
 	 */
-	function updateShares(array $add, array $remove) {
-		/** @var CardDavBackend $carddavBackend */
-		$carddavBackend = $this->carddavBackend;
-		$carddavBackend->updateShares($this, $add, $remove);
+	public function updateShares(array $add, array $remove) {
+		if ($this->isShared()) {
+			throw new Forbidden();
+		}
+		$this->carddavBackend->updateShares($this, $add, $remove);
 	}
 
 	/**
@@ -83,13 +91,14 @@ class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable {
 	 *
 	 * @return array
 	 */
-	function getShares() {
-		/** @var CardDavBackend $carddavBackend */
-		$carddavBackend = $this->carddavBackend;
-		return $carddavBackend->getShares($this->getResourceId());
+	public function getShares() {
+		if ($this->isShared()) {
+			return [];
+		}
+		return $this->carddavBackend->getShares($this->getResourceId());
 	}
 
-	function getACL() {
+	public function getACL() {
 		$acl =  [
 			[
 				'privilege' => '{DAV:}read',
@@ -123,16 +132,18 @@ class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable {
 			];
 		}
 
-		/** @var CardDavBackend $carddavBackend */
-		$carddavBackend = $this->carddavBackend;
-		return $carddavBackend->applyShareAcl($this->getResourceId(), $acl);
+		if ($this->isShared()) {
+			return $acl;
+		}
+
+		return $this->carddavBackend->applyShareAcl($this->getResourceId(), $acl);
 	}
 
-	function getChildACL() {
+	public function getChildACL() {
 		return $this->getACL();
 	}
 
-	function getChild($name) {
+	public function getChild($name) {
 
 		$obj = $this->carddavBackend->getCard($this->addressBookInfo['id'], $name);
 		if (!$obj) {
@@ -150,17 +161,17 @@ class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable {
 		return $this->addressBookInfo['id'];
 	}
 
-	function getOwner() {
+	public function getOwner() {
 		if (isset($this->addressBookInfo['{http://owncloud.org/ns}owner-principal'])) {
 			return $this->addressBookInfo['{http://owncloud.org/ns}owner-principal'];
 		}
 		return parent::getOwner();
 	}
 
-	function delete() {
+	public function delete() {
 		if (isset($this->addressBookInfo['{http://owncloud.org/ns}owner-principal'])) {
 			$principal = 'principal:' . parent::getOwner();
-			$shares = $this->getShares();
+			$shares = $this->carddavBackend->getShares($this->getResourceId());
 			$shares = array_filter($shares, function($share) use ($principal){
 				return $share['href'] === $principal;
 			});
@@ -168,9 +179,7 @@ class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable {
 				throw new Forbidden();
 			}
 
-			/** @var CardDavBackend $cardDavBackend */
-			$cardDavBackend = $this->carddavBackend;
-			$cardDavBackend->updateShares($this, [], [
+			$this->carddavBackend->updateShares($this, [], [
 				'href' => $principal
 			]);
 			return;
@@ -178,7 +187,7 @@ class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable {
 		parent::delete();
 	}
 
-	function propPatch(PropPatch $propPatch) {
+	public function propPatch(PropPatch $propPatch) {
 		if (isset($this->addressBookInfo['{http://owncloud.org/ns}owner-principal'])) {
 			throw new Forbidden();
 		}
@@ -186,10 +195,15 @@ class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable {
 	}
 
 	public function getContactsGroups() {
-		/** @var CardDavBackend $cardDavBackend */
-		$cardDavBackend = $this->carddavBackend;
+		return $this->carddavBackend->collectCardProperties($this->getResourceId(), 'CATEGORIES');
+	}
+
+	private function isShared() {
+		if (!isset($this->addressBookInfo['{http://owncloud.org/ns}owner-principal'])) {
+			return false;
+		}
 
-		return $cardDavBackend->collectCardProperties($this->getResourceId(), 'CATEGORIES');
+		return $this->addressBookInfo['{http://owncloud.org/ns}owner-principal'] !== $this->addressBookInfo['principaluri'];
 	}
 
 	private function canWrite() {
diff --git a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php b/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php
index 22ef232dac4..63ca03b0d3d 100644
--- a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php
+++ b/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php
@@ -143,8 +143,6 @@ class CalDavBackendTest extends AbstractCalDavBackendTest {
 		$this->assertAcl(self::UNIT_TEST_USER, '{DAV:}write', $acl);
 		$this->assertAccess($userCanRead, self::UNIT_TEST_USER1, '{DAV:}read', $acl);
 		$this->assertAccess($userCanWrite, self::UNIT_TEST_USER1, '{DAV:}write', $acl);
-		$this->assertAccess($groupCanRead, self::UNIT_TEST_GROUP, '{DAV:}read', $acl);
-		$this->assertAccess($groupCanWrite, self::UNIT_TEST_GROUP, '{DAV:}write', $acl);
 		$this->assertEquals(self::UNIT_TEST_USER, $calendar->getOwner());
 
 		// test acls on the child
@@ -178,8 +176,6 @@ EOD;
 		$this->assertAcl(self::UNIT_TEST_USER, '{DAV:}write', $acl);
 		$this->assertAccess($userCanRead, self::UNIT_TEST_USER1, '{DAV:}read', $acl);
 		$this->assertAccess($userCanWrite, self::UNIT_TEST_USER1, '{DAV:}write', $acl);
-		$this->assertAccess($groupCanRead, self::UNIT_TEST_GROUP, '{DAV:}read', $acl);
-		$this->assertAccess($groupCanWrite, self::UNIT_TEST_GROUP, '{DAV:}write', $acl);
 
 		// delete the address book
 		$this->dispatcher->expects($this->at(0))
diff --git a/apps/dav/tests/unit/CardDAV/AddressBookTest.php b/apps/dav/tests/unit/CardDAV/AddressBookTest.php
index 22992d564f6..132fa4796db 100644
--- a/apps/dav/tests/unit/CardDAV/AddressBookTest.php
+++ b/apps/dav/tests/unit/CardDAV/AddressBookTest.php
@@ -40,6 +40,7 @@ class AddressBookTest extends TestCase {
 		]);
 		$calendarInfo = [
 			'{http://owncloud.org/ns}owner-principal' => 'user1',
+			'{DAV:}displayname' => 'Test address book',
 			'principaluri' => 'user2',
 			'id' => 666,
 			'uri' => 'default',
@@ -61,6 +62,7 @@ class AddressBookTest extends TestCase {
 		]);
 		$calendarInfo = [
 			'{http://owncloud.org/ns}owner-principal' => 'user1',
+			'{DAV:}displayname' => 'Test address book',
 			'principaluri' => 'user2',
 			'id' => 666,
 			'uri' => 'default',
@@ -78,6 +80,7 @@ class AddressBookTest extends TestCase {
 		$backend = $this->getMockBuilder('OCA\DAV\CardDAV\CardDavBackend')->disableOriginalConstructor()->getMock();
 		$calendarInfo = [
 			'{http://owncloud.org/ns}owner-principal' => 'user1',
+			'{DAV:}displayname' => 'Test address book',
 			'principaluri' => 'user2',
 			'id' => 666,
 			'uri' => 'default',
@@ -95,6 +98,7 @@ class AddressBookTest extends TestCase {
 		$backend = $this->getMockBuilder('OCA\DAV\CardDAV\CardDavBackend')->disableOriginalConstructor()->getMock();
 		$backend->expects($this->any())->method('applyShareAcl')->willReturnArgument(1);
 		$calendarInfo = [
+			'{DAV:}displayname' => 'Test address book',
 			'principaluri' => 'user2',
 			'id' => 666,
 			'uri' => 'default'
diff --git a/apps/dav/tests/unit/CardDAV/ContactsManagerTest.php b/apps/dav/tests/unit/CardDAV/ContactsManagerTest.php
index 062ef72dbf0..a6f0384cc38 100644
--- a/apps/dav/tests/unit/CardDAV/ContactsManagerTest.php
+++ b/apps/dav/tests/unit/CardDAV/ContactsManagerTest.php
@@ -39,7 +39,7 @@ class ContactsManagerTest extends TestCase {
 		/** @var CardDavBackend | \PHPUnit_Framework_MockObject_MockObject $backEnd */
 		$backEnd = $this->getMockBuilder('OCA\DAV\CardDAV\CardDavBackend')->disableOriginalConstructor()->getMock();
 		$backEnd->method('getAddressBooksForUser')->willReturn([
-				['uri' => 'default'],
+				['{DAV:}displayname' => 'Test address book', 'uri' => 'default'],
 			]);
 
 		$l = $this->createMock(IL10N::class);