diff --git a/apps/federation/lib/BackgroundJob/RequestSharedSecret.php b/apps/federation/lib/BackgroundJob/RequestSharedSecret.php index 13ac9178eda1f..d410af2a7f421 100644 --- a/apps/federation/lib/BackgroundJob/RequestSharedSecret.php +++ b/apps/federation/lib/BackgroundJob/RequestSharedSecret.php @@ -1,10 +1,14 @@ * @author Björn Schießle * @author Christoph Wurst + * @author Côme Chilliet * @author Joas Schilling * @author Lukas Reschke * @author Morris Jobke @@ -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 @@ -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); @@ -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(); @@ -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 @@ -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']; diff --git a/apps/federation/tests/BackgroundJob/RequestSharedSecretTest.php b/apps/federation/tests/BackgroundJob/RequestSharedSecretTest.php index d013ad221bcac..5aca6005f94f9 100644 --- a/apps/federation/tests/BackgroundJob/RequestSharedSecretTest.php +++ b/apps/federation/tests/BackgroundJob/RequestSharedSecretTest.php @@ -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 */ @@ -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([]); @@ -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( [ @@ -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'); @@ -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],