Merge pull request #651 from nextcloud/chore/psalm

chore: Add psalm

.github/workflows/psalm.yml

@@ -0,0 +1,44 @@
+# This workflow is provided via the organization template repository
+#
+# https://github.com/nextcloud/.github
+# https://docs.github.com/en/actions/learn-github-actions/sharing-workflows-with-your-organization
+#
+# SPDX-FileCopyrightText: 2022-2024 Nextcloud GmbH and Nextcloud contributors
+# SPDX-License-Identifier: MIT
+
+name: Static analysis
+
+on: pull_request
+
+concurrency:
+  group: psalm-${{ github.head_ref || github.run_id }}
+  cancel-in-progress: true
+
+jobs:
+  static-analysis:
+    runs-on: ubuntu-latest
+
+    name: static-psalm-analysis
+    steps:
+      - name: Checkout
+        uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
+
+      - name: Get php version
+        id: versions
+        uses: icewind1991/nextcloud-version-matrix@58becf3b4bb6dc6cef677b15e2fd8e7d48c0908f # v1.3.1
+
+      - name: Set up php${{ steps.versions.outputs.php-available }}
+        uses: shivammathur/setup-php@c541c155eee45413f5b09a52248675b1a2575231 # v2.31.1
+        with:
+          php-version: ${{ steps.versions.outputs.php-available }}
+          extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, sqlite, pdo_sqlite
+          coverage: none
+          ini-file: development
+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+
+      - name: Install dependencies
+        run: composer i
+
+      - name: Run coding standards check
+        run: composer run psalm

REUSE.toml

@@ -6,7 +6,7 @@ SPDX-PackageSupplier = "Nextcloud <info@nextcloud.com>"
 SPDX-PackageDownloadLocation = "https://github.com/nextcloud/external"
 
 [[annotations]]
-path = ["package-lock.json", "package.json", ".l10nignore", "composer.json", "composer.lock", "vendor-bin/**/composer.json", "vendor-bin/**/composer.lock", ".tx/config", "js/vendor.LICENSE.txt", ".github/CODEOWNERS", ".eslintrc.json"]
+path = ["package-lock.json", "package.json", ".l10nignore", "composer.json", "composer.lock", "vendor-bin/**/composer.json", "vendor-bin/**/composer.lock", ".tx/config", "js/vendor.LICENSE.txt", ".github/CODEOWNERS", ".eslintrc.json", "tests/psalm-baseline.xml"]
 precedence = "aggregate"
 SPDX-FileCopyrightText = "none"
 SPDX-License-Identifier = "CC0-1.0"

composer.json

