diff options
author | Bjoern Schiessle <bjoern@schiessle.org> | 2018-04-20 16:53:10 +0300 |
---|---|---|
committer | Morris Jobke <hey@morrisjobke.de> | 2018-05-29 16:34:32 +0300 |
commit | 58be1f1b2f02ff6aace5424556b3ff27d0d8da5e (patch) | |
tree | 2bf0feb5c614235c37b8360b8e849ddb14d3f68a /apps | |
parent | af2ecbbd5a81d8e1621ecda1784a99adb0fc43dd (diff) |
improve error reporting and move format parameter to the options
Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
Diffstat (limited to 'apps')
6 files changed, 25 insertions, 89 deletions
diff --git a/apps/federation/lib/BackgroundJob/GetSharedSecret.php b/apps/federation/lib/BackgroundJob/GetSharedSecret.php index 92bb31e369e..8bfa86b1304 100644 --- a/apps/federation/lib/BackgroundJob/GetSharedSecret.php +++ b/apps/federation/lib/BackgroundJob/GetSharedSecret.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; @@ -69,9 +68,6 @@ class GetSharedSecret extends Job { /** @var TrustedServers */ private $trustedServers; - /** @var DbHandler */ - private $dbHandler; - /** @var IDiscoveryService */ private $ocsDiscoveryService; @@ -84,8 +80,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 */ @@ -99,7 +93,6 @@ class GetSharedSecret extends Job { * @param IJobList $jobList * @param TrustedServers $trustedServers * @param ILogger $logger - * @param DbHandler $dbHandler * @param IDiscoveryService $ocsDiscoveryService * @param ITimeFactory $timeFactory */ @@ -109,7 +102,6 @@ class GetSharedSecret extends Job { IJobList $jobList, TrustedServers $trustedServers, ILogger $logger, - DbHandler $dbHandler, IDiscoveryService $ocsDiscoveryService, ITimeFactory $timeFactory ) { @@ -117,7 +109,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; @@ -173,7 +164,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 { @@ -183,7 +174,8 @@ class GetSharedSecret extends Job { 'query' => [ 'url' => $source, - 'token' => $token + 'token' => $token, + 'format' => 'json', ], 'timeout' => 3, 'connect_timeout' => 3, @@ -216,9 +208,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) { @@ -231,7 +220,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 ad7504da7ad..55a8a97860e 100644 --- a/apps/federation/lib/BackgroundJob/RequestSharedSecret.php +++ b/apps/federation/lib/BackgroundJob/RequestSharedSecret.php @@ -37,7 +37,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; @@ -66,9 +65,6 @@ class RequestSharedSecret extends Job { /** @var IURLGenerator */ private $urlGenerator; - /** @var DbHandler */ - private $dbHandler; - /** @var TrustedServers */ private $trustedServers; @@ -84,8 +80,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 */ @@ -98,7 +92,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 @@ -108,7 +101,6 @@ class RequestSharedSecret extends Job { IURLGenerator $urlGenerator, IJobList $jobList, TrustedServers $trustedServers, - DbHandler $dbHandler, IDiscoveryService $ocsDiscoveryService, ILogger $logger, ITimeFactory $timeFactory @@ -116,7 +108,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; @@ -175,7 +166,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( @@ -184,6 +175,7 @@ class RequestSharedSecret extends Job { 'body' => [ 'url' => $source, 'token' => $token, + 'format' => 'json', ], 'timeout' => 3, 'connect_timeout' => 3, @@ -218,11 +210,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 { |