From 741c44385f388c01ba77b55401b2bec8bbbfe955 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 18 Mar 2022 18:07:24 +0100 Subject: Increase loglevel of Webcal parsing errors and modernize code Closes #31612 Signed-off-by: Thomas Citharel --- .../CalDAV/WebcalCaching/RefreshWebcalService.php | 50 +++++++--------------- .../WebcalCaching/RefreshWebcalServiceTest.php | 21 +++++---- 2 files changed, 26 insertions(+), 45 deletions(-) (limited to 'apps') diff --git a/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php b/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php index 49f66735345..57c9e3bf014 100644 --- a/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php +++ b/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php @@ -56,14 +56,15 @@ use function count; class RefreshWebcalService { /** @var CalDavBackend */ - private $calDavBackend; + private CalDavBackend $calDavBackend; /** @var IClientService */ - private $clientService; + private IClientService $clientService; /** @var IConfig */ - private $config; + private IConfig $config; + /** @var LoggerInterface */ private LoggerInterface $logger; public const REFRESH_RATE = '{http://apple.com/ns/ical/}refreshrate'; @@ -71,13 +72,7 @@ class RefreshWebcalService { public const STRIP_ATTACHMENTS = '{http://calendarserver.org/ns/}subscribed-strip-attachments'; public const STRIP_TODOS = '{http://calendarserver.org/ns/}subscribed-strip-todos'; - /** - * RefreshWebcalJob constructor. - */ - public function __construct(CalDavBackend $calDavBackend, - IClientService $clientService, - IConfig $config, - LoggerInterface $logger) { + public function __construct(CalDavBackend $calDavBackend, IClientService $clientService, IConfig $config, LoggerInterface $logger) { $this->calDavBackend = $calDavBackend; $this->clientService = $clientService; $this->config = $config; @@ -131,12 +126,12 @@ class RefreshWebcalService { continue; } - $uri = $this->getRandomCalendarObjectUri(); + $objectUri = $this->getRandomCalendarObjectUri(); $calendarData = $vObject->serialize(); try { - $this->calDavBackend->createCalendarObject($subscription['id'], $uri, $calendarData, CalDavBackend::CALENDAR_TYPE_SUBSCRIPTION); + $this->calDavBackend->createCalendarObject($subscription['id'], $objectUri, $calendarData, CalDavBackend::CALENDAR_TYPE_SUBSCRIPTION); } catch (NoInstancesException | BadRequest $ex) { - $this->logger->error($ex->getMessage(), ['exception' => $ex]); + $this->logger->error('Unable to create calendar object from subscription {subscriptionId}', ['exception' => $ex, 'subscriptionId' => $subscription['id'], 'source' => $subscription['source']]); } } @@ -147,20 +142,14 @@ class RefreshWebcalService { $this->updateSubscription($subscription, $mutations); } catch (ParseException $ex) { - $subscriptionId = $subscription['id']; - - $this->logger->error("Subscription $subscriptionId could not be refreshed due to a parsing error", ['exception' => $ex]); + $this->logger->error("Subscription {subscriptionId} could not be refreshed due to a parsing error", ['exception' => $ex, 'subscriptionId' => $subscription['id']]); } } /** * loads subscription from backend - * - * @param string $principalUri - * @param string $uri - * @return array|null */ - public function getSubscription(string $principalUri, string $uri) { + public function getSubscription(string $principalUri, string $uri): ?array { $subscriptions = array_values(array_filter( $this->calDavBackend->getSubscriptionsForUser($principalUri), function ($sub) use ($uri) { @@ -177,12 +166,8 @@ class RefreshWebcalService { /** * gets webcal feed from remote server - * - * @param array $subscription - * @param array &$mutations - * @return null|string */ - private function queryWebcalFeed(array $subscription, array &$mutations) { + private function queryWebcalFeed(array $subscription, array &$mutations): ?string { $client = $this->clientService->newClient(); $didBreak301Chain = false; @@ -244,7 +229,7 @@ class RefreshWebcalService { $jCalendar = Reader::readJson($body, Reader::OPTION_FORGIVING); } catch (Exception $ex) { // In case of a parsing error return null - $this->logger->debug("Subscription $subscriptionId could not be parsed"); + $this->logger->warning("Subscription $subscriptionId could not be parsed", ['exception' => $ex]); return null; } return $jCalendar->serialize(); @@ -254,7 +239,7 @@ class RefreshWebcalService { $xCalendar = Reader::readXML($body); } catch (Exception $ex) { // In case of a parsing error return null - $this->logger->debug("Subscription $subscriptionId could not be parsed"); + $this->logger->warning("Subscription $subscriptionId could not be parsed", ['exception' => $ex]); return null; } return $xCalendar->serialize(); @@ -265,7 +250,7 @@ class RefreshWebcalService { $vCalendar = Reader::read($body); } catch (Exception $ex) { // In case of a parsing error return null - $this->logger->debug("Subscription $subscriptionId could not be parsed"); + $this->logger->warning("Subscription $subscriptionId could not be parsed", ['exception' => $ex]); return null; } return $vCalendar->serialize(); @@ -291,11 +276,8 @@ class RefreshWebcalService { * - the webcal feed suggests a refreshrate * - return suggested refreshrate if user didn't set a custom one * - * @param array $subscription - * @param string $webcalData - * @return string|null */ - private function checkWebcalDataForRefreshRate($subscription, $webcalData) { + private function checkWebcalDataForRefreshRate(array $subscription, string $webcalData): ?string { // if there is no refreshrate stored in the database, check the webcal feed // whether it suggests any refresh rate and store that in the database if (isset($subscription[self::REFRESH_RATE]) && $subscription[self::REFRESH_RATE] !== null) { @@ -353,7 +335,7 @@ class RefreshWebcalService { * @param string $url * @return string|null */ - private function cleanURL(string $url) { + private function cleanURL(string $url): ?string { $parsed = parse_url($url); if ($parsed === false) { return null; diff --git a/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php b/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php index 6bd1f6b3206..71d93bf851e 100644 --- a/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php +++ b/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php @@ -74,7 +74,7 @@ class RefreshWebcalServiceTest extends TestCase { */ public function testRun(string $body, string $contentType, string $result) { $refreshWebcalService = $this->getMockBuilder(RefreshWebcalService::class) - ->setMethods(['getRandomCalendarObjectUri']) + ->onlyMethods(['getRandomCalendarObjectUri']) ->setConstructorArgs([$this->caldavBackend, $this->clientService, $this->config, $this->logger]) ->getMock(); @@ -156,7 +156,7 @@ class RefreshWebcalServiceTest extends TestCase { $client = $this->createMock(IClient::class); $response = $this->createMock(IResponse::class); $refreshWebcalService = $this->getMockBuilder(RefreshWebcalService::class) - ->setMethods(['getRandomCalendarObjectUri', 'getSubscription', 'queryWebcalFeed']) + ->onlyMethods(['getRandomCalendarObjectUri', 'getSubscription', 'queryWebcalFeed']) ->setConstructorArgs([$this->caldavBackend, $this->clientService, $this->config, $this->logger]) ->getMock(); @@ -217,7 +217,7 @@ class RefreshWebcalServiceTest extends TestCase { $this->logger->expects($this->once()) ->method('error') - ->with($noInstanceException->getMessage(), ['exception' => $noInstanceException]); + ->with('Unable to create calendar object from subscription {subscriptionId}', ['exception' => $noInstanceException, 'subscriptionId' => '42', 'source' => 'webcal://foo.bar/bla2']); $refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123'); } @@ -233,7 +233,7 @@ class RefreshWebcalServiceTest extends TestCase { $client = $this->createMock(IClient::class); $response = $this->createMock(IResponse::class); $refreshWebcalService = $this->getMockBuilder(RefreshWebcalService::class) - ->setMethods(['getRandomCalendarObjectUri', 'getSubscription', 'queryWebcalFeed']) + ->onlyMethods(['getRandomCalendarObjectUri', 'getSubscription', 'queryWebcalFeed']) ->setConstructorArgs([$this->caldavBackend, $this->clientService, $this->config, $this->logger]) ->getMock(); @@ -294,7 +294,7 @@ class RefreshWebcalServiceTest extends TestCase { $this->logger->expects($this->once()) ->method('error') - ->with($badRequestException->getMessage(), ['exception' => $badRequestException]); + ->with('Unable to create calendar object from subscription {subscriptionId}', ['exception' => $badRequestException, 'subscriptionId' => '42', 'source' => 'webcal://foo.bar/bla2']); $refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123'); } @@ -324,10 +324,8 @@ class RefreshWebcalServiceTest extends TestCase { /** * @dataProvider runLocalURLDataProvider - * - * @param string $source */ - public function testRunLocalURL($source) { + public function testRunLocalURL(string $source) { $refreshWebcalService = new RefreshWebcalService( $this->caldavBackend, $this->clientService, @@ -361,14 +359,15 @@ class RefreshWebcalServiceTest extends TestCase { ->with('dav', 'webcalAllowLocalAccess', 'no') ->willReturn('no'); - $exception = new LocalServerException(); + $localServerException = new LocalServerException(); + $client->expects($this->once()) ->method('get') - ->willThrowException($exception); + ->willThrowException($localServerException); $this->logger->expects($this->once()) ->method('warning') - ->with($this->anything(), ['exception' => $exception]); + ->with("Subscription 42 was not refreshed because it violates local access rules", ['exception' => $localServerException]); $refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123'); } -- cgit v1.2.3