Skip to content

improve error reporting and move format parameter to the options #9256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MorrisJobke merged 1 commit into from
Apr 25, 2018
Merged

improve error reporting and move format parameter to the options #9256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 4 additions & 15 deletions apps/federation/lib/BackgroundJob/GetSharedSecret.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
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;
Expand Down Expand Up @@ -68,9 +67,6 @@ class GetSharedSecret extends Job {
/** @var TrustedServers */
private $trustedServers;

/** @var DbHandler */
private $dbHandler;

/** @var IDiscoveryService */
private $ocsDiscoveryService;

Expand All @@ -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 */
Expand All @@ -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
*/
Expand All @@ -108,15 +101,13 @@ public function __construct(
IJobList $jobList,
TrustedServers $trustedServers,
ILogger $logger,
DbHandler $dbHandler,
IDiscoveryService $ocsDiscoveryService,
ITimeFactory $timeFactory
) {
$this->logger = $logger;
$this->httpClient = $httpClientService->newClient();
$this->jobList = $jobList;
$this->urlGenerator = $urlGenerator;
$this->dbHandler = $dbHandler;
$this->ocsDiscoveryService = $ocsDiscoveryService;
$this->trustedServers = $trustedServers;
$this->timeFactory = $timeFactory;
Expand Down Expand Up @@ -172,7 +163,7 @@ protected function run($argument) {
$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 {
Expand All @@ -182,7 +173,8 @@ protected function run($argument) {
'query' =>
[
'url' => $source,
'token' => $token
'token' => $token,
'format' => 'json',
],
'timeout' => 3,
'connect_timeout' => 3,
Expand Down Expand Up @@ -223,9 +215,6 @@ protected function run($argument) {
&& $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) {
Expand All @@ -238,7 +227,7 @@ protected function run($argument) {
);
} 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);
Expand Down
17 changes: 2 additions & 15 deletions apps/federation/lib/BackgroundJob/RequestSharedSecret.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
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;
Expand Down Expand Up @@ -65,9 +64,6 @@ class RequestSharedSecret extends Job {
/** @var IURLGenerator */
private $urlGenerator;

/** @var DbHandler */
private $dbHandler;

/** @var TrustedServers */
private $trustedServers;

Expand All @@ -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 */
Expand All @@ -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
Expand All @@ -107,15 +100,13 @@ public function __construct(
IURLGenerator $urlGenerator,
IJobList $jobList,
TrustedServers $trustedServers,
DbHandler $dbHandler,
IDiscoveryService $ocsDiscoveryService,
ILogger $logger,
ITimeFactory $timeFactory
) {
$this->httpClient = $httpClientService->newClient();
$this->jobList = $jobList;
$this->urlGenerator = $urlGenerator;
$this->dbHandler = $dbHandler;
$this->logger = $logger;
$this->ocsDiscoveryService = $ocsDiscoveryService;
$this->trustedServers = $trustedServers;
Expand Down Expand Up @@ -174,7 +165,7 @@ protected function run($argument) {
$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(
Expand All @@ -183,6 +174,7 @@ protected function run($argument) {
'body' => [
'url' => $source,
'token' => $token,
'format' => 'json',
],
'timeout' => 3,
'connect_timeout' => 3,
Expand Down Expand Up @@ -217,11 +209,6 @@ protected function run($argument) {
$this->retainJob = true;
}

if ($status === Http::STATUS_FORBIDDEN) {
// clear token if remote server refuses to ask for shared secret
$this->dbHandler->addToken($target, '');
}

}

/**
Expand Down
3 changes: 1 addition & 2 deletions apps/federation/lib/Controller/OCSAuthAPIController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ public function requestSharedSecret($url, $token) {
* @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();
Expand All @@ -199,8 +200,6 @@ public function getSharedSecret($url, $token) {
$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, '');
Copy link
Member

@rullzer rullzer Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain why?

Copy link
Member

@MorrisJobke MorrisJobke Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is not needed to be reset. The code knows what is the bigger and what is the smaller token. It often confused people why one server deletes it. And also there is code wise no good reason why it should be read. On the other hand this could give insights into debugging sessions so we keep it just as it is.


return new Http\DataResponse([
'sharedSecret' => $sharedSecret
Expand Down
34 changes: 9 additions & 25 deletions apps/federation/tests/BackgroundJob/GetSharedSecretTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
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;
Expand Down Expand Up @@ -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;

Expand All @@ -95,8 +91,6 @@ public function setUp() {
$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();
Expand All @@ -111,7 +105,6 @@ public function setUp() {
$this->jobList,
$this->trustedServers,
$this->logger,
$this->dbHandler,
$this->discoverService,
$this->timeFactory
);
Expand All @@ -133,7 +126,6 @@ public function testExecute($isTrustedServer, $retainBackgroundJob) {
$this->jobList,
$this->trustedServers,
$this->logger,
$this->dbHandler,
$this->discoverService,
$this->timeFactory
]
Expand Down Expand Up @@ -198,12 +190,13 @@ public function testRun($statusCode) {
->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,
Expand All @@ -213,15 +206,6 @@ public function testRun($statusCode) {
$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"}}}');
Expand Down Expand Up @@ -297,19 +281,19 @@ public function testRunConnectionError() {
->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]);
Expand All @@ -334,19 +318,19 @@ public function testRunRingException() {
->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]);
Expand Down
Loading