From 9c7598e77e1bcc53d5ab97ed87da46f7ba426748 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Wed, 4 May 2022 15:24:40 +0200 Subject: [PATCH 1/7] Create parser for Accept header Signed-off-by: Christian Wolf --- lib/Helper/AcceptHeaderParsingHelper.php | 117 ++++++++++++++++++ .../Helper/AcceptHeaderParsingHelperTest.php | 49 ++++++++ 2 files changed, 166 insertions(+) create mode 100644 lib/Helper/AcceptHeaderParsingHelper.php create mode 100644 tests/Unit/Helper/AcceptHeaderParsingHelperTest.php diff --git a/lib/Helper/AcceptHeaderParsingHelper.php b/lib/Helper/AcceptHeaderParsingHelper.php new file mode 100644 index 000000000..f73f2c25b --- /dev/null +++ b/lib/Helper/AcceptHeaderParsingHelper.php @@ -0,0 +1,117 @@ +sortParts($parts); + $weightedParts = $this->sortAndWeightParts($parts); + + $extensions = []; + + foreach ($weightedParts as $wp) { + $ex = $this->getFileTypes($wp['type']); + + foreach ($ex as $e) { + if (array_search($e, $extensions) === false) { + $extensions[] = $e; + } + } + } + return $extensions; + } + + /** + * Return the list of all supported file extensions by the app. + * The return value in the same format as with the parseHeader function + * + * @return array The list of supported file extensions by the app + */ + public function getDefaultExtensions(): array { + return ['jpg']; + } + + private function sortAndWeightParts(array $parts): array { + $weightedParts = array_map(function ($x) { + return $this->parsePart($x); + }, $parts); + + usort($weightedParts, function ($a, $b) { + $tmp = $a['weight'] - $b['weight']; + if ($tmp < - 0.001) { + return -1; + } elseif ($tmp > 0.001) { + return 1; + } else { + return 0; + } + }); + $weightedParts = array_reverse($weightedParts); + + return $weightedParts; + } + + private function addWeightedPart($weight, $part, &$weightedParts): void { + if (! array_key_exists($weight, $weightedParts)) { + $weightedParts[$weight] = []; + } + + array_push($weightedParts[$weight], $part); + } + + private function parsePart($part): array { + if (preg_match('/\s*(.+?)\s*;q=([0-9.]+)\s*$/', $part, $matches) === 0) { + // No qualifier was found + $mime = trim($part); + $weight = 1; + } else { + // Separate qualifier and part + $mime = trim($matches[1]); + $weight = $matches[2]; + } + + return [ + 'type' => $mime, + 'weight' => $weight, + ]; + } + + private function getFileTypes(string $mime): array { + $parts = explode(';', $mime, 2); + switch ($parts[0]) { + case 'image/jpeg': + case 'image/jpg': + return ['jpg']; + case 'image/png': + return ['png']; + case 'image/svg+xml': + return ['svg']; + case 'image/*': + return ['jpg', 'png', 'svg']; + case '*/*': + return ['jpg', 'png', 'svg']; + } + return []; + } +} diff --git a/tests/Unit/Helper/AcceptHeaderParsingHelperTest.php b/tests/Unit/Helper/AcceptHeaderParsingHelperTest.php new file mode 100644 index 000000000..98aac11a7 --- /dev/null +++ b/tests/Unit/Helper/AcceptHeaderParsingHelperTest.php @@ -0,0 +1,49 @@ +dut = new AcceptHeaderParsingHelper(); + } + + public function testDefaultExtensions() { + $ret = $this->dut->getDefaultExtensions(); + + $this->assertEquals(['jpg'], $ret); + } + + public function dataProvider() { + yield ['image/jpeg', ['jpg']]; + yield ['image/jpg', ['jpg']]; + yield ['image/png', ['png']]; + yield ['image/svg+xml', ['svg']]; + yield ['image/*', ['jpg', 'png', 'svg']]; + yield ['*/*', ['jpg', 'png', 'svg']]; + yield ['image/jpeg, image/*', ['jpg', 'png', 'svg']]; + yield ['image/jpeg;q=0.9, image/*;q=0.5', ['jpg', 'png', 'svg']]; + yield ['image/webn, image/jpeg', ['jpg']]; + yield ['image/webn', []]; + yield ['image/png;q=0.5, image/jpg', ['jpg', 'png']]; + yield ['image/png;q=0.5, image/jpeg;q=0.3, image/svg+xml', ['svg', 'png', 'jpg']]; + yield ['image/png, image/jpeg;q=0.3, image/svg+xml;q=0.9', ['png', 'svg', 'jpg']]; + } + + /** + * @dataProvider dataProvider + */ + public function testParseHeader($header, $expected) { + $this->assertEquals($expected, $this->dut->parseHeader($header)); + } +} From 80a49f2d85776ae8ce6bd432ff75a6ab30368c76 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Tue, 10 May 2022 08:36:40 +0200 Subject: [PATCH 2/7] Only serve SVG if requested/accepted Signed-off-by: Christian Wolf --- lib/Controller/RecipeController.php | 43 ++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/lib/Controller/RecipeController.php b/lib/Controller/RecipeController.php index 35e4995cc..7c6834370 100755 --- a/lib/Controller/RecipeController.php +++ b/lib/Controller/RecipeController.php @@ -14,8 +14,10 @@ use OCP\IURLGenerator; use OCA\Cookbook\Service\DbCacheService; use OCA\Cookbook\Exception\RecipeExistsException; +use OCA\Cookbook\Helper\AcceptHeaderParsingHelper; use OCA\Cookbook\Helper\RestParameterParser; use OCP\AppFramework\Http\JSONResponse; +use OCP\IL10N; class RecipeController extends Controller { /** @@ -37,13 +39,34 @@ class RecipeController extends Controller { */ private $restParser; - public function __construct($AppName, IRequest $request, IURLGenerator $urlGenerator, RecipeService $recipeService, DbCacheService $dbCacheService, RestParameterParser $restParser) { + /** + * @var AcceptHeaderParsingHelper + */ + private $acceptHeaderParser; + + /** + * @var IL10N + */ + private $l; + + public function __construct( + $AppName, + IRequest $request, + IURLGenerator $urlGenerator, + RecipeService $recipeService, + DbCacheService $dbCacheService, + RestParameterParser $restParser, + AcceptHeaderParsingHelper $acceptHeaderParser, + IL10N $l + ) { parent::__construct($AppName, $request); $this->service = $recipeService; $this->urlGenerator = $urlGenerator; $this->dbCacheService = $dbCacheService; $this->restParser = $restParser; + $this->acceptHeaderParser = $acceptHeaderParser; + $this->l = $l; } /** @@ -178,6 +201,9 @@ public function destroy($id) { public function image($id) { $this->dbCacheService->triggerCheck(); + $acceptHeader = $this->request->getHeader('Accept'); + $acceptedExtensions = $this->acceptHeaderParser->parseHeader($acceptHeader); + $size = isset($_GET['size']) ? $_GET['size'] : null; try { @@ -185,9 +211,18 @@ public function image($id) { return new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/jpeg', 'Cache-Control' => 'public, max-age=604800']); } catch (\Exception $e) { - $file = file_get_contents(dirname(__FILE__) . '/../../img/recipe.svg'); - - return new DataDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/svg+xml']); + if (array_search('svg', $acceptedExtensions, true) === false) { + // We may not serve a SVG image. Tell the client about the missing image. + $json = [ + 'msg' => $this->l->t('No image with the matching mime type was fount on the server.'), + ]; + return new JSONResponse($json, Http::STATUS_NOT_ACCEPTABLE); + } else { + // The client accepts the SVG file. Send it. + $file = file_get_contents(dirname(__FILE__) . '/../../img/recipe.svg'); + + return new DataDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/svg+xml']); + } } } } From ed65b4b14b72f9481b382b01ed2efe2a958761d9 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Tue, 10 May 2022 08:46:25 +0200 Subject: [PATCH 3/7] Fix API documentation Signed-off-by: Christian Wolf --- docs/dev/api/0.0.2/index.html | 2 +- docs/dev/api/0.0.3/index.html | 2 +- docs/dev/api/0.0.3/openapi-cookbook.yaml | 13 ++++++++++++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/docs/dev/api/0.0.2/index.html b/docs/dev/api/0.0.2/index.html index c86a109cf..f22ad2818 100644 --- a/docs/dev/api/0.0.2/index.html +++ b/docs/dev/api/0.0.2/index.html @@ -39,7 +39,7 @@ window.onload = function() { // Begin Swagger UI call region const ui = SwaggerUIBundle({ - url: "https://nextcloud.github.io/cookbook/dev/api/0.0.2/openapi-cookbook.yaml", + url: "openapi-cookbook.yaml", dom_id: '#swagger-ui', deepLinking: true, presets: [ diff --git a/docs/dev/api/0.0.3/index.html b/docs/dev/api/0.0.3/index.html index f07215b01..f22ad2818 100644 --- a/docs/dev/api/0.0.3/index.html +++ b/docs/dev/api/0.0.3/index.html @@ -39,7 +39,7 @@ window.onload = function() { // Begin Swagger UI call region const ui = SwaggerUIBundle({ - url: "https://nextcloud.github.io/cookbook/dev/api/0.0.3/openapi-cookbook.yaml", + url: "openapi-cookbook.yaml", dom_id: '#swagger-ui', deepLinking: true, presets: [ diff --git a/docs/dev/api/0.0.3/openapi-cookbook.yaml b/docs/dev/api/0.0.3/openapi-cookbook.yaml index 94162ed0a..d7fb9e925 100644 --- a/docs/dev/api/0.0.3/openapi-cookbook.yaml +++ b/docs/dev/api/0.0.3/openapi-cookbook.yaml @@ -297,7 +297,18 @@ paths: type: integer responses: 200: - description: Image was obtained and will be inresponse either as image/jpeg or image/svg+xml + description: Image was obtained and will be in response either as image/jpeg or image/svg+xml + content: + image/jpeg: + schema: + type: string + format: binary + image/svg+xml: + schema: + type: string + format: binary + 406: + description: The recipe has no image whose MIME type matches the Accept header /api/search/{query}: parameters: - in: path From c3105ea7c4600dec72d7686bfaad2ac7cc417434 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Tue, 10 May 2022 19:34:15 +0200 Subject: [PATCH 4/7] Update unit tests Signed-off-by: Christian Wolf --- .../Unit/Controller/RecipeControllerTest.php | 49 ++++++++++++++++++- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/tests/Unit/Controller/RecipeControllerTest.php b/tests/Unit/Controller/RecipeControllerTest.php index 8808fb94b..182cd8900 100644 --- a/tests/Unit/Controller/RecipeControllerTest.php +++ b/tests/Unit/Controller/RecipeControllerTest.php @@ -19,6 +19,9 @@ use OCA\Cookbook\Exception\NoRecipeNameGivenException; use OCP\AppFramework\Http\FileDisplayResponse; use OCA\Cookbook\Exception\RecipeExistsException; +use OCA\Cookbook\Helper\AcceptHeaderParsingHelper; +use OCP\IL10N; +use PHPUnit\Framework\MockObject\Stub; /** * @covers \OCA\Cookbook\Controller\RecipeController @@ -47,6 +50,16 @@ class RecipeControllerTest extends TestCase { */ private $sut; + /** + * @var IRequest|MockObject + */ + private $request; + + /** + * @var AcceptHeaderParsingHelper|Stub + */ + private $acceptHeaderParser; + public function setUp(): void { parent::setUp(); @@ -54,9 +67,16 @@ public function setUp(): void { $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->dbCacheService = $this->createMock(DbCacheService::class); $this->restParser = $this->createMock(RestParameterParser::class); - $request = $this->createStub(IRequest::class); + $this->request = $this->createMock(IRequest::class); + $this->acceptHeaderParser = $this->createStub(AcceptHeaderParsingHelper::class); - $this->sut = new RecipeController('cookbook', $request, $this->urlGenerator, $this->recipeService, $this->dbCacheService, $this->restParser); + /** + * @var Stub|IL10N $l + */ + $l = $this->createStub(IL10N::class); + $l->method('t')->willReturnArgument(0); + + $this->sut = new RecipeController('cookbook', $this->request, $this->urlGenerator, $this->recipeService, $this->dbCacheService, $this->restParser, $this->acceptHeaderParser, $l); } public function testConstructor(): void { @@ -300,6 +320,31 @@ public function dataProviderImage(): array { ]; } + public function dpImageNotFound() { + yield [['jpg', 'png'], 406]; + yield [['jpg', 'png', 'svg'], 200]; + } + + /** + * @dataProvider dpImageNotFound + */ + public function testImageNotFound($accept, $expectedStatus) { + $id = 123; + + $ex = new Exception(); + $this->recipeService->method('getRecipeImageFileByFolderId')->willThrowException($ex); + + $headerContent = 'The content of the header as supposed by teh framework'; + $this->request->method('getHeader')->with('Accept')->willReturn($headerContent); + $this->acceptHeaderParser->method('parseHeader')->willReturnMap([ + [$headerContent, $accept], + ]); + + $ret = $this->sut->image($id); + + $this->assertEquals($expectedStatus, $ret->getStatus()); + } + /** * @dataProvider dataProviderIndex * @todo no work on controller From 30593f67377a0478adbb0b79db3b853979417f58 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Tue, 10 May 2022 21:14:23 +0200 Subject: [PATCH 5/7] Update changelog Signed-off-by: Christian Wolf --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 22394a843..d869f53b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ ### Added - Add IDE configuration to codebase to prevent small issues [#978](https://github.com/nextcloud/cookbook/pull/978) @christianlupus +- Allow client to specify accepted image types + [#982](https://github.com/nextcloud/cookbook/pull/982) @christianlupus ### Fixed - Refactor the code for image handling to make it testable From efd53c14047fc641b82584f00ee10b23a6730a08 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Tue, 10 May 2022 21:17:53 +0200 Subject: [PATCH 6/7] Removed obsolete private method from new class Signed-off-by: Christian Wolf --- lib/Helper/AcceptHeaderParsingHelper.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lib/Helper/AcceptHeaderParsingHelper.php b/lib/Helper/AcceptHeaderParsingHelper.php index f73f2c25b..93f9abf9a 100644 --- a/lib/Helper/AcceptHeaderParsingHelper.php +++ b/lib/Helper/AcceptHeaderParsingHelper.php @@ -72,14 +72,6 @@ private function sortAndWeightParts(array $parts): array { return $weightedParts; } - private function addWeightedPart($weight, $part, &$weightedParts): void { - if (! array_key_exists($weight, $weightedParts)) { - $weightedParts[$weight] = []; - } - - array_push($weightedParts[$weight], $part); - } - private function parsePart($part): array { if (preg_match('/\s*(.+?)\s*;q=([0-9.]+)\s*$/', $part, $matches) === 0) { // No qualifier was found From 68a3171554c358bb290c313f9a81d058f0f8701e Mon Sep 17 00:00:00 2001 From: Christian Date: Thu, 12 May 2022 07:53:50 +0200 Subject: [PATCH 7/7] Apply suggestions from code review Co-authored-by: Sebastian Fey Signed-off-by: Christian Wolf --- lib/Controller/RecipeController.php | 2 +- lib/Helper/AcceptHeaderParsingHelper.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/Controller/RecipeController.php b/lib/Controller/RecipeController.php index 7c6834370..abdd078b0 100755 --- a/lib/Controller/RecipeController.php +++ b/lib/Controller/RecipeController.php @@ -214,7 +214,7 @@ public function image($id) { if (array_search('svg', $acceptedExtensions, true) === false) { // We may not serve a SVG image. Tell the client about the missing image. $json = [ - 'msg' => $this->l->t('No image with the matching mime type was fount on the server.'), + 'msg' => $this->l->t('No image with the matching mime type was found on the server.'), ]; return new JSONResponse($json, Http::STATUS_NOT_ACCEPTABLE); } else { diff --git a/lib/Helper/AcceptHeaderParsingHelper.php b/lib/Helper/AcceptHeaderParsingHelper.php index 93f9abf9a..c21330923 100644 --- a/lib/Helper/AcceptHeaderParsingHelper.php +++ b/lib/Helper/AcceptHeaderParsingHelper.php @@ -3,9 +3,9 @@ namespace OCA\Cookbook\Helper; /** - * This class parses the Accepts header from a HTTP request and returns an array of accepted file extensions. + * This class parses the Accepts header of an HTTP request and returns an array of accepted file extensions. * - * The return values is a list of extensions that the client is willing to accept. + * The return value is a list of extensions that the client is willing to accept. * Higher priorities are sorted first in the array. */ class AcceptHeaderParsingHelper {