diff --git a/.github/workflows/api-integration-tests.yml b/.github/workflows/api-integration-tests.yml index c184cf44d..6e2b35263 100644 --- a/.github/workflows/api-integration-tests.yml +++ b/.github/workflows/api-integration-tests.yml @@ -112,6 +112,10 @@ jobs: check-code: false force: ${{ matrix.experimental }} + - name: Allow local remote servers + working-directory: ../server + run: ./occ config:system:set allow_local_remote_servers --type=boolean --value=true + - name: Run API tests working-directory: ../server run: | diff --git a/.github/workflows/updater-test.yml b/.github/workflows/updater-test.yml index 00f071e1d..1ab3f62ed 100644 --- a/.github/workflows/updater-test.yml +++ b/.github/workflows/updater-test.yml @@ -63,6 +63,10 @@ jobs: check-code: false force: ${{ matrix.experimental }} + - name: Allow local remote servers + working-directory: ../server + run: ./occ config:system:set allow_local_remote_servers --type=boolean --value=true + - name: Install composer install php-feed-generator working-directory: ../server run: composer install -d apps/news/tests/test_helper/php-feed-generator diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dfc04fa1..28adf780b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ You can also check [on GitHub](https://github.com/nextcloud/news/releases), the # Unreleased ### Changed - +- Refactor FaviconDataAccess to use Nextclouds IClientService (#3671) ### Fixed # Releases diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 70f917532..c41bd7017 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -41,6 +41,7 @@ use OCP\Config\BeforePreferenceDeletedEvent; use OCP\Config\BeforePreferenceSetEvent; use OCP\DB\Events\AddMissingIndicesEvent; +use OCP\Http\Client\IClientService; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; @@ -122,7 +123,7 @@ public function register(IRegistrationContext $context): void $context->registerService(FaviconDataAccess::class, function (ContainerInterface $c): FaviconDataAccess { $config = $c->get(FetcherConfig::class); - return new FaviconDataAccess($config); + return new FaviconDataAccess($config, $c->get(IClientService::class), $c->get(LoggerInterface::class)); }); $context->registerService(Favicon::class, function (ContainerInterface $c): Favicon { diff --git a/lib/Fetcher/FaviconDataAccess.php b/lib/Fetcher/FaviconDataAccess.php index b95359b49..cdca952f3 100644 --- a/lib/Fetcher/FaviconDataAccess.php +++ b/lib/Fetcher/FaviconDataAccess.php @@ -13,49 +13,72 @@ use OCA\News\Vendor\Favicon\DataAccess; +use OCP\Http\Client\IClientService; + use OCA\News\Config\FetcherConfig; +use Psr\Log\LoggerInterface; /** * Modified version of DataAccess with a configurable user agent header. */ class FaviconDataAccess extends DataAccess { - /** - * @var FetcherConfig - */ - private $fetcherConfig; - public function __construct( - FetcherConfig $fetcherConfig, + private readonly FetcherConfig $fetcherConfig, + private readonly IClientService $clientService, + private readonly LoggerInterface $logger ) { - $this->fetcherConfig = $fetcherConfig; } public function retrieveUrl($url) { - $this->setContext(); - return @file_get_contents($url); + try { + $response = $this->clientService->newClient()->get( + $url, + $this->getRequestOptions() + ); + return $response->getBody(); + } catch (\Throwable $e) { + $this->logger->warning( + 'Could not fetch favicon URL {url}: {error}', + ['url' => $url, 'error' => $e->getMessage()] + ); + return false; + } } public function retrieveHeader($url) { - $this->setContext(); - $headers = @get_headers($url, 1); - return is_array($headers) ? array_change_key_case($headers) : []; + try { + $response = $this->clientService->newClient()->head( + $url, + $this->getRequestOptions() + ); + $statusCode = $response->getStatusCode(); + $headers = array_change_key_case($response->getHeaders()); + // The Favicon library expects $headers[0] to be the HTTP status line, + // matching the format returned by PHP's get_headers(). + // Prepend it to ensure correct ordering. + $headers = [0 => 'HTTP/1.1 ' . $statusCode] + $headers; + return $headers; + } catch (\Throwable $e) { + $this->logger->warning( + 'Could not fetch favicon headers for {url}: {error}', + ['url' => $url, 'error' => $e->getMessage()] + ); + return []; + } } - private function setContext() + private function getRequestOptions(): array { - stream_context_set_default( - [ - 'http' => [ - 'method' => 'GET', - 'follow_location' => 0, - 'max_redirects' => 1, - 'timeout' => 10, - 'header' => 'User-Agent: ' . $this->fetcherConfig->getUserAgent(), - ] - ] - ); + return [ + 'timeout' => 10, + 'allow_redirects' => false, + 'http_errors' => false, + 'headers' => [ + 'User-Agent' => $this->fetcherConfig->getUserAgent(), + ], + ]; } } diff --git a/tests/Unit/Fetcher/FaviconDataAccessTest.php b/tests/Unit/Fetcher/FaviconDataAccessTest.php new file mode 100644 index 000000000..0a1884724 --- /dev/null +++ b/tests/Unit/Fetcher/FaviconDataAccessTest.php @@ -0,0 +1,156 @@ +fetcherConfig = $this->createMock(FetcherConfig::class); + $this->clientService = $this->createMock(IClientService::class); + $this->client = $this->createMock(IClient::class); + $this->logger = $this->createMock(LoggerInterface::class); + + $this->clientService->method('newClient') + ->willReturn($this->client); + + $this->class = new FaviconDataAccess($this->fetcherConfig, $this->clientService, $this->logger); + } + + public function testRetrieveUrlReturnsBodyOnSuccess(): void + { + $url = 'https://example.com/favicon.ico'; + $userAgent = 'NextCloud-News/25.0.0'; + + $this->fetcherConfig->expects($this->once()) + ->method('getUserAgent') + ->willReturn($userAgent); + + $response = $this->createMock(IResponse::class); + $response->expects($this->once()) + ->method('getBody') + ->willReturn('favicon-body'); + + $this->client->expects($this->once()) + ->method('get') + ->with($url, [ + 'timeout' => 10, + 'allow_redirects' => false, + 'http_errors' => false, + 'headers' => [ + 'User-Agent' => $userAgent, + ], + ]) + ->willReturn($response); + + $this->assertSame('favicon-body', $this->class->retrieveUrl($url)); + } + + public function testRetrieveUrlReturnsFalseOnClientError(): void + { + $url = 'https://example.com/favicon.ico'; + + $this->fetcherConfig->expects($this->once()) + ->method('getUserAgent') + ->willReturn('NextCloud-News/25.0.0'); + + $this->client->expects($this->once()) + ->method('get') + ->willThrowException(new \Exception('request failed')); + + $this->logger->expects($this->once()) + ->method('warning'); + + $this->assertFalse($this->class->retrieveUrl($url)); + } + + public function testRetrieveHeaderReturnsLowercaseHeadersOnSuccess(): void + { + $url = 'https://example.com/favicon.ico'; + $userAgent = 'NextCloud-News/25.0.0'; + + $this->fetcherConfig->expects($this->once()) + ->method('getUserAgent') + ->willReturn($userAgent); + + $response = $this->createMock(IResponse::class); + $response->expects($this->once()) + ->method('getHeaders') + ->willReturn([ + 'Content-Type' => ['image/x-icon'], + 'X-Test' => ['1'], + ]); + $response->expects($this->once()) + ->method('getStatusCode') + ->willReturn(200); + + $this->client->expects($this->once()) + ->method('head') + ->with($url, [ + 'timeout' => 10, + 'allow_redirects' => false, + 'http_errors' => false, + 'headers' => [ + 'User-Agent' => $userAgent, + ], + ]) + ->willReturn($response); + + $this->assertSame([ + 0 => 'HTTP/1.1 200', + 'content-type' => ['image/x-icon'], + 'x-test' => ['1'], + ], $this->class->retrieveHeader($url)); + } + + public function testRetrieveHeaderReturnsEmptyArrayOnClientError(): void + { + $url = 'https://example.com/favicon.ico'; + $userAgent = 'NextCloud-News/25.0.0'; + + $this->fetcherConfig->expects($this->once()) + ->method('getUserAgent') + ->willReturn($userAgent); + + $this->client->expects($this->once()) + ->method('head') + ->with($url, [ + 'timeout' => 10, + 'allow_redirects' => false, + 'http_errors' => false, + 'headers' => [ + 'User-Agent' => $userAgent, + ], + ]) + ->willThrowException(new \Exception('request failed')); + + $this->logger->expects($this->once()) + ->method('warning'); + + $this->assertSame([], $this->class->retrieveHeader($url)); + } +}