Merge pull request #23342 from owncloud/fix-group-sharing-for-v1-caldav-and-carddav

Fix group shares on v1 caldav and carddav

apps/dav/appinfo/v1/caldav.php

@@ -24,11 +24,11 @@
 // Backends
 use OCA\DAV\CalDAV\CalDavBackend;
 use OCA\DAV\Connector\LegacyDAVACL;
+use OCA\DAV\CalDAV\CalendarRoot;
 use OCA\DAV\Connector\Sabre\Auth;
 use OCA\DAV\Connector\Sabre\ExceptionLoggerPlugin;
 use OCA\DAV\Connector\Sabre\MaintenancePlugin;
 use OCA\DAV\Connector\Sabre\Principal;
-use Sabre\CalDAV\CalendarRoot;
 
 $authBackend = new Auth(
 	\OC::$server->getSession(),

apps/dav/lib/caldav/caldavbackend.php

@@ -138,6 +138,7 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription
 	 * @return array
 	 */
 	function getCalendarsForUser($principalUri) {
+		$principalUriOriginal = $principalUri;
 		$principalUri = $this->convertPrincipal($principalUri, true);
 		$fields = array_values($this->propertyMap);
 		$fields[] = 'id';
@@ -184,7 +185,7 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription
 		$stmt->closeCursor();
 
 		// query for shared calendars
-		$principals = $this->principalBackend->getGroupMembership($principalUri);
+		$principals = $this->principalBackend->getGroupMembership($principalUriOriginal, true);
 		$principals[]= $principalUri;
 
 		$fields = array_values($this->propertyMap);
@@ -194,6 +195,7 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription
 		$fields[] = 'a.components';
 		$fields[] = 'a.principaluri';
 		$fields[] = 'a.transparent';
+		$fields[] = 's.access';
 		$query = $this->db->getQueryBuilder();
 		$result = $query->select($fields)
 			->from('dav_shares', 's')
@@ -221,6 +223,7 @@ 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' => $row['principaluri'],
+				'{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}read-only' => (int)$row['access'] === Backend::ACCESS_READ,
 			];
 
 			foreach($this->propertyMap as $xmlName=>$dbName) {

apps/dav/lib/caldav/calendar.php

@@ -86,7 +86,31 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 	}
 
 	function getACL() {
-		$acl = parent::getACL();
+		$acl =  [
+			[
+				'privilege' => '{DAV:}read',
+				'principal' => $this->getOwner(),
+				'protected' => true,
+			]];
+		$acl[] = [
+			'privilege' => '{DAV:}write',
+			'principal' => $this->getOwner(),
+			'protected' => true,
+		];
+		if ($this->getOwner() !== parent::getOwner()) {
+			$acl[] =  [
+					'privilege' => '{DAV:}read',
+					'principal' => parent::getOwner(),
+					'protected' => true,
+				];
+			if ($this->canWrite()) {
+				$acl[] = [
+					'privilege' => '{DAV:}write',
+					'principal' => parent::getOwner(),
+					'protected' => true,
+				];
+			}
+		}
 
 		/** @var CalDavBackend $calDavBackend */
 		$calDavBackend = $this->caldavBackend;
@@ -94,11 +118,7 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 	}
 
 	function getChildACL() {
-		$acl = parent::getChildACL();
+		return $this->getACL();
-
-		/** @var CalDavBackend $calDavBackend */
-		$calDavBackend = $this->caldavBackend;
-		return $calDavBackend->applyShareAcl($this->getResourceId(), $acl);
 	}
 
 	function getOwner() {
@@ -137,4 +157,12 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IShareable {
 		}
 		parent::propPatch($propPatch);
 	}
+
+	private function canWrite() {
+		if (isset($this->calendarInfo['{http://owncloud.org/ns}read-only'])) {
+			return !$this->calendarInfo['{http://owncloud.org/ns}read-only'];
+		}
+		return true;
+	}
+
 }

apps/dav/lib/carddav/addressbook.php

@@ -21,6 +21,7 @@
 namespace OCA\DAV\CardDAV;
 
 use OCA\DAV\DAV\Sharing\IShareable;
+use Sabre\CardDAV\Card;
 use Sabre\DAV\Exception\Forbidden;
 use Sabre\DAV\Exception\NotFound;
 use Sabre\DAV\PropPatch;
