Skip to content

Commit

Permalink
Merge pull request #39018 from nextcloud/fix/migrate-to-loggerinterfa…
Browse files Browse the repository at this point in the history 
…ce-federation

Migrate federation application to LoggerInterface
  • Loading branch information
@come-nc
come-nc committed Jun 29, 2023
2 parents 7b7148c + d34662f commit c7ea6ce
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 81 deletions.
89 changes: 28 additions & 61 deletions apps/federation/lib/BackgroundJob/RequestSharedSecret.php
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2016, ownCloud, Inc.
*
* @author Bjoern Schiessle <[email protected]>
* @author Björn Schießle <[email protected]>
* @author Christoph Wurst <[email protected]>
* @author Côme Chilliet <[email protected]>
* @author Joas Schilling <[email protected]>
* @author Lukas Reschke <[email protected]>
* @author Morris Jobke <[email protected]>
Expand Down Expand Up @@ -37,9 +41,9 @@
use OCP\BackgroundJob\Job;
use OCP\Http\Client\IClient;
use OCP\Http\Client\IClientService;
use OCP\ILogger;
use OCP\IURLGenerator;
use OCP\OCS\IDiscoveryService;
use Psr\Log\LoggerInterface;

/**
* Class RequestSharedSecret
Expand All @@ -49,74 +53,37 @@
* @package OCA\Federation\Backgroundjob
*/
class RequestSharedSecret extends Job {
private IClient $httpClient;

/** @var IClient */
private $httpClient;

/** @var IJobList */
private $jobList;

/** @var IURLGenerator */
private $urlGenerator;

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

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

/** @var ILogger */
private $logger;
protected bool $retainJob = false;

/** @var bool */
protected $retainJob = false;
private string $defaultEndPoint = '/ocs/v2.php/apps/federation/api/v1/request-shared-secret';

private $defaultEndPoint = '/ocs/v2.php/apps/federation/api/v1/request-shared-secret';
/** @var int 30 day = 2592000sec */
private int $maxLifespan = 2592000;

/** @var int 30 day = 2592000sec */
private $maxLifespan = 2592000;

/**
* RequestSharedSecret constructor.
*
* @param IClientService $httpClientService
* @param IURLGenerator $urlGenerator
* @param IJobList $jobList
* @param TrustedServers $trustedServers
* @param IDiscoveryService $ocsDiscoveryService
* @param ILogger $logger
* @param ITimeFactory $timeFactory
*/
public function __construct(
IClientService $httpClientService,
IURLGenerator $urlGenerator,
IJobList $jobList,
TrustedServers $trustedServers,
IDiscoveryService $ocsDiscoveryService,
ILogger $logger,
ITimeFactory $timeFactory
private IURLGenerator $urlGenerator,
private IJobList $jobList,
private TrustedServers $trustedServers,
private IDiscoveryService $ocsDiscoveryService,
private LoggerInterface $logger,
ITimeFactory $timeFactory,
) {
parent::__construct($timeFactory);
$this->httpClient = $httpClientService->newClient();
$this->jobList = $jobList;
$this->urlGenerator = $urlGenerator;
$this->logger = $logger;
$this->ocsDiscoveryService = $ocsDiscoveryService;
$this->trustedServers = $trustedServers;
}


/**
* run the job, then remove it from the joblist
*
* @param IJobList $jobList
* @param ILogger|null $logger
*/
public function execute(IJobList $jobList, ILogger $logger = null) {
public function start(IJobList $jobList): void {
$target = $this->argument['url'];
// only execute if target is still in the list of trusted domains
if ($this->trustedServers->isTrustedServer($target)) {
$this->parentExecute($jobList, $logger);
$this->parentStart($jobList);
}

$jobList->remove($this, $this->argument);
Expand All @@ -127,15 +94,17 @@ public function execute(IJobList $jobList, ILogger $logger = null) {
}

/**
* call execute() method of parent
*
* @param IJobList $jobList
* @param ILogger $logger
* Call start() method of parent
* Useful for unit tests
*/
protected function parentExecute($jobList, $logger) {
parent::execute($jobList, $logger);
protected function parentStart(IJobList $jobList): void {
parent::start($jobList);
}

/**
* @param array $argument
* @return void
*/
protected function run($argument) {
$target = $argument['url'];
$created = isset($argument['created']) ? (int)$argument['created'] : $this->time->getTime();
Expand Down Expand Up @@ -185,7 +154,7 @@ protected function run($argument) {
$this->logger->info('Could not connect to ' . $target, ['app' => 'federation']);
} catch (\Throwable $e) {
$status = Http::STATUS_INTERNAL_SERVER_ERROR;
$this->logger->logException($e, ['app' => 'federation']);
$this->logger->error($e->getMessage(), ['app' => 'federation', 'exception' => $e]);
}

// if we received a unexpected response we try again later
Expand All @@ -199,10 +168,8 @@ protected function run($argument) {

/**
* re-add background job
*
* @param array $argument
*/
protected function reAddJob(array $argument) {
protected function reAddJob(array $argument): void {
$url = $argument['url'];
$created = isset($argument['created']) ? (int)$argument['created'] : $this->time->getTime();
$token = $argument['token'];
Expand Down
40 changes: 20 additions & 20 deletions apps/federation/tests/BackgroundJob/RequestSharedSecretTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,38 +34,38 @@
use OCP\Http\Client\IClient;
use OCP\Http\Client\IClientService;
use OCP\Http\Client\IResponse;
use OCP\ILogger;
use OCP\IURLGenerator;
use OCP\OCS\IDiscoveryService;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\LoggerInterface;
use Test\TestCase;

class RequestSharedSecretTest extends TestCase {

/** @var \PHPUnit\Framework\MockObject\MockObject|IClientService */
/** @var MockObject|IClientService */
private $httpClientService;

/** @var \PHPUnit\Framework\MockObject\MockObject|IClient */
/** @var MockObject|IClient */
private $httpClient;

/** @var \PHPUnit\Framework\MockObject\MockObject|IJobList */
/** @var MockObject|IJobList */
private $jobList;

/** @var \PHPUnit\Framework\MockObject\MockObject|IURLGenerator */
/** @var MockObject|IURLGenerator */
private $urlGenerator;

/** @var \PHPUnit\Framework\MockObject\MockObject|TrustedServers */
/** @var MockObject|TrustedServers */
private $trustedServers;

/** @var \PHPUnit\Framework\MockObject\MockObject|IResponse */
/** @var MockObject|IResponse */
private $response;

/** @var \PHPUnit\Framework\MockObject\MockObject|IDiscoveryService */
/** @var MockObject|IDiscoveryService */
private $discoveryService;

/** @var \PHPUnit\Framework\MockObject\MockObject|ILogger */
/** @var MockObject|LoggerInterface */
private $logger;

/** @var \PHPUnit\Framework\MockObject\MockObject|ITimeFactory */
/** @var MockObject|ITimeFactory */
private $timeFactory;

/** @var RequestSharedSecret */
Expand All @@ -82,7 +82,7 @@ protected function setUp(): void {
->disableOriginalConstructor()->getMock();
$this->response = $this->getMockBuilder(IResponse::class)->getMock();
$this->discoveryService = $this->getMockBuilder(IDiscoveryService::class)->getMock();
$this->logger = $this->createMock(ILogger::class);
$this->logger = $this->createMock(LoggerInterface::class);
$this->timeFactory = $this->createMock(ITimeFactory::class);

$this->discoveryService->expects($this->any())->method('discover')->willReturn([]);
Expand All @@ -100,13 +100,13 @@ protected function setUp(): void {
}

/**
* @dataProvider dataTestExecute
* @dataProvider dataTestStart
*
* @param bool $isTrustedServer
* @param bool $retainBackgroundJob
*/
public function testExecute($isTrustedServer, $retainBackgroundJob) {
/** @var RequestSharedSecret |\PHPUnit\Framework\MockObject\MockObject $requestSharedSecret */
public function testStart($isTrustedServer, $retainBackgroundJob) {
/** @var RequestSharedSecret |MockObject $requestSharedSecret */
$requestSharedSecret = $this->getMockBuilder('OCA\Federation\BackgroundJob\RequestSharedSecret')
->setConstructorArgs(
[
Expand All @@ -118,15 +118,15 @@ public function testExecute($isTrustedServer, $retainBackgroundJob) {
$this->logger,
$this->timeFactory
]
)->setMethods(['parentExecute'])->getMock();
)->setMethods(['parentStart'])->getMock();
$this->invokePrivate($requestSharedSecret, 'argument', [['url' => 'url', 'token' => 'token']]);

$this->trustedServers->expects($this->once())->method('isTrustedServer')
->with('url')->willReturn($isTrustedServer);
if ($isTrustedServer) {
$requestSharedSecret->expects($this->once())->method('parentExecute');
$requestSharedSecret->expects($this->once())->method('parentStart');
} else {
$requestSharedSecret->expects($this->never())->method('parentExecute');
$requestSharedSecret->expects($this->never())->method('parentStart');
}
$this->invokePrivate($requestSharedSecret, 'retainJob', [$retainBackgroundJob]);
$this->jobList->expects($this->once())->method('remove');
Expand All @@ -148,10 +148,10 @@ public function testExecute($isTrustedServer, $retainBackgroundJob) {
$this->jobList->expects($this->never())->method('add');
}

$requestSharedSecret->execute($this->jobList);
$requestSharedSecret->start($this->jobList);
}

public function dataTestExecute() {
public function dataTestStart() {
return [
[true, true],
[true, false],
Expand Down

0 comments on commit c7ea6ce

Please sign in to comment.