fix(caldav): add missing handlers

Signed-off-by: Anna Larch <anna@nextcloud.com>
nextcloud · Oct 1, 2024 · e862c88 · e862c88
1 parent da8ef3c
commit e862c88
Show file tree

Hide file tree

Showing 2 changed files with 81 additions and 121 deletions.
diff --git a/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php b/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php
@@ -31,18 +31,14 @@
 namespace OCA\DAV\CalDAV\WebcalCaching;
 
 use Exception;
-use GuzzleHttp\HandlerStack;
-use GuzzleHttp\Middleware;
+use GuzzleHttp\RequestOptions;
 use OCA\DAV\CalDAV\CalDavBackend;
 use OCP\Http\Client\IClientService;
 use OCP\Http\Client\LocalServerException;
-use OCP\IConfig;
-use Psr\Http\Message\RequestInterface;
-use Psr\Http\Message\ResponseInterface;
+use OCP\IAppConfig;
 use Psr\Log\LoggerInterface;
 use Sabre\DAV\Exception\BadRequest;
 use Sabre\DAV\PropPatch;
-use Sabre\DAV\Xml\Property\Href;
 use Sabre\VObject\Component;
 use Sabre\VObject\DateTimeParser;
 use Sabre\VObject\InvalidDataException;
@@ -59,7 +55,7 @@ class RefreshWebcalService {
 
 	private IClientService $clientService;
 
-	private IConfig $config;
+	private IAppConfig $config;
 
 	/** @var LoggerInterface */
 	private LoggerInterface $logger;
@@ -69,22 +65,22 @@ class RefreshWebcalService {
 	public const STRIP_ATTACHMENTS = '{http://calendarserver.org/ns/}subscribed-strip-attachments';
 	public const STRIP_TODOS = '{http://calendarserver.org/ns/}subscribed-strip-todos';
 
-	public function __construct(CalDavBackend $calDavBackend, IClientService $clientService, IConfig $config, LoggerInterface $logger) {
+	public function __construct(CalDavBackend $calDavBackend, IClientService $clientService, IAppConfig $config, LoggerInterface $logger) {
 		$this->calDavBackend = $calDavBackend;
 		$this->clientService = $clientService;
 		$this->config = $config;
 		$this->logger = $logger;
 	}
 
-	public function refreshSubscription(string $principalUri, string $uri) {
+	public function refreshSubscription(string $principalUri, string $uri): void {
 		$subscription = $this->getSubscription($principalUri, $uri);
 		$mutations = [];
 		if (!$subscription) {
 			return;
 		}
 
 		$webcalData = $this->queryWebcalFeed($subscription, $mutations);
-		if (!$webcalData) {
+		if ($webcalData === null) {
 			return;
 		}
 
@@ -127,7 +123,7 @@ public function refreshSubscription(string $principalUri, string $uri) {
 				$calendarData = $vObject->serialize();
 				try {
 					$this->calDavBackend->createCalendarObject($subscription['id'], $objectUri, $calendarData, CalDavBackend::CALENDAR_TYPE_SUBSCRIPTION);
-				} catch (NoInstancesException | BadRequest $ex) {
+				} catch (NoInstancesException|BadRequest $ex) {
 					$this->logger->error('Unable to create calendar object from subscription {subscriptionId}', ['exception' => $ex, 'subscriptionId' => $subscription['id'], 'source' => $subscription['source']]);
 				}
 			}
@@ -139,7 +135,7 @@ public function refreshSubscription(string $principalUri, string $uri) {
 
 			$this->updateSubscription($subscription, $mutations);
 		} catch (ParseException $ex) {
-			$this->logger->error("Subscription {subscriptionId} could not be refreshed due to a parsing error", ['exception' => $ex, 'subscriptionId' => $subscription['id']]);
+			$this->logger->error('Subscription {subscriptionId} could not be refreshed due to a parsing error', ['exception' => $ex, 'subscriptionId' => $subscription['id']]);
 		}
 	}
 
@@ -165,106 +161,81 @@ function ($sub) use ($uri) {
 	 * gets webcal feed from remote server
 	 */
 	private function queryWebcalFeed(array $subscription, array &$mutations): ?string {
-		$client = $this->clientService->newClient();
-
-		$didBreak301Chain = false;
-		$latestLocation = null;
-
-		$handlerStack = HandlerStack::create();
-		$handlerStack->push(Middleware::mapRequest(function (RequestInterface $request) {
-			return $request
-				->withHeader('Accept', 'text/calendar, application/calendar+json, application/calendar+xml')
-				->withHeader('User-Agent', 'Nextcloud Webcal Service');
-		}));
-		$handlerStack->push(Middleware::mapResponse(function (ResponseInterface $response) use (&$didBreak301Chain, &$latestLocation) {
-			if (!$didBreak301Chain) {
-				if ($response->getStatusCode() !== 301) {
-					$didBreak301Chain = true;
-				} else {
-					$latestLocation = $response->getHeader('Location');
-				}
-			}
-			return $response;
-		}));
-
-		$allowLocalAccess = $this->config->getAppValue('dav', 'webcalAllowLocalAccess', 'no');
 		$subscriptionId = $subscription['id'];
 		$url = $this->cleanURL($subscription['source']);
 		if ($url === null) {
 			return null;
 		}
 
-		try {
-			$params = [
-				'allow_redirects' => [
-					'redirects' => 10
-				],
-				'handler' => $handlerStack,
-				'nextcloud' => [
-					'allow_local_address' => $allowLocalAccess === 'yes',
-				]
-			];
-
-			$user = parse_url($subscription['source'], PHP_URL_USER);
-			$pass = parse_url($subscription['source'], PHP_URL_PASS);
-			if ($user !== null && $pass !== null) {
-				$params['auth'] = [$user, $pass];
-			}
+		$allowLocalAccess = $this->config->getValueString('dav', 'webcalAllowLocalAccess', 'no');
+
+		$params = [
+			'nextcloud' => [
+				'allow_local_address' => $allowLocalAccess === 'yes',
+			],
+			RequestOptions::HEADERS => [
+				'User-Agent' => 'Nextcloud Webcal Service',
+				'Accept' => 'text/calendar, application/calendar+json, application/calendar+xml',
+			],
+		];
+
+		$user = parse_url($subscription['source'], PHP_URL_USER);
+		$pass = parse_url($subscription['source'], PHP_URL_PASS);
+		if ($user !== null && $pass !== null) {
+			$params[RequestOptions::AUTH] = [$user, $pass];
+		}
 
+		try {
+			$client = $this->clientService->newClient();
 			$response = $client->get($url, $params);
-			$body = $response->getBody();
-
-			if ($latestLocation) {
-				$mutations['{http://calendarserver.org/ns/}source'] = new Href($latestLocation);
-			}
-
-			$contentType = $response->getHeader('Content-Type');
-			$contentType = explode(';', $contentType, 2)[0];
-			switch ($contentType) {
-				case 'application/calendar+json':
-					try {
-						$jCalendar = Reader::readJson($body, Reader::OPTION_FORGIVING);
-					} catch (Exception $ex) {
-						// In case of a parsing error return null
-						$this->logger->warning("Subscription $subscriptionId could not be parsed", ['exception' => $ex]);
-						return null;
-					}
-					return $jCalendar->serialize();
-
-				case 'application/calendar+xml':
-					try {
-						$xCalendar = Reader::readXML($body);
-					} catch (Exception $ex) {
-						// In case of a parsing error return null
-						$this->logger->warning("Subscription $subscriptionId could not be parsed", ['exception' => $ex]);
-						return null;
-					}
-					return $xCalendar->serialize();
-
-				case 'text/calendar':
-				default:
-					try {
-						$vCalendar = Reader::read($body);
-					} catch (Exception $ex) {
-						// In case of a parsing error return null
-						$this->logger->warning("Subscription $subscriptionId could not be parsed", ['exception' => $ex]);
-						return null;
-					}
-					return $vCalendar->serialize();
-			}
 		} catch (LocalServerException $ex) {
 			$this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules", [
 				'exception' => $ex,
 			]);
-
 			return null;
 		} catch (Exception $ex) {
 			$this->logger->warning("Subscription $subscriptionId could not be refreshed due to a network error", [
 				'exception' => $ex,
 			]);
-
 			return null;
 		}
+
+		$body = $response->getBody();
+
+		$contentType = $response->getHeader('Content-Type');
+		$contentType = explode(';', $contentType, 2)[0];
+		switch ($contentType) {
+			case 'application/calendar+json':
+				try {
+					$jCalendar = Reader::readJson($body, Reader::OPTION_FORGIVING);
+				} catch (Exception $ex) {
+					// In case of a parsing error return null
+					$this->logger->warning("Subscription $subscriptionId could not be parsed", ['exception' => $ex]);
+					return null;
+				}
+				return $jCalendar->serialize();
+
+			case 'application/calendar+xml':
+				try {
+					$xCalendar = Reader::readXML($body);
+				} catch (Exception $ex) {
+					// In case of a parsing error return null
+					$this->logger->warning("Subscription $subscriptionId could not be parsed", ['exception' => $ex]);
+					return null;
+				}
+				return $xCalendar->serialize();
+
+			case 'text/calendar':
+			default:
+				try {
+					$vCalendar = Reader::read($body);
+				} catch (Exception $ex) {
+					// In case of a parsing error return null
+					$this->logger->warning("Subscription $subscriptionId could not be parsed", ['exception' => $ex]);
+					return null;
+				}
+				return $vCalendar->serialize();
+		}
 	}
 
 	/**

diff --git a/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php b/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php
@@ -26,14 +26,13 @@
  */
 namespace OCA\DAV\Tests\unit\CalDAV\WebcalCaching;
 
-use GuzzleHttp\HandlerStack;
 use OCA\DAV\CalDAV\CalDavBackend;
 use OCA\DAV\CalDAV\WebcalCaching\RefreshWebcalService;
 use OCP\Http\Client\IClient;
 use OCP\Http\Client\IClientService;
 use OCP\Http\Client\IResponse;
 use OCP\Http\Client\LocalServerException;
-use OCP\IConfig;
+use OCP\IAppConfig;
 use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 use Sabre\DAV\Exception\BadRequest;
@@ -50,7 +49,7 @@ class RefreshWebcalServiceTest extends TestCase {
 	/** @var IClientService | MockObject */
 	private $clientService;
 
-	/** @var IConfig | MockObject */
+	/** @var IAppConfig | MockObject */
 	private $config;
 
 	/** @var LoggerInterface | MockObject */
@@ -61,7 +60,7 @@ protected function setUp(): void {
 
 		$this->caldavBackend = $this->createMock(CalDavBackend::class);
 		$this->clientService = $this->createMock(IClientService::class);
-		$this->config = $this->createMock(IConfig::class);
+		$this->config = $this->createMock(IAppConfig::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 	}
 
@@ -114,15 +113,13 @@ public function testRun(string $body, string $contentType, string $result): void
 			->willReturn($client);
 
 		$this->config->expects($this->once())
-			->method('getAppValue')
+			->method('getValueString')
 			->with('dav', 'webcalAllowLocalAccess', 'no')
 			->willReturn('no');
 
 		$client->expects($this->once())
 			->method('get')
-			->with('https://foo.bar/bla2', $this->callback(function ($obj) {
-				return $obj['allow_redirects']['redirects'] === 10 && $obj['handler'] instanceof HandlerStack;
-			}))
+			->with('https://foo.bar/bla2')
 			->willReturn($response);
 
 		$response->expects($this->once())
@@ -182,15 +179,13 @@ public function testRunCreateCalendarNoException(string $body, string $contentTy
 			->willReturn($client);
 
 		$this->config->expects($this->once())
-			->method('getAppValue')
+			->method('getValueString')
 			->with('dav', 'webcalAllowLocalAccess', 'no')
 			->willReturn('no');
 
 		$client->expects($this->once())
 			->method('get')
-			->with('https://foo.bar/bla2', $this->callback(function ($obj) {
-				return $obj['allow_redirects']['redirects'] === 10 && $obj['handler'] instanceof HandlerStack;
-			}))
+			->with('https://foo.bar/bla2')
 			->willReturn($response);
 
 		$response->expects($this->once())
@@ -212,7 +207,7 @@ public function testRunCreateCalendarNoException(string $body, string $contentTy
 
 		$noInstanceException = new NoInstancesException("can't add calendar object");
 		$this->caldavBackend->expects($this->once())
-			->method("createCalendarObject")
+			->method('createCalendarObject')
 			->willThrowException($noInstanceException);
 
 		$this->logger->expects($this->once())
@@ -259,15 +254,13 @@ public function testRunCreateCalendarBadRequest(string $body, string $contentTyp
 			->willReturn($client);
 
 		$this->config->expects($this->once())
-			->method('getAppValue')
+			->method('getValueString')
 			->with('dav', 'webcalAllowLocalAccess', 'no')
 			->willReturn('no');
 
 		$client->expects($this->once())
 			->method('get')
-			->with('https://foo.bar/bla2', $this->callback(function ($obj) {
-				return $obj['allow_redirects']['redirects'] === 10 && $obj['handler'] instanceof HandlerStack;
-			}))
+			->with('https://foo.bar/bla2')
 			->willReturn($response);
 
 		$response->expects($this->once())
@@ -289,7 +282,7 @@ public function testRunCreateCalendarBadRequest(string $body, string $contentTyp
 
 		$badRequestException = new BadRequest("can't add reach calendar url");
 		$this->caldavBackend->expects($this->once())
-			->method("createCalendarObject")
+			->method('createCalendarObject')
 			->willThrowException($badRequestException);
 
 		$this->logger->expects($this->once())
@@ -355,7 +348,7 @@ public function testRunLocalURL(string $source): void {
 			->willReturn($client);
 
 		$this->config->expects($this->once())
-			->method('getAppValue')
+			->method('getValueString')
 			->with('dav', 'webcalAllowLocalAccess', 'no')
 			->willReturn('no');
 
@@ -367,7 +360,7 @@ public function testRunLocalURL(string $source): void {
 
 		$this->logger->expects($this->once())
 			->method('warning')
-			->with("Subscription 42 was not refreshed because it violates local access rules", ['exception' => $localServerException]);
+			->with('Subscription 42 was not refreshed because it violates local access rules', ['exception' => $localServerException]);
 
 		$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
 	}
@@ -411,15 +404,11 @@ public function testInvalidUrl(): void {
 			]);
 
 		$client = $this->createMock(IClient::class);
-		$this->clientService->expects($this->once())
-			->method('newClient')
-			->with()
-			->willReturn($client);
+		$this->clientService->expects($this->never())
+			->method('newClient');
 
-		$this->config->expects($this->once())
-			->method('getAppValue')
-			->with('dav', 'webcalAllowLocalAccess', 'no')
-			->willReturn('no');
+		$this->config->expects($this->never())
+			->method('getValueString');
 
 		$client->expects($this->never())
 			->method('get');