Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase loglevel of Webcal parsing errors and modernize code #31622

Merged
merged 1 commit into from
Jun 10, 2022
Merged

Increase loglevel of Webcal parsing errors and modernize code #31622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 16 additions & 34 deletions apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,28 +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 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.
*/
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;
Expand Down Expand Up @@ -131,12 +126,12 @@ public function refreshSubscription(string $principalUri, string $uri) {
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']]);
}
}

Expand All @@ -147,20 +142,14 @@ public function refreshSubscription(string $principalUri, string $uri) {

$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) {
Expand All @@ -177,12 +166,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 @@ -244,7 +229,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 @@ -254,7 +239,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 @@ -265,7 +250,7 @@ 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();
Expand All @@ -291,11 +276,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 @@ -353,7 +335,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
21 changes: 10 additions & 11 deletions apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 @@ -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 @@ -217,7 +217,7 @@ public function testRunCreateCalendarNoException(string $body, string $contentTy

$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');
}
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 @@ -294,7 +294,7 @@ public function testRunCreateCalendarBadRequest(string $body, string $contentTyp

$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');
}
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,14 +359,15 @@ public function testRunLocalURL($source) {
->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');
}
Expand Down