From bafb9744c87255ebe28945879da85e587175d241 Mon Sep 17 00:00:00 2001 From: Kevin Decherf Date: Sat, 29 Dec 2018 19:22:05 +0100 Subject: fixtures: refactor EntryData, TagData, add a new tag Signed-off-by: Kevin Decherf --- .../CoreBundle/DataFixtures/ORM/LoadEntryData.php | 197 +++++++++++---------- .../CoreBundle/DataFixtures/ORM/LoadTagData.php | 43 ++--- 2 files changed, 122 insertions(+), 118 deletions(-) diff --git a/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadEntryData.php b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadEntryData.php index 0e1510a2..8e7a1d2a 100644 --- a/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadEntryData.php +++ b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadEntryData.php @@ -14,97 +14,112 @@ class LoadEntryData extends AbstractFixture implements OrderedFixtureInterface */ public function load(ObjectManager $manager) { - $entry1 = new Entry($this->getReference('admin-user')); - $entry1->setUrl('http://0.0.0.0/entry1'); - $entry1->setReadingTime(11); - $entry1->setDomainName('domain.io'); - $entry1->setMimetype('text/html'); - $entry1->setTitle('test title entry1'); - $entry1->setContent('This is my content /o/'); - $entry1->setLanguage('en'); - - $entry1->addTag($this->getReference('foo-tag')); - $entry1->addTag($this->getReference('baz-tag')); - - $manager->persist($entry1); - - $this->addReference('entry1', $entry1); - - $entry2 = new Entry($this->getReference('admin-user')); - $entry2->setUrl('http://0.0.0.0/entry2'); - $entry2->setReadingTime(1); - $entry2->setDomainName('domain.io'); - $entry2->setMimetype('text/html'); - $entry2->setTitle('test title entry2'); - $entry2->setContent('This is my content /o/'); - $entry2->setOriginUrl('ftp://oneftp.tld'); - $entry2->setLanguage('fr'); - - $manager->persist($entry2); - - $this->addReference('entry2', $entry2); - - $entry3 = new Entry($this->getReference('bob-user')); - $entry3->setUrl('http://0.0.0.0/entry3'); - $entry3->setReadingTime(1); - $entry3->setDomainName('domain.io'); - $entry3->setMimetype('text/html'); - $entry3->setTitle('test title entry3'); - $entry3->setContent('This is my content /o/'); - $entry3->setLanguage('en'); - - $entry3->addTag($this->getReference('foo-tag')); - $entry3->addTag($this->getReference('bar-tag')); - - $manager->persist($entry3); - - $this->addReference('entry3', $entry3); - - $entry4 = new Entry($this->getReference('admin-user')); - $entry4->setUrl('http://0.0.0.0/entry4'); - $entry4->setReadingTime(12); - $entry4->setDomainName('domain.io'); - $entry4->setMimetype('text/html'); - $entry4->setTitle('test title entry4'); - $entry4->setContent('This is my content /o/'); - $entry4->setLanguage('en'); - - $entry4->addTag($this->getReference('foo-tag')); - $entry4->addTag($this->getReference('bar-tag')); - - $manager->persist($entry4); - - $this->addReference('entry4', $entry4); - - $entry5 = new Entry($this->getReference('admin-user')); - $entry5->setUrl('http://0.0.0.0/entry5'); - $entry5->setReadingTime(12); - $entry5->setDomainName('domain.io'); - $entry5->setMimetype('text/html'); - $entry5->setTitle('test title entry5'); - $entry5->setContent('This is my content /o/'); - $entry5->setStarred(true); - $entry5->setLanguage('fr'); - $entry5->setPreviewPicture('http://0.0.0.0/image.jpg'); - - $manager->persist($entry5); - - $this->addReference('entry5', $entry5); - - $entry6 = new Entry($this->getReference('admin-user')); - $entry6->setUrl('http://0.0.0.0/entry6'); - $entry6->setReadingTime(12); - $entry6->setDomainName('domain.io'); - $entry6->setMimetype('text/html'); - $entry6->setTitle('test title entry6'); - $entry6->setContent('This is my content /o/'); - $entry6->setArchived(true); - $entry6->setLanguage('de'); - $entry6->addTag($this->getReference('bar-tag')); - - $manager->persist($entry6); - - $this->addReference('entry6', $entry6); + $entries = [ + 'entry1' => [ + 'user' => 'admin-user', + 'url' => 'http://0.0.0.0/entry1', + 'reading_time' => 11, + 'domain' => 'domain.io', + 'mime' => 'text/html', + 'title' => 'test title entry1', + 'content' => 'This is my content /o/', + 'language' => 'en', + 'tags' => ['foo-tag', 'baz-tag'], + ], + 'entry2' => [ + 'user' => 'admin-user', + 'url' => 'http://0.0.0.0/entry2', + 'reading_time' => 1, + 'domain' => 'domain.io', + 'mime' => 'text/html', + 'title' => 'test title entry2', + 'content' => 'This is my content /o/', + 'origin' => 'ftp://oneftp.tld', + 'language' => 'fr', + ], + 'entry3' => [ + 'user' => 'bob-user', + 'url' => 'http://0.0.0.0/entry3', + 'reading_time' => 1, + 'domain' => 'domain.io', + 'mime' => 'text/html', + 'title' => 'test title entry3', + 'content' => 'This is my content /o/', + 'language' => 'en', + 'tags' => ['foo-tag', 'bar-tag', 'bob-tag'], + ], + 'entry4' => [ + 'user' => 'admin-user', + 'url' => 'http://0.0.0.0/entry4', + 'reading_time' => 12, + 'domain' => 'domain.io', + 'mime' => 'text/html', + 'title' => 'test title entry4', + 'content' => 'This is my content /o/', + 'language' => 'en', + 'tags' => ['foo-tag', 'bar-tag'], + ], + 'entry5' => [ + 'user' => 'admin-user', + 'url' => 'http://0.0.0.0/entry5', + 'reading_time' => 12, + 'domain' => 'domain.io', + 'mime' => 'text/html', + 'title' => 'test title entry5', + 'content' => 'This is my content /o/', + 'language' => 'fr', + 'starred' => true, + 'preview' => 'http://0.0.0.0/image.jpg', + ], + 'entry6' => [ + 'user' => 'admin-user', + 'url' => 'http://0.0.0.0/entry6', + 'reading_time' => 12, + 'domain' => 'domain.io', + 'mime' => 'text/html', + 'title' => 'test title entry6', + 'content' => 'This is my content /o/', + 'language' => 'de', + 'archived' => true, + 'tags' => ['bar-tag'], + ], + ]; + + foreach ($entries as $reference => $item) { + $entry = new Entry($this->getReference($item['user'])); + $entry->setUrl($item['url']); + $entry->setReadingTime($item['reading_time']); + $entry->setDomainName($item['domain']); + $entry->setMimetype($item['mime']); + $entry->setTitle($item['title']); + $entry->setContent($item['content']); + $entry->setLanguage($item['language']); + + if (isset($item['tags'])) { + foreach ($item['tags'] as $tag) { + $entry->addTag($this->getReference($tag)); + } + } + + if (isset($item['origin'])) { + $entry->setOriginUrl($item['origin']); + } + + if (isset($item['starred'])) { + $entry->setStarred($item['starred']); + } + + if (isset($item['archived'])) { + $entry->setArchived($item['archived']); + } + + if (isset($item['preview'])) { + $entry->setPreviewPicture($item['preview']); + } + + $manager->persist($entry); + $this->addReference($reference, $entry); + } $manager->flush(); } diff --git a/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.php b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.php index 0ecfd18b..485445c1 100644 --- a/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.php +++ b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadTagData.php @@ -14,33 +14,22 @@ class LoadTagData extends AbstractFixture implements OrderedFixtureInterface */ public function load(ObjectManager $manager) { - $tag1 = new Tag(); - $tag1->setLabel('foo bar'); - - $manager->persist($tag1); - - $this->addReference('foo-bar-tag', $tag1); - - $tag2 = new Tag(); - $tag2->setLabel('bar'); - - $manager->persist($tag2); - - $this->addReference('bar-tag', $tag2); - - $tag3 = new Tag(); - $tag3->setLabel('baz'); - - $manager->persist($tag3); - - $this->addReference('baz-tag', $tag3); - - $tag4 = new Tag(); - $tag4->setLabel('foo'); - - $manager->persist($tag4); - - $this->addReference('foo-tag', $tag4); + $tags = [ + 'foo-bar-tag' => 'foo bar', //tag used for EntryControllerTest + 'bar-tag' => 'bar', + 'baz-tag' => 'baz', // tag used for ExportControllerTest + 'foo-tag' => 'foo', + 'bob-tag' => 'bob', // tag used for TagRestControllerTest + ]; + + foreach ($tags as $reference => $label) { + $tag = new Tag(); + $tag->setLabel($label); + + $manager->persist($tag); + + $this->addReference($reference, $tag); + } $manager->flush(); } -- cgit v1.2.3 From 6708bf238de46d7ce861e3c0eeb6a9b4623931ed Mon Sep 17 00:00:00 2001 From: Kevin Decherf Date: Sat, 29 Dec 2018 19:42:37 +0100 Subject: TagRepository: refactor query builder for queries by userId Signed-off-by: Kevin Decherf --- .../CoreBundle/Repository/TagRepository.php | 36 ++++++++++++---------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/Wallabag/CoreBundle/Repository/TagRepository.php b/src/Wallabag/CoreBundle/Repository/TagRepository.php index 3ae9d414..bd2d9f97 100644 --- a/src/Wallabag/CoreBundle/Repository/TagRepository.php +++ b/src/Wallabag/CoreBundle/Repository/TagRepository.php @@ -3,6 +3,7 @@ namespace Wallabag\CoreBundle\Repository; use Doctrine\ORM\EntityRepository; +use Doctrine\ORM\QueryBuilder; use Wallabag\CoreBundle\Entity\Tag; class TagRepository extends EntityRepository @@ -45,12 +46,8 @@ class TagRepository extends EntityRepository */ public function findAllTags($userId) { - $ids = $this->createQueryBuilder('t') + $ids = $this->getQueryBuilderByUser($userId) ->select('t.id') - ->leftJoin('t.entries', 'e') - ->where('e.user = :userId')->setParameter('userId', $userId) - ->groupBy('t.id') - ->orderBy('t.slug') ->getQuery() ->getArrayResult(); @@ -71,14 +68,9 @@ class TagRepository extends EntityRepository */ public function findAllFlatTagsWithNbEntries($userId) { - return $this->createQueryBuilder('t') + return $this->getQueryBuilderByUser($userId) ->select('t.id, t.label, t.slug, count(e.id) as nbEntries') ->distinct(true) - ->leftJoin('t.entries', 'e') - ->where('e.user = :userId') - ->groupBy('t.id') - ->orderBy('t.slug') - ->setParameter('userId', $userId) ->getQuery() ->getArrayResult(); } @@ -101,13 +93,9 @@ class TagRepository extends EntityRepository public function findForArchivedArticlesByUser($userId) { - $ids = $this->createQueryBuilder('t') + $ids = $this->getQueryBuilderByUser($userId) ->select('t.id') - ->leftJoin('t.entries', 'e') - ->where('e.user = :userId')->setParameter('userId', $userId) ->andWhere('e.isArchived = true') - ->groupBy('t.id') - ->orderBy('t.slug') ->getQuery() ->getArrayResult(); @@ -118,4 +106,20 @@ class TagRepository extends EntityRepository return $tags; } + + /** + * Retrieve a sorted list of tags used by a user. + * + * @param int $userId + * + * @return QueryBuilder + */ + private function getQueryBuilderByUser($userId) + { + return $this->createQueryBuilder('t') + ->leftJoin('t.entries', 'e') + ->where('e.user = :userId')->setParameter('userId', $userId) + ->groupBy('t.id') + ->orderBy('t.slug'); + } } -- cgit v1.2.3 From 0ee9848231d0a7a02fdc8e915d830ebaf6cc09c0 Mon Sep 17 00:00:00 2001 From: Kevin Decherf Date: Sat, 29 Dec 2018 19:43:07 +0100 Subject: TagRestController: add tests to ensure that other user's tags are unreachable Signed-off-by: Kevin Decherf --- .../ApiBundle/Controller/TagRestControllerTest.php | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/Wallabag/ApiBundle/Controller/TagRestControllerTest.php b/tests/Wallabag/ApiBundle/Controller/TagRestControllerTest.php index 430e548d..8f1e6f02 100644 --- a/tests/Wallabag/ApiBundle/Controller/TagRestControllerTest.php +++ b/tests/Wallabag/ApiBundle/Controller/TagRestControllerTest.php @@ -7,6 +7,8 @@ use Wallabag\CoreBundle\Entity\Tag; class TagRestControllerTest extends WallabagApiTestCase { + private $otherUserTagLabel = 'bob'; + public function testGetUserTags() { $this->client->request('GET', '/api/tags.json'); @@ -19,6 +21,12 @@ class TagRestControllerTest extends WallabagApiTestCase $this->assertArrayHasKey('id', $content[0]); $this->assertArrayHasKey('label', $content[0]); + $tagLabels = array_map(function ($i) { + return $i['label']; + }, $content); + + $this->assertNotContains($this->otherUserTagLabel, $tagLabels, 'There is a possible tag leak'); + return end($content); } @@ -53,6 +61,16 @@ class TagRestControllerTest extends WallabagApiTestCase $this->assertNull($tag, $tagLabel . ' was removed because it begun an orphan tag'); } + public function testDeleteOtherUserTag() + { + $em = $this->client->getContainer()->get('doctrine.orm.entity_manager'); + $tag = $em->getRepository('WallabagCoreBundle:Tag')->findOneByLabel($this->otherUserTagLabel); + + $this->client->request('DELETE', '/api/tags/' . $tag->getId() . '.json'); + + $this->assertSame(404, $this->client->getResponse()->getStatusCode()); + } + public function dataForDeletingTagByLabel() { return [ @@ -112,6 +130,13 @@ class TagRestControllerTest extends WallabagApiTestCase $this->assertSame(404, $this->client->getResponse()->getStatusCode()); } + public function testDeleteTagByLabelOtherUser() + { + $this->client->request('DELETE', '/api/tag/label.json', ['tag' => $this->otherUserTagLabel]); + + $this->assertSame(404, $this->client->getResponse()->getStatusCode()); + } + /** * @dataProvider dataForDeletingTagByLabel */ @@ -180,4 +205,11 @@ class TagRestControllerTest extends WallabagApiTestCase $this->assertSame(404, $this->client->getResponse()->getStatusCode()); } + + public function testDeleteTagsByLabelOtherUser() + { + $this->client->request('DELETE', '/api/tags/label.json', ['tags' => $this->otherUserTagLabel]); + + $this->assertSame(404, $this->client->getResponse()->getStatusCode()); + } } -- cgit v1.2.3 From 2a0e0a47d853937702d235bdb91df0ca0e3116b6 Mon Sep 17 00:00:00 2001 From: Kevin Decherf Date: Sat, 29 Dec 2018 20:42:41 +0100 Subject: TagRestController: rewrite delete actions to only retrieve tags related to the user Fixes #3815 Signed-off-by: Kevin Decherf --- .../ApiBundle/Controller/TagRestController.php | 22 +++++++++++----------- .../CoreBundle/Repository/TagRepository.php | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/src/Wallabag/ApiBundle/Controller/TagRestController.php b/src/Wallabag/ApiBundle/Controller/TagRestController.php index c6d6df6a..f3498f55 100644 --- a/src/Wallabag/ApiBundle/Controller/TagRestController.php +++ b/src/Wallabag/ApiBundle/Controller/TagRestController.php @@ -46,12 +46,14 @@ class TagRestController extends WallabagRestController $this->validateAuthentication(); $label = $request->get('tag', ''); - $tag = $this->getDoctrine()->getRepository('WallabagCoreBundle:Tag')->findOneByLabel($label); + $tags = $this->getDoctrine()->getRepository('WallabagCoreBundle:Tag')->findByLabelsAndUser([$label], $this->getUser()->getId()); - if (empty($tag)) { + if (empty($tags)) { throw $this->createNotFoundException('Tag not found'); } + $tag = $tags[0]; + $this->getDoctrine() ->getRepository('WallabagCoreBundle:Entry') ->removeTag($this->getUser()->getId(), $tag); @@ -80,15 +82,7 @@ class TagRestController extends WallabagRestController $tagsLabels = $request->get('tags', ''); - $tags = []; - - foreach (explode(',', $tagsLabels) as $tagLabel) { - $tagEntity = $this->getDoctrine()->getRepository('WallabagCoreBundle:Tag')->findOneByLabel($tagLabel); - - if (!empty($tagEntity)) { - $tags[] = $tagEntity; - } - } + $tags = $this->getDoctrine()->getRepository('WallabagCoreBundle:Tag')->findByLabelsAndUser(explode(',', $tagsLabels), $this->getUser()->getId()); if (empty($tags)) { throw $this->createNotFoundException('Tags not found'); @@ -120,6 +114,12 @@ class TagRestController extends WallabagRestController { $this->validateAuthentication(); + $tagFromDb = $this->getDoctrine()->getRepository('WallabagCoreBundle:Tag')->findByLabelsAndUser([$tag->getLabel()], $this->getUser()->getId()); + + if (empty($tagFromDb)) { + throw $this->createNotFoundException('Tag not found'); + } + $this->getDoctrine() ->getRepository('WallabagCoreBundle:Entry') ->removeTag($this->getUser()->getId(), $tag); diff --git a/src/Wallabag/CoreBundle/Repository/TagRepository.php b/src/Wallabag/CoreBundle/Repository/TagRepository.php index bd2d9f97..8464a6a5 100644 --- a/src/Wallabag/CoreBundle/Repository/TagRepository.php +++ b/src/Wallabag/CoreBundle/Repository/TagRepository.php @@ -75,6 +75,23 @@ class TagRepository extends EntityRepository ->getArrayResult(); } + public function findByLabelsAndUser($labels, $userId) + { + $qb = $this->getQueryBuilderByUser($userId) + ->select('t.id'); + + $ids = $qb->andWhere($qb->expr()->in('t.label', $labels)) + ->getQuery() + ->getArrayResult(); + + $tags = []; + foreach ($ids as $id) { + $tags[] = $this->find($id); + } + + return $tags; + } + /** * Used only in test case to get a tag for our entry. * -- cgit v1.2.3 From 6c40d7fc85b98e335adf765d1c6b4465647da62c Mon Sep 17 00:00:00 2001 From: Kevin Decherf Date: Sat, 29 Dec 2018 20:43:34 +0100 Subject: TagRestController: fix test for tag without entries As the deletion now requires that at least one entry for the user must be linked to the given tag, we fix the test testDeleteUserTag by linking it to an entry. Signed-off-by: Kevin Decherf --- .../ApiBundle/Controller/TagRestControllerTest.php | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/Wallabag/ApiBundle/Controller/TagRestControllerTest.php b/tests/Wallabag/ApiBundle/Controller/TagRestControllerTest.php index 8f1e6f02..9daa94cd 100644 --- a/tests/Wallabag/ApiBundle/Controller/TagRestControllerTest.php +++ b/tests/Wallabag/ApiBundle/Controller/TagRestControllerTest.php @@ -32,12 +32,22 @@ class TagRestControllerTest extends WallabagApiTestCase public function testDeleteUserTag() { + $em = $this->client->getContainer()->get('doctrine.orm.entity_manager'); + $entry = $this->client->getContainer() + ->get('doctrine.orm.entity_manager') + ->getRepository('WallabagCoreBundle:Entry') + ->findOneWithTags($this->user->getId()); + + $entry = $entry[0]; + $tagLabel = 'tagtest'; $tag = new Tag(); $tag->setLabel($tagLabel); - - $em = $this->client->getContainer()->get('doctrine.orm.entity_manager'); $em->persist($tag); + + $entry->addTag($tag); + + $em->persist($entry); $em->flush(); $em->clear(); -- cgit v1.2.3