From f90ea4bd9807753c8a9c050a258cf64a74856256 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Nov 2020 18:58:07 +0100 Subject: [PATCH 1/8] Updated dependencies --- composer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 0bbe7651..b2faf734 100644 --- a/composer.json +++ b/composer.json @@ -16,7 +16,7 @@ "ext-json": "*", "ext-pdo": "*", "akrabat/ip-address-middleware": "^1.0", - "cakephp/chronos": "^1.2", + "cakephp/chronos": "^2.0", "cocur/slugify": "^4.0", "doctrine/cache": "^1.9", "doctrine/dbal": "^2.10", @@ -49,7 +49,7 @@ "predis/predis": "^1.1", "pugx/shortid-php": "^0.5", "ramsey/uuid": "^3.9", - "shlinkio/shlink-common": "^3.2.0", + "shlinkio/shlink-common": "^3.3.0", "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^1.4", "shlinkio/shlink-importer": "^2.0.1", From 00255b04eb2b1601e14867dc6ecf028d87196656 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Nov 2020 19:43:05 +0100 Subject: [PATCH 2/8] Added migration to create new author_api_key_id in short_urls --- data/migrations/Version20201102113208.php | 84 +++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 data/migrations/Version20201102113208.php diff --git a/data/migrations/Version20201102113208.php b/data/migrations/Version20201102113208.php new file mode 100644 index 00000000..14ce6504 --- /dev/null +++ b/data/migrations/Version20201102113208.php @@ -0,0 +1,84 @@ +getTable('short_urls'); + $this->skipIf($shortUrls->hasColumn(self::API_KEY_COLUMN)); + + $shortUrls->addColumn(self::API_KEY_COLUMN, Types::BIGINT, [ + 'unsigned' => true, + 'notnull' => false, + ]); + + $shortUrls->addForeignKeyConstraint('api_keys', [self::API_KEY_COLUMN], ['id'], [ + 'onDelete' => 'SET NULL', + 'onUpdate' => 'RESTRICT', + ], 'FK_' . self::API_KEY_COLUMN); + } + + public function postUp(Schema $schema): void + { + // If there's only one API key and it's active, link all existing URLs with it + $qb = $this->connection->createQueryBuilder(); + $qb->select('id') + ->from('api_keys') + ->where($qb->expr()->eq('enabled', ':enabled')) + ->andWhere($qb->expr()->or( + $qb->expr()->isNull('expiration_date'), + $qb->expr()->gt('expiration_date', ':expiration'), + )) + ->setParameters([ + 'enabled' => true, + 'expiration' => Chronos::now()->toDateTimeString(), + ]); + + $id = $this->resolveOneApiKeyId($qb->execute()); + if ($id === null) { + return; + } + + $qb = $this->connection->createQueryBuilder(); + $qb->update('short_urls') + ->set(self::API_KEY_COLUMN, ':apiKeyId') + ->setParameter('apiKeyId', $id) + ->execute(); + } + + private function resolveOneApiKeyId(Result $result): ?string + { + $results = []; + while ($row = $result->fetchAssociative()) { + // As soon as we have to iterate more than once, then we cannot resolve a single API key + if (! empty($results)) { + return null; + } + + $results[] = $row['id'] ?? null; + } + + return $results[0] ?? null; + } + + public function down(Schema $schema): void + { + $shortUrls = $schema->getTable('short_urls'); + $this->skipIf(! $shortUrls->hasColumn(self::API_KEY_COLUMN)); + + $shortUrls->removeForeignKey('FK_' . self::API_KEY_COLUMN); + $shortUrls->dropColumn(self::API_KEY_COLUMN); + } +} From 97f89bcede81fac7915c2f406c24f40b88f301b5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 6 Nov 2020 20:05:57 +0100 Subject: [PATCH 3/8] Simplified transactional URL shortening --- config/autoload/redirects.global.php | 2 +- .../ShortUrl/GenerateShortUrlCommand.php | 2 +- .../ShortUrl/GenerateShortUrlCommandTest.php | 10 +++--- .../Shlinkio.Shlink.Core.Entity.ShortUrl.php | 5 +++ module/Core/src/Entity/ShortUrl.php | 2 ++ module/Core/src/Service/UrlShortener.php | 23 ++++-------- .../src/Service/UrlShortenerInterface.php | 2 +- module/Core/test/Service/UrlShortenerTest.php | 36 +++++-------------- .../ShortUrl/AbstractCreateShortUrlAction.php | 2 +- .../ShortUrl/CreateShortUrlActionTest.php | 4 +-- .../SingleStepCreateShortUrlActionTest.php | 2 +- 11 files changed, 35 insertions(+), 55 deletions(-) diff --git a/config/autoload/redirects.global.php b/config/autoload/redirects.global.php index 90137aa8..173c435c 100644 --- a/config/autoload/redirects.global.php +++ b/config/autoload/redirects.global.php @@ -5,7 +5,7 @@ declare(strict_types=1); return [ 'not_found_redirects' => [ - 'invalid_short_url' => null, // Formerly url_shortener.not_found_short_url.redirect_to + 'invalid_short_url' => null, 'regular_404' => null, 'base_url' => null, ], diff --git a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php index 36293377..12bbb3fb 100644 --- a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php @@ -142,7 +142,7 @@ class GenerateShortUrlCommand extends Command $doValidateUrl = $this->doValidateUrl($input); try { - $shortUrl = $this->urlShortener->urlToShortCode($longUrl, $tags, ShortUrlMeta::fromRawData([ + $shortUrl = $this->urlShortener->shorten($longUrl, $tags, ShortUrlMeta::fromRawData([ ShortUrlMetaInputFilter::VALID_SINCE => $input->getOption('validSince'), ShortUrlMetaInputFilter::VALID_UNTIL => $input->getOption('validUntil'), ShortUrlMetaInputFilter::CUSTOM_SLUG => $customSlug, diff --git a/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php index b11b455b..82f38713 100644 --- a/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GenerateShortUrlCommandTest.php @@ -44,7 +44,7 @@ class GenerateShortUrlCommandTest extends TestCase public function properShortCodeIsCreatedIfLongUrlIsCorrect(): void { $shortUrl = new ShortUrl(''); - $urlToShortCode = $this->urlShortener->urlToShortCode(Argument::cetera())->willReturn($shortUrl); + $urlToShortCode = $this->urlShortener->shorten(Argument::cetera())->willReturn($shortUrl); $this->commandTester->execute([ 'longUrl' => 'http://domain.com/foo/bar', @@ -61,7 +61,7 @@ class GenerateShortUrlCommandTest extends TestCase public function exceptionWhileParsingLongUrlOutputsError(): void { $url = 'http://domain.com/invalid'; - $this->urlShortener->urlToShortCode(Argument::cetera())->willThrow(InvalidUrlException::fromUrl($url)) + $this->urlShortener->shorten(Argument::cetera())->willThrow(InvalidUrlException::fromUrl($url)) ->shouldBeCalledOnce(); $this->commandTester->execute(['longUrl' => $url]); @@ -74,7 +74,7 @@ class GenerateShortUrlCommandTest extends TestCase /** @test */ public function providingNonUniqueSlugOutputsError(): void { - $urlToShortCode = $this->urlShortener->urlToShortCode(Argument::cetera())->willThrow( + $urlToShortCode = $this->urlShortener->shorten(Argument::cetera())->willThrow( NonUniqueSlugException::fromSlug('my-slug'), ); @@ -90,7 +90,7 @@ class GenerateShortUrlCommandTest extends TestCase public function properlyProcessesProvidedTags(): void { $shortUrl = new ShortUrl(''); - $urlToShortCode = $this->urlShortener->urlToShortCode( + $urlToShortCode = $this->urlShortener->shorten( Argument::type('string'), Argument::that(function (array $tags) { Assert::assertEquals(['foo', 'bar', 'baz', 'boo', 'zar'], $tags); @@ -117,7 +117,7 @@ class GenerateShortUrlCommandTest extends TestCase public function urlValidationHasExpectedValueBasedOnProvidedTags(array $options, ?bool $expectedValidateUrl): void { $shortUrl = new ShortUrl(''); - $urlToShortCode = $this->urlShortener->urlToShortCode( + $urlToShortCode = $this->urlShortener->shorten( Argument::type('string'), Argument::type('array'), Argument::that(function (ShortUrlMeta $meta) use ($expectedValidateUrl) { diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php index 4f9b3747..da4506af 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php @@ -8,6 +8,7 @@ use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; use Doctrine\ORM\Mapping\ClassMetadata; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; +use Shlinkio\Shlink\Rest\Entity\ApiKey; return static function (ClassMetadata $metadata, array $emConfig): void { $builder = new ClassMetadataBuilder($metadata); @@ -78,5 +79,9 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->cascadePersist() ->build(); + $builder->createManyToOne('authorApiKey', ApiKey::class) + ->addJoinColumn('author_api_key_id', 'id', true, false, 'SET NULL') + ->build(); + $builder->addUniqueConstraint(['short_code', 'domain_id'], 'unique_short_code_plus_domain'); }; diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index 2d5cb6de..3d7ea0dd 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -16,6 +16,7 @@ use Shlinkio\Shlink\Core\Model\ShortUrlEdit; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; +use Shlinkio\Shlink\Rest\Entity\ApiKey; use function count; use function Shlinkio\Shlink\Core\generateRandomShortCode; @@ -37,6 +38,7 @@ class ShortUrl extends AbstractEntity private int $shortCodeLength; private ?string $importSource = null; private ?string $importOriginalShortCode = null; + private ?ApiKey $authorApiKey = null; public function __construct( string $longUrl, diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index d399b534..fb745114 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -43,7 +43,7 @@ class UrlShortener implements UrlShortenerInterface * @throws InvalidUrlException * @throws Throwable */ - public function urlToShortCode(string $url, array $tags, ShortUrlMeta $meta): ShortUrl + public function shorten(string $url, array $tags, ShortUrlMeta $meta): ShortUrl { // First, check if a short URL exists for all provided params $existingShortUrl = $this->findExistingShortUrlIfExists($url, $tags, $meta); @@ -52,25 +52,16 @@ class UrlShortener implements UrlShortenerInterface } $this->urlValidator->validateUrl($url, $meta->doValidateUrl()); - $this->em->beginTransaction(); - $shortUrl = new ShortUrl($url, $meta, $this->domainResolver); - $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); - try { + return $this->em->transactional(function () use ($url, $tags, $meta) { + $shortUrl = new ShortUrl($url, $meta, $this->domainResolver); + $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); + $this->verifyShortCodeUniqueness($meta, $shortUrl); $this->em->persist($shortUrl); - $this->em->flush(); - $this->em->commit(); - } catch (Throwable $e) { - if ($this->em->getConnection()->isTransactionActive()) { - $this->em->rollback(); - $this->em->close(); - } - throw $e; - } - - return $shortUrl; + return $shortUrl; + }); } private function findExistingShortUrlIfExists(string $url, array $tags, ShortUrlMeta $meta): ?ShortUrl diff --git a/module/Core/src/Service/UrlShortenerInterface.php b/module/Core/src/Service/UrlShortenerInterface.php index e26530ca..45b1eb8a 100644 --- a/module/Core/src/Service/UrlShortenerInterface.php +++ b/module/Core/src/Service/UrlShortenerInterface.php @@ -16,5 +16,5 @@ interface UrlShortenerInterface * @throws NonUniqueSlugException * @throws InvalidUrlException */ - public function urlToShortCode(string $url, array $tags, ShortUrlMeta $meta): ShortUrl; + public function shorten(string $url, array $tags, ShortUrlMeta $meta): ShortUrl; } diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 467aeb7b..436b42e3 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -8,7 +8,6 @@ use Cake\Chronos\Chronos; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\DBAL\Connection; use Doctrine\ORM\EntityManagerInterface; -use Doctrine\ORM\ORMException; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; @@ -42,16 +41,18 @@ class UrlShortenerTest extends TestCase $this->em = $this->prophesize(EntityManagerInterface::class); $conn = $this->prophesize(Connection::class); - $conn->isTransactionActive()->willReturn(false); $this->em->getConnection()->willReturn($conn->reveal()); - $this->em->flush()->willReturn(null); - $this->em->commit()->willReturn(null); - $this->em->beginTransaction()->willReturn(null); $this->em->persist(Argument::any())->will(function ($arguments): void { /** @var ShortUrl $shortUrl */ [$shortUrl] = $arguments; $shortUrl->setId('10'); }); + $this->em->transactional(Argument::type('callable'))->will(function (array $args) { + /** @var callable $callback */ + [$callback] = $args; + + return $callback(); + }); $repo = $this->prophesize(ShortUrlRepository::class); $repo->shortCodeIsInUse(Argument::cetera())->willReturn(false); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); @@ -70,7 +71,7 @@ class UrlShortenerTest extends TestCase /** @test */ public function urlIsProperlyShortened(): void { - $shortUrl = $this->urlShortener->urlToShortCode( + $shortUrl = $this->urlShortener->shorten( 'http://foobar.com/12345/hello?foo=bar', [], ShortUrlMeta::createEmpty(), @@ -87,32 +88,13 @@ class UrlShortenerTest extends TestCase $ensureUniqueness->shouldBeCalledOnce(); $this->expectException(NonUniqueSlugException::class); - $this->urlShortener->urlToShortCode( + $this->urlShortener->shorten( 'http://foobar.com/12345/hello?foo=bar', [], ShortUrlMeta::fromRawData(['customSlug' => 'custom-slug']), ); } - /** @test */ - public function transactionIsRolledBackAndExceptionRethrownWhenExceptionIsThrown(): void - { - $conn = $this->prophesize(Connection::class); - $conn->isTransactionActive()->willReturn(true); - $this->em->getConnection()->willReturn($conn->reveal()); - $this->em->rollback()->shouldBeCalledOnce(); - $this->em->close()->shouldBeCalledOnce(); - - $this->em->flush()->willThrow(new ORMException()); - - $this->expectException(ORMException::class); - $this->urlShortener->urlToShortCode( - 'http://foobar.com/12345/hello?foo=bar', - [], - ShortUrlMeta::createEmpty(), - ); - } - /** * @test * @dataProvider provideExistingShortUrls @@ -127,7 +109,7 @@ class UrlShortenerTest extends TestCase $findExisting = $repo->findOneMatching(Argument::cetera())->willReturn($expected); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - $result = $this->urlShortener->urlToShortCode($url, $tags, $meta); + $result = $this->urlShortener->shorten($url, $tags, $meta); $findExisting->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); diff --git a/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php index feed626d..8d4ea777 100644 --- a/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/AbstractCreateShortUrlAction.php @@ -31,7 +31,7 @@ abstract class AbstractCreateShortUrlAction extends AbstractRestAction $tags = $shortUrlData->getTags(); $shortUrlMeta = $shortUrlData->getMeta(); - $shortUrl = $this->urlShortener->urlToShortCode($longUrl, $tags, $shortUrlMeta); + $shortUrl = $this->urlShortener->shorten($longUrl, $tags, $shortUrlMeta); $transformer = new ShortUrlDataTransformer($this->domainConfig); return new JsonResponse($transformer->transform($shortUrl)); diff --git a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php index 1adc01a2..fd26da99 100644 --- a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php @@ -51,7 +51,7 @@ class CreateShortUrlActionTest extends TestCase public function properShortcodeConversionReturnsData(array $body, ShortUrlMeta $expectedMeta): void { $shortUrl = new ShortUrl(''); - $shorten = $this->urlShortener->urlToShortCode( + $shorten = $this->urlShortener->shorten( Argument::type('string'), Argument::type('array'), $expectedMeta, @@ -88,7 +88,7 @@ class CreateShortUrlActionTest extends TestCase public function anInvalidDomainReturnsError(string $domain): void { $shortUrl = new ShortUrl(''); - $urlToShortCode = $this->urlShortener->urlToShortCode(Argument::cetera())->willReturn($shortUrl); + $urlToShortCode = $this->urlShortener->shorten(Argument::cetera())->willReturn($shortUrl); $request = (new ServerRequest())->withParsedBody([ 'longUrl' => 'http://www.domain.com/foo/bar', diff --git a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php index 53bb6d8b..d7b432c2 100644 --- a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php @@ -72,7 +72,7 @@ class SingleStepCreateShortUrlActionTest extends TestCase 'longUrl' => 'http://foobar.com', ]); $findApiKey = $this->apiKeyService->check('abc123')->willReturn(true); - $generateShortCode = $this->urlShortener->urlToShortCode( + $generateShortCode = $this->urlShortener->shorten( Argument::that(function (string $argument): string { Assert::assertEquals('http://foobar.com', $argument); return $argument; From 2732b05834c06f14c5d5d1da26b47381dda70067 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 7 Nov 2020 09:31:46 +0100 Subject: [PATCH 4/8] Added mechanisms to be able to provide the API key when creating a short URL --- module/Core/config/dependencies.config.php | 9 ++-- module/Core/src/Entity/ShortUrl.php | 14 ++++--- .../src/Importer/ImportedLinksProcessor.php | 10 ++--- module/Core/src/Model/ShortUrlMeta.php | 7 ++++ module/Core/src/Service/UrlShortener.php | 10 ++--- .../PersistenceShortUrlRelationResolver.php | 41 +++++++++++++++++++ .../ShortUrlRelationResolverInterface.php | 15 +++++++ .../SimpleShortUrlRelationResolver.php | 21 ++++++++++ .../Validation/ShortUrlMetaInputFilter.php | 3 ++ .../Importer/ImportedLinksProcessorTest.php | 4 +- module/Core/test/Service/UrlShortenerTest.php | 4 +- 11 files changed, 113 insertions(+), 25 deletions(-) create mode 100644 module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php create mode 100644 module/Core/src/ShortUrl/Resolver/ShortUrlRelationResolverInterface.php create mode 100644 module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 4d68101b..5c7c0b54 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -7,7 +7,6 @@ namespace Shlinkio\Shlink\Core; use Laminas\ServiceManager\AbstractFactory\ConfigAbstractFactory; use Mezzio\Template\TemplateRendererInterface; use Psr\EventDispatcher\EventDispatcherInterface; -use Shlinkio\Shlink\Core\Domain\Resolver; use Shlinkio\Shlink\Core\ErrorHandler; use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; @@ -42,7 +41,7 @@ return [ Action\PixelAction::class => ConfigAbstractFactory::class, Action\QrCodeAction::class => ConfigAbstractFactory::class, - Resolver\PersistenceDomainResolver::class => ConfigAbstractFactory::class, + ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ConfigAbstractFactory::class, Mercure\MercureUpdatesGenerator::class => ConfigAbstractFactory::class, @@ -66,7 +65,7 @@ return [ Service\UrlShortener::class => [ Util\UrlValidator::class, 'em', - Resolver\PersistenceDomainResolver::class, + ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class, Service\ShortUrl\ShortCodeHelper::class, ], Service\VisitsTracker::class => [ @@ -109,13 +108,13 @@ return [ 'Logger_Shlink', ], - Resolver\PersistenceDomainResolver::class => ['em'], + ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ['em'], Mercure\MercureUpdatesGenerator::class => ['config.url_shortener.domain'], Importer\ImportedLinksProcessor::class => [ 'em', - Resolver\PersistenceDomainResolver::class, + ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class, Service\ShortUrl\ShortCodeHelper::class, Util\DoctrineBatchHelper::class, ], diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index 3d7ea0dd..6f7493aa 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -9,11 +9,11 @@ use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; use Laminas\Diactoros\Uri; use Shlinkio\Shlink\Common\Entity\AbstractEntity; -use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface; -use Shlinkio\Shlink\Core\Domain\Resolver\SimpleDomainResolver; use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException; use Shlinkio\Shlink\Core\Model\ShortUrlEdit; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; +use Shlinkio\Shlink\Core\ShortUrl\Resolver\ShortUrlRelationResolverInterface; +use Shlinkio\Shlink\Core\ShortUrl\Resolver\SimpleShortUrlRelationResolver; use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -43,9 +43,10 @@ class ShortUrl extends AbstractEntity public function __construct( string $longUrl, ?ShortUrlMeta $meta = null, - ?DomainResolverInterface $domainResolver = null + ?ShortUrlRelationResolverInterface $relationResolver = null ) { $meta = $meta ?? ShortUrlMeta::createEmpty(); + $relationResolver = $relationResolver ?? new SimpleShortUrlRelationResolver(); $this->longUrl = $longUrl; $this->dateCreated = Chronos::now(); @@ -57,13 +58,14 @@ class ShortUrl extends AbstractEntity $this->customSlugWasProvided = $meta->hasCustomSlug(); $this->shortCodeLength = $meta->getShortCodeLength(); $this->shortCode = $meta->getCustomSlug() ?? generateRandomShortCode($this->shortCodeLength); - $this->domain = ($domainResolver ?? new SimpleDomainResolver())->resolveDomain($meta->getDomain()); + $this->domain = $relationResolver->resolveDomain($meta->getDomain()); + $this->authorApiKey = $relationResolver->resolveApiKey($meta->getApiKey()); } public static function fromImport( ImportedShlinkUrl $url, bool $importShortCode, - ?DomainResolverInterface $domainResolver = null + ?ShortUrlRelationResolverInterface $relationResolver = null ): self { $meta = [ ShortUrlMetaInputFilter::DOMAIN => $url->domain(), @@ -73,7 +75,7 @@ class ShortUrl extends AbstractEntity $meta[ShortUrlMetaInputFilter::CUSTOM_SLUG] = $url->shortCode(); } - $instance = new self($url->longUrl(), ShortUrlMeta::fromRawData($meta), $domainResolver); + $instance = new self($url->longUrl(), ShortUrlMeta::fromRawData($meta), $relationResolver); $instance->importSource = $url->source(); $instance->importOriginalShortCode = $url->shortCode(); $instance->dateCreated = Chronos::instance($url->createdAt()); diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php index e072fbb8..8bac7395 100644 --- a/module/Core/src/Importer/ImportedLinksProcessor.php +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -5,10 +5,10 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Importer; use Doctrine\ORM\EntityManagerInterface; -use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; +use Shlinkio\Shlink\Core\ShortUrl\Resolver\ShortUrlRelationResolverInterface; use Shlinkio\Shlink\Core\Util\DoctrineBatchHelperInterface; use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; @@ -22,18 +22,18 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface use TagManagerTrait; private EntityManagerInterface $em; - private DomainResolverInterface $domainResolver; + private ShortUrlRelationResolverInterface $relationResolver; private ShortCodeHelperInterface $shortCodeHelper; private DoctrineBatchHelperInterface $batchHelper; public function __construct( EntityManagerInterface $em, - DomainResolverInterface $domainResolver, + ShortUrlRelationResolverInterface $relationResolver, ShortCodeHelperInterface $shortCodeHelper, DoctrineBatchHelperInterface $batchHelper ) { $this->em = $em; - $this->domainResolver = $domainResolver; + $this->relationResolver = $relationResolver; $this->shortCodeHelper = $shortCodeHelper; $this->batchHelper = $batchHelper; } @@ -58,7 +58,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface continue; } - $shortUrl = ShortUrl::fromImport($url, $importShortCodes, $this->domainResolver); + $shortUrl = ShortUrl::fromImport($url, $importShortCodes, $this->relationResolver); $shortUrl->setTags($this->tagNamesToEntities($this->em, $url->tags())); if (! $this->handleShortCodeUniqueness($url, $shortUrl, $io, $importShortCodes)) { diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index 23121d71..fa82919e 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -24,6 +24,7 @@ final class ShortUrlMeta private ?string $domain = null; private int $shortCodeLength = 5; private ?bool $validateUrl = null; + private ?string $apiKey = null; // Enforce named constructors private function __construct() @@ -66,6 +67,7 @@ final class ShortUrlMeta $inputFilter, ShortUrlMetaInputFilter::SHORT_CODE_LENGTH, ) ?? DEFAULT_SHORT_CODES_LENGTH; + $this->apiKey = $inputFilter->getValue(ShortUrlMetaInputFilter::API_KEY); } public function getValidSince(): ?Chronos @@ -132,4 +134,9 @@ final class ShortUrlMeta { return $this->validateUrl; } + + public function getApiKey(): ?string + { + return $this->apiKey; + } } diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index fb745114..3ed4d2df 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -5,13 +5,13 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; use Doctrine\ORM\EntityManagerInterface; -use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; +use Shlinkio\Shlink\Core\ShortUrl\Resolver\ShortUrlRelationResolverInterface; use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; use Throwable; @@ -22,18 +22,18 @@ class UrlShortener implements UrlShortenerInterface private EntityManagerInterface $em; private UrlValidatorInterface $urlValidator; - private DomainResolverInterface $domainResolver; + private ShortUrlRelationResolverInterface $relationResolver; private ShortCodeHelperInterface $shortCodeHelper; public function __construct( UrlValidatorInterface $urlValidator, EntityManagerInterface $em, - DomainResolverInterface $domainResolver, + ShortUrlRelationResolverInterface $relationResolver, ShortCodeHelperInterface $shortCodeHelper ) { $this->urlValidator = $urlValidator; $this->em = $em; - $this->domainResolver = $domainResolver; + $this->relationResolver = $relationResolver; $this->shortCodeHelper = $shortCodeHelper; } @@ -54,7 +54,7 @@ class UrlShortener implements UrlShortenerInterface $this->urlValidator->validateUrl($url, $meta->doValidateUrl()); return $this->em->transactional(function () use ($url, $tags, $meta) { - $shortUrl = new ShortUrl($url, $meta, $this->domainResolver); + $shortUrl = new ShortUrl($url, $meta, $this->relationResolver); $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); $this->verifyShortCodeUniqueness($meta, $shortUrl); diff --git a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php new file mode 100644 index 00000000..d898fb37 --- /dev/null +++ b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php @@ -0,0 +1,41 @@ +em = $em; + } + + public function resolveDomain(?string $domain): ?Domain + { + if ($domain === null) { + return null; + } + + /** @var Domain|null $existingDomain */ + $existingDomain = $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]); + return $existingDomain ?? new Domain($domain); + } + + public function resolveApiKey(?string $key): ?ApiKey + { + if ($key === null) { + return null; + } + + /** @var ApiKey|null $existingApiKey */ + $existingApiKey = $this->em->getRepository(ApiKey::class)->findOneBy(['key' => $key]); + return $existingApiKey; + } +} diff --git a/module/Core/src/ShortUrl/Resolver/ShortUrlRelationResolverInterface.php b/module/Core/src/ShortUrl/Resolver/ShortUrlRelationResolverInterface.php new file mode 100644 index 00000000..0a708cf6 --- /dev/null +++ b/module/Core/src/ShortUrl/Resolver/ShortUrlRelationResolverInterface.php @@ -0,0 +1,15 @@ +createInput(self::DOMAIN, false); $domain->getValidatorChain()->attach(new Validation\HostAndPortValidator()); $this->add($domain); + + $this->add($this->createInput(self::API_KEY, false)); } private function createPositiveNumberInput(string $name, int $min = 1): Input diff --git a/module/Core/test/Importer/ImportedLinksProcessorTest.php b/module/Core/test/Importer/ImportedLinksProcessorTest.php index f8a54aee..174e9afc 100644 --- a/module/Core/test/Importer/ImportedLinksProcessorTest.php +++ b/module/Core/test/Importer/ImportedLinksProcessorTest.php @@ -10,11 +10,11 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; -use Shlinkio\Shlink\Core\Domain\Resolver\SimpleDomainResolver; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Importer\ImportedLinksProcessor; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; +use Shlinkio\Shlink\Core\ShortUrl\Resolver\SimpleShortUrlRelationResolver; use Shlinkio\Shlink\Core\Util\DoctrineBatchHelperInterface; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use Symfony\Component\Console\Style\StyleInterface; @@ -46,7 +46,7 @@ class ImportedLinksProcessorTest extends TestCase $this->processor = new ImportedLinksProcessor( $this->em->reveal(), - new SimpleDomainResolver(), + new SimpleShortUrlRelationResolver(), $this->shortCodeHelper->reveal(), $batchHelper->reveal(), ); diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 436b42e3..316b7557 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -12,7 +12,6 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; -use Shlinkio\Shlink\Core\Domain\Resolver\SimpleDomainResolver; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; @@ -20,6 +19,7 @@ use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; use Shlinkio\Shlink\Core\Service\UrlShortener; +use Shlinkio\Shlink\Core\ShortUrl\Resolver\SimpleShortUrlRelationResolver; use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; class UrlShortenerTest extends TestCase @@ -63,7 +63,7 @@ class UrlShortenerTest extends TestCase $this->urlShortener = new UrlShortener( $this->urlValidator->reveal(), $this->em->reveal(), - new SimpleDomainResolver(), + new SimpleShortUrlRelationResolver(), $this->shortCodeHelper->reveal(), ); } From 7c9f572eb1d9eb8c948ac82743121dec9f9cdc70 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 7 Nov 2020 09:49:09 +0100 Subject: [PATCH 5/8] Deleted old domain resolvers and added tests for new short url relation resolvers --- .../Resolver/DomainResolverInterface.php | 12 --- .../Resolver/PersistenceDomainResolver.php | 29 ----- .../Domain/Resolver/SimpleDomainResolver.php | 15 --- .../PersistenceDomainResolverTest.php | 65 ------------ .../Resolver/SimpleDomainResolverTest.php | 41 ------- ...ersistenceShortUrlRelationResolverTest.php | 100 ++++++++++++++++++ .../SimpleShortUrlRelationResolverTest.php | 56 ++++++++++ 7 files changed, 156 insertions(+), 162 deletions(-) delete mode 100644 module/Core/src/Domain/Resolver/DomainResolverInterface.php delete mode 100644 module/Core/src/Domain/Resolver/PersistenceDomainResolver.php delete mode 100644 module/Core/src/Domain/Resolver/SimpleDomainResolver.php delete mode 100644 module/Core/test/Domain/Resolver/PersistenceDomainResolverTest.php delete mode 100644 module/Core/test/Domain/Resolver/SimpleDomainResolverTest.php create mode 100644 module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php create mode 100644 module/Core/test/ShortUrl/Resolver/SimpleShortUrlRelationResolverTest.php diff --git a/module/Core/src/Domain/Resolver/DomainResolverInterface.php b/module/Core/src/Domain/Resolver/DomainResolverInterface.php deleted file mode 100644 index c6ff55ff..00000000 --- a/module/Core/src/Domain/Resolver/DomainResolverInterface.php +++ /dev/null @@ -1,12 +0,0 @@ -em = $em; - } - - public function resolveDomain(?string $domain): ?Domain - { - if ($domain === null) { - return null; - } - - /** @var Domain|null $existingDomain */ - $existingDomain = $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]); - return $existingDomain ?? new Domain($domain); - } -} diff --git a/module/Core/src/Domain/Resolver/SimpleDomainResolver.php b/module/Core/src/Domain/Resolver/SimpleDomainResolver.php deleted file mode 100644 index b0c0dd58..00000000 --- a/module/Core/src/Domain/Resolver/SimpleDomainResolver.php +++ /dev/null @@ -1,15 +0,0 @@ -em = $this->prophesize(EntityManagerInterface::class); - $this->domainResolver = new PersistenceDomainResolver($this->em->reveal()); - } - - /** @test */ - public function returnsEmptyWhenNoDomainIsProvided(): void - { - $getRepository = $this->em->getRepository(Domain::class); - - self::assertNull($this->domainResolver->resolveDomain(null)); - $getRepository->shouldNotHaveBeenCalled(); - } - - /** - * @test - * @dataProvider provideFoundDomains - */ - public function findsOrCreatesDomainWhenValueIsProvided(?Domain $foundDomain, string $authority): void - { - $repo = $this->prophesize(ObjectRepository::class); - $findDomain = $repo->findOneBy(['authority' => $authority])->willReturn($foundDomain); - $getRepository = $this->em->getRepository(Domain::class)->willReturn($repo->reveal()); - - $result = $this->domainResolver->resolveDomain($authority); - - if ($foundDomain !== null) { - self::assertSame($result, $foundDomain); - } - self::assertInstanceOf(Domain::class, $result); - self::assertEquals($authority, $result->getAuthority()); - $findDomain->shouldHaveBeenCalledOnce(); - $getRepository->shouldHaveBeenCalledOnce(); - } - - public function provideFoundDomains(): iterable - { - $authority = 'doma.in'; - - yield 'without found domain' => [null, $authority]; - yield 'with found domain' => [new Domain($authority), $authority]; - } -} diff --git a/module/Core/test/Domain/Resolver/SimpleDomainResolverTest.php b/module/Core/test/Domain/Resolver/SimpleDomainResolverTest.php deleted file mode 100644 index ff5b6b90..00000000 --- a/module/Core/test/Domain/Resolver/SimpleDomainResolverTest.php +++ /dev/null @@ -1,41 +0,0 @@ -domainResolver = new SimpleDomainResolver(); - } - - /** - * @test - * @dataProvider provideDomains - */ - public function resolvesExpectedDomain(?string $domain): void - { - $result = $this->domainResolver->resolveDomain($domain); - - if ($domain === null) { - self::assertNull($result); - } else { - self::assertInstanceOf(Domain::class, $result); - self::assertEquals($domain, $result->getAuthority()); - } - } - - public function provideDomains(): iterable - { - yield 'with empty domain' => [null]; - yield 'with non-empty domain' => ['domain.com']; - } -} diff --git a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php new file mode 100644 index 00000000..5791d579 --- /dev/null +++ b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php @@ -0,0 +1,100 @@ +em = $this->prophesize(EntityManagerInterface::class); + $this->resolver = new PersistenceShortUrlRelationResolver($this->em->reveal()); + } + + /** @test */ + public function returnsEmptyWhenNoDomainIsProvided(): void + { + $getRepository = $this->em->getRepository(Domain::class); + + self::assertNull($this->resolver->resolveDomain(null)); + $getRepository->shouldNotHaveBeenCalled(); + } + + /** + * @test + * @dataProvider provideFoundDomains + */ + public function findsOrCreatesDomainWhenValueIsProvided(?Domain $foundDomain, string $authority): void + { + $repo = $this->prophesize(ObjectRepository::class); + $findDomain = $repo->findOneBy(['authority' => $authority])->willReturn($foundDomain); + $getRepository = $this->em->getRepository(Domain::class)->willReturn($repo->reveal()); + + $result = $this->resolver->resolveDomain($authority); + + if ($foundDomain !== null) { + self::assertSame($result, $foundDomain); + } + self::assertInstanceOf(Domain::class, $result); + self::assertEquals($authority, $result->getAuthority()); + $findDomain->shouldHaveBeenCalledOnce(); + $getRepository->shouldHaveBeenCalledOnce(); + } + + public function provideFoundDomains(): iterable + { + $authority = 'doma.in'; + + yield 'not found domain' => [null, $authority]; + yield 'found domain' => [new Domain($authority), $authority]; + } + + /** @test */ + public function returnsEmptyWhenNoApiKeyIsProvided(): void + { + $getRepository = $this->em->getRepository(ApiKey::class); + + self::assertNull($this->resolver->resolveApiKey(null)); + $getRepository->shouldNotHaveBeenCalled(); + } + + /** + * @test + * @dataProvider provideFoundApiKeys + */ + public function triesToFindApiKeyWhenValueIsProvided(?ApiKey $foundApiKey, string $key): void + { + $repo = $this->prophesize(ObjectRepository::class); + $find = $repo->findOneBy(['key' => $key])->willReturn($foundApiKey); + $getRepository = $this->em->getRepository(ApiKey::class)->willReturn($repo->reveal()); + + $result = $this->resolver->resolveApiKey($key); + + self::assertSame($result, $foundApiKey); + $find->shouldHaveBeenCalledOnce(); + $getRepository->shouldHaveBeenCalledOnce(); + } + + public function provideFoundApiKeys(): iterable + { + $key = 'abc123'; + + yield 'not found api key' => [null, $key]; + yield 'found api key' => [new ApiKey(), $key]; + } +} diff --git a/module/Core/test/ShortUrl/Resolver/SimpleShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/SimpleShortUrlRelationResolverTest.php new file mode 100644 index 00000000..e2d0822c --- /dev/null +++ b/module/Core/test/ShortUrl/Resolver/SimpleShortUrlRelationResolverTest.php @@ -0,0 +1,56 @@ +resolver = new SimpleShortUrlRelationResolver(); + } + + /** + * @test + * @dataProvider provideDomains + */ + public function resolvesExpectedDomain(?string $domain): void + { + $result = $this->resolver->resolveDomain($domain); + + if ($domain === null) { + self::assertNull($result); + } else { + self::assertInstanceOf(Domain::class, $result); + self::assertEquals($domain, $result->getAuthority()); + } + } + + public function provideDomains(): iterable + { + yield 'empty domain' => [null]; + yield 'non-empty domain' => ['domain.com']; + } + + /** + * @test + * @dataProvider provideKeys + */ + public function alwaysReturnsNullForApiKeys(?string $key): void + { + self::assertNull($this->resolver->resolveApiKey($key)); + } + + public function provideKeys(): iterable + { + yield 'empty api key' => [null]; + yield 'non-empty api key' => ['abc123']; + } +} From 27bc8d48235551dc3e821c06e392a75971a6dfdb Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 7 Nov 2020 10:23:08 +0100 Subject: [PATCH 6/8] Ensured API key is tracked when creating short URLs from the REST API --- .../src/Action/ShortUrl/CreateShortUrlAction.php | 12 ++++++++---- .../ShortUrl/SingleStepCreateShortUrlAction.php | 12 +++++++++--- .../Action/ShortUrl/CreateShortUrlActionTest.php | 16 +++++++++++++--- .../SingleStepCreateShortUrlActionTest.php | 2 +- 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php index 97097808..af6b3ecf 100644 --- a/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/CreateShortUrlAction.php @@ -8,6 +8,8 @@ use Psr\Http\Message\ServerRequestInterface as Request; use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Model\CreateShortUrlData; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; +use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; +use Shlinkio\Shlink\Rest\Authentication\Plugin\ApiKeyHeaderPlugin; class CreateShortUrlAction extends AbstractCreateShortUrlAction { @@ -19,14 +21,16 @@ class CreateShortUrlAction extends AbstractCreateShortUrlAction */ protected function buildShortUrlData(Request $request): CreateShortUrlData { - $postData = (array) $request->getParsedBody(); - if (! isset($postData['longUrl'])) { + $payload = (array) $request->getParsedBody(); + if (! isset($payload['longUrl'])) { throw ValidationException::fromArray([ 'longUrl' => 'A URL was not provided', ]); } - $meta = ShortUrlMeta::fromRawData($postData); - return new CreateShortUrlData($postData['longUrl'], (array) ($postData['tags'] ?? []), $meta); + $payload[ShortUrlMetaInputFilter::API_KEY] = $request->getHeaderLine(ApiKeyHeaderPlugin::HEADER_NAME); + $meta = ShortUrlMeta::fromRawData($payload); + + return new CreateShortUrlData($payload['longUrl'], (array) ($payload['tags'] ?? []), $meta); } } diff --git a/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php index 46385556..fe8c44aa 100644 --- a/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php @@ -7,7 +7,9 @@ namespace Shlinkio\Shlink\Rest\Action\ShortUrl; use Psr\Http\Message\ServerRequestInterface as Request; use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Model\CreateShortUrlData; +use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; +use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; class SingleStepCreateShortUrlAction extends AbstractCreateShortUrlAction @@ -32,19 +34,23 @@ class SingleStepCreateShortUrlAction extends AbstractCreateShortUrlAction protected function buildShortUrlData(Request $request): CreateShortUrlData { $query = $request->getQueryParams(); + $apiKey = $query['apiKey'] ?? ''; + $longUrl = $query['longUrl'] ?? null; - if (! $this->apiKeyService->check($query['apiKey'] ?? '')) { + if (! $this->apiKeyService->check($apiKey)) { throw ValidationException::fromArray([ 'apiKey' => 'No API key was provided or it is not valid', ]); } - if (! isset($query['longUrl'])) { + if ($longUrl === null) { throw ValidationException::fromArray([ 'longUrl' => 'A URL was not provided', ]); } - return new CreateShortUrlData($query['longUrl']); + return new CreateShortUrlData($longUrl, [], ShortUrlMeta::fromRawData([ + ShortUrlMetaInputFilter::API_KEY => $apiKey, + ])); } } diff --git a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php index fd26da99..91e6014c 100644 --- a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php @@ -48,7 +48,7 @@ class CreateShortUrlActionTest extends TestCase * @test * @dataProvider provideRequestBodies */ - public function properShortcodeConversionReturnsData(array $body, ShortUrlMeta $expectedMeta): void + public function properShortcodeConversionReturnsData(array $body, ShortUrlMeta $expectedMeta, ?string $apiKey): void { $shortUrl = new ShortUrl(''); $shorten = $this->urlShortener->shorten( @@ -58,6 +58,10 @@ class CreateShortUrlActionTest extends TestCase )->willReturn($shortUrl); $request = ServerRequestFactory::fromGlobals()->withParsedBody($body); + if ($apiKey !== null) { + $request = $request->withHeader('X-Api-Key', $apiKey); + } + $response = $this->action->handle($request); self::assertEquals(200, $response->getStatusCode()); @@ -77,8 +81,14 @@ class CreateShortUrlActionTest extends TestCase 'domain' => 'my-domain.com', ]; - yield [['longUrl' => 'http://www.domain.com/foo/bar'], ShortUrlMeta::createEmpty()]; - yield [$fullMeta, ShortUrlMeta::fromRawData($fullMeta)]; + yield 'no data' => [['longUrl' => 'http://www.domain.com/foo/bar'], ShortUrlMeta::createEmpty(), null]; + yield 'all data' => [$fullMeta, ShortUrlMeta::fromRawData($fullMeta), null]; + yield 'all data and API key' => (static function (array $meta): array { + $apiKey = 'abc123'; + $meta['apiKey'] = $apiKey; + + return [$meta, ShortUrlMeta::fromRawData($meta), $apiKey]; + })($fullMeta); } /** diff --git a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php index d7b432c2..62005c8d 100644 --- a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php @@ -78,7 +78,7 @@ class SingleStepCreateShortUrlActionTest extends TestCase return $argument; }), [], - ShortUrlMeta::createEmpty(), + ShortUrlMeta::fromRawData(['apiKey' => 'abc123']), )->willReturn(new ShortUrl('')); $resp = $this->action->handle($request); From d99ea8276139025e4dc9213638549cd8d7f727cf Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 7 Nov 2020 10:27:35 +0100 Subject: [PATCH 7/8] Added migrations folder to the static analysis --- composer.json | 2 +- data/migrations/Version20171021093246.php | 4 ++-- data/migrations/Version20201102113208.php | 4 +++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/composer.json b/composer.json index b2faf734..0fef97db 100644 --- a/composer.json +++ b/composer.json @@ -109,7 +109,7 @@ ], "cs": "phpcs", "cs:fix": "phpcbf", - "stan": "phpstan analyse module/*/src/ module/*/config config docker/config --level=6", + "stan": "phpstan analyse module/*/src/ module/*/config config docker/config data/migrations --level=6", "test": [ "@test:unit", "@test:db", diff --git a/data/migrations/Version20171021093246.php b/data/migrations/Version20171021093246.php index b66a2c3f..83f08e41 100644 --- a/data/migrations/Version20171021093246.php +++ b/data/migrations/Version20171021093246.php @@ -24,10 +24,10 @@ class Version20171021093246 extends AbstractMigration return; } - $shortUrls->addColumn('valid_since', Types::DATETIME, [ + $shortUrls->addColumn('valid_since', Types::DATETIME_MUTABLE, [ 'notnull' => false, ]); - $shortUrls->addColumn('valid_until', Types::DATETIME, [ + $shortUrls->addColumn('valid_until', Types::DATETIME_MUTABLE, [ 'notnull' => false, ]); } diff --git a/data/migrations/Version20201102113208.php b/data/migrations/Version20201102113208.php index 14ce6504..4b169532 100644 --- a/data/migrations/Version20201102113208.php +++ b/data/migrations/Version20201102113208.php @@ -46,7 +46,9 @@ final class Version20201102113208 extends AbstractMigration 'expiration' => Chronos::now()->toDateTimeString(), ]); - $id = $this->resolveOneApiKeyId($qb->execute()); + /** @var Result $result */ + $result = $qb->execute(); + $id = $this->resolveOneApiKeyId($result); if ($id === null) { return; } From fe4e171ecbcd2927cbc82c12aee1f240f90ff9be Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 7 Nov 2020 10:30:25 +0100 Subject: [PATCH 8/8] Removed unused mock --- module/Core/test/Service/UrlShortenerTest.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index 316b7557..9d8c5273 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -6,7 +6,6 @@ namespace ShlinkioTest\Shlink\Core\Service; use Cake\Chronos\Chronos; use Doctrine\Common\Collections\ArrayCollection; -use Doctrine\DBAL\Connection; use Doctrine\ORM\EntityManagerInterface; use PHPUnit\Framework\TestCase; use Prophecy\Argument; @@ -40,8 +39,6 @@ class UrlShortenerTest extends TestCase ); $this->em = $this->prophesize(EntityManagerInterface::class); - $conn = $this->prophesize(Connection::class); - $this->em->getConnection()->willReturn($conn->reveal()); $this->em->persist(Argument::any())->will(function ($arguments): void { /** @var ShortUrl $shortUrl */ [$shortUrl] = $arguments;