Merge pull request #3825 from nextcloud/federation-fixes

create correct VCard and return correct error codes

6 files changed, 121 insertions, 65 deletions

diff --git a/apps/dav/lib/CardDAV/Converter.php b/apps/dav/lib/CardDAV/Converter.php
index ba0c4c2a2d5..9e106a0fe28 100644
--- a/apps/dav/lib/CardDAV/Converter.php
+++ b/apps/dav/lib/CardDAV/Converter.php
@@ -55,14 +55,21 @@ class Converter {
 		$image = $this->getAvatarImage($user);
 
 		$vCard = new VCard();
-		$vCard->add(new Text($vCard, 'UID', $uid));
+		$vCard->VERSION = '3.0';
+		$vCard->UID = $uid;
 
 		$publish = false;
 
 		foreach ($userData as $property => $value) {
-			if ($value['scope'] === AccountManager::VISIBILITY_CONTACTS_ONLY ||
-				$value['scope'] === AccountManager::VISIBILITY_PUBLIC
-			) {
+
+			$shareWithTrustedServers =
+				$value['scope'] === AccountManager::VISIBILITY_CONTACTS_ONLY ||
+				$value['scope'] === AccountManager::VISIBILITY_PUBLIC;
+
+			$emptyValue = !isset($value['value']) || $value['value'] === '';
+			$noImage = $image === null;
+
+			if ($shareWithTrustedServers && (!$emptyValue || !$noImage)) {
 				$publish = true;
 				switch ($property) {
 					case AccountManager::PROPERTY_DISPLAYNAME:
@@ -71,7 +78,7 @@ class Converter {
 						break;
 					case AccountManager::PROPERTY_AVATAR:
 						if ($image !== null) {
-							$vCard->add('PHOTO', 'data:'.$image->mimeType().';base64,' . base64_encode($image->data()));
+							$vCard->add('PHOTO', $image->data(), ['ENCODING' => 'b', 'TYPE' => $image->mimeType()]);
 						}
 						break;
 					case AccountManager::PROPERTY_EMAIL:
diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php
index 1293d8ae8a0..477e912a797 100644
--- a/apps/dav/lib/CardDAV/SyncService.php
+++ b/apps/dav/lib/CardDAV/SyncService.php
@@ -26,6 +26,7 @@ namespace OCA\DAV\CardDAV;
 
 use OC\Accounts\AccountManager;
 use OCP\AppFramework\Http;
+use OCP\ICertificateManager;
 use OCP\ILogger;
 use OCP\IUser;
 use OCP\IUserManager;
@@ -52,6 +53,9 @@ class SyncService {
 	/** @var AccountManager */
 	private $accountManager;
 
+	/** @var string */
+	protected $certPath;
+
 	/**
 	 * SyncService constructor.
 	 *
@@ -65,6 +69,7 @@ class SyncService {
 		$this->userManager = $userManager;
 		$this->logger = $logger;
 		$this->accountManager = $accountManager;
+		$this->certPath = '';
 	}
 
 	/**
@@ -133,21 +138,60 @@ class SyncService {
 	}
 
 	/**
+	 * Check if there is a valid certPath we should use
+	 *
+	 * @return string
+	 */
+	protected function getCertPath() {
+
+		// we already have a valid certPath
+		if ($this->certPath !== '') {
+			return $this->certPath;
+		}
+
+		/** @var ICertificateManager $certManager */
+		$certManager = \OC::$server->getCertificateManager(null);
+		$certPath = $certManager->getAbsoluteBundlePath();
+		if (file_exists($certPath)) {
+			$this->certPath = $certPath;
+		}
+
+		return $this->certPath;
+	}
+
+	/**
 	 * @param string $url
 	 * @param string $userName
 	 * @param string $sharedSecret
-	 * @param string $syncToken
-	 * @return array
+	 * @return Client
 	 */
-	protected function requestSyncReport($url, $userName, $sharedSecret, $syncToken) {
+	protected function getClient($url, $userName, $sharedSecret) {
 		$settings = [
 			'baseUri' => $url . '/',
 			'userName' => $userName,
 			'password' => $sharedSecret,
 		];
 		$client = new Client($settings);
+		$certPath = $this->getCertPath();
 		$client->setThrowExceptions(true);
 
+		if ($certPath !== '' && strpos($url, 'http://') !== 0) {
+			$client->addCurlSetting(CURLOPT_CAINFO, $this->certPath);
+		}
+
+		return $client;
+	}
+
+	/**
+	 * @param string $url
+	 * @param string $userName
+	 * @param string $sharedSecret
+	 * @param string $syncToken
+	 * @return array
+	 */
+	protected function requestSyncReport($url, $userName, $sharedSecret, $syncToken) {
+		$client = $this->getClient($url, $userName, $sharedSecret);
+
 		$addressBookUrl = "remote.php/dav/addressbooks/system/system/system";
 		$body = $this->buildSyncCollectionRequestBody($syncToken);
 
@@ -155,9 +199,7 @@ class SyncService {
 			'Content-Type' => 'application/xml'
 		]);
 
-		$result = $this->parseMultiStatus($response['body']);
-
-		return $result;
+		return $this->parseMultiStatus($response['body']);
 	}
 
 	/**
@@ -167,16 +209,8 @@ class SyncService {
 	 * @return array
 	 */
 	protected function download($url, $sharedSecret, $resourcePath) {
-		$settings = [
-			'baseUri' => $url,
-			'userName' => 'system',
-			'password' => $sharedSecret,
-		];
-		$client = new Client($settings);
-		$client->setThrowExceptions(true);
-
-		$response = $client->request('GET', $resourcePath);
-		return $response;
+		$client = $this->getClient($url, 'system', $sharedSecret);
+		return $client->request('GET', $resourcePath);
 	}
 
 	/**
diff --git a/apps/dav/tests/unit/CardDAV/ConverterTest.php b/apps/dav/tests/unit/CardDAV/ConverterTest.php
index 528b3aa9ef4..448d80f3070 100644
--- a/apps/dav/tests/unit/CardDAV/ConverterTest.php
+++ b/apps/dav/tests/unit/CardDAV/ConverterTest.php
@@ -146,7 +146,7 @@ class ConverterTest extends  TestCase {
 				[
 					'cloud' => 'foo@cloud.net',
 					'email' => 'foo@bar.net',
-					'photo' => 'data:image/jpeg;base64,MTIzNDU2Nzg5',
+					'photo' => 'MTIzNDU2Nzg5',
 				],
 				null,
 				'foo@bar.net',
@@ -157,7 +157,7 @@ class ConverterTest extends  TestCase {
 					'cloud' => 'foo@cloud.net',
 					'email' => 'foo@bar.net',
 					'fn' => 'Dr. Foo Bar',
-					'photo' => 'data:image/jpeg;base64,MTIzNDU2Nzg5',
+					'photo' => 'MTIzNDU2Nzg5',
 				],
 				"Dr. Foo Bar",
 				"foo@bar.net",
@@ -167,12 +167,22 @@ class ConverterTest extends  TestCase {
 				[
 					'cloud' => 'foo@cloud.net',
 					'fn' => 'Dr. Foo Bar',
-					'photo' => 'data:image/jpeg;base64,MTIzNDU2Nzg5',
+					'photo' => 'MTIzNDU2Nzg5',
 				],
 				"Dr. Foo Bar",
 				null,
 				"foo@cloud.net"
 			],
+			[
+				[
+					'cloud' => 'foo@cloud.net',
+					'fn' => 'Dr. Foo Bar',
+					'photo' => 'MTIzNDU2Nzg5',
+				],
+				'Dr. Foo Bar',
+				'',
+				'foo@cloud.net'
+			],
 		];
 	}
 
diff --git a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php
index 68345def66b..c06e4857743 100644
--- a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php
+++ b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php
@@ -103,50 +103,50 @@ class SyncServiceTest extends TestCase {
 		$user->method('getBackendClassName')->willReturn('unittest');
 		$user->method('getUID')->willReturn('test-user');
 		$user->method('getCloudId')->willReturn('cloudId');
+		$user->method('getDisplayName')->willReturn('test-user');
 		$accountManager = $this->getMockBuilder('OC\Accounts\AccountManager')->disableOriginalConstructor()->getMock();
 		$accountManager->expects($this->any())->method('getUser')
 			->willReturn([
-				AccountManager::PROPERTY_DISPLAYNAME =>
-					[
-						'value' => $user->getDisplayName(),
-						'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY,
-					],
-				AccountManager::PROPERTY_ADDRESS =>
-					[
-						'value' => '',
-						'scope' => AccountManager::VISIBILITY_PRIVATE,
-					],
-				AccountManager::PROPERTY_WEBSITE =>
-					[
-						'value' => '',
-						'scope' => AccountManager::VISIBILITY_PRIVATE,
-					],
-				AccountManager::PROPERTY_EMAIL =>
-					[
-						'value' => $user->getEMailAddress(),
-						'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY,
-					],
-				AccountManager::PROPERTY_AVATAR =>
-					[
-						'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY
-					],
-				AccountManager::PROPERTY_PHONE =>
-					[
-						'value' => '',
-						'scope' => AccountManager::VISIBILITY_PRIVATE,
-					],
-				AccountManager::PROPERTY_TWITTER =>
-					[
-						'value' => '',
-						'scope' => AccountManager::VISIBILITY_PRIVATE,
-					],
-			]);
+					AccountManager::PROPERTY_DISPLAYNAME =>
+						[
+							'value' => $user->getDisplayName(),
+							'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY,
+						],
+					AccountManager::PROPERTY_ADDRESS =>
+						[
+							'value' => '',
+							'scope' => AccountManager::VISIBILITY_PRIVATE,
+						],
+					AccountManager::PROPERTY_WEBSITE =>
+						[
+							'value' => '',
+							'scope' => AccountManager::VISIBILITY_PRIVATE,
+						],
+					AccountManager::PROPERTY_EMAIL =>
+						[
+							'value' => $user->getEMailAddress(),
+							'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY,
+						],
+					AccountManager::PROPERTY_AVATAR =>
+						[
+							'scope' => AccountManager::VISIBILITY_CONTACTS_ONLY
+						],
+					AccountManager::PROPERTY_PHONE =>
+						[
+							'value' => '',
+							'scope' => AccountManager::VISIBILITY_PRIVATE,
+						],
+					AccountManager::PROPERTY_TWITTER =>
+						[
+							'value' => '',
+							'scope' => AccountManager::VISIBILITY_PRIVATE,
+						],
+				]
+			);
 
 		$ss = new SyncService($backend, $userManager, $logger, $accountManager);
 		$ss->updateUser($user);
 
-		$user->method('getDisplayName')->willReturn('A test user for unit testing');
-
 		$ss->updateUser($user);
 
 		$ss->deleteUser($user);
@@ -179,7 +179,7 @@ class SyncServiceTest extends TestCase {
 		$accountManager = $this->getMockBuilder('OC\Accounts\AccountManager')->disableOriginalConstructor()->getMock();
 		/** @var SyncService | \PHPUnit_Framework_MockObject_MockObject $ss */
 		$ss = $this->getMockBuilder(SyncService::class)
-			->setMethods(['ensureSystemAddressBookExists', 'requestSyncReport', 'download'])
+			->setMethods(['ensureSystemAddressBookExists', 'requestSyncReport', 'download', 'getCertPath'])
 			->setConstructorArgs([$backend, $userManager, $logger, $accountManager])
 			->getMock();
 		$ss->method('requestSyncReport')->withAnyParameters()->willReturn(['response' => $response, 'token' => 'sync-token-1']);
@@ -189,6 +189,7 @@ class SyncServiceTest extends TestCase {
 			'statusCode' => 200,
 			'headers' => []
 		]);
+		$ss->method('getCertPath')->willReturn('');
 		return $ss;
 	}
 
diff --git a/apps/federation/lib/Middleware/AddServerMiddleware.php b/apps/federation/lib/Middleware/AddServerMiddleware.php
index d920aee3e4e..71e517f6b87 100644
--- a/apps/federation/lib/Middleware/AddServerMiddleware.php
+++ b/apps/federation/lib/Middleware/AddServerMiddleware.php
@@ -25,6 +25,7 @@
 namespace OCA\Federation\Middleware;
 
 use OC\HintException;
+use OCA\Federation\Controller\SettingsController;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Middleware;
@@ -57,6 +58,9 @@ class AddServerMiddleware extends Middleware {
 	 * @return JSONResponse
 	 */
 	public function afterException($controller, $methodName, \Exception $exception) {
+		if (($controller instanceof SettingsController) === false) {
+			throw $exception;
+		}
 		$this->logger->error($exception->getMessage(), ['app' => $this->appName]);
 		if ($exception instanceof HintException) {
 			$message = $exception->getHint();
diff --git a/apps/federation/tests/Middleware/AddServerMiddlewareTest.php b/apps/federation/tests/Middleware/AddServerMiddlewareTest.php
index b2096cb3730..6c502c66f5f 100644
--- a/apps/federation/tests/Middleware/AddServerMiddlewareTest.php
+++ b/apps/federation/tests/Middleware/AddServerMiddlewareTest.php
@@ -26,8 +26,8 @@ namespace OCA\Federation\Tests\Middleware;
 
 
 use OC\HintException;
+use OCA\Federation\Controller\SettingsController;
 use OCA\Federation\Middleware\AddServerMiddleware;
-use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
 use OCP\IL10N;
 use OCP\ILogger;
@@ -44,7 +44,7 @@ class AddServerMiddlewareTest extends TestCase {
 	/** @var  AddServerMiddleware */
 	private $middleware;
 
-	/** @var  \PHPUnit_Framework_MockObject_MockObject | Controller */
+	/** @var  \PHPUnit_Framework_MockObject_MockObject | SettingsController */
 	private $controller;
 
 	public function setUp() {
@@ -52,7 +52,7 @@ class AddServerMiddlewareTest extends TestCase {
 
 		$this->logger = $this->getMockBuilder(ILogger::class)->getMock();
 		$this->l10n = $this->getMockBuilder(IL10N::class)->getMock();
-		$this->controller = $this->getMockBuilder(Controller::class)
+		$this->controller = $this->getMockBuilder(SettingsController::class)
 			->disableOriginalConstructor()->getMock();
 
 		$this->middleware = new AddServerMiddleware(