From 0e0cfa0fa1b4ebc220977dfa5789f65b8d830c6e Mon Sep 17 00:00:00 2001
From: Bjoern Schiessle <bjoern@schiessle.org>
Date: Fri, 20 Apr 2018 15:53:10 +0200
Subject: [PATCH] improve error reporting and move format parameter to the
options
Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
---
.../lib/BackgroundJob/GetSharedSecret.php | 19 ++--------
.../lib/BackgroundJob/RequestSharedSecret.php | 17 +--------
.../lib/Controller/OCSAuthAPIController.php | 3 +-
.../BackgroundJob/GetSharedSecretTest.php | 34 +++++------------
.../BackgroundJob/RequestSharedSecretTest.php | 38 +++++--------------
.../Controller/OCSAuthAPIControllerTest.php | 3 --
6 files changed, 25 insertions(+), 89 deletions(-)
diff --git a/apps/federation/lib/BackgroundJob/GetSharedSecret.php b/apps/federation/lib/BackgroundJob/GetSharedSecret.php
index e5e30406f0d..6f901062aca 100644
--- a/apps/federation/lib/BackgroundJob/GetSharedSecret.php
+++ b/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);
diff --git a/apps/federation/lib/BackgroundJob/RequestSharedSecret.php b/apps/federation/lib/BackgroundJob/RequestSharedSecret.php
index e165c24bdf2..fb9fd25888f 100644
--- a/apps/federation/lib/BackgroundJob/RequestSharedSecret.php
+++ b/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, '');
- }
-
}
/**
diff --git a/apps/federation/lib/Controller/OCSAuthAPIController.php b/apps/federation/lib/Controller/OCSAuthAPIController.php
index a1284a4e3ad..0433cd04b1b 100644
--- a/apps/federation/lib/Controller/OCSAuthAPIController.php
+++ b/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
diff --git a/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php b/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php
index 1e264919e78..d195c81de31 100644
--- a/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php
+++ b/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]);
diff --git a/apps/federation/tests/BackgroundJob/RequestSharedSecretTest.php b/apps/federation/tests/BackgroundJob/RequestSharedSecretTest.php
index 20610f1f0fb..45bed657ef8 100644
--- a/apps/federation/tests/BackgroundJob/RequestSharedSecretTest.php
+++ b/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'));
}
diff --git a/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php b/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php
index b489bc16e50..7fb84c8bad2 100644
--- a/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php
+++ b/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 {
--
GitLab