Merge pull request #22403 from owncloud/improved-error-handling

Remove background job if the server accepted to ask for the shared secret

5 files changed, 41 insertions, 9 deletions

diff --git a/apps/federation/api/ocsauthapi.php b/apps/federation/api/ocsauthapi.php
index 058a5966374..1c4e73cc8de 100644
--- a/apps/federation/api/ocsauthapi.php
+++ b/apps/federation/api/ocsauthapi.php
@@ -33,7 +33,6 @@ use OCP\BackgroundJob\IJobList;
 use OCP\ILogger;
 use OCP\IRequest;
 use OCP\Security\ISecureRandom;
-use OCP\Security\StringUtils;
 
 /**
  * Class OCSAuthAPI
@@ -99,7 +98,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', ['app' => 'federation']);
+			$this->logger->error('remote server not trusted (' . $url . ') while requesting shared secret', ['app' => 'federation']);
 			return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN);
 		}
 
@@ -107,10 +106,22 @@ 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', ['app' => 'federation']);
+			$this->logger->info(
+				'remote server (' . $url . ') presented lower token. We will initiate the exchange of the shared secret.',
+				['app' => 'federation']
+			);
 			return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN);
 		}
 
+		// we ask for the shared secret so we no longer have to ask the other server
+		// to request the shared secret
+		$this->jobList->remove('OCA\Federation\BackgroundJob\RequestSharedSecret',
+			[
+				'url' => $url,
+				'token' => $localToken
+			]
+		);
+
 		$this->jobList->add(
 			'OCA\Federation\BackgroundJob\GetSharedSecret',
 			[
@@ -134,12 +145,16 @@ 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 getting shared secret', ['app' => 'federation']);
+			$this->logger->error('remote server not trusted (' . $url . ') while getting shared secret', ['app' => 'federation']);
 			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', ['app' => 'federation']);
+			$expectedToken = $this->dbHandler->getToken($url);
+			$this->logger->error(
+				'remote server (' . $url . ') didn\'t send a valid token (got "' . $token . '" but expected "'. $expectedToken . '") while getting shared secret',
+				['app' => 'federation']
+			);
 			return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN);
 		}
 
diff --git a/apps/federation/backgroundjob/getsharedsecret.php b/apps/federation/backgroundjob/getsharedsecret.php
index a98a17e323b..ebc106ba94e 100644
--- a/apps/federation/backgroundjob/getsharedsecret.php
+++ b/apps/federation/backgroundjob/getsharedsecret.php
@@ -150,10 +150,14 @@ class GetSharedSecret extends QueuedJob{
 
 		} catch (ClientException $e) {
 			$status = $e->getCode();
-			$this->logger->logException($e);
+			if ($status === Http::STATUS_FORBIDDEN) {
+				$this->logger->info($target . ' refused to exchange a shared secret with you.', ['app' => 'federation']);
+			} else {
+				$this->logger->logException($e, ['app' => 'federation']);
+			}
 		} catch (\Exception $e) {
 			$status = HTTP::STATUS_INTERNAL_SERVER_ERROR;
-			$this->logger->logException($e);
+			$this->logger->logException($e, ['app' => 'federation']);
 		}
 
 		// 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 2db5d09545a..302711af27f 100644
--- a/apps/federation/backgroundjob/requestsharedsecret.php
+++ b/apps/federation/backgroundjob/requestsharedsecret.php
@@ -148,10 +148,14 @@ class RequestSharedSecret extends QueuedJob {
 
 		} catch (ClientException $e) {
 			$status = $e->getCode();
-			$this->logger->logException($e);
+			if ($status === Http::STATUS_FORBIDDEN) {
+				$this->logger->info($target . ' refused to ask for a shared secret.', ['app' => 'federation']);
+			} else {
+				$this->logger->logException($e, ['app' => 'federation']);
+			}
 		} catch (\Exception $e) {
 			$status = HTTP::STATUS_INTERNAL_SERVER_ERROR;
-			$this->logger->logException($e);
+			$this->logger->logException($e, ['app' => 'federation']);
 		}
 
 		// if we received a unexpected response we try again later
diff --git a/apps/federation/lib/dbhandler.php b/apps/federation/lib/dbhandler.php
index 9e44c72cc42..3ea84baa3eb 100644
--- a/apps/federation/lib/dbhandler.php
+++ b/apps/federation/lib/dbhandler.php
@@ -156,6 +156,7 @@ class DbHandler {
 	 *
 	 * @param string $url
 	 * @return string
+	 * @throws \Exception
 	 */
 	public function getToken($url) {
 		$hash = $this->hash($url);
@@ -165,6 +166,11 @@ class DbHandler {
 			->setParameter('url_hash', $hash);
 
 		$result = $query->execute()->fetch();
+
+		if (!isset($result['token'])) {
+			throw new \Exception('No token found for: ' . $url);
+		}
+
 		return $result['token'];
 	}
 
diff --git a/apps/federation/tests/api/ocsauthapitest.php b/apps/federation/tests/api/ocsauthapitest.php
index 9c751fff895..d3e61c0641a 100644
--- a/apps/federation/tests/api/ocsauthapitest.php
+++ b/apps/federation/tests/api/ocsauthapitest.php
@@ -105,8 +105,11 @@ class OCSAuthAPITest extends TestCase {
 		if ($expected === Http::STATUS_OK) {
 			$this->jobList->expects($this->once())->method('add')
 				->with('OCA\Federation\BackgroundJob\GetSharedSecret', ['url' => $url, 'token' => $token]);
+			$this->jobList->expects($this->once())->method('remove')
+				->with('OCA\Federation\BackgroundJob\RequestSharedSecret', ['url' => $url, 'token' => $localToken]);
 		} else {
 			$this->jobList->expects($this->never())->method('add');
+			$this->jobList->expects($this->never())->method('remove');
 		}
 
 		$result = $this->ocsAuthApi->requestSharedSecret();