Merge pull request #3 from binsky08/fix/834-private

General backend type safety improvements and php code modernization

Author: binsky08

.editorconfig

@@ -0,0 +1,13 @@
+# SPDX-FileCopyrightText: 2019 Nextcloud GmbH and Nextcloud contributors
+# SPDX-License-Identifier: AGPL-3.0-or-later
+
+[*]
+charset = utf-8
+indent_style = tab
+end_of_line = lf
+insert_final_newline = true
+trim_trailing_whitespace = true
+
+[{package.json,webpack.config.js}]
+indent_style = space
+indent_size = 2

appinfo/routes.php

@@ -88,7 +88,7 @@
 		['name' => 'Icon#getLocalIconList', 'url' => '/api/v2/icon/list', 'verb' => 'GET'],
 
 		//
-		['name' => 'Vault#preflighted_cors', 'url' => '/api/v2/{path}', 'verb' => 'OPTIONS', 'requirements' => array('path' => '.+')],
+		['name' => 'Vault#preflighted_cors', 'url' => '/api/v2/{path}', 'verb' => 'OPTIONS', 'requirements' => ['path' => '.+']],
 		//Internal API
 		['name' => 'Internal#remind', 'url' => '/api/internal/notifications/remind/{credential_id}', 'verb' => 'POST'],
 		['name' => 'Internal#read', 'url' => '/api/internal/notifications/read/{credential_id}', 'verb' => 'DELETE'],

composer.json

@@ -6,5 +6,12 @@
             "email": "brantje@gmail.com"
         }
     ],
-    "require": {}
+    "license": "AGPL-3.0-or-later",
+    "scripts": {
+        "rector:check": "rector --dry-run --clear-cache",
+        "rector:fix": "rector"
+    },
+    "require-dev": {
+        "rector/rector": "^2.1"
+    }
 }

lib/Activity.php

