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/.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:
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
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;
}
/**
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.
+ * @throws NoDownloadWasCarriedOutException if the download fails for some reason
+ */
+ 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/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/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
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());
}
}