Merge pull request #9256 from nextcloud/fed-sharing-improvements

improve error reporting and move format parameter to the options

apps/federation/lib/BackgroundJob/GetSharedSecret.php

@@ -35,7 +35,6 @@ use GuzzleHttp\Exception\RequestException;
 use GuzzleHttp\Ring\Exception\RingException;
 use OC\BackgroundJob\JobList;
 use OC\BackgroundJob\Job;
-use OCA\Federation\DbHandler;
 use OCA\Federation\TrustedServers;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Utility\ITimeFactory;
@@ -68,9 +67,6 @@ class GetSharedSecret extends Job {
 	/** @var TrustedServers  */
 	private $trustedServers;
 
-	/** @var DbHandler */
-	private $dbHandler;
-
 	/** @var IDiscoveryService  */
 	private $ocsDiscoveryService;
 
@@ -83,8 +79,6 @@ class GetSharedSecret extends Job {
 	/** @var bool */
 	protected $retainJob = false;
 
-	private $format = '?format=json';
-
 	private $defaultEndPoint = '/ocs/v2.php/apps/federation/api/v1/shared-secret';
 
 	/** @var  int  30 day = 2592000sec */
@@ -98,7 +92,6 @@ class GetSharedSecret extends Job {
 	 * @param IJobList $jobList
 	 * @param TrustedServers $trustedServers
 	 * @param ILogger $logger
-	 * @param DbHandler $dbHandler
 	 * @param IDiscoveryService $ocsDiscoveryService
 	 * @param ITimeFactory $timeFactory
 	 */
@@ -108,7 +101,6 @@ class GetSharedSecret extends Job {
 		IJobList $jobList,
 		TrustedServers $trustedServers,
 		ILogger $logger,
-		DbHandler $dbHandler,
 		IDiscoveryService $ocsDiscoveryService,
 		ITimeFactory $timeFactory
 	) {
@@ -116,7 +108,6 @@ class GetSharedSecret extends Job {
 		$this->httpClient = $httpClientService->newClient();
 		$this->jobList = $jobList;
 		$this->urlGenerator = $urlGenerator;
-		$this->dbHandler = $dbHandler;
 		$this->ocsDiscoveryService = $ocsDiscoveryService;
 		$this->trustedServers = $trustedServers;
 		$this->timeFactory = $timeFactory;
@@ -172,7 +163,7 @@ class GetSharedSecret extends Job {
 		$endPoint = isset($endPoints['shared-secret']) ? $endPoints['shared-secret'] : $this->defaultEndPoint;
 
 		// make sure that we have a well formatted url
-		$url = rtrim($target, '/') . '/' . trim($endPoint, '/') . $this->format;
+		$url = rtrim($target, '/') . '/' . trim($endPoint, '/');
 
 		$result = null;
 		try {
@@ -182,7 +173,8 @@ class GetSharedSecret extends Job {
 					'query' =>
 						[
 							'url' => $source,
-							'token' => $token
+							'token' => $token,
+							'format' => 'json',
 						],
 					'timeout' => 3,
 					'connect_timeout' => 3,
@@ -223,9 +215,6 @@ class GetSharedSecret extends Job {
 			&& $status !== Http::STATUS_FORBIDDEN
 		) {
 			$this->retainJob = true;
-		}  else {
-			// reset token if we received a valid response
-			$this->dbHandler->addToken($target, '');
 		}
 
 		if ($status === Http::STATUS_OK && $result instanceof IResponse) {
@@ -238,7 +227,7 @@ class GetSharedSecret extends Job {
 				);
 			} else {
 				$this->logger->error(
-						'remote server "' . $target . '"" does not return a valid shared secret',
+						'remote server "' . $target . '"" does not return a valid shared secret. Received data: ' . $body,
 						['app' => 'federation']
 				);
 				$this->trustedServers->setServerStatus($target, TrustedServers::STATUS_FAILURE);

apps/federation/lib/BackgroundJob/RequestSharedSecret.php

@@ -36,7 +36,6 @@ use GuzzleHttp\Exception\RequestException;
 use GuzzleHttp\Ring\Exception\RingException;
 use OC\BackgroundJob\JobList;
 use OC\BackgroundJob\Job;
-use OCA\Federation\DbHandler;
 use OCA\Federation\TrustedServers;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Utility\ITimeFactory;
@@ -65,9 +64,6 @@ class RequestSharedSecret extends Job {
 	/** @var IURLGenerator */
 	private $urlGenerator;
 
-	/** @var DbHandler */
-	private $dbHandler;
-
 	/** @var TrustedServers */
 	private $trustedServers;
 
@@ -83,8 +79,6 @@ class RequestSharedSecret extends Job {
 	/** @var bool */
 	protected $retainJob = false;
 
-	private $format = '?format=json';
-
 	private $defaultEndPoint = '/ocs/v2.php/apps/federation/api/v1/request-shared-secret';
 
 	/** @var  int  30 day = 2592000sec */
@@ -97,7 +91,6 @@ class RequestSharedSecret extends Job {
 	 * @param IURLGenerator $urlGenerator
 	 * @param IJobList $jobList
 	 * @param TrustedServers $trustedServers
-	 * @param DbHandler $dbHandler
 	 * @param IDiscoveryService $ocsDiscoveryService
 	 * @param ILogger $logger
 	 * @param ITimeFactory $timeFactory
@@ -107,7 +100,6 @@ class RequestSharedSecret extends Job {
 		IURLGenerator $urlGenerator,
 		IJobList $jobList,
 		TrustedServers $trustedServers,
-		DbHandler $dbHandler,
 		IDiscoveryService $ocsDiscoveryService,
 		ILogger $logger,
 		ITimeFactory $timeFactory
@@ -115,7 +107,6 @@ class RequestSharedSecret extends Job {
 		$this->httpClient = $httpClientService->newClient();
 		$this->jobList = $jobList;
 		$this->urlGenerator = $urlGenerator;
-		$this->dbHandler = $dbHandler;
 		$this->logger = $logger;
 		$this->ocsDiscoveryService = $ocsDiscoveryService;
 		$this->trustedServers = $trustedServers;
@@ -174,7 +165,7 @@ class RequestSharedSecret extends Job {
 		$endPoint = isset($endPoints['shared-secret']) ? $endPoints['shared-secret'] : $this->defaultEndPoint;
 
 		// make sure that we have a well formated url
-		$url = rtrim($target, '/') . '/' . trim($endPoint, '/') . $this->format;
+		$url = rtrim($target, '/') . '/' . trim($endPoint, '/');
 
 		try {
 			$result = $this->httpClient->post(
@@ -183,6 +174,7 @@ class RequestSharedSecret extends Job {
 					'body' => [
 						'url' => $source,
 						'token' => $token,
+						'format' => 'json',
 					],
 					'timeout' => 3,
 					'connect_timeout' => 3,
@@ -217,11 +209,6 @@ class RequestSharedSecret extends Job {
 			$this->retainJob = true;
 		}
 
-		if ($status === Http::STATUS_FORBIDDEN) {
-			// clear token if remote server refuses to ask for shared secret
-			$this->dbHandler->addToken($target, '');
-		}
-
 	}
 
 	/**

apps/federation/lib/Controller/OCSAuthAPIController.php

@@ -182,6 +182,7 @@ class OCSAuthAPIController extends OCSController{
 	 * @throws OCSForbiddenException
 	 */
 	public function getSharedSecret($url, $token) {
+
 		if ($this->trustedServers->isTrustedServer($url) === false) {
 			$this->logger->error('remote server not trusted (' . $url . ') while getting shared secret', ['app' => 'federation']);
 			throw new OCSForbiddenException();
@@ -199,8 +200,6 @@ class OCSAuthAPIController extends OCSController{
 		$sharedSecret = $this->secureRandom->generate(32);
 
 		$this->trustedServers->addSharedSecret($url, $sharedSecret);
-		// reset token after the exchange of the shared secret was successful
-		$this->dbHandler->addToken($url, '');
 
 		return new Http\DataResponse([
 			'sharedSecret' => $sharedSecret

apps/federation/tests/BackgroundJob/GetSharedSecretTest.php

@@ -32,7 +32,6 @@ use GuzzleHttp\Exception\ConnectException;
 use GuzzleHttp\Ring\Exception\RingException;
 use OCA\Federation\BackgroundJob\GetSharedSecret;
 use OCA\Files_Sharing\Tests\TestCase;
-use OCA\Federation\DbHandler;
 use OCA\Federation\TrustedServers;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Utility\ITimeFactory;
@@ -68,9 +67,6 @@ class GetSharedSecretTest extends TestCase {
 	/** @var \PHPUnit_Framework_MockObject_MockObject|TrustedServers  */
 	private $trustedServers;
 
-	/** @var \PHPUnit_Framework_MockObject_MockObject|DbHandler */
-	private $dbHandler;
-
 	/** @var \PHPUnit_Framework_MockObject_MockObject|ILogger */
 	private $logger;
 
@@ -95,8 +91,6 @@ class GetSharedSecretTest extends TestCase {
 		$this->urlGenerator = $this->getMockBuilder(IURLGenerator::class)->getMock();
 		$this->trustedServers = $this->getMockBuilder(TrustedServers::class)
 			->disableOriginalConstructor()->getMock();
-		$this->dbHandler = $this->getMockBuilder(DbHandler::class)
-			->disableOriginalConstructor()->getMock();
 		$this->logger = $this->getMockBuilder(ILogger::class)->getMock();
 		$this->response = $this->getMockBuilder(IResponse::class)->getMock();
 		$this->discoverService = $this->getMockBuilder(IDiscoveryService::class)->getMock();
@@ -111,7 +105,6 @@ class GetSharedSecretTest extends TestCase {
 			$this->jobList,
 			$this->trustedServers,
 			$this->logger,
-			$this->dbHandler,
 			$this->discoverService,
 			$this->timeFactory
 		);
@@ -133,7 +126,6 @@ class GetSharedSecretTest extends TestCase {
 					$this->jobList,
 					$this->trustedServers,
 					$this->logger,
-					$this->dbHandler,
 					$this->discoverService,
 					$this->timeFactory
 				]
@@ -198,12 +190,13 @@ class GetSharedSecretTest extends TestCase {
 			->willReturn($source);
 		$this->httpClient->expects($this->once())->method('get')
 			->with(
-				$target . '/ocs/v2.php/apps/federation/api/v1/shared-secret?format=json',
+				$target . '/ocs/v2.php/apps/federation/api/v1/shared-secret',
 				[
 					'query' =>
 						[
 							'url' => $source,
-							'token' => $token
+							'token' => $token,
+							'format' => 'json',
 						],
 					'timeout' => 3,
 					'connect_timeout' => 3,
@@ -213,15 +206,6 @@ class GetSharedSecretTest extends TestCase {
 		$this->response->expects($this->once())->method('getStatusCode')
 			->willReturn($statusCode);
 
-		if (
-			$statusCode !== Http::STATUS_OK
-			&& $statusCode !== Http::STATUS_FORBIDDEN
-		) {
-			$this->dbHandler->expects($this->never())->method('addToken');
-		}  else {
-			$this->dbHandler->expects($this->once())->method('addToken')->with($target, '');
-		}
-
 		if ($statusCode === Http::STATUS_OK) {
 			$this->response->expects($this->once())->method('getBody')
 				->willReturn('{"ocs":{"data":{"sharedSecret":"secret"}}}');
@@ -297,19 +281,19 @@ class GetSharedSecretTest extends TestCase {
 			->willReturn($source);
 		$this->httpClient->expects($this->once())->method('get')
 			->with(
-				$target . '/ocs/v2.php/apps/federation/api/v1/shared-secret?format=json',
+				$target . '/ocs/v2.php/apps/federation/api/v1/shared-secret',
 				[
 					'query' =>
 						[
 							'url' => $source,
-							'token' => $token
+							'token' => $token,
+							'format' => 'json',
 						],
 					'timeout' => 3,
 					'connect_timeout' => 3,
 				]
 			)->willThrowException($this->createMock(ConnectException::class));
 
-		$this->dbHandler->expects($this->never())->method('addToken');
 		$this->trustedServers->expects($this->never())->method('addSharedSecret');
 
 		$this->invokePrivate($this->getSharedSecret, 'run', [$argument]);
@@ -334,19 +318,19 @@ class GetSharedSecretTest extends TestCase {
 			->willReturn($source);
 		$this->httpClient->expects($this->once())->method('get')
 			->with(
-				$target . '/ocs/v2.php/apps/federation/api/v1/shared-secret?format=json',
+				$target . '/ocs/v2.php/apps/federation/api/v1/shared-secret',
 				[
 					'query' =>
 						[
 							'url' => $source,
-							'token' => $token
+							'token' => $token,
+							'format' => 'json',
 						],
 					'timeout' => 3,
 					'connect_timeout' => 3,
 				]
 			)->willThrowException($this->createMock(RingException::class));
 
-		$this->dbHandler->expects($this->never())->method('addToken');
 		$this->trustedServers->expects($this->never())->method('addSharedSecret');
 
 		$this->invokePrivate($this->getSharedSecret, 'run', [$argument]);

apps/federation/tests/BackgroundJob/RequestSharedSecretTest.php

@@ -30,7 +30,6 @@ namespace OCA\Federation\Tests\BackgroundJob;
 use GuzzleHttp\Exception\ConnectException;
 use GuzzleHttp\Ring\Exception\RingException;
 use OCA\Federation\BackgroundJob\RequestSharedSecret;
-use OCA\Federation\DbHandler;
 use OCA\Federation\TrustedServers;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Utility\ITimeFactory;
@@ -57,9 +56,6 @@ class RequestSharedSecretTest extends TestCase {
 	/** @var \PHPUnit_Framework_MockObject_MockObject|IURLGenerator */
 	private $urlGenerator;
 
-	/** @var \PHPUnit_Framework_MockObject_MockObject|DbHandler */
-	private $dbHandler;
-
 	/** @var \PHPUnit_Framework_MockObject_MockObject|TrustedServers */
 	private $trustedServers;
 
@@ -87,8 +83,6 @@ class RequestSharedSecretTest extends TestCase {
 		$this->urlGenerator = $this->getMockBuilder(IURLGenerator::class)->getMock();
 		$this->trustedServers = $this->getMockBuilder(TrustedServers::class)
 			->disableOriginalConstructor()->getMock();
-		$this->dbHandler = $this->getMockBuilder(DbHandler::class)
-			->disableOriginalConstructor()->getMock();
 		$this->response = $this->getMockBuilder(IResponse::class)->getMock();
 		$this->discoveryService = $this->getMockBuilder(IDiscoveryService::class)->getMock();
 		$this->logger = $this->createMock(ILogger::class);
@@ -102,7 +96,6 @@ class RequestSharedSecretTest extends TestCase {
 			$this->urlGenerator,
 			$this->jobList,
 			$this->trustedServers,
-			$this->dbHandler,
 			$this->discoveryService,
 			$this->logger,
 			$this->timeFactory
@@ -124,7 +117,6 @@ class RequestSharedSecretTest extends TestCase {
 					$this->urlGenerator,
 					$this->jobList,
 					$this->trustedServers,
-					$this->dbHandler,
 					$this->discoveryService,
 					$this->logger,
 					$this->timeFactory
@@ -190,12 +182,13 @@ class RequestSharedSecretTest extends TestCase {
 			->willReturn($source);
 		$this->httpClient->expects($this->once())->method('post')
 			->with(
-				$target . '/ocs/v2.php/apps/federation/api/v1/request-shared-secret?format=json',
+				$target . '/ocs/v2.php/apps/federation/api/v1/request-shared-secret',
 				[
 					'body' =>
 						[
 							'url' => $source,
-							'token' => $token
+							'token' => $token,
+							'format' => 'json',
 						],
 					'timeout' => 3,
 					'connect_timeout' => 3,
@@ -205,17 +198,6 @@ class RequestSharedSecretTest extends TestCase {
 		$this->response->expects($this->once())->method('getStatusCode')
 			->willReturn($statusCode);
 
-		if (
-			$statusCode !== Http::STATUS_OK
-			&& $statusCode !== Http::STATUS_FORBIDDEN
-		) {
-			$this->dbHandler->expects($this->never())->method('addToken');
-		}
-
-		if ($statusCode === Http::STATUS_FORBIDDEN) {
-			$this->dbHandler->expects($this->once())->method('addToken')->with($target, '');
-		}
-
 		$this->invokePrivate($this->requestSharedSecret, 'run', [$argument]);
 		if (
 			$statusCode !== Http::STATUS_OK
@@ -284,20 +266,19 @@ class RequestSharedSecretTest extends TestCase {
 			->expects($this->once())
 			->method('post')
 			->with(
-				$target . '/ocs/v2.php/apps/federation/api/v1/request-shared-secret?format=json',
+				$target . '/ocs/v2.php/apps/federation/api/v1/request-shared-secret',
 				[
 					'body' =>
 						[
 							'url' => $source,
-							'token' => $token
+							'token' => $token,
+							'format' => 'json',
 						],
 					'timeout' => 3,
 					'connect_timeout' => 3,
 				]
 			)->willThrowException($this->createMock(ConnectException::class));
 
-		$this->dbHandler->expects($this->never())->method('addToken');
-
 		$this->invokePrivate($this->requestSharedSecret, 'run', [$argument]);
 		$this->assertTrue($this->invokePrivate($this->requestSharedSecret, 'retainJob'));
 	}
@@ -321,20 +302,19 @@ class RequestSharedSecretTest extends TestCase {
 			->expects($this->once())
 			->method('post')
 			->with(
-				$target . '/ocs/v2.php/apps/federation/api/v1/request-shared-secret?format=json',
+				$target . '/ocs/v2.php/apps/federation/api/v1/request-shared-secret',
 				[
 					'body' =>
 						[
 							'url' => $source,
-							'token' => $token
+							'token' => $token,
+							'format' => 'json',
 						],
 					'timeout' => 3,
 					'connect_timeout' => 3,
 				]
 			)->willThrowException($this->createMock(RingException::class));
 
-		$this->dbHandler->expects($this->never())->method('addToken');
-
 		$this->invokePrivate($this->requestSharedSecret, 'run', [$argument]);
 		$this->assertTrue($this->invokePrivate($this->requestSharedSecret, 'retainJob'));
 	}

apps/federation/tests/Controller/OCSAuthAPIControllerTest.php

@@ -176,13 +176,10 @@ class OCSAuthAPIControllerTest extends TestCase {
 				->willReturn('secret');
 			$this->trustedServers->expects($this->once())
 				->method('addSharedSecret')->willReturn($url, 'secret');
-			$this->dbHandler->expects($this->once())
-				->method('addToken')->with($url, '');
 		} else {
 			$this->secureRandom->expects($this->never())->method('getMediumStrengthGenerator');
 			$this->secureRandom->expects($this->never())->method('generate');
 			$this->trustedServers->expects($this->never())->method('addSharedSecret');
-			$this->dbHandler->expects($this->never())->method('addToken');
 		}
 
 		try {