Add error logging to federated sharing handshake

5 files changed, 37 insertions, 10 deletions

diff --git a/apps/federation/api/ocsauthapi.php b/apps/federation/api/ocsauthapi.php
index d165a0bd22f..b94550fd4f2 100644
--- a/apps/federation/api/ocsauthapi.php
+++ b/apps/federation/api/ocsauthapi.php
@@ -26,6 +26,7 @@ use OCA\Federation\DbHandler;
 use OCA\Federation\TrustedServers;
 use OCP\AppFramework\Http;
 use OCP\BackgroundJob\IJobList;
+use OCP\ILogger;
 use OCP\IRequest;
 use OCP\Security\ISecureRandom;
 use OCP\Security\StringUtils;
@@ -54,6 +55,9 @@ class OCSAuthAPI {
 	/** @var DbHandler */
 	private $dbHandler;
 
+	/** @var ILogger */
+	private $logger;
+
 	/**
 	 * OCSAuthAPI constructor.
 	 *
@@ -62,19 +66,22 @@ class OCSAuthAPI {
 	 * @param IJobList $jobList
 	 * @param TrustedServers $trustedServers
 	 * @param DbHandler $dbHandler
+	 * @param ILogger $logger
 	 */
 	public function __construct(
 		IRequest $request,
 		ISecureRandom $secureRandom,
 		IJobList $jobList,
 		TrustedServers $trustedServers,
-		DbHandler $dbHandler
+		DbHandler $dbHandler,
+		ILogger $logger
 	) {
 		$this->request = $request;
 		$this->secureRandom = $secureRandom;
 		$this->jobList = $jobList;
 		$this->trustedServers = $trustedServers;
 		$this->dbHandler = $dbHandler;
+		$this->logger = $logger;
 	}
 
 	/**
@@ -88,6 +95,7 @@ class OCSAuthAPI {
 		$token = $this->request->getParam('token');
 
 		if ($this->trustedServers->isTrustedServer($url) === false) {
+			$this->logger->log(\OCP\Util::ERROR, 'remote server not trusted (' . $url . ') while requesting shared secret');
 			return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN);
 		}
 
@@ -95,6 +103,7 @@ class OCSAuthAPI {
 		// token wins
 		$localToken = $this->dbHandler->getToken($url);
 		if (strcmp($localToken, $token) > 0) {
+			$this->logger->log(\OCP\Util::ERROR, 'remote server (' . $url . ') presented lower token');
 			return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN);
 		}
 
@@ -120,10 +129,13 @@ class OCSAuthAPI {
 		$url = $this->request->getParam('url');
 		$token = $this->request->getParam('token');
 
-		if (
-			$this->trustedServers->isTrustedServer($url) === false
-			|| $this->isValidToken($url, $token) === false
-		) {
+		if ($this->trustedServers->isTrustedServer($url) === false) {
+			$this->logger->log(\OCP\Util::ERROR, 'remote server not trusted (' . $url . ') while getting shared secret');
+			return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN);
+		}
+
+		if ($this->isValidToken($url, $token) === false) {
+			$this->logger->log(\OCP\Util::ERROR, 'remote server (' . $url . ') didn\'t send a valid token (got ' . $token . ') while getting shared secret');
 			return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN);
 		}
 
diff --git a/apps/federation/appinfo/application.php b/apps/federation/appinfo/application.php
index 172283536b4..45d88548b70 100644
--- a/apps/federation/appinfo/application.php
+++ b/apps/federation/appinfo/application.php
@@ -108,7 +108,8 @@ class Application extends \OCP\AppFramework\App {
 			$server->getSecureRandom(),
 			$server->getJobList(),
 			$container->query('TrustedServers'),
-			$container->query('DbHandler')
+			$container->query('DbHandler'),
+			$server->getLogger()
 
 		);
 
diff --git a/apps/federation/backgroundjob/getsharedsecret.php b/apps/federation/backgroundjob/getsharedsecret.php
index eb55fa2d6ab..8aa8a08e07b 100644
--- a/apps/federation/backgroundjob/getsharedsecret.php
+++ b/apps/federation/backgroundjob/getsharedsecret.php
@@ -91,7 +91,7 @@ class GetSharedSecret extends QueuedJob{
 			$this->trustedServers = new TrustedServers(
 					$this->dbHandler,
 					\OC::$server->getHTTPClientService(),
-					\OC::$server->getLogger(),
+					$this->logger,
 					$this->jobList,
 					\OC::$server->getSecureRandom(),
 					\OC::$server->getConfig()
@@ -148,6 +148,7 @@ class GetSharedSecret extends QueuedJob{
 
 		} catch (ClientException $e) {
 			$status = $e->getCode();
+			$this->logger->logException($e);
 		}
 
 		// if we received a unexpected response we try again later
diff --git a/apps/federation/backgroundjob/requestsharedsecret.php b/apps/federation/backgroundjob/requestsharedsecret.php
index 24d8adada11..a1906d20823 100644
--- a/apps/federation/backgroundjob/requestsharedsecret.php
+++ b/apps/federation/backgroundjob/requestsharedsecret.php
@@ -60,6 +60,9 @@ class RequestSharedSecret extends QueuedJob {
 
 	private $endPoint = '/ocs/v2.php/apps/federation/api/v1/request-shared-secret?format=json';
 
+	/** @var ILogger */
+	private $logger;
+
 	/**
 	 * RequestSharedSecret constructor.
 	 *
@@ -80,13 +83,14 @@ class RequestSharedSecret extends QueuedJob {
 		$this->jobList = $jobList ? $jobList : \OC::$server->getJobList();
 		$this->urlGenerator = $urlGenerator ? $urlGenerator : \OC::$server->getURLGenerator();
 		$this->dbHandler = $dbHandler ? $dbHandler : new DbHandler(\OC::$server->getDatabaseConnection(), \OC::$server->getL10N('federation'));
+		$this->logger = \OC::$server->getLogger();
 		if ($trustedServers) {
 			$this->trustedServers = $trustedServers;
 		} else {
 			$this->trustedServers = new TrustedServers(
 				$this->dbHandler,
 				\OC::$server->getHTTPClientService(),
-				\OC::$server->getLogger(),
+				$this->logger,
 				$this->jobList,
 				\OC::$server->getSecureRandom(),
 				\OC::$server->getConfig()
@@ -142,6 +146,7 @@ class RequestSharedSecret extends QueuedJob {
 
 		} catch (ClientException $e) {
 			$status = $e->getCode();
+			$this->logger->logException($e);
 		}
 
 		// if we received a unexpected response we try again later
diff --git a/apps/federation/tests/api/ocsauthapitest.php b/apps/federation/tests/api/ocsauthapitest.php
index a334686c24e..e6a95af8585 100644
--- a/apps/federation/tests/api/ocsauthapitest.php
+++ b/apps/federation/tests/api/ocsauthapitest.php
@@ -28,6 +28,7 @@ use OCA\Federation\API\OCSAuthAPI;
 use OCA\Federation\DbHandler;
 use OCA\Federation\TrustedServers;
 use OCP\AppFramework\Http;
+use OCP\ILogger;
 use OCP\IRequest;
 use OCP\Security\ISecureRandom;
 use Test\TestCase;
@@ -49,6 +50,9 @@ class OCSAuthAPITest extends TestCase {
 	/** @var \PHPUnit_Framework_MockObject_MockObject | DbHandler */
 	private $dbHandler;
 
+	/** @var \PHPUnit_Framework_MockObject_MockObject | ILogger */
+	private $logger;
+
 	/** @var  OCSAuthApi */
 	private $ocsAuthApi;
 
@@ -63,13 +67,16 @@ class OCSAuthAPITest extends TestCase {
 			->disableOriginalConstructor()->getMock();
 		$this->jobList = $this->getMockBuilder('OC\BackgroundJob\JobList')
 			->disableOriginalConstructor()->getMock();
+		$this->logger = $this->getMockBuilder('OCP\ILogger')
+			->disableOriginalConstructor()->getMock();
 
 		$this->ocsAuthApi = new OCSAuthAPI(
 			$this->request,
 			$this->secureRandom,
 			$this->jobList,
 			$this->trustedServers,
-			$this->dbHandler
+			$this->dbHandler,
+			$this->logger
 		);
 
 	}
@@ -136,7 +143,8 @@ class OCSAuthAPITest extends TestCase {
 					$this->secureRandom,
 					$this->jobList,
 					$this->trustedServers,
-					$this->dbHandler
+					$this->dbHandler,
+					$this->logger
 				]
 			)->setMethods(['isValidToken'])->getMock();