diff --git a/src/bundle/Core/Resources/config/routing/internal.yml b/src/bundle/Core/Resources/config/routing/internal.yml index 2bb2682533..80dccc2021 100644 --- a/src/bundle/Core/Resources/config/routing/internal.yml +++ b/src/bundle/Core/Resources/config/routing/internal.yml @@ -33,15 +33,24 @@ ibexa.content.preview.default: ibexa.user_hash: path: /_fos_user_context_hash +# Must be defined before ibexa.content.download, so that numeric {fieldId} takes precedence over {fieldIdentifier} +ibexa.content.download.field_id.filename: + path: /content/download/{contentId}/{fieldId}/{filename} + defaults: { _controller: Ibexa\Core\MVC\Symfony\Controller\Content\DownloadController::downloadBinaryFileByIdAction } + requirements: + contentId: !php/const Symfony\Component\Routing\Requirement\Requirement::DIGITS + fieldId: !php/const Symfony\Component\Routing\Requirement\Requirement::DIGITS + ibexa.content.download: path: /content/download/{contentId}/{fieldIdentifier}/{filename} defaults: { _controller: Ibexa\Core\MVC\Symfony\Controller\Content\DownloadController::downloadBinaryFileAction } requirements: - contentId: '\d+' + contentId: !php/const Symfony\Component\Routing\Requirement\Requirement::DIGITS +# Deprecated since 5.0, will be removed in 6.0. Use ibexa.content.download.field_id.filename instead. ibexa.content.download.field_id: path: /content/download/{contentId}/{fieldId} defaults: { _controller: Ibexa\Core\MVC\Symfony\Controller\Content\DownloadController::downloadBinaryFileByIdAction } requirements: - contentId: '\d+' - fieldId: '\d+' + contentId: !php/const Symfony\Component\Routing\Requirement\Requirement::DIGITS + fieldId: !php/const Symfony\Component\Routing\Requirement\Requirement::DIGITS diff --git a/src/bundle/Core/Resources/config/services.yml b/src/bundle/Core/Resources/config/services.yml index 1a00718c3d..7d90aea210 100644 --- a/src/bundle/Core/Resources/config/services.yml +++ b/src/bundle/Core/Resources/config/services.yml @@ -130,6 +130,10 @@ services: Ibexa\Core\MVC\Symfony\Controller\Content\DownloadRedirectionController: class: Ibexa\Core\MVC\Symfony\Controller\Content\DownloadRedirectionController + deprecated: + package: 'ibexa/core' + version: '5.0' + message: 'Since ibexa/core 5.0: The "%service_id%" service is deprecated and will be removed in 6.0. No route references it, use Ibexa\Core\MVC\Symfony\Controller\Content\DownloadController::downloadBinaryFileByIdAction instead' arguments: $contentService: '@ibexa.api.service.content' $router: "@router" diff --git a/src/lib/MVC/Symfony/Controller/Content/DownloadController.php b/src/lib/MVC/Symfony/Controller/Content/DownloadController.php index 3d87345b0a..9f1fb9ebdb 100644 --- a/src/lib/MVC/Symfony/Controller/Content/DownloadController.php +++ b/src/lib/MVC/Symfony/Controller/Content/DownloadController.php @@ -49,8 +49,23 @@ public function __construct( * @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException If the content is trashed, or can't be found. * @throws \Ibexa\Contracts\Core\Repository\Exceptions\UnauthorizedException If the user has no access to read content and in case of un-published content: read versions. */ - public function downloadBinaryFileByIdAction(Request $request, int $contentId, int $fieldId): BinaryStreamResponse - { + public function downloadBinaryFileByIdAction( + Request $request, + int $contentId, + int $fieldId, + ?string $filename = null + ): BinaryStreamResponse { + if ($filename === null) { + trigger_deprecation( + 'ibexa/core', + '5.0', + 'The "ibexa.content.download.field_id" route (/content/download/{contentId}/{fieldId}) is deprecated' + . ' and will be removed in 6.0.' + . ' Use the "ibexa.content.download.field_id.filename" route' + . ' (/content/download/{contentId}/{fieldId}/{filename}) instead.' + ); + } + $versionNo = $request->query->has('version') ? $request->query->getInt('version') : null; $language = $request->query->has('inLanguage') ? $request->query->get('inLanguage') : null; @@ -65,6 +80,10 @@ public function downloadBinaryFileByIdAction(Request $request, int $contentId, i throw new NotFoundException('File', $fieldId); } + if ($filename !== null && $field->value->fileName !== $filename) { + throw new NotFoundException('File', $filename); + } + return $this->downloadBinaryFileAction($contentId, $field->fieldDefIdentifier, $field->value->fileName, $request); } diff --git a/src/lib/MVC/Symfony/Controller/Content/DownloadRedirectionController.php b/src/lib/MVC/Symfony/Controller/Content/DownloadRedirectionController.php index 2e33317b27..a550134db3 100644 --- a/src/lib/MVC/Symfony/Controller/Content/DownloadRedirectionController.php +++ b/src/lib/MVC/Symfony/Controller/Content/DownloadRedirectionController.php @@ -20,6 +20,11 @@ use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Routing\RouterInterface; +/** + * @deprecated since ibexa/core 5.0, will be removed in 6.0. No route has referenced this controller since the + * "ibexa.content.download.field_id" route started serving files directly. + * Use {@see \Ibexa\Core\MVC\Symfony\Controller\Content\DownloadController::downloadBinaryFileByIdAction} instead. + */ class DownloadRedirectionController extends Controller { private ContentService $contentService; diff --git a/src/lib/MVC/Symfony/FieldType/BinaryBase/ContentDownloadUrlGenerator.php b/src/lib/MVC/Symfony/FieldType/BinaryBase/ContentDownloadUrlGenerator.php index e067fb62b2..2605748eef 100644 --- a/src/lib/MVC/Symfony/FieldType/BinaryBase/ContentDownloadUrlGenerator.php +++ b/src/lib/MVC/Symfony/FieldType/BinaryBase/ContentDownloadUrlGenerator.php @@ -14,11 +14,7 @@ class ContentDownloadUrlGenerator implements RouteAwarePathGenerator { - /** @var \Symfony\Component\Routing\RouterInterface */ - private $router; - - /** @var string */ - private $route = 'ibexa.content.download.field_id'; + private RouterInterface $router; public function __construct(RouterInterface $router) { @@ -27,7 +23,10 @@ public function __construct(RouterInterface $router) public function getStoragePathForField(Field $field, VersionInfo $versionInfo): string { - return $this->generate($this->route, $this->getParameters($field, $versionInfo)); + return $this->generate( + $this->getRoute($field, $versionInfo), + $this->getParameters($field, $versionInfo) + ); } public function generate(string $route, ?array $parameters = []): string @@ -37,7 +36,7 @@ public function generate(string $route, ?array $parameters = []): string public function getRoute(Field $field, VersionInfo $versionInfo): string { - return $this->route; + return 'ibexa.content.download.field_id.filename'; } public function getParameters(Field $field, VersionInfo $versionInfo): array @@ -46,6 +45,7 @@ public function getParameters(Field $field, VersionInfo $versionInfo): array 'contentId' => $versionInfo->contentInfo->id, 'fieldId' => $field->id, 'version' => $versionInfo->versionNo, + 'filename' => $field->value->externalData['fileName'] ?? '', ]; } } diff --git a/tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php b/tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php new file mode 100644 index 0000000000..538a8aef43 --- /dev/null +++ b/tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php @@ -0,0 +1,187 @@ +contentService = $this->createMock(ContentService::class); + $this->ioService = $this->createMock(IOServiceInterface::class); + $this->translationHelper = $this->createMock(TranslationHelper::class); + } + + public function testDownloadBinaryFileByIdActionReturnsFileWhenFilenameMatches(): void + { + $content = $this->createContent(); + $field = $content->getField('file'); + self::assertInstanceOf(Field::class, $field); + + $this->contentService + ->method('loadContent') + ->willReturn($content); + $this->translationHelper + ->expects(self::once()) + ->method('getTranslatedField') + ->with($content, 'file') + ->willReturn($field); + $this->ioService + ->expects(self::once()) + ->method('loadBinaryFile') + ->with('binary-file-id') + ->willReturn($this->createBinaryFile()); + + $response = $this->createController()->downloadBinaryFileByIdAction(new Request(), 42, 7, self::FILENAME); + + self::assertStringContainsString('attachment;', (string)$response->headers->get('Content-Disposition')); + self::assertStringContainsString(self::FILENAME, (string)$response->headers->get('Content-Disposition')); + } + + /** + * @dataProvider provideNotFoundCases + */ + public function testDownloadBinaryFileByIdActionReturnsNotFound(int $fieldId, string $filename): void + { + $content = $this->createContent(); + + $this->contentService + ->expects(self::once()) + ->method('loadContent') + ->with(42, null, null) + ->willReturn($content); + $this->translationHelper + ->expects(self::never()) + ->method('getTranslatedField'); + $this->ioService + ->expects(self::never()) + ->method('loadBinaryFile'); + + $this->expectException(NotFoundException::class); + + $this->createController()->downloadBinaryFileByIdAction(new Request(), 42, $fieldId, $filename); + } + + /** + * @return iterable + */ + public static function provideNotFoundCases(): iterable + { + yield 'filename does not match the field value' => [7, 'SomeRandomText.txt']; + yield 'field id does not exist in content' => [123, self::FILENAME]; + } + + /** + * @group legacy + */ + public function testDownloadBinaryFileByIdActionTriggersDeprecationWithoutFilename(): void + { + $content = $this->createContent(); + $field = $content->getField('file'); + self::assertInstanceOf(Field::class, $field); + + $this->contentService + ->method('loadContent') + ->willReturn($content); + $this->translationHelper + ->method('getTranslatedField') + ->willReturn($field); + $this->ioService + ->method('loadBinaryFile') + ->willReturn($this->createBinaryFile()); + + $this->expectDeprecation( + 'Since ibexa/core 5.0: The "ibexa.content.download.field_id" route (/content/download/{contentId}/{fieldId})' + . ' is deprecated and will be removed in 6.0.' + . ' Use the "ibexa.content.download.field_id.filename" route' + . ' (/content/download/{contentId}/{fieldId}/{filename}) instead.' + ); + + $response = $this->createController()->downloadBinaryFileByIdAction(new Request(), 42, 7); + + self::assertStringContainsString(self::FILENAME, (string)$response->headers->get('Content-Disposition')); + } + + private function createController(): DownloadController + { + return new DownloadController( + $this->createMock(ContainerInterface::class), + $this->contentService, + $this->ioService, + $this->translationHelper + ); + } + + private function createContent(): Content + { + return new Content([ + 'internalFields' => [ + new Field([ + 'id' => 7, + 'fieldDefIdentifier' => 'file', + 'languageCode' => 'eng-GB', + 'value' => new BinaryFileValue([ + 'id' => 'binary-file-id', + 'fileName' => self::FILENAME, + ]), + ]), + ], + 'versionInfo' => new VersionInfo([ + 'contentInfo' => new ContentInfo([ + 'id' => 42, + 'mainLanguageCode' => 'eng-GB', + 'name' => 'Test content', + 'status' => ContentInfo::STATUS_PUBLISHED, + ]), + ]), + ]); + } + + private function createBinaryFile(): BinaryFile + { + return new BinaryFile([ + 'id' => 'binary-file-id', + 'mtime' => new DateTime(), + 'size' => 123, + 'uri' => 'binary-file-uri', + ]); + } +} diff --git a/tests/lib/MVC/Symfony/FieldType/BinaryBase/ContentDownloadUrlGeneratorTest.php b/tests/lib/MVC/Symfony/FieldType/BinaryBase/ContentDownloadUrlGeneratorTest.php new file mode 100644 index 0000000000..8d1949b3b4 --- /dev/null +++ b/tests/lib/MVC/Symfony/FieldType/BinaryBase/ContentDownloadUrlGeneratorTest.php @@ -0,0 +1,103 @@ +router = $this->createMock(RouterInterface::class); + $this->generator = new ContentDownloadUrlGenerator($this->router); + } + + public function testGetRouteReturnsFilenameAwareRoute(): void + { + self::assertSame( + self::ROUTE, + $this->generator->getRoute($this->createField(), $this->createVersionInfo()) + ); + } + + public function testGetParametersContainFilename(): void + { + self::assertSame( + [ + 'contentId' => 42, + 'fieldId' => 7, + 'version' => 2, + 'filename' => 'Test-file.pdf', + ], + $this->generator->getParameters($this->createField(), $this->createVersionInfo()) + ); + } + + public function testGetStoragePathForFieldGeneratesFilenameAwareUrl(): void + { + $this->router + ->expects(self::once()) + ->method('generate') + ->with( + self::ROUTE, + [ + 'contentId' => 42, + 'fieldId' => 7, + 'version' => 2, + 'filename' => 'Test-file.pdf', + ] + ) + ->willReturn('/content/download/42/7/Test-file.pdf?version=2'); + + self::assertSame( + '/content/download/42/7/Test-file.pdf?version=2', + $this->generator->getStoragePathForField($this->createField(), $this->createVersionInfo()) + ); + } + + private function createField(): Field + { + return new Field([ + 'id' => 7, + 'value' => new FieldValue([ + 'externalData' => [ + 'fileName' => 'Test-file.pdf', + ], + ]), + ]); + } + + private function createVersionInfo(): VersionInfo + { + return new VersionInfo([ + 'versionNo' => 2, + 'contentInfo' => new ContentInfo([ + 'id' => 42, + ]), + ]); + } +}