From 589b83a053a4b13426aabf956240eefc92280f64 Mon Sep 17 00:00:00 2001 From: Hamid Dehnavi Date: Wed, 5 Jul 2023 21:57:29 +0330 Subject: [PATCH 1/5] refactor federatedfilesharing app controllers Signed-off-by: Hamid Dehnavi --- .../Controller/MountPublicLinkController.php | 60 +++------ .../Controller/RequestHandlerController.php | 114 ++++++------------ 2 files changed, 52 insertions(+), 122 deletions(-) diff --git a/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php b/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php index 404077e46affe..6b5dd74045348 100644 --- a/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php +++ b/apps/federatedfilesharing/lib/Controller/MountPublicLinkController.php @@ -57,30 +57,6 @@ */ class MountPublicLinkController extends Controller { - /** @var FederatedShareProvider */ - private $federatedShareProvider; - - /** @var AddressHandler */ - private $addressHandler; - - /** @var IManager */ - private $shareManager; - - /** @var ISession */ - private $session; - - /** @var IL10N */ - private $l; - - /** @var IUserSession */ - private $userSession; - - /** @var IClientService */ - private $clientService; - - /** @var ICloudIdManager */ - private $cloudIdManager; - /** * MountPublicLinkController constructor. * @@ -95,27 +71,19 @@ class MountPublicLinkController extends Controller { * @param IClientService $clientService * @param ICloudIdManager $cloudIdManager */ - public function __construct($appName, - IRequest $request, - FederatedShareProvider $federatedShareProvider, - IManager $shareManager, - AddressHandler $addressHandler, - ISession $session, - IL10N $l, - IUserSession $userSession, - IClientService $clientService, - ICloudIdManager $cloudIdManager + public function __construct( + $appName, + IRequest $request, + private FederatedShareProvider $federatedShareProvider, + private IManager $shareManager, + private AddressHandler $addressHandler, + private ISession $session, + private IL10N $l, + private IUserSession $userSession, + private IClientService $clientService, + private ICloudIdManager $cloudIdManager, ) { parent::__construct($appName, $request); - - $this->federatedShareProvider = $federatedShareProvider; - $this->shareManager = $shareManager; - $this->addressHandler = $addressHandler; - $this->session = $session; - $this->l = $l; - $this->userSession = $userSession; - $this->clientService = $clientService; - $this->cloudIdManager = $cloudIdManager; } /** @@ -130,7 +98,7 @@ public function __construct($appName, * @param string $password * @return JSONResponse */ - public function createFederatedShare($shareWith, $token, $password = '') { + public function createFederatedShare(string $shareWith, string $token, string $password = ''): JSONResponse { if (!$this->federatedShareProvider->isOutgoingServer2serverShareEnabled()) { return new JSONResponse( ['message' => 'This server doesn\'t support outgoing federated shares'], @@ -198,7 +166,7 @@ public function createFederatedShare($shareWith, $token, $password = '') { * @param string $name (only for legacy reasons, can be removed with legacyMountPublicLink()) * @return JSONResponse */ - public function askForFederatedShare($token, $remote, $password = '', $owner = '', $ownerDisplayName = '', $name = '') { + public function askForFederatedShare(string $token, string $remote, string $password = '', string $owner = '', string $ownerDisplayName = '', string $name = ''): JSONResponse { // check if server admin allows to mount public links from other servers if ($this->federatedShareProvider->isIncomingServer2serverShareEnabled() === false) { return new JSONResponse(['message' => $this->l->t('Server to server sharing is not enabled on this server')], Http::STATUS_BAD_REQUEST); @@ -236,7 +204,7 @@ public function askForFederatedShare($token, $remote, $password = '', $owner = ' return new JSONResponse(['message' => $this->l->t('Federated Share request sent, you will receive an invitation. Check your notifications.')]); } - // if we doesn't get the expected response we assume that we try to add + // if we don't get the expected response we assume that we try to add // a federated share from a Nextcloud <= 9 server $message = $this->l->t("Couldn't establish a federated share, it looks like the server to federate with is too old (Nextcloud <= 9)."); return new JSONResponse(['message' => $message], Http::STATUS_BAD_REQUEST); diff --git a/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php b/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php index 2dd8098b3b072..b5865799f5496 100644 --- a/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php +++ b/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php @@ -31,11 +31,13 @@ use OCA\FederatedFileSharing\FederatedShareProvider; use OCA\FederatedFileSharing\Notifications; use OCP\AppFramework\Http; +use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSBadRequestException; use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCSController; use OCP\App\IAppManager; use OCP\Constants; +use OCP\DB\Exception; use OCP\EventDispatcher\IEventDispatcher; use OCP\Federation\Exceptions\ProviderCouldNotAddShareException; use OCP\Federation\Exceptions\ProviderDoesNotExistsException; @@ -48,73 +50,31 @@ use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Share; use OCP\Share\Exceptions\ShareNotFound; +use Psr\Container\ContainerExceptionInterface; +use Psr\Container\NotFoundExceptionInterface; use Psr\Log\LoggerInterface; class RequestHandlerController extends OCSController { - /** @var FederatedShareProvider */ - private $federatedShareProvider; - - /** @var IDBConnection */ - private $connection; - - /** @var Share\IManager */ - private $shareManager; - - /** @var Notifications */ - private $notifications; - - /** @var AddressHandler */ - private $addressHandler; - - /** @var IUserManager */ - private $userManager; - /** @var string */ - private $shareTable = 'share'; - - /** @var ICloudIdManager */ - private $cloudIdManager; - - /** @var LoggerInterface */ - private $logger; - - /** @var ICloudFederationFactory */ - private $cloudFederationFactory; - - /** @var ICloudFederationProviderManager */ - private $cloudFederationProviderManager; - - /** @var IEventDispatcher */ - private $eventDispatcher; - - public function __construct(string $appName, - IRequest $request, - FederatedShareProvider $federatedShareProvider, - IDBConnection $connection, - Share\IManager $shareManager, - Notifications $notifications, - AddressHandler $addressHandler, - IUserManager $userManager, - ICloudIdManager $cloudIdManager, - LoggerInterface $logger, - ICloudFederationFactory $cloudFederationFactory, - ICloudFederationProviderManager $cloudFederationProviderManager, - IEventDispatcher $eventDispatcher + private string $shareTable = 'share'; + + public function __construct( + string $appName, + IRequest $request, + private FederatedShareProvider $federatedShareProvider, + private IDBConnection $connection, + private Share\IManager $shareManager, + private Notifications $notifications, + private AddressHandler $addressHandler, + private IUserManager $userManager, + private ICloudIdManager $cloudIdManager, + private LoggerInterface $logger, + private ICloudFederationFactory $cloudFederationFactory, + private ICloudFederationProviderManager $cloudFederationProviderManager, + private IEventDispatcher $eventDispatcher, ) { parent::__construct($appName, $request); - - $this->federatedShareProvider = $federatedShareProvider; - $this->connection = $connection; - $this->shareManager = $shareManager; - $this->notifications = $notifications; - $this->addressHandler = $addressHandler; - $this->userManager = $userManager; - $this->cloudIdManager = $cloudIdManager; - $this->logger = $logger; - $this->cloudFederationFactory = $cloudFederationFactory; - $this->cloudFederationProviderManager = $cloudFederationProviderManager; - $this->eventDispatcher = $eventDispatcher; } /** @@ -145,7 +105,7 @@ public function createShare( ?int $remoteId = null, ?string $sharedByFederatedId = null, ?string $ownerFederatedId = null, - ) { + ): Http\DataResponse { if ($ownerFederatedId === null) { $ownerFederatedId = $this->cloudIdManager->getCloudId($owner, $this->cleanupRemote($remote))->getId(); } @@ -203,7 +163,7 @@ public function createShare( * @throws OCSBadRequestException * @throws OCSException */ - public function reShare(int $id, ?string $token = null, ?string $shareWith = null, ?int $permission = 0, ?int $remoteId = 0) { + public function reShare(int $id, ?string $token = null, ?string $shareWith = null, ?int $permission = 0, ?int $remoteId = 0): Http\DataResponse { if ($token === null || $shareWith === null || $permission === null || @@ -246,12 +206,10 @@ public function reShare(int $id, ?string $token = null, ?string $shareWith = nul * * @param int $id * @param string|null $token - * @return Http\DataResponse + * @return DataResponse * @throws OCSException - * @throws ShareNotFound - * @throws \OCP\HintException */ - public function acceptShare(int $id, ?string $token = null) { + public function acceptShare(int $id, ?string $token = null): DataResponse { $notification = [ 'sharedSecret' => $token, 'message' => 'Recipient accept the share' @@ -283,7 +241,7 @@ public function acceptShare(int $id, ?string $token = null) { * @return Http\DataResponse * @throws OCSException */ - public function declineShare(int $id, ?string $token = null) { + public function declineShare(int $id, ?string $token = null): DataResponse { $notification = [ 'sharedSecret' => $token, 'message' => 'Recipient declined the share' @@ -315,7 +273,7 @@ public function declineShare(int $id, ?string $token = null) { * @return Http\DataResponse * @throws OCSException */ - public function unshare(int $id, ?string $token = null) { + public function unshare(int $id, ?string $token = null): DataResponse { if (!$this->isS2SEnabled()) { throw new OCSException('Server does not support federated cloud sharing', 503); } @@ -332,7 +290,7 @@ public function unshare(int $id, ?string $token = null) { return new Http\DataResponse(); } - private function cleanupRemote($remote) { + private function cleanupRemote($remote): string { $remote = substr($remote, strpos($remote, '://') + 3); return rtrim($remote, '/'); @@ -350,7 +308,7 @@ private function cleanupRemote($remote) { * @return Http\DataResponse * @throws OCSBadRequestException */ - public function revoke(int $id, ?string $token = null) { + public function revoke(int $id, ?string $token = null): DataResponse { try { $provider = $this->cloudFederationProviderManager->getCloudFederationProvider('file'); $notification = ['sharedSecret' => $token]; @@ -366,8 +324,10 @@ public function revoke(int $id, ?string $token = null) { * * @param bool $incoming * @return bool + * @throws ContainerExceptionInterface + * @throws NotFoundExceptionInterface */ - private function isS2SEnabled($incoming = false) { + private function isS2SEnabled(bool $incoming = false): bool { $result = \OCP\Server::get(IAppManager::class)->isEnabledForUser('files_sharing'); if ($incoming) { @@ -391,7 +351,7 @@ private function isS2SEnabled($incoming = false) { * @return Http\DataResponse * @throws OCSBadRequestException */ - public function updatePermissions(int $id, ?string $token = null, ?int $permissions = null) { + public function updatePermissions(int $id, ?string $token = null, ?int $permissions = null): DataResponse { $ncPermissions = $permissions; try { @@ -414,7 +374,7 @@ public function updatePermissions(int $id, ?string $token = null, ?int $permissi * @param $ncPermissions * @return array */ - protected function ncPermissions2ocmPermissions($ncPermissions) { + protected function ncPermissions2ocmPermissions($ncPermissions): array { $ocmPermissions = []; if ($ncPermissions & Constants::PERMISSION_SHARE) { @@ -443,12 +403,14 @@ protected function ncPermissions2ocmPermissions($ncPermissions) { * @param string|null $token * @param string|null $remote * @param string|null $remote_id - * @return Http\DataResponse + * @return DataResponse + * @throws ContainerExceptionInterface + * @throws NotFoundExceptionInterface * @throws OCSBadRequestException * @throws OCSException - * @throws \OCP\DB\Exception + * @throws Exception */ - public function move(int $id, ?string $token = null, ?string $remote = null, ?string $remote_id = null) { + public function move(int $id, ?string $token = null, ?string $remote = null, ?string $remote_id = null): DataResponse { if (!$this->isS2SEnabled()) { throw new OCSException('Server does not support federated cloud sharing', 503); } From 9aee7e0da803ae4436cc05c27fc3b7fba11ed058 Mon Sep 17 00:00:00 2001 From: Hamid Dehnavi Date: Thu, 6 Jul 2023 12:02:22 +0330 Subject: [PATCH 2/5] Make adjustments based on the review Signed-off-by: Hamid Dehnavi --- .../lib/Controller/RequestHandlerController.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php b/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php index b5865799f5496..abf780cf9be48 100644 --- a/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php +++ b/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php @@ -290,7 +290,7 @@ public function unshare(int $id, ?string $token = null): DataResponse { return new Http\DataResponse(); } - private function cleanupRemote($remote): string { + private function cleanupRemote(?string $remote = null): string { $remote = substr($remote, strpos($remote, '://') + 3); return rtrim($remote, '/'); @@ -374,7 +374,7 @@ public function updatePermissions(int $id, ?string $token = null, ?int $permissi * @param $ncPermissions * @return array */ - protected function ncPermissions2ocmPermissions($ncPermissions): array { + protected function ncPermissions2ocmPermissions(?int $ncPermissions = null): array { $ocmPermissions = []; if ($ncPermissions & Constants::PERMISSION_SHARE) { From 112b83a59c25fbb922458d21d013e1d2621542e6 Mon Sep 17 00:00:00 2001 From: Hamid Dehnavi Date: Thu, 6 Jul 2023 12:22:27 +0330 Subject: [PATCH 3/5] Make adjustments based on the review Signed-off-by: Hamid Dehnavi --- .../lib/Controller/RequestHandlerController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php b/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php index abf780cf9be48..e62a63cdac361 100644 --- a/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php +++ b/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php @@ -371,7 +371,7 @@ public function updatePermissions(int $id, ?string $token = null, ?int $permissi /** * translate Nextcloud permissions to OCM Permissions * - * @param $ncPermissions + * @param int|null $ncPermissions * @return array */ protected function ncPermissions2ocmPermissions(?int $ncPermissions = null): array { From a9dbe71e7f21b89b91d69d4dcb0d10e2b5a73c79 Mon Sep 17 00:00:00 2001 From: Hamid Dehnavi Date: Fri, 14 Jul 2023 17:22:28 +0330 Subject: [PATCH 4/5] Make adjustments based on the review Co-authored-by: Joas Schilling Signed-off-by: Hamid Dehnavi --- .../lib/Controller/RequestHandlerController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php b/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php index e62a63cdac361..d91a765cbd9fb 100644 --- a/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php +++ b/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php @@ -290,7 +290,7 @@ public function unshare(int $id, ?string $token = null): DataResponse { return new Http\DataResponse(); } - private function cleanupRemote(?string $remote = null): string { + private function cleanupRemote(string $remote): string { $remote = substr($remote, strpos($remote, '://') + 3); return rtrim($remote, '/'); From 6b79ad7484c3ff71bcf262f6e534898fcd58f612 Mon Sep 17 00:00:00 2001 From: Hamid Dehnavi Date: Tue, 18 Jul 2023 13:28:56 +0330 Subject: [PATCH 5/5] Make adjustments based on the review Co-authored-by: Joas Schilling Signed-off-by: Hamid Dehnavi --- .../lib/Controller/RequestHandlerController.php | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php b/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php index d91a765cbd9fb..85b5313f648da 100644 --- a/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php +++ b/apps/federatedfilesharing/lib/Controller/RequestHandlerController.php @@ -290,7 +290,7 @@ public function unshare(int $id, ?string $token = null): DataResponse { return new Http\DataResponse(); } - private function cleanupRemote(string $remote): string { + private function cleanupRemote(?string $remote = null): string { $remote = substr($remote, strpos($remote, '://') + 3); return rtrim($remote, '/'); @@ -324,8 +324,6 @@ public function revoke(int $id, ?string $token = null): DataResponse { * * @param bool $incoming * @return bool - * @throws ContainerExceptionInterface - * @throws NotFoundExceptionInterface */ private function isS2SEnabled(bool $incoming = false): bool { $result = \OCP\Server::get(IAppManager::class)->isEnabledForUser('files_sharing'); @@ -371,10 +369,10 @@ public function updatePermissions(int $id, ?string $token = null, ?int $permissi /** * translate Nextcloud permissions to OCM Permissions * - * @param int|null $ncPermissions + * @param int $ncPermissions * @return array */ - protected function ncPermissions2ocmPermissions(?int $ncPermissions = null): array { + protected function ncPermissions2ocmPermissions(int $ncPermissions): array { $ocmPermissions = []; if ($ncPermissions & Constants::PERMISSION_SHARE) { @@ -404,8 +402,6 @@ protected function ncPermissions2ocmPermissions(?int $ncPermissions = null): arr * @param string|null $remote * @param string|null $remote_id * @return DataResponse - * @throws ContainerExceptionInterface - * @throws NotFoundExceptionInterface * @throws OCSBadRequestException * @throws OCSException * @throws Exception