@@ -227,18 +227,15 @@ public function getSpecialParameterList($app, $text) {
 	 * @param string $type
 	 * @return string|false
 	 */
-	public function getTypeIcon($type) {
-		switch ($type) {
-			case self::TYPE_ITEM_ACTION:
-			case self::TYPE_ITEM_EXPIRED:
-				return 'icon-password';
-			case self::TYPE_ITEM_SHARED:
-				return 'icon-share';
-			case self::TYPE_ITEM_RENAMED:
-				return 'icon-rename';
-		}
-		return false;
-	}
+	public function getTypeIcon($type)
+    {
+        return match ($type) {
+            self::TYPE_ITEM_ACTION, self::TYPE_ITEM_EXPIRED => 'icon-password',
+            self::TYPE_ITEM_SHARED => 'icon-share',
+            self::TYPE_ITEM_RENAMED => 'icon-rename',
+            default => false,
+        };
+    }
 
 	/**
 	 * The extension can define the parameter grouping by returning the index as integer.

lib/AppInfo/Application.php

@@ -84,13 +84,9 @@ public function register(IRegistrationContext $context): void {
 
 		$context->registerSearchProvider(Provider::class);
 
-		$context->registerService(View::class, function () {
-			return new View('');
-		}, false);
+		$context->registerService(View::class, fn() => new View(''), false);
 
-		$context->registerService('isCLI', function () {
-			return \OC::$CLI;
-		});
+		$context->registerService('isCLI', fn() => \OC::$CLI);
 
 		$context->registerMiddleware(ShareMiddleware::class);
 		$context->registerMiddleware(APIMiddleware::class);
@@ -121,20 +117,16 @@ public function register(IRegistrationContext $context): void {
 		});
 
 
-		$context->registerService('CronService', function (ContainerInterface $c) {
-			return new CronService(
+		$context->registerService('CronService', fn(ContainerInterface $c) => new CronService(
 				$c->get(CredentialService::class),
 				$c->get(LoggerInterface::class),
 				$c->get(Utils::class),
 				$c->get(NotificationService::class),
 				$c->get(ActivityService::class),
 				$c->get(IDBConnection::class)
-			);
-		});
+			));
 
-		$context->registerService('Logger', function (ContainerInterface $c) {
-			return $c->get(ServerContainer::class)->getLogger();
-		});
+		$context->registerService('Logger', fn(ContainerInterface $c) => $c->get(ServerContainer::class)->getLogger());
 	}
 
 	public function boot(IBootContext $context): void {

lib/BackgroundJob/ExpireCredentials.php

@@ -45,7 +45,7 @@ class ExpireCredentials extends TimedJob {
 	public function __construct(
 		ITimeFactory $timeFactory,
 		protected IConfig $config,
-		private CronService $cronService,
+		private readonly CronService $cronService,
 	) {
         parent::__construct($timeFactory);
 

lib/Controller/AdminController.php

@@ -22,33 +22,28 @@
 use OCA\PassmanNext\Utility\Utils;
 use OCP\AppFramework\ApiController;
 use OCP\AppFramework\Http\JSONResponse;
-use OCP\IConfig;
 use OCP\IRequest;
 use OCP\IUserManager;
 
 
 class AdminController extends ApiController {
-	private $userId;
-
 	public function __construct(
 		$AppName,
 		IRequest $request,
-		$UserId,
-		private VaultService $vaultService,
-		private CredentialService $credentialService,
-		private FileService $fileService,
-		private CredentialRevisionService $revisionService,
-		private DeleteVaultRequestService $deleteVaultRequestService,
-		private IConfig $config,
-		private IUserManager $userManager,
+		private $userId,
+		private readonly VaultService $vaultService,
+		private readonly CredentialService $credentialService,
+		private readonly FileService $fileService,
+		private readonly CredentialRevisionService $revisionService,
+		private readonly DeleteVaultRequestService $deleteVaultRequestService,
+		private readonly IUserManager $userManager,
 	) {
 		parent::__construct(
 			$AppName,
 			$request,
 			'GET, POST, DELETE, PUT, PATCH, OPTIONS',
 			'Authorization, Content-Type, Accept',
 			86400);
-		$this->userId = $UserId;
 
 	}
 
@@ -121,7 +116,7 @@ public function acceptRequestDeletion($vault_guid, $requested_by){
 		$req = $this->deleteVaultRequestService->getDeleteRequestForVault($vault_guid);
 		try{
 			$vault = $this->vaultService->getByGuid($vault_guid, $requested_by);
-		} catch (\Exception $e){
+		} catch (\Exception){
 			//Ignore
 		}
 
@@ -175,7 +170,7 @@ public function deleteRequestDeletion($vault_guid) {
 		$result = false;
 		try {
 			$delete_request = $this->deleteVaultRequestService->getDeleteRequestForVault($vault_guid);
-		} catch (\Exception $exception){
+		} catch (\Exception){
 			// Ignore it
 		}
 

lib/Controller/CredentialController.php

@@ -28,25 +28,22 @@
 
 
 class CredentialController extends ApiController {
-	private $userId;
-
 	public function __construct(
 		$AppName,
 		IRequest $request,
-		$userId,
-		private CredentialService $credentialService,
-		private ActivityService $activityService,
-		private CredentialRevisionService $credentialRevisionService,
-		private ShareService $sharingService,
-		private SettingsService $settings,
+		private $userId,
+		private readonly CredentialService $credentialService,
+		private readonly ActivityService $activityService,
+		private readonly CredentialRevisionService $credentialRevisionService,
+		private readonly ShareService $sharingService,
+		private readonly SettingsService $settings,
 	) {
 		parent::__construct(
 			$AppName,
 			$request,
 			'GET, POST, DELETE, PUT, PATCH, OPTIONS',
 			'Authorization, Content-Type, Accept',
 			86400);
-		$this->userId = $userId;
 	}
 
 
@@ -146,8 +143,8 @@ public function updateCredential($changed, $created,
 
 
 		if (!hash_equals($storedCredential->getUserId(), $this->userId)) {
-			$acl = $this->sharingService->getCredentialAclForUser($this->userId, $storedCredential->getGuid());
-			if ($acl->hasPermission(SharingACL::WRITE)) {
+			$sharingAcl = $this->sharingService->getACL($this->userId, $storedCredential->getGuid());
+			if ($sharingAcl->hasPermission(SharingACL::WRITE)) {
 				$credential['shared_key'] = $storedCredential->getSharedKey();
 			} else {
 				return new DataResponse(['msg' => 'Not authorized'], Http::STATUS_UNAUTHORIZED);
@@ -156,7 +153,7 @@ public function updateCredential($changed, $created,
 				return new DataResponse(['msg' => 'Not authorized'], Http::STATUS_UNAUTHORIZED);
 			}
 
-			if (!$acl->hasPermission(SharingACL::FILES)) {
+			if (!$sharingAcl->hasPermission(SharingACL::FILES)) {
 				// what ever the client transmitted, if it has no files permission, the previous files content will be preserved
 				$credential['files'] = $storedCredential->getFiles();
 			}
@@ -199,7 +196,7 @@ public function updateCredential($changed, $created,
 
 		try {
 			$acl_list = $this->sharingService->getCredentialAclList($storedCredential->getGuid());
-		} catch (\Exception $exception) {
+		} catch (\Exception) {
 			// Just check if we have an acl list
 		}
 		if (!empty($acl_list)) {
@@ -264,7 +261,7 @@ public function updateCredential($changed, $created,
 	public function deleteCredential($credential_guid) {
 		try {
 			$credential = $this->credentialService->getCredentialByGUID($credential_guid, $this->userId);
-		} catch (\Exception $e) {
+		} catch (\Exception) {
 			return new NotFoundJSONResponse();
 		}
 		if ($credential instanceof Credential) {
@@ -285,7 +282,7 @@ public function deleteCredential($credential_guid) {
 	public function getRevision($credential_guid) {
 		try {
 			$credential = $this->credentialService->getCredentialByGUID($credential_guid);
-		} catch (\Exception $ex) {
+		} catch (\Exception) {
 			return new NotFoundJSONResponse();
 		}
 		// If the request was made by the owner of the credential
@@ -320,7 +317,7 @@ public function updateRevision($revision_id, $credential_data) {
 		$revision = null;
 		try {
 			$revision = $this->credentialRevisionService->getRevision($revision_id);
-		} catch (\Exception $exception) {
+		} catch (\Exception) {
 			return new JSONResponse([]);
 		}
 

lib/Controller/FileController.php

@@ -18,22 +18,19 @@
 use Psr\Log\LoggerInterface;
 
 class FileController extends ApiController {
-	private $userId;
-
 	public function __construct(
 		$AppName,
 		IRequest $request,
-		$UserId,
-		private FileService $fileService,
-		private LoggerInterface $logger,
+		private $userId,
+		private readonly FileService $fileService,
+		private readonly LoggerInterface $logger,
 	) {
 		parent::__construct(
 			$AppName,
 			$request,
 			'GET, POST, DELETE, PUT, PATCH, OPTIONS',
 			'Authorization, Content-Type, Accept',
 			86400);
-		$this->userId = $UserId;
 	}
 
 
@@ -72,9 +69,9 @@ public function deleteFile($file_id) {
 	 * @NoAdminRequired
 	 * @NoCSRFRequired
 	 */
-	public function deleteFiles($file_ids) {
+	public function deleteFiles(string $file_ids) {
 		$failed_file_ids = [];
-		if ($file_ids != null && !empty($file_ids)) {
+		if (!empty($file_ids)) {
 			$decoded_file_ids = json_decode($file_ids);
 			foreach ($decoded_file_ids as $file_id) {
 				try {
@@ -97,8 +94,8 @@ public function deleteFiles($file_ids) {
 	public function updateFile($file_id, $file_data, $filename) {
 		try {
 			$file = $this->fileService->getFile($file_id, $this->userId);
-		} catch (\Exception $doesNotExistException) {
-
+		} catch (\Exception) {
+			// do just nothing when having problems getting the requested file
 		}
 		if ($file) {
 			if ($file_data) {

lib/Controller/IconController.php

@@ -24,25 +24,22 @@
 use OCP\IURLGenerator;
 
 class IconController extends ApiController {
-	private $userId;
 	const ICON_CACHE_OFFSET = 2592000;  // 3600 * 24 * 30
 
 	public function __construct(
 		$AppName,
 		IRequest $request,
-		$UserId,
-		private CredentialService $credentialService,
-		private AppManager $am,
-		private IURLGenerator $urlGenerator,
+		private $userId,
+		private readonly CredentialService $credentialService,
+		private readonly AppManager $am,
+		private readonly IURLGenerator $urlGenerator,
 	) {
 		parent::__construct(
 			$AppName,
 			$request,
 			'GET, POST, DELETE, PUT, PATCH, OPTIONS',
 			'Authorization, Content-Type, Accept',
 			86400);
-		$this->userId = $UserId;
-
 	}
 
 	/**
@@ -78,7 +75,7 @@ public function getIcon($base64Url, $credentialId) {
 			try {
 				$credential = $this->credentialService->getCredentialById($credentialId, $this->userId);
 				$credential = $credential->jsonSerialize();
-			} catch (DoesNotExistException $e) {
+			} catch (DoesNotExistException) {
 				// Credential is not found, continue
 				$credential = false;
 			}
@@ -97,22 +94,22 @@ public function getIcon($base64Url, $credentialId) {
 				$data = $icon->icoData;
 				$type = $icon->icoType;
 			}
-		} catch (\InvalidArgumentException $e) {
+		} catch (\InvalidArgumentException) {
 			//no need to do stuff in catch
 			//if IconService fails the predefined $data and $type are used
 		}
 
 		if (isset($credential) && $credential['user_id'] == $this->userId) {
 			$iconData = [
-				'type' => ($type) ? $type : 'x-icon',
+				'type' => $type ?: 'x-icon',
 				'content' => base64_encode($data)
 			];
 			$credential['icon'] = json_encode($iconData);
 			try {
 				if ($credential) {
 					$this->credentialService->updateCredential($credential);
 				}
-			} catch (DriverException $exception) {
+			} catch (DriverException) {
 				/**
 				 * @FIXME Syntax error or access violation: 1118 Row size too large
 				 * This happens when favicons are quite big.
@@ -146,7 +143,7 @@ public function getLocalIconList() {
 		$icons = [];
 		foreach ($result as $icon) {
 			$iconPath = $icon;
-			$path = explode('passman/', $iconPath);
+			$path = explode('passman/', (string) $iconPath);
 			$pack = explode('/', $path[1])[2];
 			$mime = mime_content_type($iconPath);
 			if ($mime !== 'directory') {