From 2da490bf256382af599a67cbf7633c2137c49ed2 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 2 Mar 2017 11:45:07 +0100 Subject: [PATCH 1/4] Unit test cleanup Signed-off-by: Joas Schilling --- .../tests/unit/CalDAV/CalDavBackendTest.php | 73 +++++++++---------- 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php b/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php index bd6c9f2788..60be690eb3 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() { @@ -98,9 +98,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 +108,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 +128,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 +165,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 +175,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 +200,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 +244,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 +268,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 +286,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 +315,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 +384,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 +414,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 +427,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 +449,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); } /** From d8b769bd856cb9b57243ce38fb994e96463f239e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 2 Mar 2017 12:27:59 +0100 Subject: [PATCH 2/4] Add a unit test for the broken case Signed-off-by: Joas Schilling --- .../unit/CalDAV/AbstractCalDavBackendTest.php | 3 ++- .../tests/unit/CalDAV/CalDavBackendTest.php | 20 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/apps/dav/tests/unit/CalDAV/AbstractCalDavBackendTest.php b/apps/dav/tests/unit/CalDAV/AbstractCalDavBackendTest.php index d15be72c77..ffdba9c5c8 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 60be690eb3..22ef232dac 100644 --- a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php +++ b/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, From 6dbdca0721902cf9224f8ec42ff5170810afd388 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 2 Mar 2017 11:24:56 +0100 Subject: [PATCH 3/4] Don't waste energy unless necessary Signed-off-by: Joas Schilling --- apps/dav/lib/CalDAV/CalDavBackend.php | 8 ++++--- apps/dav/lib/CardDAV/CardDavBackend.php | 29 ++++++++++++++----------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/apps/dav/lib/CalDAV/CalDavBackend.php b/apps/dav/lib/CalDAV/CalDavBackend.php index dfef311132..e8f1cf979a 100644 --- a/apps/dav/lib/CalDAV/CalDavBackend.php +++ b/apps/dav/lib/CalDAV/CalDavBackend.php @@ -278,6 +278,10 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription ->execute(); while($row = $result->fetch()) { + if (isset($calendars[$row['id']])) { + continue; + } + list(, $name) = URLUtil::splitPath($row['principaluri']); $uri = $row['uri'] . '_shared_by_' . $name; $row['displayname'] = $row['displayname'] . ' (' . $this->getUserDisplayName($name) . ')'; @@ -301,9 +305,7 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription $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 b71c6380a9..ac4b58f538 100644 --- a/apps/dav/lib/CardDAV/CardDavBackend.php +++ b/apps/dav/lib/CardDAV/CardDavBackend.php @@ -173,22 +173,25 @@ class CardDavBackend implements BackendInterface, SyncSupport { ->execute(); while($row = $result->fetch()) { + if (isset($addressBooks[$row['id']])) { + 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'], + '{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}read-only' => (int)$row['access'] === Backend::ACCESS_READ, + ]; } $result->closeCursor(); From 2eb27c636d7fc73e9d25f8531b49f96908506a19 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 2 Mar 2017 11:34:27 +0100 Subject: [PATCH 4/4] Make sure shares use read-write when available Signed-off-by: Joas Schilling --- apps/dav/lib/CalDAV/CalDavBackend.php | 14 ++++++++++++-- apps/dav/lib/CardDAV/CardDavBackend.php | 14 ++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/apps/dav/lib/CalDAV/CalDavBackend.php b/apps/dav/lib/CalDAV/CalDavBackend.php index e8f1cf979a..dbe8643823 100644 --- a/apps/dav/lib/CalDAV/CalDavBackend.php +++ b/apps/dav/lib/CalDAV/CalDavBackend.php @@ -277,9 +277,19 @@ 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']])) { - continue; + 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']); @@ -298,7 +308,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' => $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) { diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php index ac4b58f538..6f9a73298e 100644 --- a/apps/dav/lib/CardDAV/CardDavBackend.php +++ b/apps/dav/lib/CardDAV/CardDavBackend.php @@ -172,9 +172,19 @@ 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']])) { - continue; + 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']); @@ -190,7 +200,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' => (int)$row['access'] === Backend::ACCESS_READ, + $readOnlyPropertyName => $readOnly, ]; } $result->closeCursor();