diff --git a/module/VuFind/src/VuFind/Auth/ILSAuthenticator.php b/module/VuFind/src/VuFind/Auth/ILSAuthenticator.php index f1eb7460c11..cd08508a8c1 100644 --- a/module/VuFind/src/VuFind/Auth/ILSAuthenticator.php +++ b/module/VuFind/src/VuFind/Auth/ILSAuthenticator.php @@ -116,12 +116,12 @@ 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); } @@ -129,12 +129,12 @@ public function decrypt(string $text) /** * 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(); } diff --git a/module/VuFind/src/VuFind/Db/Row/User.php b/module/VuFind/src/VuFind/Db/Row/User.php index 091e68faeda..fb7f812ed62 100644 --- a/module/VuFind/src/VuFind/Db/Row/User.php +++ b/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 */ diff --git a/module/VuFind/tests/unit-tests/src/VuFindTest/Auth/ILSAuthenticatorTest.php b/module/VuFind/tests/unit-tests/src/VuFindTest/Auth/ILSAuthenticatorTest.php index a6e6145ecdc..f484def76ff 100644 --- a/module/VuFind/tests/unit-tests/src/VuFindTest/Auth/ILSAuthenticatorTest.php +++ b/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