Fix type handling/annotation related to encrypted passwords. (#3620)

module/VuFind/src/VuFind/Auth/ILSAuthenticator.php

@@ -116,25 +116,25 @@ public function passwordEncryptionEnabled()
     /**
      * Decrypt text.
      *
-     * @param string $text The text to decrypt
+     * @param ?string $text The text to decrypt (null values will be returned as null)
      *
-     * @return string|bool The decrypted string (or false if invalid)
+     * @return ?string|bool The decrypted string (null if empty or false if invalid)
      * @throws \VuFind\Exception\PasswordSecurity
      */
-    public function decrypt(string $text)
+    public function decrypt(?string $text)
     {
         return $this->encryptOrDecrypt($text, false);
     }
 
     /**
      * Encrypt text.
      *
-     * @param string $text The text to encrypt
+     * @param ?string $text The text to encrypt (null values will be returned as null)
      *
-     * @return string|bool The encrypted string (or false if invalid)
+     * @return ?string|bool The encrypted string (null if empty or false if invalid)
      * @throws \VuFind\Exception\PasswordSecurity
      */
-    public function encrypt(string $text)
+    public function encrypt(?string $text)
     {
         return $this->encryptOrDecrypt($text, true);
     }
@@ -143,18 +143,18 @@ public function encrypt(string $text)
      * This is a central function for encrypting and decrypting so that
      * logic is all in one location
      *
-     * @param string $text    The text to be encrypted or decrypted
-     * @param bool   $encrypt True if we wish to encrypt text, False if we wish to
+     * @param ?string $text    The text to be encrypted or decrypted
+     * @param bool    $encrypt True if we wish to encrypt text, False if we wish to
      * decrypt text.
      *
-     * @return string|bool    The encrypted/decrypted string
+     * @return ?string|bool    The encrypted/decrypted string (null = empty input; false = error)
      * @throws \VuFind\Exception\PasswordSecurity
      */
-    protected function encryptOrDecrypt($text, $encrypt = true)
+    protected function encryptOrDecrypt(?string $text, bool $encrypt = true)
     {
         // Ignore empty text:
-        if (empty($text)) {
-            return $text;
+        if ($text === null || $text === '') {
+            return null;
         }
 
         $configAuth = $this->config->Authentication ?? new \Laminas\Config\Config([]);
@@ -198,17 +198,22 @@ protected function encryptOrDecrypt($text, $encrypt = true)
     }
 
     /**
-     * Given a user object, retrieve the decrypted password.
+     * Given a user object, retrieve the decrypted password (or null if unset/invalid).
      *
      * @param UserEntityInterface $user User
      *
-     * @return string
+     * @return ?string
      */
     public function getCatPasswordForUser(UserEntityInterface $user)
     {
         if ($this->passwordEncryptionEnabled()) {
             $encrypted = $user->getCatPassEnc();
-            return !empty($encrypted) ? $this->decrypt($encrypted) : null;
+            $decrypted = !empty($encrypted) ? $this->decrypt($encrypted) : null;
+            if ($decrypted === false) {
+                // Unexpected error decrypting password; let's treat it as unset for now:
+                return null;
+            }
+            return $decrypted;
         }
         return $user->getRawCatPassword();
     }

module/VuFind/src/VuFind/Db/Row/User.php

@@ -136,8 +136,8 @@ public function saveCatalogId($catId)
     /**
      * Set ILS login credentials without saving them.
      *
-     * @param string $username Username to save
-     * @param string $password Password to save
+     * @param string  $username Username to save
+     * @param ?string $password Password to save (null for none)
      *
      * @return void
      */

module/VuFind/tests/unit-tests/src/VuFindTest/Auth/ILSAuthenticatorTest.php

@@ -29,6 +29,8 @@
 
 namespace VuFindTest\Auth;
 
+use PHPUnit\Framework\MockObject\MockObject;
+use VuFind\Auth\EmailAuthenticator;
 use VuFind\Auth\ILSAuthenticator;
 use VuFind\Auth\Manager;
 use VuFind\Db\Row\User;
@@ -50,7 +52,7 @@ class ILSAuthenticatorTest extends \PHPUnit\Framework\TestCase
      *
      * @return void
      */
-    public function testNewCatalogLoginSuccess()
+    public function testNewCatalogLoginSuccess(): void
     {
         $user = $this->getMockUser(['saveCredentials']);
         $user->expects($this->once())->method('saveCredentials')
@@ -71,7 +73,7 @@ public function testNewCatalogLoginSuccess()
      *
      * @return void
      */
-    public function testNewCatalogFailure()
+    public function testNewCatalogFailure(): void
     {
         $manager = $this->getMockManager(['getUserObject']);
         $manager->expects($this->any())->method('getUserObject')->willReturn(null);
@@ -88,7 +90,7 @@ public function testNewCatalogFailure()
      *
      * @return void
      */
-    public function testNewCatalogFailureByException()
+    public function testNewCatalogFailureByException(): void
     {
         $this->expectException(\VuFind\Exception\ILS::class);
         $this->expectExceptionMessage('kaboom');
@@ -107,7 +109,7 @@ public function testNewCatalogFailureByException()
      *
      * @return void
      */
-    public function testLoggedOutStoredLoginAttempt()
+    public function testLoggedOutStoredLoginAttempt(): void
     {
         $manager = $this->getMockManager(['getUserObject']);
         $manager->expects($this->any())->method('getUserObject')->willReturn(null);
@@ -120,7 +122,7 @@ public function testLoggedOutStoredLoginAttempt()
      *
      * @return void
      */
-    public function testSuccessfulStoredLoginAttempt()
+    public function testSuccessfulStoredLoginAttempt(): void
     {
         $user = $this->getMockUser(['getCatUsername', 'getRawCatPassword']);
         $user->expects($this->any())->method('getCatUsername')->willReturn('user');
@@ -144,7 +146,7 @@ public function testSuccessfulStoredLoginAttempt()
      *
      * @return void
      */
-    public function testUnsuccessfulStoredLoginAttempt()
+    public function testUnsuccessfulStoredLoginAttempt(): void
     {
         $user = $this->getMockUser(['clearCredentials', 'getCatUsername', 'getRawCatPassword']);
         $user->expects($this->any())->method('getCatUsername')->willReturn('user');
@@ -164,7 +166,7 @@ public function testUnsuccessfulStoredLoginAttempt()
      *
      * @return void
      */
-    public function testExceptionDuringStoredLoginAttempt()
+    public function testExceptionDuringStoredLoginAttempt(): void
     {
         $this->expectException(\VuFind\Exception\ILS::class);
         $this->expectExceptionMessage('kaboom');
@@ -183,16 +185,63 @@ public function testExceptionDuringStoredLoginAttempt()
         $auth->storedCatalogLogin();
     }
 
+    /**
+     * Test encryption and decryption of a string.
+     *
+     * @return void
+     */
+    public function testStringEncryptionAndDecryption(): void
+    {
+        $string = 'gobbledygook';
+        $auth = $this->getAuthenticator(config: $this->getAuthConfig());
+        $encrypted = $auth->encrypt($string);
+        $this->assertNotEquals($string, $encrypted);
+        $this->assertEquals($string, $auth->decrypt($encrypted));
+    }
+
+    /**
+     * Test encryption and decryption of null.
+     *
+     * @return void
+     */
+    public function testNullEncryptionAndDecryption(): void
+    {
+        $auth = $this->getAuthenticator(config: $this->getAuthConfig());
+        $this->assertNull($auth->encrypt(null));
+        $this->assertNull($auth->decrypt(null));
+    }
+
+    /**
+     * Get authentication-specific configuration.
+     *
+     * @return array
+     */
+    protected function getAuthConfig(): array
+    {
+        return [
+            'Authentication' => [
+                'ils_encryption_key' => 'foo',
+                'ils_encryption_algo' => 'aes',
+            ],
+        ];
+    }
+
     /**
      * Get an authenticator
      *
-     * @param Manager       $manager    Auth manager (null for default mock)
-     * @param ILSConnection $connection ILS connection (null for default mock)
+     * @param Manager            $manager    Auth manager (null for default mock)
+     * @param ILSConnection      $connection ILS connection (null for default mock)
+     * @param EmailAuthenticator $emailAuth  Email authenticator (null for default mock)
+     * @param array              $config     Configuration (null for empty)
      *
      * @return ILSAuthenticator
      */
-    protected function getAuthenticator(Manager $manager = null, ILSConnection $connection = null)
-    {
+    protected function getAuthenticator(
+        Manager $manager = null,
+        ILSConnection $connection = null,
+        EmailAuthenticator $emailAuth = null,
+        array $config = []
+    ): ILSAuthenticator {
         if (null === $manager) {
             $manager = $this->getMockManager();
         }
@@ -203,7 +252,9 @@ protected function getAuthenticator(Manager $manager = null, ILSConnection $conn
             function () use ($manager) {
                 return $manager;
             },
-            $connection
+            $connection,
+            $emailAuth ?? $this->createMock(EmailAuthenticator::class),
+            new \Laminas\Config\Config($config)
         );
     }
 
@@ -212,9 +263,9 @@ function () use ($manager) {
      *
      * @param array $methods Methods to mock
      *
-     * @return User
+     * @return MockObject&User
      */
-    protected function getMockUser($methods = [])
+    protected function getMockUser(array $methods = []): MockObject&User
     {
         return $this->getMockBuilder(\VuFind\Db\Row\User::class)
             ->disableOriginalConstructor()
@@ -227,9 +278,9 @@ protected function getMockUser($methods = [])
      *
      * @param array $methods Methods to mock
      *
-     * @return Manager
+     * @return MockObject&Manager
      */
-    protected function getMockManager($methods = [])
+    protected function getMockManager(array $methods = []): MockObject&Manager
     {
         return $this->getMockBuilder(\VuFind\Auth\Manager::class)
             ->disableOriginalConstructor()
@@ -242,9 +293,9 @@ protected function getMockManager($methods = [])
      *
      * @param array $methods Methods to mock
      *
-     * @return ILSConnection
+     * @return MockObject&ILSConnection
      */
-    protected function getMockConnection($methods = [])
+    protected function getMockConnection(array $methods = []): MockObject&ILSConnection
     {
         // We need to use addMethods here instead of onlyMethods, because
         // we're generally mocking behavior that gets handled by __call