From 8670df2bc509ce3065c550689085ac9860a177d8 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Thu, 13 Jun 2024 18:27:53 +0200 Subject: [PATCH] refactor(dav): migrate to new http client The request method is available since 29 and thus we can finally use the modern http client to send the report request for the addressbook sync. Signed-off-by: Daniel Kesselberg --- apps/dav/lib/CardDAV/SyncService.php | 85 ++-- .../tests/unit/CardDAV/SyncServiceTest.php | 373 +++++++++++++++--- 2 files changed, 355 insertions(+), 103 deletions(-) diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php index 01747a9b105..c32704b30d2 100644 --- a/apps/dav/lib/CardDAV/SyncService.php +++ b/apps/dav/lib/CardDAV/SyncService.php @@ -32,14 +32,14 @@ namespace OCA\DAV\CardDAV; use OCP\AppFramework\Db\TTransactional; use OCP\AppFramework\Http; +use OCP\Http\Client\IClientService; use OCP\IDBConnection; use OCP\IUser; use OCP\IUserManager; +use Psr\Http\Client\ClientExceptionInterface; use Psr\Log\LoggerInterface; -use Sabre\DAV\Client; use Sabre\DAV\Xml\Response\MultiStatus; use Sabre\DAV\Xml\Service; -use Sabre\HTTP\ClientHttpException; use Sabre\VObject\Reader; use function is_null; @@ -54,18 +54,21 @@ class SyncService { private ?array $localSystemAddressBook = null; private Converter $converter; protected string $certPath; + private IClientService $clientService; public function __construct(CardDavBackend $backend, IUserManager $userManager, IDBConnection $dbConnection, LoggerInterface $logger, - Converter $converter) { + Converter $converter, + IClientService $clientService) { $this->backend = $backend; $this->userManager = $userManager; $this->logger = $logger; $this->converter = $converter; $this->certPath = ''; $this->dbConnection = $dbConnection; + $this->clientService = $clientService; } /** @@ -79,7 +82,7 @@ class SyncService { // 2. query changes try { $response = $this->requestSyncReport($url, $userName, $addressBookUrl, $sharedSecret, $syncToken); - } catch (ClientHttpException $ex) { + } catch (ClientExceptionInterface $ex) { if ($ex->getCode() === Http::STATUS_UNAUTHORIZED) { // remote server revoked access to the address book, remove it $this->backend->deleteAddressBook($addressBookId); @@ -99,9 +102,9 @@ class SyncService { $this->atomic(function () use ($addressBookId, $cardUri, $vCard) { $existingCard = $this->backend->getCard($addressBookId, $cardUri); if ($existingCard === false) { - $this->backend->createCard($addressBookId, $cardUri, $vCard['body']); + $this->backend->createCard($addressBookId, $cardUri, $vCard); } else { - $this->backend->updateCard($addressBookId, $cardUri, $vCard['body']); + $this->backend->updateCard($addressBookId, $cardUri, $vCard); } }, $this->dbConnection); } else { @@ -128,56 +131,50 @@ class SyncService { } /** - * Check if there is a valid certPath we should use + * @throws ClientExceptionInterface */ - protected function getCertPath(): string { - - // we already have a valid certPath - if ($this->certPath !== '') { - return $this->certPath; - } - - $certManager = \OC::$server->getCertificateManager(); - $certPath = $certManager->getAbsoluteBundlePath(); - if (file_exists($certPath)) { - $this->certPath = $certPath; - } + protected function requestSyncReport(string $url, string $userName, string $addressBookUrl, string $sharedSecret, ?string $syncToken): array { + $client = $this->clientService->newClient(); - return $this->certPath; - } + // the trailing slash is important for merging base_uri and uri + $url = rtrim($url, '/') . '/'; - protected function getClient(string $url, string $userName, string $sharedSecret): Client { - $settings = [ - 'baseUri' => $url . '/', - 'userName' => $userName, - 'password' => $sharedSecret, + $options = [ + 'auth' => [$userName, $sharedSecret], + 'base_uri' => $url, + 'body' => $this->buildSyncCollectionRequestBody($syncToken), + 'headers' => ['Content-Type' => 'application/xml'] ]; - $client = new Client($settings); - $certPath = $this->getCertPath(); - $client->setThrowExceptions(true); - if ($certPath !== '' && !str_starts_with($url, 'http://')) { - $client->addCurlSetting(CURLOPT_CAINFO, $this->certPath); - } + $response = $client->request( + 'REPORT', + $addressBookUrl, + $options + ); + + $body = $response->getBody(); + assert(is_string($body)); - return $client; + return $this->parseMultiStatus($body); } - protected function requestSyncReport(string $url, string $userName, string $addressBookUrl, string $sharedSecret, ?string $syncToken): array { - $client = $this->getClient($url, $userName, $sharedSecret); + protected function download(string $url, string $userName, string $sharedSecret, string $resourcePath): string { + $client = $this->clientService->newClient(); - $body = $this->buildSyncCollectionRequestBody($syncToken); + // the trailing slash is important for merging base_uri and uri + $url = rtrim($url, '/') . '/'; - $response = $client->request('REPORT', $addressBookUrl, $body, [ - 'Content-Type' => 'application/xml' - ]); + $options = [ + 'auth' => [$userName, $sharedSecret], + 'base_uri' => $url, + ]; - return $this->parseMultiStatus($response['body']); - } + $response = $client->get( + $resourcePath, + $options + ); - protected function download(string $url, string $userName, string $sharedSecret, string $resourcePath): array { - $client = $this->getClient($url, $userName, $sharedSecret); - return $client->request('GET', $resourcePath); + return (string)$response->getBody(); } private function buildSyncCollectionRequestBody(?string $syncToken): string { diff --git a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php index c031f8cbb97..d928201bfc7 100644 --- a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php +++ b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php @@ -27,49 +27,285 @@ */ namespace OCA\DAV\Tests\unit\CardDAV; +use GuzzleHttp\Exception\ClientException; +use GuzzleHttp\Psr7\Request as PsrRequest; +use GuzzleHttp\Psr7\Response as PsrResponse; +use OC\Http\Client\Response; use OCA\DAV\CardDAV\CardDavBackend; use OCA\DAV\CardDAV\Converter; use OCA\DAV\CardDAV\SyncService; +use OCP\Http\Client\IClient; +use OCP\Http\Client\IClientService; use OCP\IDBConnection; use OCP\IUser; use OCP\IUserManager; +use Psr\Http\Client\ClientExceptionInterface; use Psr\Log\LoggerInterface; +use Psr\Log\NullLogger; use Sabre\VObject\Component\VCard; use Test\TestCase; class SyncServiceTest extends TestCase { + + protected CardDavBackend $backend; + protected IUserManager $userManager; + protected IDBConnection $dbConnection; + protected LoggerInterface $logger; + protected Converter $converter; + protected IClient $client; + protected SyncService $service; + public function setUp(): void { + $addressBook = [ + 'id' => 1, + 'uri' => 'system', + 'principaluri' => 'principals/system/system', + '{DAV:}displayname' => 'system', + // watch out, incomplete address book mock. + ]; + + $this->backend = $this->createMock(CardDavBackend::class); + $this->backend->method('getAddressBooksByUri') + ->with('principals/system/system', 1) + ->willReturn($addressBook); + + $this->userManager = $this->createMock(IUserManager::class); + $this->dbConnection = $this->createMock(IDBConnection::class); + $this->logger = new NullLogger(); + $this->converter = $this->createMock(Converter::class); + $this->client = $this->createMock(IClient::class); + + $clientService = $this->createMock(IClientService::class); + $clientService->method('newClient') + ->willReturn($this->client); + + $this->service = new SyncService( + $this->backend, + $this->userManager, + $this->dbConnection, + $this->logger, + $this->converter, + $clientService + ); + } + public function testEmptySync(): void { - $backend = $this->getBackendMock(0, 0, 0); + $this->backend->expects($this->exactly(0)) + ->method('createCard'); + $this->backend->expects($this->exactly(0)) + ->method('updateCard'); + $this->backend->expects($this->exactly(0)) + ->method('deleteCard'); + + $body = ' + + http://sabre.io/ns/sync/1 +'; + + $requestResponse = new Response(new PsrResponse( + 207, + ['Content-Type' => 'application/xml; charset=utf-8', 'Content-Length' => strlen($body)], + $body + )); - $ss = $this->getSyncServiceMock($backend, []); - $return = $ss->syncRemoteAddressBook('', 'system', 'system', '1234567890', null, '1', 'principals/system/system', []); - $this->assertEquals('sync-token-1', $return); + $this->client + ->method('request') + ->willReturn($requestResponse); + + $token = $this->service->syncRemoteAddressBook( + '', + 'system', + 'system', + '1234567890', + null, + '1', + 'principals/system/system', + [] + ); + + $this->assertEquals('http://sabre.io/ns/sync/1', $token); } public function testSyncWithNewElement(): void { - $backend = $this->getBackendMock(1, 0, 0); - $backend->method('getCard')->willReturn(false); + $this->backend->expects($this->exactly(1)) + ->method('createCard'); + $this->backend->expects($this->exactly(0)) + ->method('updateCard'); + $this->backend->expects($this->exactly(0)) + ->method('deleteCard'); + + $this->backend->method('getCard') + ->willReturn(false); - $ss = $this->getSyncServiceMock($backend, ['0' => [200 => '']]); - $return = $ss->syncRemoteAddressBook('', 'system', 'system', '1234567890', null, '1', 'principals/system/system', []); - $this->assertEquals('sync-token-1', $return); + + $body = ' + + + /remote.php/dav/addressbooks/system/system/system/Database:alice.vcf + + + text/vcard; charset=utf-8 + "2df155fa5c2a24cd7f750353fc63f037" + + HTTP/1.1 200 OK + + + http://sabre.io/ns/sync/2 +'; + + $reportResponse = new Response(new PsrResponse( + 207, + ['Content-Type' => 'application/xml; charset=utf-8', 'Content-Length' => strlen($body)], + $body + )); + + $this->client + ->method('request') + ->willReturn($reportResponse); + + $vCard = 'BEGIN:VCARD +VERSION:3.0 +PRODID:-//Sabre//Sabre VObject 4.5.4//EN +UID:alice +FN;X-NC-SCOPE=v2-federated:alice +N;X-NC-SCOPE=v2-federated:alice;;;; +X-SOCIALPROFILE;TYPE=NEXTCLOUD;X-NC-SCOPE=v2-published:https://server2.internal/index.php/u/alice +CLOUD:alice@server2.internal +END:VCARD'; + + $getResponse = new Response(new PsrResponse( + 200, + ['Content-Type' => 'text/vcard; charset=utf-8', 'Content-Length' => strlen($vCard)], + $vCard, + )); + + $this->client + ->method('get') + ->willReturn($getResponse); + + $token = $this->service->syncRemoteAddressBook( + '', + 'system', + 'system', + '1234567890', + null, + '1', + 'principals/system/system', + [] + ); + + $this->assertEquals('http://sabre.io/ns/sync/2', $token); } public function testSyncWithUpdatedElement(): void { - $backend = $this->getBackendMock(0, 1, 0); - $backend->method('getCard')->willReturn(true); + $this->backend->expects($this->exactly(0)) + ->method('createCard'); + $this->backend->expects($this->exactly(1)) + ->method('updateCard'); + $this->backend->expects($this->exactly(0)) + ->method('deleteCard'); + + $this->backend->method('getCard') + ->willReturn(true); + + + $body = ' + + + /remote.php/dav/addressbooks/system/system/system/Database:alice.vcf + + + text/vcard; charset=utf-8 + "2df155fa5c2a24cd7f750353fc63f037" + + HTTP/1.1 200 OK + + + http://sabre.io/ns/sync/3 +'; + + $reportResponse = new Response(new PsrResponse( + 207, + ['Content-Type' => 'application/xml; charset=utf-8', 'Content-Length' => strlen($body)], + $body + )); + + $this->client + ->method('request') + ->willReturn($reportResponse); - $ss = $this->getSyncServiceMock($backend, ['0' => [200 => '']]); - $return = $ss->syncRemoteAddressBook('', 'system', 'system', '1234567890', null, '1', 'principals/system/system', []); - $this->assertEquals('sync-token-1', $return); + $vCard = 'BEGIN:VCARD +VERSION:3.0 +PRODID:-//Sabre//Sabre VObject 4.5.4//EN +UID:alice +FN;X-NC-SCOPE=v2-federated:alice +N;X-NC-SCOPE=v2-federated:alice;;;; +X-SOCIALPROFILE;TYPE=NEXTCLOUD;X-NC-SCOPE=v2-published:https://server2.internal/index.php/u/alice +CLOUD:alice@server2.internal +END:VCARD'; + + $getResponse = new Response(new PsrResponse( + 200, + ['Content-Type' => 'text/vcard; charset=utf-8', 'Content-Length' => strlen($vCard)], + $vCard, + )); + + $this->client + ->method('get') + ->willReturn($getResponse); + + $token = $this->service->syncRemoteAddressBook( + '', + 'system', + 'system', + '1234567890', + null, + '1', + 'principals/system/system', + [] + ); + + $this->assertEquals('http://sabre.io/ns/sync/3', $token); } public function testSyncWithDeletedElement(): void { - $backend = $this->getBackendMock(0, 0, 1); + $this->backend->expects($this->exactly(0)) + ->method('createCard'); + $this->backend->expects($this->exactly(0)) + ->method('updateCard'); + $this->backend->expects($this->exactly(1)) + ->method('deleteCard'); + + $body = ' + + + /remote.php/dav/addressbooks/system/system/system/Database:alice.vcf + HTTP/1.1 404 Not Found + +http://sabre.io/ns/sync/4 +'; + + $reportResponse = new Response(new PsrResponse( + 207, + ['Content-Type' => 'application/xml; charset=utf-8', 'Content-Length' => strlen($body)], + $body + )); + + $this->client + ->method('request') + ->willReturn($reportResponse); - $ss = $this->getSyncServiceMock($backend, ['0' => [404 => '']]); - $return = $ss->syncRemoteAddressBook('', 'system', 'system', '1234567890', null, '1', 'principals/system/system', []); - $this->assertEquals('sync-token-1', $return); + $token = $this->service->syncRemoteAddressBook( + '', + 'system', + 'system', + '1234567890', + null, + '1', + 'principals/system/system', + [] + ); + + $this->assertEquals('http://sabre.io/ns/sync/4', $token); } public function testEnsureSystemAddressBookExists(): void { @@ -88,8 +324,9 @@ class SyncServiceTest extends TestCase { $dbConnection = $this->createMock(IDBConnection::class); $logger = $this->getMockBuilder(LoggerInterface::class)->disableOriginalConstructor()->getMock(); $converter = $this->createMock(Converter::class); + $clientService = $this->createMock(IClientService::class); - $ss = new SyncService($backend, $userManager, $dbConnection, $logger, $converter); + $ss = new SyncService($backend, $userManager, $dbConnection, $logger, $converter, $clientService); $ss->ensureSystemAddressBookExists('principals/users/adam', 'contacts', []); } @@ -142,7 +379,9 @@ class SyncServiceTest extends TestCase { ->method('createCardFromUser') ->willReturn($this->createMock(VCard::class)); - $ss = new SyncService($backend, $userManager, $dbConnection, $logger, $converter); + $clientService = $this->createMock(IClientService::class); + + $ss = new SyncService($backend, $userManager, $dbConnection, $logger, $converter, $clientService); $ss->updateUser($user); $ss->updateUser($user); @@ -150,45 +389,61 @@ class SyncServiceTest extends TestCase { $ss->deleteUser($user); } - /** - * @param int $createCount - * @param int $updateCount - * @param int $deleteCount - * @return \PHPUnit\Framework\MockObject\MockObject|CardDavBackend - */ - private function getBackendMock($createCount, $updateCount, $deleteCount) { - $backend = $this->getMockBuilder(CardDavBackend::class) - ->disableOriginalConstructor() - ->getMock(); - $backend->expects($this->exactly($createCount))->method('createCard'); - $backend->expects($this->exactly($updateCount))->method('updateCard'); - $backend->expects($this->exactly($deleteCount))->method('deleteCard'); - return $backend; - } + public function testDeleteAddressbookWhenAccessRevoked(): void { + $this->expectException(ClientExceptionInterface::class); - /** - * @param $backend - * @param $response - * @return SyncService|\PHPUnit\Framework\MockObject\MockObject - */ - private function getSyncServiceMock($backend, $response) { - $userManager = $this->getMockBuilder(IUserManager::class)->disableOriginalConstructor()->getMock(); - $dbConnection = $this->createMock(IDBConnection::class); - $logger = $this->getMockBuilder(LoggerInterface::class)->disableOriginalConstructor()->getMock(); - $converter = $this->createMock(Converter::class); - /** @var SyncService | \PHPUnit\Framework\MockObject\MockObject $ss */ - $ss = $this->getMockBuilder(SyncService::class) - ->setMethods(['ensureSystemAddressBookExists', 'requestSyncReport', 'download', 'getCertPath']) - ->setConstructorArgs([$backend, $userManager, $dbConnection, $logger, $converter]) - ->getMock(); - $ss->method('requestSyncReport')->withAnyParameters()->willReturn(['response' => $response, 'token' => 'sync-token-1']); - $ss->method('ensureSystemAddressBookExists')->willReturn(['id' => 1]); - $ss->method('download')->willReturn([ - 'body' => '', - 'statusCode' => 200, - 'headers' => [] - ]); - $ss->method('getCertPath')->willReturn(''); - return $ss; + $this->backend->expects($this->exactly(0)) + ->method('createCard'); + $this->backend->expects($this->exactly(0)) + ->method('updateCard'); + $this->backend->expects($this->exactly(0)) + ->method('deleteCard'); + $this->backend->expects($this->exactly(1)) + ->method('deleteAddressBook'); + + $request = new PsrRequest( + 'REPORT', + 'https://server2.internal/remote.php/dav/addressbooks/system/system/system', + ['Content-Type' => 'application/xml'], + ); + + $body = ' + + Sabre\DAV\Exception\NotAuthenticated + No public access to this resource., Username or password was incorrect, No \'Authorization: Bearer\' header found. Either the client didn\'t send one, or the server is mis-configured, Username or password was incorrect +'; + + $response = new PsrResponse( + 401, + ['Content-Type' => 'application/xml; charset=utf-8', 'Content-Length' => strlen($body)], + $body + ); + + $message = 'Client error: `REPORT https://server2.internal/cloud/remote.php/dav/addressbooks/system/system/system` resulted in a `401 Unauthorized` response: + + + Sabre\DA (truncated...) +'; + + $reportException = new ClientException( + $message, + $request, + $response + ); + + $this->client + ->method('request') + ->willThrowException($reportException); + + $this->service->syncRemoteAddressBook( + '', + 'system', + 'system', + '1234567890', + null, + '1', + 'principals/system/system', + [] + ); } } -- 2.39.5