Skip to content

Commit

Permalink
Increase loglevel of Webcal parsing errors and modernize code
Browse files Browse the repository at this point in the history 
Closes #31612

Signed-off-by: Thomas Citharel <[email protected]>
  • Loading branch information
@tcitworld
tcitworld committed Mar 18, 2022
1 parent 39b5c2c commit 2c7a883
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 64 deletions.
63 changes: 20 additions & 43 deletions apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@
use OCP\Http\Client\IClientService;
use OCP\Http\Client\LocalServerException;
use OCP\IConfig;
use OCP\ILogger;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Log\LoggerInterface;
use Sabre\DAV\Exception\BadRequest;
use Sabre\DAV\PropPatch;
use Sabre\DAV\Xml\Property\Href;
Expand All @@ -56,31 +56,23 @@
class RefreshWebcalService {

/** @var CalDavBackend */
private $calDavBackend;
private CalDavBackend $calDavBackend;

/** @var IClientService */
private $clientService;
private IClientService $clientService;

/** @var IConfig */
private $config;
private IConfig $config;

/** @var ILogger */
private $logger;
/** @var LoggerInterface */
private LoggerInterface $logger;

public const REFRESH_RATE = '{http://apple.com/ns/ical/}refreshrate';
public const STRIP_ALARMS = '{http://calendarserver.org/ns/}subscribed-strip-alarms';
public const STRIP_ATTACHMENTS = '{http://calendarserver.org/ns/}subscribed-strip-attachments';
public const STRIP_TODOS = '{http://calendarserver.org/ns/}subscribed-strip-todos';

/**
* RefreshWebcalJob constructor.
*
* @param CalDavBackend $calDavBackend
* @param IClientService $clientService
* @param IConfig $config
* @param ILogger $logger
*/
public function __construct(CalDavBackend $calDavBackend, IClientService $clientService, IConfig $config, ILogger $logger) {
public function __construct(CalDavBackend $calDavBackend, IClientService $clientService, IConfig $config, LoggerInterface $logger) {
$this->calDavBackend = $calDavBackend;
$this->clientService = $clientService;
$this->config = $config;
Expand Down Expand Up @@ -143,7 +135,7 @@ public function refreshSubscription(string $principalUri, string $uri) {
try {
$this->calDavBackend->createCalendarObject($subscription['id'], $uri, $calendarData, CalDavBackend::CALENDAR_TYPE_SUBSCRIPTION);
} catch (NoInstancesException | BadRequest $ex) {
$this->logger->logException($ex);
$this->logger->error('Unable to create calendar object from subscription', ['exception' => $ex]);
}
}

Expand All @@ -155,20 +147,14 @@ public function refreshSubscription(string $principalUri, string $uri) {
$this->updateSubscription($subscription, $mutations);
} catch (ParseException $ex) {
$subscriptionId = $subscription['id'];

$this->logger->logException($ex);
$this->logger->warning("Subscription $subscriptionId could not be refreshed due to a parsing error");
$this->logger->warning("Subscription $subscriptionId could not be refreshed due to a parsing error", ['exception' => $ex]);
}
}

/**
* 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) {
Expand All @@ -185,12 +171,8 @@ function ($sub) use ($uri) {

/**
* 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;
Expand Down Expand Up @@ -252,7 +234,7 @@ private function queryWebcalFeed(array $subscription, array &$mutations) {
$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();
Expand All @@ -262,7 +244,7 @@ private function queryWebcalFeed(array $subscription, array &$mutations) {
$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();
Expand All @@ -273,22 +255,20 @@ private function queryWebcalFeed(array $subscription, array &$mutations) {
$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();
}
} catch (LocalServerException $ex) {
$this->logger->logException($ex, [
'message' => "Subscription $subscriptionId was not refreshed because it violates local access rules",
'level' => ILogger::WARN,
$this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules", [
'exception' => $ex,
]);

return null;
} catch (Exception $ex) {
$this->logger->logException($ex, [
'message' => "Subscription $subscriptionId could not be refreshed due to a network error",
'level' => ILogger::WARN,
$this->logger->warning("Subscription $subscriptionId could not be refreshed due to a network error", [
'exception' => $ex,
]);

return null;
Expand All @@ -301,11 +281,8 @@ private function queryWebcalFeed(array $subscription, array &$mutations) {
* - 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) {
Expand Down Expand Up @@ -363,7 +340,7 @@ private function updateSubscription(array $subscription, array $mutations) {
* @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;
Expand Down
42 changes: 21 additions & 21 deletions apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
*/
namespace OCA\DAV\Tests\unit\BackgroundJob;

use Psr\Log\LoggerInterface;
use GuzzleHttp\HandlerStack;
use OCA\DAV\CalDAV\CalDavBackend;
use OCA\DAV\CalDAV\WebcalCaching\RefreshWebcalService;
Expand All @@ -34,7 +35,6 @@
use OCP\Http\Client\IResponse;
use OCP\Http\Client\LocalServerException;
use OCP\IConfig;
use OCP\ILogger;
use PHPUnit\Framework\MockObject\MockObject;
use Sabre\DAV\Exception\BadRequest;
use Sabre\VObject;
Expand All @@ -53,7 +53,7 @@ class RefreshWebcalServiceTest extends TestCase {
/** @var IConfig | MockObject */
private $config;

/** @var ILogger | MockObject */
/** @var LoggerInterface | MockObject */
private $logger;

protected function setUp(): void {
Expand All @@ -62,7 +62,7 @@ protected function setUp(): void {
$this->caldavBackend = $this->createMock(CalDavBackend::class);
$this->clientService = $this->createMock(IClientService::class);
$this->config = $this->createMock(IConfig::class);
$this->logger = $this->createMock(ILogger::class);
$this->logger = $this->createMock(LoggerInterface::class);
}

/**
Expand All @@ -74,7 +74,7 @@ protected function setUp(): void {
*/
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();

Expand Down Expand Up @@ -144,7 +144,7 @@ public function testRun(string $body, string $contentType, string $result) {

$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
}

/**
* @param string $body
* @param string $contentType
Expand All @@ -156,7 +156,7 @@ public function testRunCreateCalendarNoException(string $body, string $contentTy
$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();

Expand Down Expand Up @@ -209,15 +209,15 @@ public function testRunCreateCalendarNoException(string $body, string $contentTy
$this->caldavBackend->expects($this->once())
->method('createCalendarObject')
->with(42, 'uri-1.ics', $result, 1);

$noInstanceException = new NoInstancesException("can't add calendar object");
$this->caldavBackend->expects($this->once())
->method("createCalendarObject")
->willThrowException($noInstanceException);

$this->logger->expects($this->once())
->method('logException')
->with($noInstanceException);
->method('error')
->with('Unable to create calendar object from subscription', ['exception' => $noInstanceException]);

$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
}
Expand All @@ -233,7 +233,7 @@ public function testRunCreateCalendarBadRequest(string $body, string $contentTyp
$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();

Expand Down Expand Up @@ -286,15 +286,15 @@ public function testRunCreateCalendarBadRequest(string $body, string $contentTyp
$this->caldavBackend->expects($this->once())
->method('createCalendarObject')
->with(42, 'uri-1.ics', $result, 1);

$badRequestException = new BadRequest("can't add reach calendar url");
$this->caldavBackend->expects($this->once())
->method("createCalendarObject")
->willThrowException($badRequestException);

$this->logger->expects($this->once())
->method('logException')
->with($badRequestException);
->method('error')
->with('Unable to create calendar object from subscription', ['exception' => $badRequestException]);

$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
}
Expand Down Expand Up @@ -324,10 +324,8 @@ public function runDataProvider():array {

/**
* @dataProvider runLocalURLDataProvider
*
* @param string $source
*/
public function testRunLocalURL($source) {
public function testRunLocalURL(string $source) {
$refreshWebcalService = new RefreshWebcalService(
$this->caldavBackend,
$this->clientService,
Expand Down Expand Up @@ -361,13 +359,15 @@ public function testRunLocalURL($source) {
->with('dav', 'webcalAllowLocalAccess', 'no')
->willReturn('no');

$localServerException = new LocalServerException();

$client->expects($this->once())
->method('get')
->willThrowException(new LocalServerException());
->willThrowException($localServerException);

$this->logger->expects($this->once())
->method('logException')
->with($this->isInstanceOf(LocalServerException::class), $this->anything());
->method('warning')
->with("Subscription 42 was not refreshed because it violates local access rules", ['exception' => $localServerException]);

$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
}
Expand Down

0 comments on commit 2c7a883

Please sign in to comment.