chore: Fix more psalm reported issues

Mostly missing type annotations and missing template parameters for EventListeners. Also fixed one typo in `SuccesfullLoginListener` (successful vs "sucessfull"). But there is also one potential issue: `HIBPValidator` was assuming `getBody` is always a string, but it could also be a resource. Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
nextcloud · Sep 2, 2024 · ee8dcc0 · ee8dcc0
1 parent 031ba33
commit ee8dcc0
Show file tree

Hide file tree

Showing 15 changed files with 63 additions and 45 deletions.
diff --git a/lib/Compliance/Expiration.php b/lib/Compliance/Expiration.php
@@ -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);
 	}

diff --git a/lib/Compliance/HistoryCompliance.php b/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);
 	}
 }
diff --git a/lib/ComplianceService.php b/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) {

diff --git a/lib/FailedLoginCompliance.php b/lib/FailedLoginCompliance.php
@@ -21,7 +21,7 @@ public function __construct(
 	) {
 	}
 
-	public function onFailedLogin(string $uid) {
+	public function onFailedLogin(string $uid): void {
 		$user = $this->userManager->get($uid);
 
 		if (!($user instanceof IUser)) {
@@ -52,7 +52,7 @@ 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);
 	}
 

diff --git a/lib/Generator.php b/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');
 	}
 }
diff --git a/lib/Listener/BeforePasswordUpdatedEventListener.php b/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;

diff --git a/lib/Listener/BeforeUserLoggedInEventListener.php b/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;

diff --git a/lib/Listener/FailedLoginListener.php b/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;

diff --git a/lib/Listener/GenerateSecurePasswordEventListener.php b/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;

diff --git a/lib/Listener/PasswordUpdatedEventListener.php b/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;

diff --git a/lib/Listener/SuccesfullLoginListener.php b/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());
 	}
 }
diff --git a/lib/Listener/ValidatePasswordPolicyEventListener.php b/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;

diff --git a/lib/PasswordPolicyConfig.php b/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;
 		}

diff --git a/lib/Validator/CommonPasswordsValidator.php b/lib/Validator/CommonPasswordsValidator.php
@@ -26,9 +26,14 @@ public function __construct(PasswordPolicyConfig $config, IL10N $l) {
 
 	public function validate(string $password): void {
 		$enforceNonCommonPassword = $this->config->getEnforceNonCommonPassword();
+		if (!$enforceNonCommonPassword) {
+			return;
+		}
+
 		$passwordFile = __DIR__ . '/../../lists/list-'.strlen($password).'.php';
-		if ($enforceNonCommonPassword && file_exists($passwordFile)) {
+		if (file_exists($passwordFile)) {
 			$commonPasswords = require_once $passwordFile;
+			assert(is_array($commonPasswords));
 			if (isset($commonPasswords[strtolower($password)])) {
 				$message = 'Password is among the 1,000,000 most common ones. Please make it unique.';
 				$message_t = $this->l->t(

diff --git a/lib/Validator/HIBPValidator.php b/lib/Validator/HIBPValidator.php
@@ -48,6 +48,9 @@ public function validate(string $password): void {
 			}
 
 			$result = $response->getBody();
+			if (is_resource($result)) {
+				$result = stream_get_contents($result);
+			}
 			$result = preg_replace('/^([0-9A-Z]+:0)$/m', '', $result);
 
 			if (strpos($result, $needle) !== false) {