@@ -19,13 +19,17 @@
 	},
 	"scripts": {
 		"post-install-cmd": [
+			"@composer bin psalm install --ansi",
 			"@composer bin phpunit install --ansi"
 		],
 		"cs:fix": "php-cs-fixer fix",
 		"cs:check": "php-cs-fixer fix --dry-run --diff",
 		"lint": "find . -name \\*.php -not -path './vendor/*' -print0 | xargs -0 -n1 php -l",
 		"test:unit": "phpunit -c tests/phpunit.xml --no-coverage",
-		"test:unit:coverage": "phpunit -c tests/phpunit.xml"
+		"test:unit:coverage": "phpunit -c tests/phpunit.xml",
+		"psalm": "psalm --threads=1",
+		"psalm:update-baseline": "psalm --threads=1 --update-baseline",
+		"psalm:clear": "psalm --clear-cache && psalm --clear-global-cache"
 	},
 	"require-dev": {
 		"nextcloud/coding-standard": "^1.2",

lib/Capabilities.php

@@ -27,7 +27,7 @@ public function __construct(PasswordPolicyConfig $config, IURLGenerator $urlGene
 	/**
 	 * Function an app uses to return the capabilities
 	 *
-	 * @return array Array containing the apps capabilities
+	 * @return array<string, array<string, mixed>> Array containing the apps capabilities
 	 * @since 12.0.0
 	 */
 	public function getCapabilities(): array {

lib/Compliance/Expiration.php

@@ -63,7 +63,7 @@ public function update(IUser $user, string $password): void {
 			$user->getUID(),
 			'password_policy',
 			'pwd_last_updated',
-			time()
+			(string)time()
 		);
 	}
 
@@ -78,7 +78,7 @@ public function entryControl(IUser $user, ?string $password): void {
 		}
 	}
 
-	protected function isPasswordExpired(IUser $user) {
+	protected function isPasswordExpired(IUser $user): bool {
 		$updatedAt = (int)$this->config->getUserValue(
 			$user->getUID(),
 			'password_policy',
@@ -97,7 +97,7 @@ protected function isPasswordExpired(IUser $user) {
 		return $expiresIn <= time();
 	}
 
-	protected function isLocalUser(IUser $user) {
+	protected function isLocalUser(IUser $user): bool {
 		$localBackends = ['Database', 'Guests'];
 		return in_array($user->getBackendClassName(), $localBackends);
 	}

lib/Compliance/HistoryCompliance.php

@@ -73,13 +73,17 @@ public function update(IUser $user, string $password): void {
 		);
 	}
 
+	/**
+	 * @return list<string> List of previously used passwords (hashed)
+	 */
 	protected function getHistory(IUser $user): array {
 		$history = $this->config->getUserValue(
 			$user->getUID(),
 			'password_policy',
 			'passwordHistory',
 			'[]'
 		);
+		/** @var string[]|string */
 		$history = \json_decode($history, true);
 		if (!is_array($history)) {
 			$this->logger->warning(
@@ -90,6 +94,6 @@ protected function getHistory(IUser $user): array {
 		}
 		$history = \array_slice($history, 0, $this->policyConfig->getHistorySize());
 
-		return $history;
+		return \array_values($history);
 	}
 }

lib/ComplianceService.php

@@ -41,13 +41,13 @@ public function __construct(
 	) {
 	}
 
-	public function update(IUser $user, string $password) {
+	public function update(IUser $user, string $password): void {
 		foreach ($this->getInstance(IUpdatable::class) as $instance) {
 			$instance->update($user, $password);
 		}
 	}
 
-	public function audit(IUser $user, string $password) {
+	public function audit(IUser $user, string $password): void {
 		foreach ($this->getInstance(IAuditor::class) as $instance) {
 			$instance->audit($user, $password);
 		}
@@ -56,15 +56,14 @@ public function audit(IUser $user, string $password) {
 	/**
 	 * @throws LoginException
 	 */
-	//public function entryControl(IUser $user, string $password, bool $isTokenLogin) {
-	public function entryControl(string $loginName, ?string $password) {
+	public function entryControl(string $loginName, ?string $password): void {
 		$uid = $loginName;
 		\OCP\Util::emitHook('\OCA\Files_Sharing\API\Server2Server', 'preLoginNameUsedAsUserName', ['uid' => &$uid]);
 
 		/** @var IEntryControl $instance */
 		foreach ($this->getInstance(IEntryControl::class) as $instance) {
 			try {
-				$user = $this->userManager->get($uid);
+				$user = $this->userManager->get((string)$uid);
 
 				if ($user === null) {
 					break;
@@ -78,7 +77,9 @@ public function entryControl(string $loginName, ?string $password) {
 	}
 
 	/**
-	 * @returns Iterable
+	 * @template T
+	 * @psalm-param class-string<T> $interface
+	 * @return Iterable<T>
 	 */
 	protected function getInstance($interface) {
 		foreach (self::COMPLIANCERS as $compliance) {

lib/FailedLoginCompliance.php

@@ -14,25 +14,14 @@
 
 class FailedLoginCompliance {
 
-	/** @var IConfig */
-	private $config;
-
-	/** @var IUserManager */
-	private $userManager;
-
-	/** @var PasswordPolicyConfig */
-	private $passwordPolicyConfig;
-
 	public function __construct(
-		IConfig $config,
-		IUserManager $userManager,
-		PasswordPolicyConfig $passwordPolicyConfig) {
-		$this->config = $config;
-		$this->userManager = $userManager;
-		$this->passwordPolicyConfig = $passwordPolicyConfig;
+		private IConfig $config,
+		private IUserManager $userManager,
+		private PasswordPolicyConfig $passwordPolicyConfig,
+	) {
 	}
 
-	public function onFailedLogin(string $uid) {
+	public function onFailedLogin(string $uid): void {
 		$user = $this->userManager->get($uid);
 
 		if (!($user instanceof IUser)) {
@@ -63,15 +52,15 @@ public function onFailedLogin(string $uid) {
 		$this->setAttempts($uid, $attempts);
 	}
 
-	public function onSucessfullLogin(IUser $user) {
+	public function onSuccessfulLogin(IUser $user): void {
 		$this->setAttempts($user->getUID(), 0);
 	}
 
 	private function getAttempts(string $uid): int {
-		return (int)$this->config->getUserValue($uid, 'password_policy', 'failedLoginAttempts', 0);
+		return (int)$this->config->getUserValue($uid, 'password_policy', 'failedLoginAttempts', '0');
 	}
 
 	private function setAttempts(string $uid, int $attempts): void {
-		$this->config->setUserValue($uid, 'password_policy', 'failedLoginAttempts', $attempts);
+		$this->config->setUserValue($uid, 'password_policy', 'failedLoginAttempts', (string)$attempts);
 	}
 }

lib/Generator.php

@@ -13,25 +13,16 @@
 
 class Generator {
 
-	/** @var PasswordPolicyConfig */
-	private $config;
+	public const PASSWORD_GENERATION_MAX_ROUNDS = 10;
 
-	/** @var PasswordValidator */
-	private $validator;
-
-	/** @var ISecureRandom */
-	private $random;
-
-	public function __construct(PasswordPolicyConfig $config,
-		PasswordValidator $validator,
-		ISecureRandom $random) {
-		$this->config = $config;
-		$this->validator = $validator;
-		$this->random = $random;
+	public function __construct(
+		private PasswordPolicyConfig $config,
+		private PasswordValidator $validator,
+		private ISecureRandom $random,
+	) {
 	}
 
 	/**
-	 * @return string
 	 * @throws HintException
 	 */
 	public function generate(): string {
@@ -41,8 +32,7 @@ public function generate(): string {
 		$password = '';
 		$chars = '';
 
-		$found = false;
-		for ($i = 0; $i < 10; $i++) {
+		for ($i = 0; $i < self::PASSWORD_GENERATION_MAX_ROUNDS; $i++) {
 			if ($this->config->getEnforceUpperLowerCase()) {
 				$password .= $this->random->generate(1, ISecureRandom::CHAR_UPPER);
 				$password .= $this->random->generate(1, ISecureRandom::CHAR_LOWER);
@@ -67,20 +57,18 @@ public function generate(): string {
 			}
 
 			$password .= $chars = $this->random->generate($length, $chars);
-
 			// Shuffle string so the order is random
 			$password = str_shuffle($password);
 
+			if ($password === '') {
+				// something went wrong
+				break;
+			}
+
 			try {
 				$this->validator->validate($password);
-
-				if ($password === null || $password === '') {
-					// something went wrong
-					break;
-				}
-
-				$found = true;
-				break;
+				// Validation succeeded
+				return $password;
 			} catch (HintException $e) {
 				/*
 				 * Invalid so lets go for another round
@@ -90,10 +78,6 @@ public function generate(): string {
 			}
 		}
 
-		if ($found === false) {
-			throw new HintException('Could not generate a valid password');
-		}
-
-		return $password;
+		throw new HintException('Could not generate a valid password');
 	}
 }

lib/Listener/BeforePasswordUpdatedEventListener.php

@@ -13,6 +13,9 @@
 use OCP\EventDispatcher\IEventListener;
 use OCP\User\Events\BeforePasswordUpdatedEvent;
 
+/**
+ * @template-implements IEventListener<BeforePasswordUpdatedEvent>
+ */
 class BeforePasswordUpdatedEventListener implements IEventListener {
 	/** @var ComplianceService */
 	private $complianceUpdater;

lib/Listener/BeforeUserLoggedInEventListener.php

@@ -13,6 +13,9 @@
 use OCP\EventDispatcher\IEventListener;
 use OCP\User\Events\BeforeUserLoggedInEvent;
 
+/**
+ * @template-implements IEventListener<BeforeUserLoggedInEvent>
+ */
 class BeforeUserLoggedInEventListener implements IEventListener {
 	/** @var ComplianceService */
 	private $complianceUpdater;

lib/Listener/FailedLoginListener.php

@@ -13,6 +13,9 @@
 use OCP\EventDispatcher\Event;
 use OCP\EventDispatcher\IEventListener;
 
+/**
+ * @template-implements IEventListener<LoginFailedEvent>
+ */
 class FailedLoginListener implements IEventListener {
 	/** @var FailedLoginCompliance */
 	private $compliance;

lib/Listener/GenerateSecurePasswordEventListener.php

@@ -13,6 +13,9 @@
 use OCP\EventDispatcher\IEventListener;
 use OCP\Security\Events\GenerateSecurePasswordEvent;
 
+/**
+ * @template-implements IEventListener<GenerateSecurePasswordEvent>
+ */
 class GenerateSecurePasswordEventListener implements IEventListener {
 	/** @var Generator */
 	private $generator;

lib/Listener/PasswordUpdatedEventListener.php

@@ -13,6 +13,9 @@
 use OCP\EventDispatcher\IEventListener;
 use OCP\User\Events\PasswordUpdatedEvent;
 
+/**
+ * @template-implements IEventListener<PasswordUpdatedEvent>
+ */
 class PasswordUpdatedEventListener implements IEventListener {
 	/** @var ComplianceService */
 	private $complianceUpdater;

lib/Listener/SuccesfullLoginListener.php

@@ -13,6 +13,9 @@
 use OCP\EventDispatcher\IEventListener;
 use OCP\User\Events\UserLoggedInEvent;
 
+/**
+ * @template-implements IEventListener<UserLoggedInEvent>
+ */
 class SuccesfullLoginListener implements IEventListener {
 	/** @var FailedLoginCompliance */
 	private $compliance;
@@ -26,6 +29,6 @@ public function handle(Event $event): void {
 			return;
 		}
 
-		$this->compliance->onSucessfullLogin($event->getUser());
+		$this->compliance->onSuccessfulLogin($event->getUser());
 	}
 }

lib/Listener/ValidatePasswordPolicyEventListener.php

@@ -13,6 +13,9 @@
 use OCP\EventDispatcher\IEventListener;
 use OCP\Security\Events\ValidatePasswordPolicyEvent;
 
+/**
+ * @template-implements IEventListener<ValidatePasswordPolicyEvent>
+ */
 class ValidatePasswordPolicyEventListener implements IEventListener {
 	/** @var PasswordValidator */
 	private $passwordValidator;

lib/PasswordPolicyConfig.php

@@ -110,7 +110,7 @@ public function setEnforceSpecialCharacters(bool $enforceSpecialCharacters): voi
 	 * Do we check against the HaveIBeenPwned passwords
 	 */
 	public function getEnforceHaveIBeenPwned(): bool {
-		$hasInternetConnection = $this->config->getSystemValue('has_internet_connection', true);
+		$hasInternetConnection = $this->config->getSystemValueBool('has_internet_connection', true);
 		if (!$hasInternetConnection) {
 			return false;
 		}