From 06e8c70400dd6e3e1c153c4258eba0357c502a7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Thu, 17 Mar 2016 15:39:08 +0100 Subject: [PATCH] Fix acls for calendar objects and cards - fixes #23273 --- apps/dav/appinfo/v1/caldav.php | 2 +- apps/dav/lib/caldav/caldavbackend.php | 2 + apps/dav/lib/caldav/calendar.php | 40 +++++++++-- apps/dav/lib/carddav/addressbook.php | 69 ++++++++++--------- apps/dav/lib/carddav/card.php | 45 ------------ apps/dav/lib/carddav/carddavbackend.php | 6 +- apps/dav/lib/connector/sabre/principal.php | 1 + apps/dav/tests/unit/caldav/calendartest.php | 60 ++++++++++++++++ .../tests/unit/carddav/addressbooktest.php | 65 ++++++++++++++++- 9 files changed, 199 insertions(+), 91 deletions(-) delete mode 100644 apps/dav/lib/carddav/card.php diff --git a/apps/dav/appinfo/v1/caldav.php b/apps/dav/appinfo/v1/caldav.php index e9b6e3759a..ac46b13025 100644 --- a/apps/dav/appinfo/v1/caldav.php +++ b/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(), diff --git a/apps/dav/lib/caldav/caldavbackend.php b/apps/dav/lib/caldav/caldavbackend.php index 79bbcefaab..5f82db10b3 100644 --- a/apps/dav/lib/caldav/caldavbackend.php +++ b/apps/dav/lib/caldav/caldavbackend.php @@ -195,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') @@ -222,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) { diff --git a/apps/dav/lib/caldav/calendar.php b/apps/dav/lib/caldav/calendar.php index 1e87f9b433..5072d96107 100644 --- a/apps/dav/lib/caldav/calendar.php +++ b/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(); - - /** @var CalDavBackend $calDavBackend */ - $calDavBackend = $this->caldavBackend; - return $calDavBackend->applyShareAcl($this->getResourceId(), $acl); + return $this->getACL(); } 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; + } + } diff --git a/apps/dav/lib/carddav/addressbook.php b/apps/dav/lib/carddav/addressbook.php index bb9d13b981..8b1b600ec3 100644 --- a/apps/dav/lib/carddav/addressbook.php +++ b/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(); - 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); + return $this->getACL(); } 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; + } } diff --git a/apps/dav/lib/carddav/card.php b/apps/dav/lib/carddav/card.php deleted file mode 100644 index d848f2e28e..0000000000 --- a/apps/dav/lib/carddav/card.php +++ /dev/null @@ -1,45 +0,0 @@ - - * - * @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 - * - */ - -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']; - } - -} diff --git a/apps/dav/lib/carddav/carddavbackend.php b/apps/dav/lib/carddav/carddavbackend.php index ccf0f4fbe0..650623225e 100644 --- a/apps/dav/lib/carddav/carddavbackend.php +++ b/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; @@ -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, ]; } } diff --git a/apps/dav/lib/connector/sabre/principal.php b/apps/dav/lib/connector/sabre/principal.php index 49fb4efdee..18f28a916f 100644 --- a/apps/dav/lib/connector/sabre/principal.php +++ b/apps/dav/lib/connector/sabre/principal.php @@ -132,6 +132,7 @@ 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 */ diff --git a/apps/dav/tests/unit/caldav/calendartest.php b/apps/dav/tests/unit/caldav/calendartest.php index e7f4d57067..9e0c3c6c7e 100644 --- a/apps/dav/tests/unit/caldav/calendartest.php +++ b/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], + ]; + } } diff --git a/apps/dav/tests/unit/carddav/addressbooktest.php b/apps/dav/tests/unit/carddav/addressbooktest.php index 854c121a95..c5cf7e5f7b 100644 --- a/apps/dav/tests/unit/carddav/addressbooktest.php +++ b/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], + ]; + }}