Merge pull request #3678 from nextcloud/issue-3677-caldav-sharing-permission-problem

CalDAV sharing permissions should not depend on the order

4 files changed, 100 insertions, 55 deletions

diff --git a/apps/dav/lib/CalDAV/CalDavBackend.php b/apps/dav/lib/CalDAV/CalDavBackend.php
index dfef3111324..dbe86438238 100644
--- a/apps/dav/lib/CalDAV/CalDavBackend.php
+++ b/apps/dav/lib/CalDAV/CalDavBackend.php
@@ -277,7 +277,21 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription
 			->setParameter('principaluri', $principals, \Doctrine\DBAL\Connection::PARAM_STR_ARRAY)
 			->execute();
 
+		$readOnlyPropertyName = '{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}read-only';
 		while($row = $result->fetch()) {
+			$readOnly = (int) $row['access'] === Backend::ACCESS_READ;
+			if (isset($calendars[$row['id']])) {
+				if ($readOnly) {
+					// New share can not have more permissions then the old one.
+					continue;
+				}
+				if (isset($calendars[$row['id']][$readOnlyPropertyName]) &&
+					$calendars[$row['id']][$readOnlyPropertyName] === 0) {
+					// Old share is already read-write, no more permissions can be gained
+					continue;
+				}
+			}
+
 			list(, $name) = URLUtil::splitPath($row['principaluri']);
 			$uri = $row['uri'] . '_shared_by_' . $name;
 			$row['displayname'] = $row['displayname'] . ' (' . $this->getUserDisplayName($name) . ')';
@@ -294,16 +308,14 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription
 				'{' . Plugin::NS_CALDAV . '}supported-calendar-component-set' => new SupportedCalendarComponentSet($components),
 				'{' . Plugin::NS_CALDAV . '}schedule-calendar-transp' => new ScheduleCalendarTransp($row['transparent']?'transparent':'opaque'),
 				'{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}owner-principal' => $this->convertPrincipal($row['principaluri'], !$this->legacyEndpoint),
-				'{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}read-only' => (int)$row['access'] === Backend::ACCESS_READ,
+				$readOnlyPropertyName => $readOnly,
 			];
 
 			foreach($this->propertyMap as $xmlName=>$dbName) {
 				$calendar[$xmlName] = $row[$dbName];
 			}
 
-			if (!isset($calendars[$calendar['id']])) {
-				$calendars[$calendar['id']] = $calendar;
-			}
+			$calendars[$calendar['id']] = $calendar;
 		}
 		$result->closeCursor();
 
diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php
index b71c6380a91..6f9a73298ef 100644
--- a/apps/dav/lib/CardDAV/CardDavBackend.php
+++ b/apps/dav/lib/CardDAV/CardDavBackend.php
@@ -172,23 +172,36 @@ class CardDavBackend implements BackendInterface, SyncSupport {
 			->setParameter('principaluri', $principals, IQueryBuilder::PARAM_STR_ARRAY)
 			->execute();
 
+		$readOnlyPropertyName = '{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}read-only';
 		while($row = $result->fetch()) {
+			$readOnly = (int) $row['access'] === Backend::ACCESS_READ;
+			if (isset($addressBooks[$row['id']])) {
+				if ($readOnly) {
+					// New share can not have more permissions then the old one.
+					continue;
+				}
+				if (isset($addressBooks[$row['id']][$readOnlyPropertyName]) &&
+					$addressBooks[$row['id']][$readOnlyPropertyName] === 0) {
+					// Old share is already read-write, no more permissions can be gained
+					continue;
+				}
+			}
+
 			list(, $name) = URLUtil::splitPath($row['principaluri']);
 			$uri = $row['uri'] . '_shared_by_' . $name;
 			$displayName = $row['displayname'] . ' (' . $this->getUserDisplayName($name) . ')';
-			if (!isset($addressBooks[$row['id']])) {
-				$addressBooks[$row['id']] = [
-					'id'  => $row['id'],
-					'uri' => $uri,
-					'principaluri' => $principalUriOriginal,
-					'{DAV:}displayname' => $displayName,
-					'{' . Plugin::NS_CARDDAV . '}addressbook-description' => $row['description'],
-					'{http://calendarserver.org/ns/}getctag' => $row['synctoken'],
-					'{http://sabredav.org/ns}sync-token' => $row['synctoken']?$row['synctoken']:'0',
-					'{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}owner-principal' => $row['principaluri'],
-					'{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}read-only' => (int)$row['access'] === Backend::ACCESS_READ,
-				];
-			}
+
+			$addressBooks[$row['id']] = [
+				'id'  => $row['id'],
+				'uri' => $uri,
+				'principaluri' => $principalUriOriginal,
+				'{DAV:}displayname' => $displayName,
+				'{' . Plugin::NS_CARDDAV . '}addressbook-description' => $row['description'],
+				'{http://calendarserver.org/ns/}getctag' => $row['synctoken'],
+				'{http://sabredav.org/ns}sync-token' => $row['synctoken']?$row['synctoken']:'0',
+				'{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}owner-principal' => $row['principaluri'],
+				$readOnlyPropertyName => $readOnly,
+			];
 		}
 		$result->closeCursor();
 
diff --git a/apps/dav/tests/unit/CalDAV/AbstractCalDavBackendTest.php b/apps/dav/tests/unit/CalDAV/AbstractCalDavBackendTest.php
index d15be72c77b..ffdba9c5c8a 100644
--- a/apps/dav/tests/unit/CalDAV/AbstractCalDavBackendTest.php
+++ b/apps/dav/tests/unit/CalDAV/AbstractCalDavBackendTest.php
@@ -55,6 +55,7 @@ abstract class AbstractCalDavBackendTest extends TestCase {
 	const UNIT_TEST_USER = 'principals/users/caldav-unit-test';
 	const UNIT_TEST_USER1 = 'principals/users/caldav-unit-test1';
 	const UNIT_TEST_GROUP = 'principals/groups/caldav-unit-test-group';
+	const UNIT_TEST_GROUP2 = 'principals/groups/caldav-unit-test-group2';
 
 	public function setUp() {
 		parent::setUp();
@@ -71,7 +72,7 @@ abstract class AbstractCalDavBackendTest extends TestCase {
 			]);
 		$this->principal->expects($this->any())->method('getGroupMembership')
 			->withAnyParameters()
-			->willReturn([self::UNIT_TEST_GROUP]);
+			->willReturn([self::UNIT_TEST_GROUP, self::UNIT_TEST_GROUP2]);
 
 		$db = \OC::$server->getDatabaseConnection();
 		$this->random = \OC::$server->getSecureRandom();
diff --git a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php b/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php
index bd6c9f27886..22ef232dac4 100644
--- a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php
+++ b/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php
@@ -57,18 +57,18 @@ class CalDavBackendTest extends AbstractCalDavBackendTest {
 		$this->backend->updateCalendar($calendarId, $patch);
 		$patch->commit();
 		$this->assertEquals(1, $this->backend->getCalendarsForUserCount(self::UNIT_TEST_USER));
-		$books = $this->backend->getCalendarsForUser(self::UNIT_TEST_USER);
-		$this->assertEquals(1, count($books));
-		$this->assertEquals('Unit test', $books[0]['{DAV:}displayname']);
-		$this->assertEquals('Calendar used for unit testing', $books[0]['{urn:ietf:params:xml:ns:caldav}calendar-description']);
+		$calendars = $this->backend->getCalendarsForUser(self::UNIT_TEST_USER);
+		$this->assertCount(1, $calendars);
+		$this->assertEquals('Unit test', $calendars[0]['{DAV:}displayname']);
+		$this->assertEquals('Calendar used for unit testing', $calendars[0]['{urn:ietf:params:xml:ns:caldav}calendar-description']);
 
 		// delete the address book
 		$this->dispatcher->expects($this->at(0))
 			->method('dispatch')
 			->with('\OCA\DAV\CalDAV\CalDavBackend::deleteCalendar');
-		$this->backend->deleteCalendar($books[0]['id']);
-		$books = $this->backend->getCalendarsForUser(self::UNIT_TEST_USER);
-		$this->assertEquals(0, count($books));
+		$this->backend->deleteCalendar($calendars[0]['id']);
+		$calendars = $this->backend->getCalendarsForUser(self::UNIT_TEST_USER);
+		$this->assertCount(0, $calendars);
 	}
 
 	public function providesSharingData() {
@@ -83,6 +83,26 @@ class CalDavBackendTest extends AbstractCalDavBackendTest {
 					'readOnly' => true
 				]
 			]],
+			[true, true, true, false, [
+				[
+					'href' => 'principal:' . self::UNIT_TEST_GROUP,
+					'readOnly' => true,
+				],
+				[
+					'href' => 'principal:' . self::UNIT_TEST_GROUP2,
+					'readOnly' => false,
+				],
+			]],
+			[true, true, true, true, [
+				[
+					'href' => 'principal:' . self::UNIT_TEST_GROUP,
+					'readOnly' => false,
+				],
+				[
+					'href' => 'principal:' . self::UNIT_TEST_GROUP2,
+					'readOnly' => true,
+				],
+			]],
 			[true, false, false, false, [
 				[
 					'href' => 'principal:' . self::UNIT_TEST_USER1,
@@ -98,9 +118,8 @@ class CalDavBackendTest extends AbstractCalDavBackendTest {
 	 */
 	public function testCalendarSharing($userCanRead, $userCanWrite, $groupCanRead, $groupCanWrite, $add) {
 
-		/** @var IL10N | \PHPUnit_Framework_MockObject_MockObject $l10n */
-		$l10n = $this->getMockBuilder('\OCP\IL10N')
-			->disableOriginalConstructor()->getMock();
+		/** @var IL10N|\PHPUnit_Framework_MockObject_MockObject $l10n */
+		$l10n = $this->createMock(IL10N::class);
 		$l10n
 			->expects($this->any())
 			->method('t')
@@ -109,16 +128,16 @@ class CalDavBackendTest extends AbstractCalDavBackendTest {
 			}));
 
 		$calendarId = $this->createTestCalendar();
-		$books = $this->backend->getCalendarsForUser(self::UNIT_TEST_USER);
-		$this->assertEquals(1, count($books));
-		$calendar = new Calendar($this->backend, $books[0], $l10n);
+		$calendars = $this->backend->getCalendarsForUser(self::UNIT_TEST_USER);
+		$this->assertCount(1, $calendars);
+		$calendar = new Calendar($this->backend, $calendars[0], $l10n);
 		$this->dispatcher->expects($this->at(0))
 			->method('dispatch')
 			->with('\OCA\DAV\CalDAV\CalDavBackend::updateShares');
 		$this->backend->updateShares($calendar, $add, []);
-		$books = $this->backend->getCalendarsForUser(self::UNIT_TEST_USER1);
-		$this->assertEquals(1, count($books));
-		$calendar = new Calendar($this->backend, $books[0], $l10n);
+		$calendars = $this->backend->getCalendarsForUser(self::UNIT_TEST_USER1);
+		$this->assertCount(1, $calendars);
+		$calendar = new Calendar($this->backend, $calendars[0], $l10n);
 		$acl = $calendar->getACL();
 		$this->assertAcl(self::UNIT_TEST_USER, '{DAV:}read', $acl);
 		$this->assertAcl(self::UNIT_TEST_USER, '{DAV:}write', $acl);
@@ -129,7 +148,7 @@ class CalDavBackendTest extends AbstractCalDavBackendTest {
 		$this->assertEquals(self::UNIT_TEST_USER, $calendar->getOwner());
 
 		// test acls on the child
-		$uri = $this->getUniqueID('calobj');
+		$uri = static::getUniqueID('calobj');
 		$calData = <<<'EOD'
 BEGIN:VCALENDAR
 VERSION:2.0
@@ -166,9 +185,9 @@ EOD;
 		$this->dispatcher->expects($this->at(0))
 			->method('dispatch')
 			->with('\OCA\DAV\CalDAV\CalDavBackend::deleteCalendar');
-		$this->backend->deleteCalendar($books[0]['id']);
-		$books = $this->backend->getCalendarsForUser(self::UNIT_TEST_USER);
-		$this->assertEquals(0, count($books));
+		$this->backend->deleteCalendar($calendars[0]['id']);
+		$calendars = $this->backend->getCalendarsForUser(self::UNIT_TEST_USER);
+		$this->assertCount(0, $calendars);
 	}
 
 	public function testCalendarObjectsOperations() {
@@ -176,7 +195,7 @@ EOD;
 		$calendarId = $this->createTestCalendar();
 
 		// create a card
-		$uri = $this->getUniqueID('calobj');
+		$uri = static::getUniqueID('calobj');
 		$calData = <<<'EOD'
 BEGIN:VCALENDAR
 VERSION:2.0
@@ -201,7 +220,7 @@ EOD;
 
 		// get all the cards
 		$calendarObjects = $this->backend->getCalendarObjects($calendarId);
-		$this->assertEquals(1, count($calendarObjects));
+		$this->assertCount(1, $calendarObjects);
 		$this->assertEquals($calendarId, $calendarObjects[0]['calendarid']);
 		$this->assertArrayHasKey('classification', $calendarObjects[0]);
 
@@ -245,7 +264,7 @@ EOD;
 			->with('\OCA\DAV\CalDAV\CalDavBackend::deleteCalendarObject');
 		$this->backend->deleteCalendarObject($calendarId, $uri);
 		$calendarObjects = $this->backend->getCalendarObjects($calendarId);
-		$this->assertEquals(0, count($calendarObjects));
+		$this->assertCount(0, $calendarObjects);
 	}
 
 	public function testMultiCalendarObjects() {
@@ -269,17 +288,17 @@ CLASS:PUBLIC
 END:VEVENT
 END:VCALENDAR
 EOD;
-		$uri0 = $this->getUniqueID('card');
+		$uri0 = static::getUniqueID('card');
 		$this->dispatcher->expects($this->at(0))
 			->method('dispatch')
 			->with('\OCA\DAV\CalDAV\CalDavBackend::createCalendarObject');
 		$this->backend->createCalendarObject($calendarId, $uri0, $calData);
-		$uri1 = $this->getUniqueID('card');
+		$uri1 = static::getUniqueID('card');
 		$this->dispatcher->expects($this->at(0))
 			->method('dispatch')
 			->with('\OCA\DAV\CalDAV\CalDavBackend::createCalendarObject');
 		$this->backend->createCalendarObject($calendarId, $uri1, $calData);
-		$uri2 = $this->getUniqueID('card');
+		$uri2 = static::getUniqueID('card');
 		$this->dispatcher->expects($this->at(0))
 			->method('dispatch')
 			->with('\OCA\DAV\CalDAV\CalDavBackend::createCalendarObject');
@@ -287,11 +306,11 @@ EOD;
 
 		// get all the cards
 		$calendarObjects = $this->backend->getCalendarObjects($calendarId);
-		$this->assertEquals(3, count($calendarObjects));
+		$this->assertCount(3, $calendarObjects);
 
 		// get the cards
 		$calendarObjects = $this->backend->getMultipleCalendarObjects($calendarId, [$uri1, $uri2]);
-		$this->assertEquals(2, count($calendarObjects));
+		$this->assertCount(2, $calendarObjects);
 		foreach($calendarObjects as $card) {
 			$this->assertArrayHasKey('id', $card);
 			$this->assertArrayHasKey('uri', $card);
@@ -316,7 +335,7 @@ EOD;
 			->with('\OCA\DAV\CalDAV\CalDavBackend::deleteCalendarObject');
 		$this->backend->deleteCalendarObject($calendarId, $uri2);
 		$calendarObjects = $this->backend->getCalendarObjects($calendarId);
-		$this->assertEquals(0, count($calendarObjects));
+		$this->assertCount(0, $calendarObjects);
 	}
 
 	/**
@@ -385,15 +404,15 @@ EOD;
 
 		$calendarInfo = $this->backend->getCalendarsForUser(self::UNIT_TEST_USER)[0];
 
-		$l10n = $this->getMockBuilder('\OCP\IL10N')
-			->disableOriginalConstructor()->getMock();
+		/** @var IL10N|\PHPUnit_Framework_MockObject_MockObject $l10n */
+		$l10n = $this->createMock(IL10N::class);
 
 		$calendar = new Calendar($this->backend, $calendarInfo, $l10n);
 		$calendar->setPublishStatus(true);
 		$this->assertNotEquals(false, $calendar->getPublishStatus());
 
 		$publicCalendars = $this->backend->getPublicCalendars();
-		$this->assertEquals(1, count($publicCalendars));
+		$this->assertCount(1, $publicCalendars);
 		$this->assertEquals(true, $publicCalendars[0]['{http://owncloud.org/ns}public']);
 
 		$publicCalendarURI = $publicCalendars[0]['uri'];
@@ -415,7 +434,7 @@ EOD;
 		]);
 
 		$subscriptions = $this->backend->getSubscriptionsForUser(self::UNIT_TEST_USER);
-		$this->assertEquals(1, count($subscriptions));
+		$this->assertCount(1, $subscriptions);
 		$this->assertEquals('#1C4587', $subscriptions[0]['{http://apple.com/ns/ical/}calendar-color']);
 		$this->assertEquals(true, $subscriptions[0]['{http://calendarserver.org/ns/}subscribed-strip-todos']);
 		$this->assertEquals($id, $subscriptions[0]['id']);
@@ -428,21 +447,21 @@ EOD;
 		$patch->commit();
 
 		$subscriptions = $this->backend->getSubscriptionsForUser(self::UNIT_TEST_USER);
-		$this->assertEquals(1, count($subscriptions));
+		$this->assertCount(1, $subscriptions);
 		$this->assertEquals($id, $subscriptions[0]['id']);
 		$this->assertEquals('Unit test', $subscriptions[0]['{DAV:}displayname']);
 		$this->assertEquals('#ac0606', $subscriptions[0]['{http://apple.com/ns/ical/}calendar-color']);
 
 		$this->backend->deleteSubscription($id);
 		$subscriptions = $this->backend->getSubscriptionsForUser(self::UNIT_TEST_USER);
-		$this->assertEquals(0, count($subscriptions));
+		$this->assertCount(0, $subscriptions);
 	}
 
 	public function testScheduling() {
 		$this->backend->createSchedulingObject(self::UNIT_TEST_USER, 'Sample Schedule', '');
 
 		$sos = $this->backend->getSchedulingObjects(self::UNIT_TEST_USER);
-		$this->assertEquals(1, count($sos));
+		$this->assertCount(1, $sos);
 
 		$so = $this->backend->getSchedulingObject(self::UNIT_TEST_USER, 'Sample Schedule');
 		$this->assertNotNull($so);
@@ -450,7 +469,7 @@ EOD;
 		$this->backend->deleteSchedulingObject(self::UNIT_TEST_USER, 'Sample Schedule');
 
 		$sos = $this->backend->getSchedulingObjects(self::UNIT_TEST_USER);
-		$this->assertEquals(0, count($sos));
+		$this->assertCount(0, $sos);
 	}
 
 	/**