IBX-12042: Deprecated filename-less download route in favor of filename-validated variant

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

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"

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);
     }
 

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;

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'] ?? '',
         ];
     }
 }

tests/lib/MVC/Symfony/Controller/Controller/Content/DownloadControllerTest.php

@@ -0,0 +1,187 @@
+<?php
+
+/**
+ * @copyright Copyright (C) Ibexa AS. All rights reserved.
+ * @license For full copyright and license information view LICENSE file distributed with this source code.
+ */
+declare(strict_types=1);
+
+namespace Ibexa\Tests\Core\MVC\Symfony\Controller\Controller\Content;
+
+use DateTime;
+use Ibexa\Contracts\Core\Repository\ContentService;
+use Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo;
+use Ibexa\Contracts\Core\Repository\Values\Content\Field;
+use Ibexa\Core\Base\Exceptions\NotFoundException;
+use Ibexa\Core\FieldType\BinaryFile\Value as BinaryFileValue;
+use Ibexa\Core\Helper\TranslationHelper;
+use Ibexa\Core\IO\IOServiceInterface;
+use Ibexa\Core\IO\Values\BinaryFile;
+use Ibexa\Core\MVC\Symfony\Controller\Content\DownloadController;
+use Ibexa\Core\Repository\Values\Content\Content;
+use Ibexa\Core\Repository\Values\Content\VersionInfo;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\HttpFoundation\Request;
+
+/**
+ * @covers \Ibexa\Core\MVC\Symfony\Controller\Content\DownloadController
+ */
+final class DownloadControllerTest extends TestCase
+{
+    use ExpectDeprecationTrait;
+
+    private const string FILENAME = 'Test-file.pdf';
+
+    private ContentService & MockObject $contentService;
+
+    private IOServiceInterface & MockObject $ioService;
+
+    private TranslationHelper & MockObject $translationHelper;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->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<string, array{int, string}>
+     */
+    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',
+        ]);
+    }
+}