From a9cfa72d1cf5eccb352b34eb823559ac52f8e22c Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 24 Jun 2022 15:24:16 +0200 Subject: Summer cleanup of the federation app - Use IEventDispatcher instead of deprecated symfony dispatcher - Use LoggerInterface where possible - Use php 7.4 properties - Add type hinting where possible - Move federation hooks to a seperate listener Signed-off-by: Carl Schwan --- .../lib/BackgroundJob/GetSharedSecret.php | 67 +++------ .../lib/Command/SyncFederationAddressBooks.php | 15 +- .../lib/Controller/OCSAuthAPIController.php | 90 ++++-------- .../lib/Controller/SettingsController.php | 44 ++---- apps/federation/lib/DbHandler.php | 155 ++++++++------------- .../lib/Listener/SabrePluginAuthInitListener.php | 3 +- .../lib/Middleware/AddServerMiddleware.php | 25 +--- apps/federation/lib/Settings/Admin.php | 14 +- apps/federation/lib/SyncFederationAddressBooks.php | 17 +-- apps/federation/lib/SyncJob.php | 25 ++-- apps/federation/lib/TrustedServers.php | 142 ++++++------------- .../tests/Controller/SettingsControllerTest.php | 39 ++---- apps/federation/tests/TrustedServersTest.php | 23 ++- 13 files changed, 211 insertions(+), 448 deletions(-) (limited to 'apps/federation') diff --git a/apps/federation/lib/BackgroundJob/GetSharedSecret.php b/apps/federation/lib/BackgroundJob/GetSharedSecret.php index 5379a837151..75faa7ce1d9 100644 --- a/apps/federation/lib/BackgroundJob/GetSharedSecret.php +++ b/apps/federation/lib/BackgroundJob/GetSharedSecret.php @@ -42,59 +42,34 @@ use OCP\Http\Client\IResponse; use OCP\ILogger; use OCP\IURLGenerator; use OCP\OCS\IDiscoveryService; +use Psr\Log\LoggerInterface; /** * Class GetSharedSecret * - * request shared secret from remote Nextcloud + * Request shared secret from remote Nextcloud * * @package OCA\Federation\Backgroundjob */ class GetSharedSecret extends Job { + private IClient $httpClient; + private IJobList $jobList; + private IURLGenerator $urlGenerator; + private TrustedServers $trustedServers; + private IDiscoveryService $ocsDiscoveryService; + private LoggerInterface $logger; + protected bool $retainJob = false; + private string $defaultEndPoint = '/ocs/v2.php/apps/federation/api/v1/shared-secret'; + + /** 30 day = 2592000sec */ + private int $maxLifespan = 2592000; - /** @var IClient */ - private $httpClient; - - /** @var IJobList */ - private $jobList; - - /** @var IURLGenerator */ - private $urlGenerator; - - /** @var TrustedServers */ - private $trustedServers; - - /** @var IDiscoveryService */ - private $ocsDiscoveryService; - - /** @var ILogger */ - private $logger; - - /** @var bool */ - protected $retainJob = false; - - private $defaultEndPoint = '/ocs/v2.php/apps/federation/api/v1/shared-secret'; - - /** @var int 30 day = 2592000sec */ - private $maxLifespan = 2592000; - - /** - * RequestSharedSecret constructor. - * - * @param IClientService $httpClientService - * @param IURLGenerator $urlGenerator - * @param IJobList $jobList - * @param TrustedServers $trustedServers - * @param ILogger $logger - * @param IDiscoveryService $ocsDiscoveryService - * @param ITimeFactory $timeFactory - */ public function __construct( IClientService $httpClientService, IURLGenerator $urlGenerator, IJobList $jobList, TrustedServers $trustedServers, - ILogger $logger, + LoggerInterface $logger, IDiscoveryService $ocsDiscoveryService, ITimeFactory $timeFactory ) { @@ -128,7 +103,7 @@ class GetSharedSecret extends Job { } /** - * call execute() method of parent + * Call execute() method of parent * * @param IJobList $jobList * @param ILogger $logger @@ -185,14 +160,16 @@ class GetSharedSecret extends Job { } } catch (RequestException $e) { $status = -1; // There is no status code if we could not connect - $this->logger->logException($e, [ - 'message' => 'Could not connect to ' . $target, - 'level' => ILogger::INFO, + $this->logger->info('Could not connect to ' . $target, [ + 'exception' => $e, '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 @@ -226,7 +203,7 @@ class GetSharedSecret extends 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/lib/Command/SyncFederationAddressBooks.php b/apps/federation/lib/Command/SyncFederationAddressBooks.php index 045c3c72009..adb0b613680 100644 --- a/apps/federation/lib/Command/SyncFederationAddressBooks.php +++ b/apps/federation/lib/Command/SyncFederationAddressBooks.php @@ -28,16 +28,12 @@ use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Helper\ProgressBar; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; +use OCA\Federation\SyncFederationAddressBooks as SyncService; class SyncFederationAddressBooks extends Command { + private SyncService $syncService; - /** @var \OCA\Federation\SyncFederationAddressBooks */ - private $syncService; - - /** - * @param \OCA\Federation\SyncFederationAddressBooks $syncService - */ - public function __construct(\OCA\Federation\SyncFederationAddressBooks $syncService) { + public function __construct(SyncService $syncService) { parent::__construct(); $this->syncService = $syncService; @@ -49,11 +45,6 @@ class SyncFederationAddressBooks extends Command { ->setDescription('Synchronizes addressbooks of all federated clouds'); } - /** - * @param InputInterface $input - * @param OutputInterface $output - * @return int - */ protected function execute(InputInterface $input, OutputInterface $output): int { $progress = new ProgressBar($output); $progress->start(); diff --git a/apps/federation/lib/Controller/OCSAuthAPIController.php b/apps/federation/lib/Controller/OCSAuthAPIController.php index dd9b94d0027..5a976720b04 100644 --- a/apps/federation/lib/Controller/OCSAuthAPIController.php +++ b/apps/federation/lib/Controller/OCSAuthAPIController.php @@ -30,14 +30,14 @@ namespace OCA\Federation\Controller; use OCA\Federation\DbHandler; use OCA\Federation\TrustedServers; -use OCP\AppFramework\Http; +use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSForbiddenException; use OCP\AppFramework\OCSController; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\IJobList; -use OCP\ILogger; use OCP\IRequest; use OCP\Security\ISecureRandom; +use Psr\Log\LoggerInterface; /** * Class OCSAuthAPI @@ -47,45 +47,21 @@ use OCP\Security\ISecureRandom; * @package OCA\Federation\Controller */ class OCSAuthAPIController extends OCSController { + private ISecureRandom $secureRandom; + private IJobList $jobList; + private TrustedServers $trustedServers; + private DbHandler $dbHandler; + private LoggerInterface $logger; + private ITimeFactory $timeFactory; - /** @var ISecureRandom */ - private $secureRandom; - - /** @var IJobList */ - private $jobList; - - /** @var TrustedServers */ - private $trustedServers; - - /** @var DbHandler */ - private $dbHandler; - - /** @var ILogger */ - private $logger; - - /** @var ITimeFactory */ - private $timeFactory; - - /** - * OCSAuthAPI constructor. - * - * @param string $appName - * @param IRequest $request - * @param ISecureRandom $secureRandom - * @param IJobList $jobList - * @param TrustedServers $trustedServers - * @param DbHandler $dbHandler - * @param ILogger $logger - * @param ITimeFactory $timeFactory - */ public function __construct( - $appName, + string $appName, IRequest $request, ISecureRandom $secureRandom, IJobList $jobList, TrustedServers $trustedServers, DbHandler $dbHandler, - ILogger $logger, + LoggerInterface $logger, ITimeFactory $timeFactory ) { parent::__construct($appName, $request); @@ -99,48 +75,36 @@ class OCSAuthAPIController extends OCSController { } /** + * Request received to ask remote server for a shared secret, for legacy end-points + * * @NoCSRFRequired * @PublicPage - * - * request received to ask remote server for a shared secret, for legacy end-points - * - * @param string $url - * @param string $token - * @return Http\DataResponse * @throws OCSForbiddenException */ - public function requestSharedSecretLegacy($url, $token) { + public function requestSharedSecretLegacy(string $url, string $token): DataResponse { return $this->requestSharedSecret($url, $token); } /** + * Create shared secret and return it, for legacy end-points + * * @NoCSRFRequired * @PublicPage - * - * create shared secret and return it, for legacy end-points - * - * @param string $url - * @param string $token - * @return Http\DataResponse * @throws OCSForbiddenException */ - public function getSharedSecretLegacy($url, $token) { + public function getSharedSecretLegacy(string $url, string $token): DataResponse { return $this->getSharedSecret($url, $token); } /** + * Request received to ask remote server for a shared secret + * * @NoCSRFRequired * @PublicPage - * - * request received to ask remote server for a shared secret - * - * @param string $url - * @param string $token - * @return Http\DataResponse * @throws OCSForbiddenException */ - public function requestSharedSecret($url, $token) { + public function requestSharedSecret(string $url, string $token): DataResponse { if ($this->trustedServers->isTrustedServer($url) === false) { $this->logger->error('remote server not trusted (' . $url . ') while requesting shared secret', ['app' => 'federation']); throw new OCSForbiddenException(); @@ -166,21 +130,17 @@ class OCSAuthAPIController extends OCSController { ] ); - return new Http\DataResponse(); + return new DataResponse(); } /** + * Create shared secret and return it + * * @NoCSRFRequired * @PublicPage - * - * create shared secret and return it - * - * @param string $url - * @param string $token - * @return Http\DataResponse * @throws OCSForbiddenException */ - public function getSharedSecret($url, $token) { + public function getSharedSecret(string $url, string $token): DataResponse { if ($this->trustedServers->isTrustedServer($url) === false) { $this->logger->error('remote server not trusted (' . $url . ') while getting shared secret', ['app' => 'federation']); throw new OCSForbiddenException(); @@ -199,12 +159,12 @@ class OCSAuthAPIController extends OCSController { $this->trustedServers->addSharedSecret($url, $sharedSecret); - return new Http\DataResponse([ + return new DataResponse([ 'sharedSecret' => $sharedSecret ]); } - protected function isValidToken($url, $token) { + protected function isValidToken(string $url, string $token): bool { $storedToken = $this->dbHandler->getToken($url); return hash_equals($storedToken, $token); } diff --git a/apps/federation/lib/Controller/SettingsController.php b/apps/federation/lib/Controller/SettingsController.php index c60a7d31d7c..8abc2f8af57 100644 --- a/apps/federation/lib/Controller/SettingsController.php +++ b/apps/federation/lib/Controller/SettingsController.php @@ -31,20 +31,10 @@ use OCP\IL10N; use OCP\IRequest; class SettingsController extends Controller { + private IL10N $l; + private TrustedServers $trustedServers; - /** @var IL10N */ - private $l; - - /** @var TrustedServers */ - private $trustedServers; - - /** - * @param string $AppName - * @param IRequest $request - * @param IL10N $l10n - * @param TrustedServers $trustedServers - */ - public function __construct($AppName, + public function __construct(string $AppName, IRequest $request, IL10N $l10n, TrustedServers $trustedServers @@ -59,31 +49,25 @@ class SettingsController extends Controller { * Add server to the list of trusted Nextclouds. * * @AuthorizedAdminSetting(settings=OCA\Federation\Settings\Admin) - * @param string $url - * @return DataResponse * @throws HintException */ - public function addServer($url) { + public function addServer(string $url): DataResponse { $this->checkServer($url); $id = $this->trustedServers->addServer($url); - return new DataResponse( - [ - 'url' => $url, - 'id' => $id, - 'message' => $this->l->t('Added to the list of trusted servers') - ] - ); + return new DataResponse([ + 'url' => $url, + 'id' => $id, + 'message' => $this->l->t('Added to the list of trusted servers') + ]); } /** * Add server to the list of trusted Nextclouds. * * @AuthorizedAdminSetting(settings=OCA\Federation\Settings\Admin) - * @param int $id - * @return DataResponse */ - public function removeServer($id) { + public function removeServer(int $id): DataResponse { $this->trustedServers->removeServer($id); return new DataResponse(); } @@ -92,23 +76,19 @@ class SettingsController extends Controller { * Check if the server should be added to the list of trusted servers or not. * * @AuthorizedAdminSetting(settings=OCA\Federation\Settings\Admin) - * @param string $url - * @return bool * @throws HintException */ - protected function checkServer($url) { + protected function checkServer(string $url): void { if ($this->trustedServers->isTrustedServer($url) === true) { $message = 'Server is already in the list of trusted servers.'; $hint = $this->l->t('Server is already in the list of trusted servers.'); throw new HintException($message, $hint); } - if ($this->trustedServers->isOwnCloudServer($url) === false) { + if ($this->trustedServers->isNextcloudServer($url) === false) { $message = 'No server to federate with found'; $hint = $this->l->t('No server to federate with found'); throw new HintException($message, $hint); } - - return true; } } diff --git a/apps/federation/lib/DbHandler.php b/apps/federation/lib/DbHandler.php index 1dd0d1fc1c4..abdabee6a08 100644 --- a/apps/federation/lib/DbHandler.php +++ b/apps/federation/lib/DbHandler.php @@ -30,31 +30,25 @@ namespace OCA\Federation; use OC\Files\Filesystem; use OCP\HintException; use OCP\IDBConnection; +use OCP\DB\Exception as DBException; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IL10N; /** * Class DbHandler * - * handles all database calls for the federation app + * Handles all database calls for the federation app + * + * @todo Port to QBMapper * * @group DB * @package OCA\Federation */ class DbHandler { + private IDBConnection $connection; + private IL10N $IL10N; + private string $dbTable = 'trusted_servers'; - /** @var IDBConnection */ - private $connection; - - /** @var IL10N */ - private $IL10N; - - /** @var string */ - private $dbTable = 'trusted_servers'; - - /** - * @param IDBConnection $connection - * @param IL10N $il10n - */ public function __construct( IDBConnection $connection, IL10N $il10n @@ -64,27 +58,23 @@ class DbHandler { } /** - * add server to the list of trusted servers + * Add server to the list of trusted servers * - * @param string $url - * @return int * @throws HintException */ - public function addServer($url) { + public function addServer(string $url): int { $hash = $this->hash($url); $url = rtrim($url, '/'); $query = $this->connection->getQueryBuilder(); $query->insert($this->dbTable) - ->values( - [ - 'url' => $query->createParameter('url'), - 'url_hash' => $query->createParameter('url_hash'), - ] - ) + ->values([ + 'url' => $query->createParameter('url'), + 'url_hash' => $query->createParameter('url_hash'), + ]) ->setParameter('url', $url) ->setParameter('url_hash', $hash); - $result = $query->execute(); + $result = $query->executeStatement(); if ($result) { return $query->getLastInsertId(); @@ -96,32 +86,29 @@ class DbHandler { } /** - * remove server from the list of trusted servers - * - * @param int $id + * Remove server from the list of trusted servers */ - public function removeServer($id) { + public function removeServer(int $id): void { $query = $this->connection->getQueryBuilder(); $query->delete($this->dbTable) ->where($query->expr()->eq('id', $query->createParameter('id'))) ->setParameter('id', $id); - $query->execute(); + $query->executeStatement(); } /** - * get trusted server with given ID + * Get trusted server with given ID * - * @param int $id - * @return array + * @return array{id: int, url: string, url_hash: string, token: string, shared_secret: string, status: int, sync_token: string} * @throws \Exception */ - public function getServerById($id) { + public function getServerById(int $id): array { $query = $this->connection->getQueryBuilder(); $query->select('*')->from($this->dbTable) ->where($query->expr()->eq('id', $query->createParameter('id'))) - ->setParameter('id', $id); + ->setParameter('id', $id, IQueryBuilder::PARAM_INT); - $qResult = $query->execute(); + $qResult = $query->executeQuery(); $result = $qResult->fetchAll(); $qResult->closeCursor(); @@ -133,34 +120,32 @@ class DbHandler { } /** - * get all trusted servers + * Get all trusted servers * - * @return array + * @return list + * @throws DBException */ - public function getAllServer() { + public function getAllServer(): array { $query = $this->connection->getQueryBuilder(); $query->select(['url', 'url_hash', 'id', 'status', 'shared_secret', 'sync_token']) ->from($this->dbTable); - $statement = $query->execute(); + $statement = $query->executeQuery(); $result = $statement->fetchAll(); $statement->closeCursor(); return $result; } /** - * check if server already exists in the database table - * - * @param string $url - * @return bool + * Check if server already exists in the database table */ - public function serverExists($url) { + public function serverExists(string $url): bool { $hash = $this->hash($url); $query = $this->connection->getQueryBuilder(); $query->select('url') ->from($this->dbTable) ->where($query->expr()->eq('url_hash', $query->createParameter('url_hash'))) ->setParameter('url_hash', $hash); - $statement = $query->execute(); + $statement = $query->executeQuery(); $result = $statement->fetchAll(); $statement->closeCursor(); @@ -168,12 +153,9 @@ class DbHandler { } /** - * write token to database. Token is used to exchange the secret - * - * @param string $url - * @param string $token + * Write token to database. Token is used to exchange the secret */ - public function addToken($url, $token) { + public function addToken(string $url, string $token): void { $hash = $this->hash($url); $query = $this->connection->getQueryBuilder(); $query->update($this->dbTable) @@ -181,24 +163,21 @@ class DbHandler { ->where($query->expr()->eq('url_hash', $query->createParameter('url_hash'))) ->setParameter('url_hash', $hash) ->setParameter('token', $token); - $query->execute(); + $query->executeStatement(); } /** - * get token stored in database - * - * @param string $url - * @return string + * Get token stored in database * @throws \Exception */ - public function getToken($url) { + public function getToken(string $url): string { $hash = $this->hash($url); $query = $this->connection->getQueryBuilder(); $query->select('token')->from($this->dbTable) ->where($query->expr()->eq('url_hash', $query->createParameter('url_hash'))) ->setParameter('url_hash', $hash); - $statement = $query->execute(); + $statement = $query->executeQuery(); $result = $statement->fetch(); $statement->closeCursor(); @@ -210,12 +189,9 @@ class DbHandler { } /** - * add shared Secret to database - * - * @param string $url - * @param string $sharedSecret + * Add shared Secret to database */ - public function addSharedSecret($url, $sharedSecret) { + public function addSharedSecret(string $url, string $sharedSecret): void { $hash = $this->hash($url); $query = $this->connection->getQueryBuilder(); $query->update($this->dbTable) @@ -223,36 +199,29 @@ class DbHandler { ->where($query->expr()->eq('url_hash', $query->createParameter('url_hash'))) ->setParameter('url_hash', $hash) ->setParameter('sharedSecret', $sharedSecret); - $query->execute(); + $query->executeStatement(); } /** - * get shared secret from database - * - * @param string $url - * @return string + * Get shared secret from database */ - public function getSharedSecret($url) { + public function getSharedSecret(string $url): string { $hash = $this->hash($url); $query = $this->connection->getQueryBuilder(); $query->select('shared_secret')->from($this->dbTable) ->where($query->expr()->eq('url_hash', $query->createParameter('url_hash'))) ->setParameter('url_hash', $hash); - $statement = $query->execute(); + $statement = $query->executeQuery(); $result = $statement->fetch(); $statement->closeCursor(); return $result['shared_secret']; } /** - * set server status - * - * @param string $url - * @param int $status - * @param string|null $token + * Set server status */ - public function setServerStatus($url, $status, $token = null) { + public function setServerStatus(string $url, int $status, ?string $token = null): void { $hash = $this->hash($url); $query = $this->connection->getQueryBuilder(); $query->update($this->dbTable) @@ -261,46 +230,37 @@ class DbHandler { if (!is_null($token)) { $query->set('sync_token', $query->createNamedParameter($token)); } - $query->execute(); + $query->executeStatement(); } /** - * get server status - * - * @param string $url - * @return int + * Get server status */ - public function getServerStatus($url) { + public function getServerStatus(string $url): int { $hash = $this->hash($url); $query = $this->connection->getQueryBuilder(); $query->select('status')->from($this->dbTable) ->where($query->expr()->eq('url_hash', $query->createParameter('url_hash'))) ->setParameter('url_hash', $hash); - $statement = $query->execute(); + $statement = $query->executeQuery(); $result = $statement->fetch(); $statement->closeCursor(); return (int)$result['status']; } /** - * create hash from URL - * - * @param string $url - * @return string + * Create hash from URL */ - protected function hash($url) { + protected function hash(string $url): string { $normalized = $this->normalizeUrl($url); return sha1($normalized); } /** - * normalize URL, used to create the sha1 hash - * - * @param string $url - * @return string + * Normalize URL, used to create the sha1 hash */ - protected function normalizeUrl($url) { + protected function normalizeUrl(string $url): string { $normalized = $url; if (strpos($url, 'https://') === 0) { @@ -315,12 +275,7 @@ class DbHandler { return $normalized; } - /** - * @param $username - * @param $password - * @return bool - */ - public function auth($username, $password) { + public function auth(string $username, string $password): bool { if ($username !== 'system') { return false; } @@ -328,7 +283,7 @@ class DbHandler { $query->select('url')->from($this->dbTable) ->where($query->expr()->eq('shared_secret', $query->createNamedParameter($password))); - $statement = $query->execute(); + $statement = $query->executeQuery(); $result = $statement->fetch(); $statement->closeCursor(); return !empty($result); diff --git a/apps/federation/lib/Listener/SabrePluginAuthInitListener.php b/apps/federation/lib/Listener/SabrePluginAuthInitListener.php index f176f21506a..322a2e483e6 100644 --- a/apps/federation/lib/Listener/SabrePluginAuthInitListener.php +++ b/apps/federation/lib/Listener/SabrePluginAuthInitListener.php @@ -35,8 +35,7 @@ use Sabre\DAV\Auth\Plugin; * @since 20.0.0 */ class SabrePluginAuthInitListener implements IEventListener { - /** @var FedAuth */ - private $fedAuth; + private FedAuth $fedAuth; public function __construct(FedAuth $fedAuth) { $this->fedAuth = $fedAuth; diff --git a/apps/federation/lib/Middleware/AddServerMiddleware.php b/apps/federation/lib/Middleware/AddServerMiddleware.php index de6f7786679..de964f1bd4a 100644 --- a/apps/federation/lib/Middleware/AddServerMiddleware.php +++ b/apps/federation/lib/Middleware/AddServerMiddleware.php @@ -35,25 +35,14 @@ use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Middleware; use OCP\HintException; use OCP\IL10N; -use OCP\ILogger; +use Psr\Log\LoggerInterface; class AddServerMiddleware extends Middleware { + protected string $appName; + protected IL10N $l; + protected LoggerInterface $logger; - /** @var string */ - protected $appName; - - /** @var IL10N */ - protected $l; - - /** @var ILogger */ - protected $logger; - - /** - * @param string $appName - * @param IL10N $l - * @param ILogger $logger - */ - public function __construct($appName, IL10N $l, ILogger $logger) { + public function __construct(string $appName, IL10N $l, LoggerInterface $logger) { $this->appName = $appName; $this->l = $l; $this->logger = $logger; @@ -72,9 +61,9 @@ class AddServerMiddleware extends Middleware { if (($controller instanceof SettingsController) === false) { throw $exception; } - $this->logger->logException($exception, [ - 'level' => ILogger::ERROR, + $this->logger->error($exception->getMessage(), [ 'app' => $this->appName, + 'exception' => $exception, ]); if ($exception instanceof HintException) { $message = $exception->getHint(); diff --git a/apps/federation/lib/Settings/Admin.php b/apps/federation/lib/Settings/Admin.php index 7d4e51a124c..bbbed36ba4e 100644 --- a/apps/federation/lib/Settings/Admin.php +++ b/apps/federation/lib/Settings/Admin.php @@ -28,19 +28,9 @@ use OCP\IL10N; use OCP\Settings\IDelegatedSettings; class Admin implements IDelegatedSettings { + private TrustedServers $trustedServers; + private IL10N $l; - /** @var TrustedServers */ - private $trustedServers; - - /** @var IL10N */ - private $l; - - /** - * Admin constructor. - * - * @param TrustedServers $trustedServers - * @param IL10N $l - */ public function __construct(TrustedServers $trustedServers, IL10N $l) { $this->trustedServers = $trustedServers; $this->l = $l; diff --git a/apps/federation/lib/SyncFederationAddressBooks.php b/apps/federation/lib/SyncFederationAddressBooks.php index ace5c07065a..c17cb7618bf 100644 --- a/apps/federation/lib/SyncFederationAddressBooks.php +++ b/apps/federation/lib/SyncFederationAddressBooks.php @@ -31,21 +31,10 @@ use OCP\AppFramework\Http; use OCP\OCS\IDiscoveryService; class SyncFederationAddressBooks { + protected DbHandler $dbHandler; + private SyncService $syncService; + private DiscoveryService $ocsDiscoveryService; - /** @var DbHandler */ - protected $dbHandler; - - /** @var SyncService */ - private $syncService; - - /** @var DiscoveryService */ - private $ocsDiscoveryService; - - /** - * @param DbHandler $dbHandler - * @param SyncService $syncService - * @param IDiscoveryService $ocsDiscoveryService - */ public function __construct(DbHandler $dbHandler, SyncService $syncService, IDiscoveryService $ocsDiscoveryService diff --git a/apps/federation/lib/SyncJob.php b/apps/federation/lib/SyncJob.php index f16d08a80d8..2498f309498 100644 --- a/apps/federation/lib/SyncJob.php +++ b/apps/federation/lib/SyncJob.php @@ -25,22 +25,16 @@ */ namespace OCA\Federation; -use OC\BackgroundJob\TimedJob; -use OCP\ILogger; +use OCP\BackgroundJob\TimedJob; +use OCP\AppFramework\Utility\ITimeFactory; +use Psr\Log\LoggerInterface; class SyncJob extends TimedJob { + protected SyncFederationAddressBooks $syncService; + protected LoggerInterface $logger; - /** @var SyncFederationAddressBooks */ - protected $syncService; - - /** @var ILogger */ - protected $logger; - - /** - * @param SyncFederationAddressBooks $syncService - * @param ILogger $logger - */ - public function __construct(SyncFederationAddressBooks $syncService, ILogger $logger) { + public function __construct(SyncFederationAddressBooks $syncService, LoggerInterface $logger, ITimeFactory $timeFactory) { + parent::__construct($timeFactory); // Run once a day $this->setInterval(24 * 60 * 60); $this->syncService = $syncService; @@ -50,10 +44,9 @@ class SyncJob extends TimedJob { protected function run($argument) { $this->syncService->syncThemAll(function ($url, $ex) { if ($ex instanceof \Exception) { - $this->logger->logException($ex, [ - 'message' => "Error while syncing $url.", - 'level' => ILogger::INFO, + $this->logger->info("Error while syncing $url.", [ 'app' => 'fed-sync', + 'exception' => $ex, ]); } }); diff --git a/apps/federation/lib/TrustedServers.php b/apps/federation/lib/TrustedServers.php index 57b9a505499..272161fd881 100644 --- a/apps/federation/lib/TrustedServers.php +++ b/apps/federation/lib/TrustedServers.php @@ -34,10 +34,11 @@ use OCP\BackgroundJob\IJobList; use OCP\HintException; use OCP\Http\Client\IClientService; use OCP\IConfig; -use OCP\ILogger; use OCP\Security\ISecureRandom; -use Symfony\Component\EventDispatcher\EventDispatcherInterface; -use Symfony\Component\EventDispatcher\GenericEvent; +use OCP\DB\Exception as DBException; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\Federation\Events\TrustedServerRemovedEvent; +use Psr\Log\LoggerInterface; class TrustedServers { @@ -50,48 +51,23 @@ class TrustedServers { /** remote server revoked access */ public const STATUS_ACCESS_REVOKED = 4; - /** @var dbHandler */ - private $dbHandler; + private DbHandler $dbHandler; + private IClientService $httpClientService; + private LoggerInterface $logger; + private IJobList $jobList; + private ISecureRandom $secureRandom; + private IConfig $config; + private IEventDispatcher $dispatcher; + private ITimeFactory $timeFactory; - /** @var IClientService */ - private $httpClientService; - - /** @var ILogger */ - private $logger; - - /** @var IJobList */ - private $jobList; - - /** @var ISecureRandom */ - private $secureRandom; - - /** @var IConfig */ - private $config; - - /** @var EventDispatcherInterface */ - private $dispatcher; - - /** @var ITimeFactory */ - private $timeFactory; - - /** - * @param DbHandler $dbHandler - * @param IClientService $httpClientService - * @param ILogger $logger - * @param IJobList $jobList - * @param ISecureRandom $secureRandom - * @param IConfig $config - * @param EventDispatcherInterface $dispatcher - * @param ITimeFactory $timeFactory - */ public function __construct( DbHandler $dbHandler, IClientService $httpClientService, - ILogger $logger, + LoggerInterface $logger, IJobList $jobList, ISecureRandom $secureRandom, IConfig $config, - EventDispatcherInterface $dispatcher, + IEventDispatcher $dispatcher, ITimeFactory $timeFactory ) { $this->dbHandler = $dbHandler; @@ -105,12 +81,9 @@ class TrustedServers { } /** - * add server to the list of trusted servers - * - * @param $url - * @return int server id + * Add server to the list of trusted servers */ - public function addServer($url) { + public function addServer(string $url): int { $url = $this->updateProtocol($url); $result = $this->dbHandler->addServer($url); if ($result) { @@ -130,82 +103,62 @@ class TrustedServers { } /** - * get shared secret for the given server - * - * @param string $url - * @return string + * Get shared secret for the given server */ - public function getSharedSecret($url) { + public function getSharedSecret(string $url): string { return $this->dbHandler->getSharedSecret($url); } /** - * add shared secret for the given server - * - * @param string $url - * @param $sharedSecret + * Add shared secret for the given server */ - public function addSharedSecret($url, $sharedSecret) { + public function addSharedSecret(string $url, string $sharedSecret): void { $this->dbHandler->addSharedSecret($url, $sharedSecret); } /** - * remove server from the list of trusted servers - * - * @param int $id + * Remove server from the list of trusted servers */ - public function removeServer($id) { + public function removeServer(int $id): void { $server = $this->dbHandler->getServerById($id); $this->dbHandler->removeServer($id); - $event = new GenericEvent($server['url_hash']); - $this->dispatcher->dispatch('OCP\Federation\TrustedServerEvent::remove', $event); + $this->dispatcher->dispatchTyped(new TrustedServerRemovedEvent($server['url_hash'])); } /** - * get all trusted servers - * - * @return array + * Get all trusted servers + * @return list */ public function getServers() { return $this->dbHandler->getAllServer(); } /** - * check if given server is a trusted Nextcloud server - * - * @param string $url - * @return bool + * Check if given server is a trusted Nextcloud server */ - public function isTrustedServer($url) { + public function isTrustedServer(string $url): bool { return $this->dbHandler->serverExists($url); } /** - * set server status - * - * @param string $url - * @param int $status + * Set server status */ - public function setServerStatus($url, $status) { + public function setServerStatus(string $url, int $status): void { $this->dbHandler->setServerStatus($url, $status); } /** - * @param string $url - * @return int + * Get server status */ - public function getServerStatus($url) { + public function getServerStatus(string $url): int { return $this->dbHandler->getServerStatus($url); } /** - * check if URL point to a ownCloud/Nextcloud server - * - * @param string $url - * @return bool + * Check if URL point to a ownCloud/Nextcloud server */ - public function isOwnCloudServer($url) { - $isValidOwnCloud = false; + public function isNextcloudServer(string $url): bool { + $isValidNextcloud = false; $client = $this->httpClientService->newClient(); try { $result = $client->get( @@ -216,28 +169,28 @@ class TrustedServers { ] ); if ($result->getStatusCode() === Http::STATUS_OK) { - $isValidOwnCloud = $this->checkOwnCloudVersion($result->getBody()); + $body = $result->getBody(); + if (is_resource($body)) { + $body = stream_get_contents($body) ?: ''; + } + $isValidNextcloud = $this->checkNextcloudVersion($body); } } catch (\Exception $e) { - \OC::$server->getLogger()->logException($e, [ - 'message' => 'No Nextcloud server.', - 'level' => ILogger::DEBUG, + $this->logger->error('No Nextcloud server.', [ 'app' => 'federation', + 'exception' => $e, ]); return false; } - return $isValidOwnCloud; + return $isValidNextcloud; } /** - * check if ownCloud version is >= 9.0 - * - * @param $status - * @return bool + * Check if ownCloud/Nextcloud version is >= 9.0 * @throws HintException */ - protected function checkOwnCloudVersion($status) { + protected function checkNextcloudVersion(string $status): bool { $decoded = json_decode($status, true); if (!empty($decoded) && isset($decoded['version'])) { if (!version_compare($decoded['version'], '9.0.0', '>=')) { @@ -249,12 +202,9 @@ class TrustedServers { } /** - * check if the URL contain a protocol, if not add https - * - * @param string $url - * @return string + * Check if the URL contain a protocol, if not add https */ - protected function updateProtocol($url) { + protected function updateProtocol(string $url): string { if ( strpos($url, 'https://') === 0 || strpos($url, 'http://') === 0 diff --git a/apps/federation/tests/Controller/SettingsControllerTest.php b/apps/federation/tests/Controller/SettingsControllerTest.php index 856dcaa533f..c33ee6feb87 100644 --- a/apps/federation/tests/Controller/SettingsControllerTest.php +++ b/apps/federation/tests/Controller/SettingsControllerTest.php @@ -5,6 +5,7 @@ * @author Björn Schießle * @author Morris Jobke * @author Roeland Jago Douma + * @author Carl Schwan * * @license AGPL-3.0 * @@ -31,9 +32,7 @@ use OCP\IRequest; use Test\TestCase; class SettingsControllerTest extends TestCase { - - /** @var SettingsController */ - private $controller; + private SettingsController $controller; /** @var \PHPUnit\Framework\MockObject\MockObject | \OCP\IRequest */ private $request; @@ -60,7 +59,7 @@ class SettingsControllerTest extends TestCase { ); } - public function testAddServer() { + public function testAddServer(): void { $this->trustedServers ->expects($this->once()) ->method('isTrustedServer') @@ -68,7 +67,7 @@ class SettingsControllerTest extends TestCase { ->willReturn(false); $this->trustedServers ->expects($this->once()) - ->method('isOwnCloudServer') + ->method('isNextcloudServer') ->with('url') ->willReturn(true); @@ -83,11 +82,8 @@ class SettingsControllerTest extends TestCase { /** * @dataProvider checkServerFails - * - * @param bool $isTrustedServer - * @param bool $isOwnCloud */ - public function testAddServerFail($isTrustedServer, $isOwnCloud) { + public function testAddServerFail(bool $isTrustedServer, bool $isNextcloud): void { $this->expectException(\OCP\HintException::class); $this->trustedServers @@ -97,14 +93,14 @@ class SettingsControllerTest extends TestCase { ->willReturn($isTrustedServer); $this->trustedServers ->expects($this->any()) - ->method('isOwnCloudServer') + ->method('isNextcloudServer') ->with('url') - ->willReturn($isOwnCloud); + ->willReturn($isNextcloud); $this->controller->addServer('url'); } - public function testRemoveServer() { + public function testRemoveServer(): void { $this->trustedServers->expects($this->once())->method('removeServer') ->with('url'); $result = $this->controller->removeServer('url'); @@ -112,7 +108,7 @@ class SettingsControllerTest extends TestCase { $this->assertSame(200, $result->getStatus()); } - public function testCheckServer() { + public function testCheckServer(): void { $this->trustedServers ->expects($this->once()) ->method('isTrustedServer') @@ -120,7 +116,7 @@ class SettingsControllerTest extends TestCase { ->willReturn(false); $this->trustedServers ->expects($this->once()) - ->method('isOwnCloudServer') + ->method('isNextcloudServer') ->with('url') ->willReturn(true); @@ -131,11 +127,8 @@ class SettingsControllerTest extends TestCase { /** * @dataProvider checkServerFails - * - * @param bool $isTrustedServer - * @param bool $isOwnCloud */ - public function testCheckServerFail($isTrustedServer, $isOwnCloud) { + public function testCheckServerFail(bool $isTrustedServer, bool $isNextcloud): void { $this->expectException(\OCP\HintException::class); $this->trustedServers @@ -145,9 +138,9 @@ class SettingsControllerTest extends TestCase { ->willReturn($isTrustedServer); $this->trustedServers ->expects($this->any()) - ->method('isOwnCloudServer') + ->method('isNextcloudServer') ->with('url') - ->willReturn($isOwnCloud); + ->willReturn($isNextcloud); $this->assertTrue( $this->invokePrivate($this->controller, 'checkServer', ['url']) @@ -155,11 +148,9 @@ class SettingsControllerTest extends TestCase { } /** - * data to simulate checkServer fails - * - * @return array + * Data to simulate checkServer fails */ - public function checkServerFails() { + public function checkServerFails(): array { return [ [true, true], [false, false] diff --git a/apps/federation/tests/TrustedServersTest.php b/apps/federation/tests/TrustedServersTest.php index 3dd93a445cd..0dc155f0b8d 100644 --- a/apps/federation/tests/TrustedServersTest.php +++ b/apps/federation/tests/TrustedServersTest.php @@ -37,7 +37,7 @@ use OCP\Http\Client\IResponse; use OCP\IConfig; use OCP\ILogger; use OCP\Security\ISecureRandom; -use Symfony\Component\EventDispatcher\EventDispatcherInterface; +use OCP\EventDispatcher\IEventDispatcher; use Test\TestCase; class TrustedServersTest extends TestCase { @@ -69,7 +69,7 @@ class TrustedServersTest extends TestCase { /** @var \PHPUnit\Framework\MockObject\MockObject | IConfig */ private $config; - /** @var \PHPUnit\Framework\MockObject\MockObject | EventDispatcherInterface */ + /** @var \PHPUnit\Framework\MockObject\MockObject | IEventDispatcher */ private $dispatcher; /** @var \PHPUnit\Framework\MockObject\MockObject|ITimeFactory */ @@ -80,7 +80,7 @@ class TrustedServersTest extends TestCase { $this->dbHandler = $this->getMockBuilder(DbHandler::class) ->disableOriginalConstructor()->getMock(); - $this->dispatcher = $this->getMockBuilder(EventDispatcherInterface::class) + $this->dispatcher = $this->getMockBuilder(IEventDispatcher::class) ->disableOriginalConstructor()->getMock(); $this->httpClientService = $this->getMockBuilder(IClientService::class)->getMock(); $this->httpClient = $this->getMockBuilder(IClient::class)->getMock(); @@ -175,13 +175,12 @@ class TrustedServersTest extends TestCase { $this->dbHandler->expects($this->once())->method('removeServer')->with($id); $this->dbHandler->expects($this->once())->method('getServerById')->with($id) ->willReturn($server); - $this->dispatcher->expects($this->once())->method('dispatch') + $this->dispatcher->expects($this->once())->method('dispatchTyped') ->willReturnCallback( - function ($eventId, $event) { - $this->assertSame($eventId, 'OCP\Federation\TrustedServerEvent::remove'); - $this->assertInstanceOf('Symfony\Component\EventDispatcher\GenericEvent', $event); - /** @var \Symfony\Component\EventDispatcher\GenericEvent $event */ - $this->assertSame('url_hash', $event->getSubject()); + function ($event) { + $this->assertTrue($event instanceof \OCP\Federated\Events\TrustedServerRemovedEvent); + /** @var \OCP\Federated\Events\TrustedServerRemovedEvent $event */ + $this->assertSame('url_hash', $event->getUrlHash()); } ); $this->trustedServers->removeServer($id); @@ -221,13 +220,13 @@ class TrustedServersTest extends TestCase { } /** - * @dataProvider dataTestIsOwnCloudServer + * @dataProvider dataTestIsNextcloudServer * * @param int $statusCode * @param bool $isValidOwnCloudVersion * @param bool $expected */ - public function testIsOwnCloudServer($statusCode, $isValidOwnCloudVersion, $expected) { + public function testIsNextcloudServer($statusCode, $isValidOwnCloudVersion, $expected) { $server = 'server1'; /** @var \PHPUnit\Framework\MockObject\MockObject | TrustedServers $trustedServers */ @@ -264,7 +263,7 @@ class TrustedServersTest extends TestCase { } $this->assertSame($expected, - $trustedServers->isOwnCloudServer($server) + $trustedServers->isNextcloudServer($server) ); } -- cgit v1.2.3