From 297985cf0164c2d5879ff80e4b0f10e04f3d4d27 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 12:58:26 +0100 Subject: [PATCH] Ensured trying to fetch a short URL for any operation through the API results in 404 if it does not match with porovided domain --- .../src/Service/ShortUrl/ShortUrlResolver.php | 8 +++--- .../Service/ShortUrl/ShortUrlResolverTest.php | 8 +++--- .../Action/DeleteShortUrlActionTest.php | 25 ++++++------------- 3 files changed, 17 insertions(+), 24 deletions(-) diff --git a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php index 4f44f671..414a3446 100644 --- a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php +++ b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php @@ -26,7 +26,7 @@ class ShortUrlResolver implements ShortUrlResolverInterface { /** @var ShortUrlRepository $shortUrlRepo */ $shortUrlRepo = $this->em->getRepository(ShortUrl::class); - $shortUrl = $shortUrlRepo->findOneWithDomainFallback($identifier->shortCode(), $identifier->domain()); + $shortUrl = $shortUrlRepo->findOne($identifier->shortCode(), $identifier->domain()); if ($shortUrl === null) { throw ShortUrlNotFoundException::fromNotFound($identifier); } @@ -39,8 +39,10 @@ class ShortUrlResolver implements ShortUrlResolverInterface */ public function resolveEnabledShortUrl(ShortUrlIdentifier $identifier): ShortUrl { - $shortUrl = $this->resolveShortUrl($identifier); - if (! $shortUrl->isEnabled()) { + /** @var ShortUrlRepository $shortUrlRepo */ + $shortUrlRepo = $this->em->getRepository(ShortUrl::class); + $shortUrl = $shortUrlRepo->findOneWithDomainFallback($identifier->shortCode(), $identifier->domain()); + if ($shortUrl === null || ! $shortUrl->isEnabled()) { throw ShortUrlNotFoundException::fromNotFound($identifier); } diff --git a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php index fb3b4e68..5b3e3e19 100644 --- a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php +++ b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php @@ -39,13 +39,13 @@ class ShortUrlResolverTest extends TestCase $shortCode = $shortUrl->getShortCode(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn($shortUrl); + $findOne = $repo->findOne($shortCode, null)->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $result = $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode)); $this->assertSame($shortUrl, $result); - $findOneByShortCode->shouldHaveBeenCalledOnce(); + $findOne->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); } @@ -55,11 +55,11 @@ class ShortUrlResolverTest extends TestCase $shortCode = 'abc123'; $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn(null); + $findOne = $repo->findOne($shortCode, null)->willReturn(null); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->expectException(ShortUrlNotFoundException::class); - $findOneByShortCode->shouldBeCalledOnce(); + $findOne->shouldBeCalledOnce(); $getRepo->shouldBeCalledOnce(); $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode)); diff --git a/module/Rest/test-api/Action/DeleteShortUrlActionTest.php b/module/Rest/test-api/Action/DeleteShortUrlActionTest.php index d9e14113..539c6296 100644 --- a/module/Rest/test-api/Action/DeleteShortUrlActionTest.php +++ b/module/Rest/test-api/Action/DeleteShortUrlActionTest.php @@ -46,25 +46,16 @@ class DeleteShortUrlActionTest extends ApiTestCase /** @test */ public function properShortUrlIsDeletedWhenDomainIsProvided(): void { - $fetchWithDomainBefore = $this->getJsonResponsePayload( - $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789?domain=example.com'), - ); - $fetchWithoutDomainBefore = $this->getJsonResponsePayload( - $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789'), - ); + $fetchWithDomainBefore = $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789?domain=example.com'); + $fetchWithoutDomainBefore = $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789'); $deleteResp = $this->callApiWithKey(self::METHOD_DELETE, '/short-urls/ghi789?domain=example.com'); - $fetchWithDomainAfter = $this->getJsonResponsePayload( - $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789?domain=example.com'), - ); - $fetchWithoutDomainAfter = $this->getJsonResponsePayload( - $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789'), - ); + $fetchWithDomainAfter = $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789?domain=example.com'); + $fetchWithoutDomainAfter = $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789'); - $this->assertEquals('example.com', $fetchWithDomainBefore['domain']); - $this->assertEquals(null, $fetchWithoutDomainBefore['domain']); + $this->assertEquals(self::STATUS_OK, $fetchWithDomainBefore->getStatusCode()); + $this->assertEquals(self::STATUS_OK, $fetchWithoutDomainBefore->getStatusCode()); $this->assertEquals(self::STATUS_NO_CONTENT, $deleteResp->getStatusCode()); - // Falls back to the one without domain, since the other one has been deleted - $this->assertEquals(null, $fetchWithDomainAfter['domain']); - $this->assertEquals(null, $fetchWithoutDomainAfter['domain']); + $this->assertEquals(self::STATUS_NOT_FOUND, $fetchWithDomainAfter->getStatusCode()); + $this->assertEquals(self::STATUS_OK, $fetchWithoutDomainAfter->getStatusCode()); } }