From b6d8c5ff2fa8602ea3ce622097350c9b28fee628 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6=20=28skjnldsv=29?= Date: Sat, 3 Nov 2018 19:03:32 +0100 Subject: [PATCH 1/2] Add check for vCard uid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- apps/dav/lib/CardDAV/CardDavBackend.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php index a8907f631c..eb94885d17 100644 --- a/apps/dav/lib/CardDAV/CardDavBackend.php +++ b/apps/dav/lib/CardDAV/CardDavBackend.php @@ -611,6 +611,19 @@ class CardDavBackend implements BackendInterface, SyncSupport { $etag = md5($cardData); $uid = $this->getUID($cardData); + $q = $this->db->getQueryBuilder(); + $q->select('uid') + ->from('cards') + ->where($q->expr()->eq('addressbookid', $q->createNamedParameter($addressBookId))) + ->andWhere($q->expr()->eq('uid', $q->createNamedParameter($uid))) + ->setMaxResults(1); + $result = $q->execute(); + $count = (bool) $result->fetchColumn(); + $result->closeCursor(); + if ($count) { + throw new \Sabre\DAV\Exception\BadRequest('VCard object with uid already exists in this addressbook collection.'); + } + $query = $this->db->getQueryBuilder(); $query->insert('cards') ->values([ From 68968a870b5edcf9c8964de518f44a919796c060 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6=20=28skjnldsv=29?= Date: Sat, 3 Nov 2018 19:33:59 +0100 Subject: [PATCH 2/2] Phpunit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- .../tests/unit/CardDAV/CardDavBackendTest.php | 165 +++++++++++++----- 1 file changed, 126 insertions(+), 39 deletions(-) diff --git a/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php b/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php index 2f5287df82..00a485a7d9 100644 --- a/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php +++ b/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php @@ -43,6 +43,7 @@ use OCP\IL10N; use OCP\IUserManager; use OCP\IUserSession; use OCP\Share\IManager as ShareManager; +use Sabre\DAV\Exception\BadRequest; use Sabre\DAV\PropPatch; use Sabre\VObject\Component\VCard; use Sabre\VObject\Property\Text; @@ -87,7 +88,7 @@ class CardDavBackendTest extends TestCase { const UNIT_TEST_USER1 = 'principals/users/carddav-unit-test1'; const UNIT_TEST_GROUP = 'principals/groups/carddav-unit-test-group'; - private $vcardTest = 'BEGIN:VCARD'.PHP_EOL. + private $vcardTest0 = 'BEGIN:VCARD'.PHP_EOL. 'VERSION:3.0'.PHP_EOL. 'PRODID:-//Sabre//Sabre VObject 4.1.2//EN'.PHP_EOL. 'UID:Test'.PHP_EOL. @@ -95,6 +96,29 @@ class CardDavBackendTest extends TestCase { 'N:Test;;;;'.PHP_EOL. 'END:VCARD'; + private $vcardTest1 = 'BEGIN:VCARD'.PHP_EOL. + 'VERSION:3.0'.PHP_EOL. + 'PRODID:-//Sabre//Sabre VObject 4.1.2//EN'.PHP_EOL. + 'UID:Test2'.PHP_EOL. + 'FN:Test2'.PHP_EOL. + 'N:Test2;;;;'.PHP_EOL. + 'END:VCARD'; + + private $vcardTest2 = 'BEGIN:VCARD'.PHP_EOL. + 'VERSION:3.0'.PHP_EOL. + 'PRODID:-//Sabre//Sabre VObject 4.1.2//EN'.PHP_EOL. + 'UID:Test3'.PHP_EOL. + 'FN:Test3'.PHP_EOL. + 'N:Test3;;;;'.PHP_EOL. + 'END:VCARD'; + + private $vcardTestNoUID = 'BEGIN:VCARD'.PHP_EOL. + 'VERSION:3.0'.PHP_EOL. + 'PRODID:-//Sabre//Sabre VObject 4.1.2//EN'.PHP_EOL. + 'FN:TestNoUID'.PHP_EOL. + 'N:TestNoUID;;;;'.PHP_EOL. + 'END:VCARD'; + public function setUp() { parent::setUp(); @@ -224,8 +248,8 @@ class CardDavBackendTest extends TestCase { $uri = $this->getUniqueID('card'); // updateProperties is expected twice, once for createCard and once for updateCard - $backend->expects($this->at(0))->method('updateProperties')->with($bookId, $uri, $this->vcardTest); - $backend->expects($this->at(1))->method('updateProperties')->with($bookId, $uri, $this->vcardTest); + $backend->expects($this->at(0))->method('updateProperties')->with($bookId, $uri, $this->vcardTest0); + $backend->expects($this->at(1))->method('updateProperties')->with($bookId, $uri, $this->vcardTest0); // Expect event $this->dispatcher->expects($this->at(0)) @@ -233,16 +257,16 @@ class CardDavBackendTest extends TestCase { ->with('\OCA\DAV\CardDAV\CardDavBackend::createCard', $this->callback(function(GenericEvent $e) use ($bookId, $uri) { return $e->getArgument('addressBookId') === $bookId && $e->getArgument('cardUri') === $uri && - $e->getArgument('cardData') === $this->vcardTest; + $e->getArgument('cardData') === $this->vcardTest0; })); // create a card - $backend->createCard($bookId, $uri, $this->vcardTest); + $backend->createCard($bookId, $uri, $this->vcardTest0); // get all the cards $cards = $backend->getCards($bookId); $this->assertEquals(1, count($cards)); - $this->assertEquals($this->vcardTest, $cards[0]['carddata']); + $this->assertEquals($this->vcardTest0, $cards[0]['carddata']); // get the cards $card = $backend->getCard($bookId, $uri); @@ -252,7 +276,7 @@ class CardDavBackendTest extends TestCase { $this->assertArrayHasKey('lastmodified', $card); $this->assertArrayHasKey('etag', $card); $this->assertArrayHasKey('size', $card); - $this->assertEquals($this->vcardTest, $card['carddata']); + $this->assertEquals($this->vcardTest0, $card['carddata']); // Expect event $this->dispatcher->expects($this->at(0)) @@ -260,13 +284,13 @@ class CardDavBackendTest extends TestCase { ->with('\OCA\DAV\CardDAV\CardDavBackend::updateCard', $this->callback(function(GenericEvent $e) use ($bookId, $uri) { return $e->getArgument('addressBookId') === $bookId && $e->getArgument('cardUri') === $uri && - $e->getArgument('cardData') === $this->vcardTest; + $e->getArgument('cardData') === $this->vcardTest0; })); // update the card - $backend->updateCard($bookId, $uri, $this->vcardTest); + $backend->updateCard($bookId, $uri, $this->vcardTest0); $card = $backend->getCard($bookId, $uri); - $this->assertEquals($this->vcardTest, $card['carddata']); + $this->assertEquals($this->vcardTest0, $card['carddata']); // Expect event $this->dispatcher->expects($this->at(0)) @@ -284,7 +308,78 @@ class CardDavBackendTest extends TestCase { } public function testMultiCard() { + $this->backend = $this->getMockBuilder(CardDavBackend::class) + ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->groupManager, $this->dispatcher]) + ->setMethods(['updateProperties'])->getMock(); + // create a new address book + $this->backend->createAddressBook(self::UNIT_TEST_USER, 'Example', []); + $books = $this->backend->getAddressBooksForUser(self::UNIT_TEST_USER); + $this->assertEquals(1, count($books)); + $bookId = $books[0]['id']; + + // create a card + $uri0 = self::getUniqueID('card'); + $this->backend->createCard($bookId, $uri0, $this->vcardTest0); + $uri1 = self::getUniqueID('card'); + $this->backend->createCard($bookId, $uri1, $this->vcardTest1); + $uri2 = self::getUniqueID('card'); + $this->backend->createCard($bookId, $uri2, $this->vcardTest2); + + // get all the cards + $cards = $this->backend->getCards($bookId); + $this->assertEquals(3, count($cards)); + usort($cards, function ($a, $b) { return $a['id'] < $b['id'] ? -1 : 1; }); + + $this->assertEquals($this->vcardTest0, $cards[0]['carddata']); + $this->assertEquals($this->vcardTest1, $cards[1]['carddata']); + $this->assertEquals($this->vcardTest2, $cards[2]['carddata']); + + // get the cards 1 & 2 (not 0) + $cards = $this->backend->getMultipleCards($bookId, [$uri1, $uri2]); + $this->assertEquals(2, count($cards)); + usort($cards, function ($a, $b) { return $a['id'] < $b['id'] ? -1 : 1; }); + foreach($cards as $index => $card) { + $this->assertArrayHasKey('id', $card); + $this->assertArrayHasKey('uri', $card); + $this->assertArrayHasKey('lastmodified', $card); + $this->assertArrayHasKey('etag', $card); + $this->assertArrayHasKey('size', $card); + $this->assertEquals($this->{ 'vcardTest'.($index+1) }, $card['carddata']); + } + + // delete the card + $this->backend->deleteCard($bookId, $uri0); + $this->backend->deleteCard($bookId, $uri1); + $this->backend->deleteCard($bookId, $uri2); + $cards = $this->backend->getCards($bookId); + $this->assertEquals(0, count($cards)); + } + + public function testMultipleUIDOnDifferentAddressbooks() { + + $this->backend = $this->getMockBuilder(CardDavBackend::class) + ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->groupManager, $this->dispatcher]) + ->setMethods(['updateProperties'])->getMock(); + + // create 2 new address books + $this->backend->createAddressBook(self::UNIT_TEST_USER, 'Example', []); + $this->backend->createAddressBook(self::UNIT_TEST_USER, 'Example2', []); + $books = $this->backend->getAddressBooksForUser(self::UNIT_TEST_USER); + $this->assertEquals(2, count($books)); + $bookId0 = $books[0]['id']; + $bookId1 = $books[1]['id']; + + // create a card + $uri0 = $this->getUniqueID('card'); + $this->backend->createCard($bookId0, $uri0, $this->vcardTest0); + + // create another card with same uid but in second address book + $uri1 = $this->getUniqueID('card'); + $this->backend->createCard($bookId1, $uri1, $this->vcardTest0); + } + + public function testMultipleUIDDenied() { $this->backend = $this->getMockBuilder(CardDavBackend::class) ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->groupManager, $this->dispatcher]) ->setMethods(['updateProperties'])->getMock(); @@ -297,37 +392,29 @@ class CardDavBackendTest extends TestCase { // create a card $uri0 = $this->getUniqueID('card'); - $this->backend->createCard($bookId, $uri0, $this->vcardTest); + $this->backend->createCard($bookId, $uri0, $this->vcardTest0); + + // create another card with same uid $uri1 = $this->getUniqueID('card'); - $this->backend->createCard($bookId, $uri1, $this->vcardTest); - $uri2 = $this->getUniqueID('card'); - $this->backend->createCard($bookId, $uri2, $this->vcardTest); + $this->expectException(BadRequest::class); + $test = $this->backend->createCard($bookId, $uri1, $this->vcardTest0); + } - // get all the cards - $cards = $this->backend->getCards($bookId); - $this->assertEquals(3, count($cards)); - $this->assertEquals($this->vcardTest, $cards[0]['carddata']); - $this->assertEquals($this->vcardTest, $cards[1]['carddata']); - $this->assertEquals($this->vcardTest, $cards[2]['carddata']); + public function testNoValidUID() { + $this->backend = $this->getMockBuilder(CardDavBackend::class) + ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->groupManager, $this->dispatcher]) + ->setMethods(['updateProperties'])->getMock(); - // get the cards - $cards = $this->backend->getMultipleCards($bookId, [$uri1, $uri2]); - $this->assertEquals(2, count($cards)); - foreach($cards as $card) { - $this->assertArrayHasKey('id', $card); - $this->assertArrayHasKey('uri', $card); - $this->assertArrayHasKey('lastmodified', $card); - $this->assertArrayHasKey('etag', $card); - $this->assertArrayHasKey('size', $card); - $this->assertEquals($this->vcardTest, $card['carddata']); - } + // create a new address book + $this->backend->createAddressBook(self::UNIT_TEST_USER, 'Example', []); + $books = $this->backend->getAddressBooksForUser(self::UNIT_TEST_USER); + $this->assertEquals(1, count($books)); + $bookId = $books[0]['id']; - // delete the card - $this->backend->deleteCard($bookId, $uri0); - $this->backend->deleteCard($bookId, $uri1); - $this->backend->deleteCard($bookId, $uri2); - $cards = $this->backend->getCards($bookId); - $this->assertEquals(0, count($cards)); + // create a card without uid + $uri1 = $this->getUniqueID('card'); + $this->expectException(BadRequest::class); + $test = $this->backend->createCard($bookId, $uri1, $this->vcardTestNoUID); } public function testDeleteWithoutCard() { @@ -364,7 +451,7 @@ class CardDavBackendTest extends TestCase { ->method('purgeProperties'); // create a card - $this->backend->createCard($bookId, $uri, $this->vcardTest); + $this->backend->createCard($bookId, $uri, $this->vcardTest0); // delete the card $this->assertTrue($this->backend->deleteCard($bookId, $uri)); @@ -387,7 +474,7 @@ class CardDavBackendTest extends TestCase { // add a change $uri0 = $this->getUniqueID('card'); - $this->backend->createCard($bookId, $uri0, $this->vcardTest); + $this->backend->createCard($bookId, $uri0, $this->vcardTest0); // look for changes $changes = $this->backend->getChangesForAddressBook($bookId, $syncToken, 1);