Merge pull request #3722 from nextcloud/backport-3678-dav-share-order-issue

[stable11] DAV sharing permissions should not depend on the order

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();
 

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']] = [
+			$addressBooks[$row['id']] = [
-					'id'  => $row['id'],
+				'id'  => $row['id'],
-					'uri' => $uri,
+				'uri' => $uri,
-					'principaluri' => $principalUriOriginal,
+				'principaluri' => $principalUriOriginal,
-					'{DAV:}displayname' => $displayName,
+				'{DAV:}displayname' => $displayName,
-					'{' . Plugin::NS_CARDDAV . '}addressbook-description' => $row['description'],
+				'{' . Plugin::NS_CARDDAV . '}addressbook-description' => $row['description'],
-					'{http://calendarserver.org/ns/}getctag' => $row['synctoken'],
+				'{http://calendarserver.org/ns/}getctag' => $row['synctoken'],
-					'{http://sabredav.org/ns}sync-token' => $row['synctoken']?$row['synctoken']:'0',
+				'{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 . '}owner-principal' => $row['principaluri'],
-					'{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}read-only' => (int)$row['access'] === Backend::ACCESS_READ,
+				$readOnlyPropertyName => $readOnly,
-				];
+			];
-			}
 		}
 		$result->closeCursor();
 

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();

apps/dav/tests/unit/CalDAV/CalDavBackendTest.php

@@ -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,