fix: only retry urldecoded password when decoding changes it (LDAP badPwdCount double-increment)

lib/private/User/Manager.php

@@ -241,12 +241,18 @@ public function checkPasswordNoLogging($loginName, $password) {
 		// since http basic auth doesn't provide a standard way of handling non ascii password we allow password to be urlencoded
 		// we only do this decoding after using the plain password fails to maintain compatibility with any password that happens
 		// to contain urlencoded patterns by "accident".
-		$password = urldecode($password);
+		$decodedPassword = urldecode($password);
+		if ($decodedPassword === $password) {
+			// decoding the password did not change it, so retrying with the
+			// decoded value would just be a redundant duplicate authentication
+			// attempt (e.g. a second LDAP bind that double-increments badPwdCount).
+			return false;
+		}
 
 		foreach ($backends as $backend) {
 			if ($backend instanceof ICheckPasswordBackend || $backend->implementsActions(Backend::CHECK_PASSWORD)) {
 				/** @var ICheckPasswordBackend|UserInterface $backend */
-				$uid = $backend->checkPassword($loginName, $password);
+				$uid = $backend->checkPassword($loginName, $decodedPassword);
 				if ($uid !== false) {
 					return $this->getUserObject($uid, $backend);
 				}

tests/lib/User/ManagerTest.php

@@ -180,6 +180,55 @@ public function testCheckPasswordNotSupported(): void {
 		$this->assertFalse($manager->checkPassword('foo', 'bar'));
 	}
 
+	public function testCheckPasswordWrongPasswordCallsBackendOnce(): void {
+		$backend = $this->createMock(\Test\Util\User\Dummy::class);
+		$backend->expects($this->once())
+			->method('checkPassword')
+			->with($this->equalTo('foo'), $this->equalTo('wrongpass'))
+			->willReturn(false);
+
+		$backend->expects($this->any())
+			->method('implementsActions')
+			->willReturnCallback(function ($actions) {
+				if ($actions === BACKEND::CHECK_PASSWORD) {
+					return true;
+				} else {
+					return false;
+				}
+			});
+
+		$manager = new Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
+		$manager->registerBackend($backend);
+
+		$this->assertFalse($manager->checkPassword('foo', 'wrongpass'));
+	}
+
+	public function testCheckPasswordUrlencodedRetriesWithDecodedValue(): void {
+		$backend = $this->createMock(\Test\Util\User\Dummy::class);
+		$backend->expects($this->exactly(2))
+			->method('checkPassword')
+			->willReturnMap([
+				['foo', '%C3%A9', false],
+				['foo', urldecode('%C3%A9'), 'foo'],
+			]);
+
+		$backend->expects($this->any())
+			->method('implementsActions')
+			->willReturnCallback(function ($actions) {
+				if ($actions === BACKEND::CHECK_PASSWORD) {
+					return true;
+				} else {
+					return false;
+				}
+			});
+
+		$manager = new Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger);
+		$manager->registerBackend($backend);
+
+		$user = $manager->checkPassword('foo', '%C3%A9');
+		$this->assertTrue($user instanceof User);
+	}
+
 	public function testGetOneBackendExists(): void {
 		$backend = $this->createMock(\Test\Util\User\Dummy::class);
 		$backend->expects($this->once())