@@ -70,7 +71,31 @@ class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable {
 	}
 
 	function getACL() {
-		$acl = parent::getACL();
+		$acl =  [
+			[
+				'privilege' => '{DAV:}read',
+				'principal' => $this->getOwner(),
+				'protected' => true,
+			]];
+		$acl[] = [
+				'privilege' => '{DAV:}write',
+				'principal' => $this->getOwner(),
+				'protected' => true,
+			];
+		if ($this->getOwner() !== parent::getOwner()) {
+			$acl[] =  [
+					'privilege' => '{DAV:}read',
+					'principal' => parent::getOwner(),
+					'protected' => true,
+				];
+			if ($this->canWrite()) {
+				$acl[] = [
+					'privilege' => '{DAV:}write',
+					'principal' => parent::getOwner(),
+					'protected' => true,
+				];
+			}
+		}
 		if ($this->getOwner() === 'principals/system/system') {
 			$acl[] = [
 					'privilege' => '{DAV:}read',
@@ -79,49 +104,24 @@ class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable {
 			];
 		}
 
-		// add the current user
-		if (isset($this->addressBookInfo['{http://owncloud.org/ns}owner-principal'])) {
-			$owner = $this->addressBookInfo['{http://owncloud.org/ns}owner-principal'];
-			$acl[] = [
-					'privilege' => '{DAV:}read',
-					'principal' => $owner,
-					'protected' => true,
-				];
-			if ($this->addressBookInfo['{http://owncloud.org/ns}read-only']) {
-				$acl[] = [
-					'privilege' => '{DAV:}write',
-					'principal' => $owner,
-					'protected' => true,
-				];
-			}
-		}
-
 		/** @var CardDavBackend $carddavBackend */
 		$carddavBackend = $this->carddavBackend;
 		return $carddavBackend->applyShareAcl($this->getResourceId(), $acl);
 	}
 
 	function getChildACL() {
-		$acl = parent::getChildACL();
+		return $this->getACL();
-		if ($this->getOwner() === 'principals/system/system') {
-			$acl[] = [
-					'privilege' => '{DAV:}read',
-					'principal' => '{DAV:}authenticated',
-					'protected' => true,
-			];
-		}
-
-		/** @var CardDavBackend $carddavBackend */
-		$carddavBackend = $this->carddavBackend;
-		return $carddavBackend->applyShareAcl($this->getResourceId(), $acl);
 	}
 
 	function getChild($name) {
-		$obj = $this->carddavBackend->getCard($this->getResourceId(), $name);
+
+		$obj = $this->carddavBackend->getCard($this->addressBookInfo['id'], $name);
 		if (!$obj) {
 			throw new NotFound('Card not found');
 		}
+		$obj['acl'] = $this->getChildACL();
 		return new Card($this->carddavBackend, $this->addressBookInfo, $obj);
+
 	}
 
 	/**
@@ -172,4 +172,11 @@ class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable {
 
 		return $cardDavBackend->collectCardProperties($this->getResourceId(), 'CATEGORIES');
 	}
+
+	private function canWrite() {
+		if (isset($this->addressBookInfo['{http://owncloud.org/ns}read-only'])) {
+			return !$this->addressBookInfo['{http://owncloud.org/ns}read-only'];
+		}
+		return true;
+	}
 }

apps/dav/lib/carddav/card.php

@@ -1,45 +0,0 @@
-<?php
-/**
- * @author Thomas Müller <thomas.mueller@tmit.eu>
- *
- * @copyright Copyright (c) 2016, ownCloud, Inc.
- * @license AGPL-3.0
- *
- * This code is free software: you can redistribute it and/or modify
- * it under the terms of the GNU Affero General Public License, version 3,
- * as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU Affero General Public License for more details.
- *
- * You should have received a copy of the GNU Affero General Public License, version 3,
- * along with this program.  If not, see <http://www.gnu.org/licenses/>
- *
- */
-
-namespace OCA\DAV\CardDAV;
-
-class Card extends \Sabre\CardDAV\Card {
-
-	function getACL() {
-		$acl = parent::getACL();
-		if ($this->getOwner() === 'principals/system/system') {
-			$acl[] = [
-				'privilege' => '{DAV:}read',
-				'principal' => '{DAV:}authenticated',
-				'protected' => true,
-			];
-		}
-
-		/** @var CardDavBackend $carddavBackend */
-		$carddavBackend = $this->carddavBackend;
-		return $carddavBackend->applyShareAcl($this->getBookId(), $acl);
-	}
-
-	private function getBookId() {
-		return $this->addressBookInfo['id'];
-	}
-
-}

apps/dav/lib/carddav/carddavbackend.php

@@ -62,10 +62,6 @@ class CardDavBackend implements BackendInterface, SyncSupport {
 			'BDAY', 'UID', 'N', 'FN', 'TITLE', 'ROLE', 'NOTE', 'NICKNAME',
 			'ORG', 'CATEGORIES', 'EMAIL', 'TEL', 'IMPP', 'ADR', 'URL', 'GEO', 'CLOUD');
 
-	const ACCESS_OWNER = 1;
-	const ACCESS_READ_WRITE = 2;
-	const ACCESS_READ = 3;
-
 	/** @var EventDispatcherInterface */
 	private $dispatcher;
 
@@ -126,7 +122,7 @@ class CardDavBackend implements BackendInterface, SyncSupport {
 		$result->closeCursor();
 
 		// query for shared calendars
-		$principals = $this->principalBackend->getGroupMembership($principalUri);
+		$principals = $this->principalBackend->getGroupMembership($principalUri, true);
 		$principals[]= $principalUri;
 
 		$query = $this->db->getQueryBuilder();
@@ -153,7 +149,7 @@ class CardDavBackend implements BackendInterface, SyncSupport {
 					'{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' => $row['access'] === self::ACCESS_READ,
+					'{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}read-only' => (int)$row['access'] === Backend::ACCESS_READ,
 				];
 			}
 		}

