From d7c7d96505cf279bcd90e4c1cb89342911385dd2 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Thu, 23 Jun 2022 20:11:04 +0200 Subject: [PATCH 1/5] Create download class Signed-off-by: Christian Wolf --- .../run-tests/config/www/000-default.conf | 2 +- .github/actions/run-tests/docker-compose.yml | 3 +- .github/actions/run-tests/run-locally.sh | 2 +- .github/actions/run-tests/www/Dockerfile | 2 + .../NoDownloadWasCarriedOutException.php | 9 ++ lib/Helper/DownloadHelper.php | 144 ++++++++++++++++++ .../DownloadHelper/DownloadHelperTest.php | 106 +++++++++++++ .../DownloadHelper/res/content-type.php | 8 + .../Helper/DownloadHelper/res/htaccess | 5 + 9 files changed, 278 insertions(+), 3 deletions(-) create mode 100644 .github/actions/run-tests/www/Dockerfile create mode 100644 lib/Exception/NoDownloadWasCarriedOutException.php create mode 100644 lib/Helper/DownloadHelper.php create mode 100644 tests/Integration/Helper/DownloadHelper/DownloadHelperTest.php create mode 100644 tests/Integration/Helper/DownloadHelper/res/content-type.php create mode 100644 tests/Integration/Helper/DownloadHelper/res/htaccess diff --git a/.github/actions/run-tests/config/www/000-default.conf b/.github/actions/run-tests/config/www/000-default.conf index a39e06794..bd9959b23 100644 --- a/.github/actions/run-tests/config/www/000-default.conf +++ b/.github/actions/run-tests/config/www/000-default.conf @@ -29,7 +29,7 @@ Options Indexes FollowSymLinks - AllowOverride None + AllowOverride All Require all granted diff --git a/.github/actions/run-tests/docker-compose.yml b/.github/actions/run-tests/docker-compose.yml index 5ce9f0e95..dcc310141 100644 --- a/.github/actions/run-tests/docker-compose.yml +++ b/.github/actions/run-tests/docker-compose.yml @@ -100,9 +100,10 @@ services: - ./volumes/cookbook:/nextcloud/custom_apps/cookbook:ro www: - image: php:apache + build: www # ports: # - 8888:80 volumes: - ./volumes/www/:/www:ro - ./config/www/000-default.conf:/etc/apache2/sites-available/000-default.conf:ro + diff --git a/.github/actions/run-tests/run-locally.sh b/.github/actions/run-tests/run-locally.sh index d5b3f85cb..cfdba9c68 100755 --- a/.github/actions/run-tests/run-locally.sh +++ b/.github/actions/run-tests/run-locally.sh @@ -92,7 +92,7 @@ build_images() { docker-compose build --pull --force-rm $PROGRESS \ --build-arg PHPVERSION=$PHP_VERSION \ dut occ php fpm - docker-compose build --pull --force-rm mysql + docker-compose build --pull --force-rm mysql www echo 'Building images finished.' } diff --git a/.github/actions/run-tests/www/Dockerfile b/.github/actions/run-tests/www/Dockerfile new file mode 100644 index 000000000..6a13e973f --- /dev/null +++ b/.github/actions/run-tests/www/Dockerfile @@ -0,0 +1,2 @@ +FROM php:apache +RUN a2enmod headers diff --git a/lib/Exception/NoDownloadWasCarriedOutException.php b/lib/Exception/NoDownloadWasCarriedOutException.php new file mode 100644 index 000000000..2465084f1 --- /dev/null +++ b/lib/Exception/NoDownloadWasCarriedOutException.php @@ -0,0 +1,9 @@ +downloaded = false; + $this->l = $l; + $this->headers = []; + } + + /** + * Download a file from the internet. + * + * The content of the file will be stored in RAM, so beware not to download huge files. + * + * @param string $url The URL of the file to fetch + * @param array $options Options to pass on for curl. This allows to fine-tune the transfer. + */ + public function downloadFile(string $url, array $options = []): void { + $this->downloaded = false; + + $ch = curl_init($url); + + $hp = tmpfile(); + + curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); + curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true); + curl_setopt($ch, CURLOPT_WRITEHEADER, $hp); + curl_setopt_array($ch, $options); + + $ret = curl_exec($ch); + if ($ret === false) { + $ex = new NoDownloadWasCarriedOutException($this->l->t('Downloading of a file failed returned the following error message: %s', [curl_error($ch)])); + fclose($hp); + curl_close($ch); + throw $ex; + } + + $this->content = $ret; + $this->status = curl_getinfo($ch, CURLINFO_RESPONSE_CODE); + + fseek($hp, 0); + $this->headers = []; + for ($buffer = fgets($hp); $buffer !== false; $buffer = fgets($hp)) { + $this->headers[] = $buffer; + } + + fclose($hp); + curl_close($ch); + $this->downloaded = true; + } + + /** + * Get the content downloaded in the last run. + * + * Note: You must first trigger the download using downloadFile method. + * + * @return string The content of the downloaded file + * @throws NoDownloadWasCarriedOutException if there was no successful download carried out before calling this method. + */ + public function getContent(): string { + if ($this->downloaded) { + return $this->content; + } else { + throw new NoDownloadWasCarriedOutException(); + } + } + + /** + * Get the Content-Type header from the last download run. + * + * Note: You must first trigger the download using downloadFile method. + * + * @return ?string The content of the Content-Type header or null if no Content-Type header was found + * @throws NoDownloadWasCarriedOutException if there was no successful download carried out before calling this method. + */ + public function getContentType(): ?string { + if (! $this->downloaded) { + throw new NoDownloadWasCarriedOutException(); + } + + foreach ($this->headers as $s) { + $parts = explode(':', $s, 2); + if (trim($parts[0]) === 'Content-Type') { + return trim($parts[1]); + } + } + + return null; + } + + /** + * Get the HTTP status code from the last download run. + * + * Note: You must first trigger the download using downloadFile method. + * + * @return int The HTTP status code + * @throws NoDownloadWasCarriedOutException if there was no successful download carried out before calling this method. + */ + public function getStatus(): int { + if (! $this->downloaded) { + throw new NoDownloadWasCarriedOutException(); + } + + return $this->status; + } +} diff --git a/tests/Integration/Helper/DownloadHelper/DownloadHelperTest.php b/tests/Integration/Helper/DownloadHelper/DownloadHelperTest.php new file mode 100644 index 000000000..b55962aa2 --- /dev/null +++ b/tests/Integration/Helper/DownloadHelper/DownloadHelperTest.php @@ -0,0 +1,106 @@ +createStub(IL10N::class); + $l->method('t')->willReturnArgument(0); + + $this->dut = new DownloadHelper($l); + + if (file_exists('/www/.htaccess')) { + unlink('/www/.htaccess'); + } + } + + protected function tearDown(): void { + if (file_exists('/www/.htaccess')) { + unlink('/www/.htaccess'); + } + } + + public function testGetContentNotDownloaded() { + $this->expectException(NoDownloadWasCarriedOutException::class); + $this->dut->getContent(); + } + + public function testGetContentTypeNotDownloaded() { + $this->expectException(NoDownloadWasCarriedOutException::class); + $this->dut->getContentType(); + } + + public function testGetStatusNotDownloaded() { + $this->expectException(NoDownloadWasCarriedOutException::class); + $this->dut->getStatus(); + } + + public function testDownload() { + $str = "This is the content of the file.\n"; + file_put_contents('/www/test.txt', $str); + $this->dut->downloadFile('http://www/test.txt'); + $this->assertEquals($str, $this->dut->getContent()); + $this->assertEquals(200, $this->dut->getStatus()); + } + + public function dpContentType() { + return [ + ['text/html;charset=ascii'], + ['text/html;charset=UTF-8'], + ['image/jpeg'], + // ['text/html;charset=ascii'], + ]; + } + + /** + * @dataProvider dpContentType + * @param mixed $contentType + */ + public function testContentType($contentType) { + copy(__DIR__ . '/res/content-type.php', '/www/test.php'); + $this->dut->downloadFile('http://www/test.php?content=' . urlencode($contentType)); + $this->assertEquals(trim($contentType), $this->dut->getContentType()); + $this->assertEquals(200, $this->dut->getStatus()); + } + + public function testDownload404() { + if (file_exists('/www/test.txt')) { + unlink('/www/test.txt'); + } + $this->dut->downloadFile('http://www/test.txt'); + $this->assertEquals(404, $this->dut->getStatus()); + } + + /** + * @medium + */ + public function testFailedDownload() { + $this->expectException(NoDownloadWasCarriedOutException::class); + $opt = [ + CURLOPT_TIMEOUT => 1, + CURLOPT_CONNECTTIMEOUT => 1, + ]; + $this->dut->downloadFile('http://www2/test.txt', $opt); + } + + public function testNoContentType() { + copy(__DIR__ . '/res/htaccess', '/www/.htaccess'); + // $this->expectException(NoDownloadWasCarriedOutException::class); + $this->dut->downloadFile('http://www/test.php'); + $this->assertNull($this->dut->getContentType()); + } +} diff --git a/tests/Integration/Helper/DownloadHelper/res/content-type.php b/tests/Integration/Helper/DownloadHelper/res/content-type.php new file mode 100644 index 000000000..69240c431 --- /dev/null +++ b/tests/Integration/Helper/DownloadHelper/res/content-type.php @@ -0,0 +1,8 @@ + +Test diff --git a/tests/Integration/Helper/DownloadHelper/res/htaccess b/tests/Integration/Helper/DownloadHelper/res/htaccess new file mode 100644 index 000000000..bb244e07b --- /dev/null +++ b/tests/Integration/Helper/DownloadHelper/res/htaccess @@ -0,0 +1,5 @@ +# Remove the Content-Type header from the request +# https://stackoverflow.com/questions/2428563/how-can-i-prevent-apache2-from-setting-the-content-type-header + +Header always set Content-Type "" +Header always unset Content-Type From ca7f322e9315cc610eff634d8284400b29e04f73 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Thu, 23 Jun 2022 21:00:21 +0200 Subject: [PATCH 2/5] Embed new download class into rest of code Signed-off-by: Christian Wolf --- lib/Helper/DownloadHelper.php | 1 + lib/Service/HtmlDownloadService.php | 36 ++- .../Unit/Service/HtmlDownloadServiceTest.php | 209 +++++------------- 3 files changed, 76 insertions(+), 170 deletions(-) diff --git a/lib/Helper/DownloadHelper.php b/lib/Helper/DownloadHelper.php index c7ea901a0..3e85be2c5 100644 --- a/lib/Helper/DownloadHelper.php +++ b/lib/Helper/DownloadHelper.php @@ -52,6 +52,7 @@ public function __construct( * * @param string $url The URL of the file to fetch * @param array $options Options to pass on for curl. This allows to fine-tune the transfer. + * @throws NoDownloadWasCarriedOutException if the download fails for some reason */ public function downloadFile(string $url, array $options = []): void { $this->downloaded = false; diff --git a/lib/Service/HtmlDownloadService.php b/lib/Service/HtmlDownloadService.php index fea956970..aa3b5d908 100644 --- a/lib/Service/HtmlDownloadService.php +++ b/lib/Service/HtmlDownloadService.php @@ -4,6 +4,8 @@ use DOMDocument; use OCA\Cookbook\Exception\ImportException; +use OCA\Cookbook\Exception\NoDownloadWasCarriedOutException; +use OCA\Cookbook\Helper\DownloadHelper; use OCA\Cookbook\Helper\HTMLFilter\AbstractHtmlFilter; use OCA\Cookbook\Helper\HTMLFilter\HtmlEntityDecodeFilter; use OCA\Cookbook\Helper\HtmlToDomParser; @@ -31,17 +33,26 @@ class HtmlDownloadService { */ private $htmlParser; + /** @var DownloadHelper */ + private $downloadHelper; + /** * @var DOMDocument */ private $dom; - public function __construct(HtmlEntityDecodeFilter $htmlEntityDecodeFilter, - ILogger $logger, IL10N $l10n, HtmlToDomParser $htmlParser) { + public function __construct( + HtmlEntityDecodeFilter $htmlEntityDecodeFilter, + ILogger $logger, + IL10N $l10n, + HtmlToDomParser $htmlParser, + DownloadHelper $downloadHelper + ) { $this->htmlFilters = [ $htmlEntityDecodeFilter ]; $this->logger = $logger; $this->l = $l10n; $this->htmlParser = $htmlParser; + $this->downloadHelper = $downloadHelper; } /** @@ -89,21 +100,24 @@ private function fetchHtmlPage(string $url): string { throw new ImportException($this->l->t('Could not parse URL')); } - $opts = [ - "http" => [ - "method" => "GET", - "header" => "User-Agent: Nextcloud Cookbook App" - ] + $opt = [ + CURLOPT_USERAGENT => 'Nextcloud Cookbook App', ]; - $context = stream_context_create($opts); + try { + $this->downloadHelper->downloadFile($url, $opt); + } catch (NoDownloadWasCarriedOutException $ex) { + throw new ImportException($this->l->t('Exception while downloading recipe from %s.', [$url]), null, $ex); + } - $html = file_get_contents($url, false, $context); + $status = $this->downloadHelper->getStatus(); - if ($html === false) { - throw new ImportException($this->l->t('Could not parse HTML code for site {url}', $url)); + if ($status < 200 || $status >= 300) { + throw new ImportException($this->l->t('Download from %s failed as HTTP status code %d is not in expected range.', [$url, $status])); } + $html = $this->downloadHelper->getContent(); + return $html; } } diff --git a/tests/Unit/Service/HtmlDownloadServiceTest.php b/tests/Unit/Service/HtmlDownloadServiceTest.php index 54d4a6852..b34f0d84f 100644 --- a/tests/Unit/Service/HtmlDownloadServiceTest.php +++ b/tests/Unit/Service/HtmlDownloadServiceTest.php @@ -1,36 +1,21 @@ triggerGetContent($url, $useIncludePath, $context); -} - namespace OCA\Cookbook\tests\Unit\Service; use DOMDocument; use OCP\IL10N; use OCP\ILogger; -use ReflectionProperty; use PHPUnit\Framework\TestCase; use OCA\Cookbook\Helper\HtmlToDomParser; use OCA\Cookbook\Exception\ImportException; +use OCA\Cookbook\Exception\NoDownloadWasCarriedOutException; +use OCA\Cookbook\Helper\DownloadHelper; use PHPUnit\Framework\MockObject\MockObject; use OCA\Cookbook\Service\HtmlDownloadService; use OCA\Cookbook\Helper\HTMLFilter\HtmlEntityDecodeFilter; -class MockFileGetter { - public function getIt($url, $internal, $context) { - return ''; - } -} - /** - * @coversDefaultClass \OCA\Cookbook\Service\HtmlDownloadService - * @covers :: - * @covers :: + * @covers \OCA\Cookbook\Service\HtmlDownloadService */ class HtmlDownloadServiceTest extends TestCase { /** @@ -49,6 +34,9 @@ class HtmlDownloadServiceTest extends TestCase { * @var HtmlToDomParser|MockObject */ private $htmlParser; + /** @var DownloadHelper|MockObject */ + private $downloadHelper; + /** * @var HtmlDownloadService */ @@ -59,16 +47,6 @@ class HtmlDownloadServiceTest extends TestCase { */ public static $instance; - /** - * @var bool - */ - private $runRealFunction; - - /** - * @var MockFileGetter|MockObject - */ - private $mockFunction; - public function setUp(): void { parent::setUp(); @@ -79,153 +57,66 @@ public function setUp(): void { $this->ilogger = $this->createStub(ILogger::class); $this->il10n = $this->createStub(IL10N::class); $this->htmlParser = $this->createMock(HtmlToDomParser::class); + $this->downloadHelper = $this->createMock(DownloadHelper::class); - $this->mockFunction = $this->createMock(MockFileGetter::class); - $this->runRealFunction = true; - - $this->sut = new HtmlDownloadService($this->htmlEntityDecodeFilter, $this->ilogger, $this->il10n, $this->htmlParser); + $this->sut = new HtmlDownloadService($this->htmlEntityDecodeFilter, $this->ilogger, $this->il10n, $this->htmlParser, $this->downloadHelper); } - /** - * @covers ::__construct - */ - public function testConstructor(): void { - $filtersProp = new ReflectionProperty(HtmlDownloadService::class, 'htmlFilters'); - $loggerProp = new ReflectionProperty(HtmlDownloadService::class, 'logger'); - $lProp = new ReflectionProperty(HtmlDownloadService::class, 'l'); - - $filtersProp->setAccessible(true); - $loggerProp->setAccessible(true); - $lProp->setAccessible(true); - - $this->assertSame([$this->htmlEntityDecodeFilter], $filtersProp->getValue($this->sut)); - $this->assertSame($this->ilogger, $loggerProp->getValue($this->sut)); - $this->assertSame($this->il10n, $lProp->getValue($this->sut)); + public function testDownloadInvalidUrl() { + $url = 'http:///example.com'; + $this->expectException(ImportException::class); + $this->sut->downloadRecipe($url); } - /** - * covers ::getDom - * @todo This must be checked - */ - public function oldTestGetterDom(): void { - $domProp = new ReflectionProperty(HtmlDownloadService::class, 'dom'); - $domProp->setAccessible(true); - /** - * @var DOMDocument $dom - */ - $dom = $this->createStub(DOMDocument::class); - $domProp->setValue($this->sut, $dom); - - $this->assertSame($dom, $this->sut->getDom()); + public function testDownloadFailing() { + $url = 'http://example.com'; + $this->downloadHelper->expects($this->once()) + ->method('downloadFile') + ->willThrowException(new NoDownloadWasCarriedOutException()); + $this->expectException(ImportException::class); + $this->sut->downloadRecipe($url); } - public function triggerGetContent($path, $useInternalPath, $context) { - if ($this->runRealFunction) { - return file_get_contents($path, $useInternalPath, $context); - } else { - return $this->mockFunction->getIt($path, $useInternalPath, $context); - } + public function dpBadStatus() { + return [ + [180], [199], [300], [404] + ]; } /** - * @dataProvider dataProviderFakeDownload - * @covers ::downloadRecipe - * @covers ::getDom - * @covers \OCA\Cookbook\Exception\ImportException - * @param mixed $url - * @param mixed $urlValid - * @param mixed $fetchedValue - * @param mixed $parserState - * @param mixed $fetchValid + * @dataProvider dpBadStatus + * @param mixed $status */ - public function testFakeDownload($url, $urlValid, $fetchedValue, $parserState, $fetchValid): void { - $this->runRealFunction = false; - - $dom = new DOMDocument(); + public function testDownloadBadStatus($status) { + $url = 'http://example.com'; + $this->downloadHelper->expects($this->once()) + ->method('downloadFile'); + $this->downloadHelper->method('getStatus')->willReturn($status); + $this->expectException(ImportException::class); + $this->sut->downloadRecipe($url); + } - if ($urlValid) { - // Mock file_get_contents - $this->mockFunction->expects($this->once())->method('getIt')->with( - $this->equalTo($url), - $this->anything(), - $this->callback(function ($context) { - $opts = stream_context_get_options($context); - if (!isset($opts['http'])) { - return false; - } - if (!isset($opts['http']['method']) || $opts['http']['method'] !== 'GET') { - return false; - } - return true; - }) - )->willReturn($fetchedValue); - } else { - $this->mockFunction->expects($this->never())->method('getIt'); - } - - if ($urlValid && $fetchValid) { - $this->htmlEntityDecodeFilter->expects($this->once())->method('apply'); - - $this->htmlParser->expects($this->once())->method('loadHtmlString')->with( + public function testDownload() { + $url = 'http://example.com'; + $content = 'The content of the html file'; + $dom = $this->createStub(DOMDocument::class); + $state = 12345; + + $this->downloadHelper->expects($this->once()) + ->method('downloadFile'); + $this->downloadHelper->method('getStatus')->willReturn(200); + $this->downloadHelper->method('getContent')->willReturn($content); + $this->htmlParser->expects($this->once())->method('loadHtmlString') + ->with( $this->anything(), $this->equalTo($url), - $this->equalTo($fetchedValue) + $this->equalTo($content) )->willReturn($dom); - $this->htmlParser->method('getState')->willReturn($parserState); - } else { - $this->htmlEntityDecodeFilter->expects($this->never())->method('apply'); - $this->htmlParser->expects($this->never())->method('loadHtmlString'); - } - - $this->assertNull($this->sut->getDom()); - - try { - $result = $this->sut->downloadRecipe($url); - - $this->assertTrue($urlValid && $fetchValid); - $this->assertSame($dom, $this->sut->getDom()); - $this->assertEquals($parserState, $result); - } catch (ImportException $ex) { - $this->assertFalse($urlValid && $fetchValid); - } - } + $this->htmlParser->method('getState')->willReturn($state); - public function dataProviderFakeDownload() { - return [ - 'invalidURL' => ['http:///example.com', false, null, null, false], - 'validURL' => ['http://example.com', true, 'Here comes the text', 1, true], - 'invalidFetch' => ['http://example.com', true, false, 1, false], - ]; - } + $ret = $this->sut->downloadRecipe($url); + $this->assertEquals($state, $ret); - /** - * @dataProvider dataProviderRealDownload - * @covers ::downloadRecipe - * @param mixed $data - */ - public function testRealDownload($data) { - $url = 'http://www/test.html'; - $this->runRealFunction = true; - - if (file_exists('/www/test.html')) { - unlink('/www/test.html'); - } - file_put_contents('/www/test.html', $data); - - $this->htmlParser->expects($this->once())->method('loadHtmlString')->with( - $this->anything(), - $this->equalTo($url), - $this->equalTo($data) - ); - - $this->htmlParser->method('getState')->willReturn(0); - - $this->sut->downloadRecipe($url); - } - - public function dataProviderRealDownload() { - yield [ - 'foo' - ]; + $this->assertSame($dom, $this->sut->getDom()); } } From 1cd2fbff5f5eca2f5c23546c08d838a2d6fe911f Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Thu, 23 Jun 2022 21:03:12 +0200 Subject: [PATCH 3/5] Build even if only CHNAGELOG is changed as this will fix codecov issue in PRs Signed-off-by: Christian Wolf --- .github/workflows/tests.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 9beb3fcfb..f3abab33e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -6,7 +6,6 @@ on: paths-ignore: - 'docs/**' - 'l10n/**' - - 'CHANGELOG.md' pull_request: jobs: From 58142c9eded0d04a269ff65d3849c5c038e2b2f0 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Thu, 23 Jun 2022 21:04:38 +0200 Subject: [PATCH 4/5] Update Changelog Signed-off-by: Christian Wolf --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66032de70..8e4d97757 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ ### Changed - Extracted user folder handling into its own helper class [#1007](https://github.com/nextcloud/cookbook/pull/1007) @christianlupus +- Switched to cURL for downloading of external files + [#1055](https://github.com/nextcloud/cookbook/pull/1055) @christianlupus ### Fixed - Fix visual regression in edit mode to prevent overflow of breadcrumbs From 58ff14f1e4f9d82cc9140cc1dc8cc7bb3e3093bf Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Thu, 23 Jun 2022 21:16:28 +0200 Subject: [PATCH 5/5] Fixed bug from other PR #1049 Signed-off-by: Christian Wolf --- lib/Db/RecipeDb.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/Db/RecipeDb.php b/lib/Db/RecipeDb.php index 37a21af79..cb01d1078 100755 --- a/lib/Db/RecipeDb.php +++ b/lib/Db/RecipeDb.php @@ -31,6 +31,7 @@ public function __construct( ) { $this->db = $db; $this->types = $polyfillTypes; + $this->l = $l; } /**