apps/dav/lib/connector/legacydavacl.php

@@ -23,7 +23,10 @@
 namespace OCA\DAV\Connector;
 
 use OCA\DAV\Connector\Sabre\DavAclPlugin;
+use Sabre\DAV\INode;
+use Sabre\DAV\PropFind;
 use Sabre\HTTP\URLUtil;
+use Sabre\DAVACL\Xml\Property\Principal;
 
 class LegacyDAVACL extends DavAclPlugin {
 
@@ -67,4 +70,16 @@ class LegacyDAVACL extends DavAclPlugin {
 		}
 		return "principals/$name";
 	}
+
+	function propFind(PropFind $propFind, INode $node) {
+		/* Overload current-user-principal */
+		$propFind->handle('{DAV:}current-user-principal', function () {
+			if ($url = parent::getCurrentUserPrincipal()) {
+				return new Principal(Principal::HREF, $url . '/');
+			} else {
+				return new Principal(Principal::UNAUTHENTICATED);
+			}
+		});
+		parent::propFind($propFind, $node);
+	}
 }

apps/dav/lib/connector/sabre/principal.php

@@ -132,10 +132,11 @@ class Principal implements BackendInterface {
 	 * Returns the list of groups a principal is a member of
 	 *
 	 * @param string $principal
+	 * @param bool $needGroups
 	 * @return array
 	 * @throws Exception
 	 */
-	public function getGroupMembership($principal) {
+	public function getGroupMembership($principal, $needGroups = false) {
 		list($prefix, $name) = URLUtil::splitPath($principal);
 
 		if ($prefix === $this->principalPrefix) {
@@ -144,7 +145,7 @@ class Principal implements BackendInterface {
 				throw new Exception('Principal not found');
 			}
 
-			if ($this->hasGroups) {
+			if ($this->hasGroups || $needGroups) {
 				$groups = $this->groupManager->getUserGroups($user);
 				$groups = array_map(function($group) {
 					/** @var IGroup $group */

apps/dav/tests/unit/caldav/calendartest.php

@@ -103,4 +103,64 @@ class CalendarTest extends TestCase {
 			$this->assertTrue(true);
 		}
 	}
+
+	/**
+	 * @dataProvider providesReadOnlyInfo
+	 */
+	public function testAcl($expectsWrite, $readOnlyValue, $hasOwnerSet) {
+		/** @var \PHPUnit_Framework_MockObject_MockObject | CalDavBackend $backend */
+		$backend = $this->getMockBuilder('OCA\DAV\CalDAV\CalDavBackend')->disableOriginalConstructor()->getMock();
+		$backend->expects($this->any())->method('applyShareAcl')->willReturnArgument(1);
+		$calendarInfo = [
+			'principaluri' => 'user2',
+			'id' => 666,
+			'uri' => 'default'
+		];
+		if (!is_null($readOnlyValue)) {
+			$calendarInfo['{http://owncloud.org/ns}read-only'] = $readOnlyValue;
+		}
+		if ($hasOwnerSet) {
+			$calendarInfo['{http://owncloud.org/ns}owner-principal'] = 'user1';
+		}
+		$c = new Calendar($backend, $calendarInfo);
+		$acl = $c->getACL();
+		$childAcl = $c->getChildACL();
+
+		$expectedAcl = [[
+			'privilege' => '{DAV:}read',
+			'principal' => $hasOwnerSet ? 'user1' : 'user2',
+			'protected' => true
+		], [
+			'privilege' => '{DAV:}write',
+			'principal' => $hasOwnerSet ? 'user1' : 'user2',
+			'protected' => true
+		]];
+		if ($hasOwnerSet) {
+			$expectedAcl[] = [
+				'privilege' => '{DAV:}read',
+				'principal' => 'user2',
+				'protected' => true
+			];
+			if ($expectsWrite) {
+				$expectedAcl[] = [
+					'privilege' => '{DAV:}write',
+					'principal' => 'user2',
+					'protected' => true
+				];
+			}
+		}
+		$this->assertEquals($expectedAcl, $acl);
+		$this->assertEquals($expectedAcl, $childAcl);
+	}
+
+	public function providesReadOnlyInfo() {
+		return [
+			'read-only property not set' => [true, null, true],
+			'read-only property is false' => [true, false, true],
+			'read-only property is true' => [false, true, true],
+			'read-only property not set and no owner' => [true, null, false],
+			'read-only property is false and no owner' => [true, false, false],
+			'read-only property is true and no owner' => [false, true, false],
+		];
+	}
 }

apps/dav/tests/unit/carddav/addressbooktest.php

@@ -32,7 +32,7 @@ class AddressBookTest extends TestCase {
 		/** @var \PHPUnit_Framework_MockObject_MockObject | CardDavBackend $backend */
 		$backend = $this->getMockBuilder('OCA\DAV\CardDAV\CardDavBackend')->disableOriginalConstructor()->getMock();
 		$backend->expects($this->once())->method('updateShares');
-		$backend->method('getShares')->willReturn([
+		$backend->expects($this->any())->method('getShares')->willReturn([
 			['href' => 'principal:user2']
 		]);
 		$calendarInfo = [
@@ -51,7 +51,7 @@ class AddressBookTest extends TestCase {
 		/** @var \PHPUnit_Framework_MockObject_MockObject | CardDavBackend $backend */
 		$backend = $this->getMockBuilder('OCA\DAV\CardDAV\CardDavBackend')->disableOriginalConstructor()->getMock();
 		$backend->expects($this->never())->method('updateShares');
-		$backend->method('getShares')->willReturn([
+		$backend->expects($this->any())->method('getShares')->willReturn([
 			['href' => 'principal:group2']
 		]);
 		$calendarInfo = [
@@ -77,4 +77,63 @@ class AddressBookTest extends TestCase {
 		$c = new AddressBook($backend, $calendarInfo);
 		$c->propPatch(new PropPatch([]));
 	}
-}
+
+	/**
+	 * @dataProvider providesReadOnlyInfo
+	 */
+	public function testAcl($expectsWrite, $readOnlyValue, $hasOwnerSet) {
+		/** @var \PHPUnit_Framework_MockObject_MockObject | CardDavBackend $backend */
+		$backend = $this->getMockBuilder('OCA\DAV\CardDAV\CardDavBackend')->disableOriginalConstructor()->getMock();
+		$backend->expects($this->any())->method('applyShareAcl')->willReturnArgument(1);
+		$calendarInfo = [
+			'principaluri' => 'user2',
+			'id' => 666,
+			'uri' => 'default'
+		];
+		if (!is_null($readOnlyValue)) {
+			$calendarInfo['{http://owncloud.org/ns}read-only'] = $readOnlyValue;
+		}
+		if ($hasOwnerSet) {
+			$calendarInfo['{http://owncloud.org/ns}owner-principal'] = 'user1';
+		}
+		$c = new AddressBook($backend, $calendarInfo);
+		$acl = $c->getACL();
+		$childAcl = $c->getChildACL();
+
+		$expectedAcl = [[
+			'privilege' => '{DAV:}read',
+			'principal' => $hasOwnerSet ? 'user1' : 'user2',
+			'protected' => true
+		], [
+			'privilege' => '{DAV:}write',
+			'principal' => $hasOwnerSet ? 'user1' : 'user2',
+			'protected' => true
+		]];
+		if ($hasOwnerSet) {
+			$expectedAcl[] = [
+				'privilege' => '{DAV:}read',
+				'principal' => 'user2',
+				'protected' => true
+			];
+			if ($expectsWrite) {
+				$expectedAcl[] = [
+					'privilege' => '{DAV:}write',
+					'principal' => 'user2',
+					'protected' => true
+				];
+			}
+		}
+		$this->assertEquals($expectedAcl, $acl);
+		$this->assertEquals($expectedAcl, $childAcl);
+	}
+
+	public function providesReadOnlyInfo() {
+		return [
+			'read-only property not set' => [true, null, true],
+			'read-only property is false' => [true, false, true],
+			'read-only property is true' => [false, true, true],
+			'read-only property not set and no owner' => [true, null, false],
+			'read-only property is false and no owner' => [true, false, false],
+			'read-only property is true and no owner' => [false, true, false],
+		];
+	}}