From 207cb8c084af44f8f34ee42a7b38c5548239c254 Mon Sep 17 00:00:00 2001 From: Bastien Wermeille Date: Tue, 9 Jun 2026 15:02:19 +0200 Subject: [PATCH 1/3] Add support for argon2 --- src/Models/Attributes/Password.php | 16 ++++++++++++++++ src/Models/Concerns/HasPassword.php | 6 ++++++ tests/Unit/Models/Attributes/PasswordTest.php | 16 ++++++++++++++++ tests/Unit/Models/OpenLDAP/UserTest.php | 15 +++++++++++++++ 4 files changed, 53 insertions(+) diff --git a/src/Models/Attributes/Password.php b/src/Models/Attributes/Password.php index 71f4d06ec..f38ae95b2 100644 --- a/src/Models/Attributes/Password.php +++ b/src/Models/Attributes/Password.php @@ -102,6 +102,22 @@ public static function md5(string $password): string return '{MD5}'.static::makeHash($password, 'md5'); } + /** + * Make an argon2i password. + */ + public static function argon2i(string $password): string + { + return '{ARGON2I}'.password_hash($password, PASSWORD_ARGON2I); + } + + /** + * Make an argon2id password. + */ + public static function argon2id(string $password): string + { + return '{ARGON2ID}'.password_hash($password, PASSWORD_ARGON2ID); + } + /** * Make a non-salted NThash password. */ diff --git a/src/Models/Concerns/HasPassword.php b/src/Models/Concerns/HasPassword.php index 8eb01ef9f..6b55ce2d9 100644 --- a/src/Models/Concerns/HasPassword.php +++ b/src/Models/Concerns/HasPassword.php @@ -29,6 +29,12 @@ public function setPasswordAttribute(array|string $password): void // If the password given is an array, we can assume we // are changing the password for the current user. if (is_array($password)) { + if (in_array(strtolower($method), ['argon2i', 'argon2id'])) { + throw new LdapRecordException( + "Argon2 passwords cannot be changed using this method. Use the LDAP Password Modify extended operation instead." + ); + } + $this->setChangedPassword( $this->getHashedPassword($method, $password[0], $this->getPasswordSalt($method)), $this->getHashedPassword($method, $password[1]), diff --git a/tests/Unit/Models/Attributes/PasswordTest.php b/tests/Unit/Models/Attributes/PasswordTest.php index 67a2c45c8..7340b90c5 100644 --- a/tests/Unit/Models/Attributes/PasswordTest.php +++ b/tests/Unit/Models/Attributes/PasswordTest.php @@ -92,6 +92,22 @@ public function test_sha512crypt() $this->assertEquals($password, Password::sha512crypt('password', Password::getSalt($password))); } + public function test_argon2i() + { + $password = Password::argon2i('password'); + + $this->assertStringStartsWith('{ARGON2I}$argon2i$', $password); + $this->assertNotEquals($password, Password::argon2i('password')); + } + + public function test_argon2id() + { + $password = Password::argon2id('password'); + + $this->assertStringStartsWith('{ARGON2ID}$argon2id$', $password); + $this->assertNotEquals($password, Password::argon2id('password')); + } + // Unsalted Hash Tests. // public function test_sha() diff --git a/tests/Unit/Models/OpenLDAP/UserTest.php b/tests/Unit/Models/OpenLDAP/UserTest.php index e03a4d11f..3e0b5541b 100644 --- a/tests/Unit/Models/OpenLDAP/UserTest.php +++ b/tests/Unit/Models/OpenLDAP/UserTest.php @@ -4,6 +4,7 @@ use LdapRecord\Connection; use LdapRecord\Container; +use LdapRecord\LdapRecordException; use LdapRecord\Models\Attributes\Password; use LdapRecord\Models\OpenLDAP\User; use LdapRecord\Tests\TestCase; @@ -75,6 +76,20 @@ public function test_algo_and_salt_is_automatically_detected_when_changing_a_use $this->assertEquals(Password::CRYPT_SALT_TYPE_SHA512, $newAlgo); } + public function test_changing_argon2_password_throws_exception() + { + $this->expectException(LdapRecordException::class); + $this->expectExceptionMessage('Argon2 passwords cannot be changed using this method.'); + + $user = (new OpenLDAPUserTestStub)->setRawAttributes([ + 'userpassword' => [ + Password::argon2id('secret'), + ], + ]); + + $user->password = ['secret', 'new-secret']; + } + public function test_correct_auth_identifier_is_returned() { $entryUuid = 'foo'; From 8e64e261c2581a635bec0fa0839d1cef41f5a6cf Mon Sep 17 00:00:00 2001 From: Bastien Wermeille Date: Sun, 21 Jun 2026 14:09:57 +0200 Subject: [PATCH 2/3] Implement password change requiring exop --- src/Connection.php | 29 +++++++++ src/Ldap.php | 16 +++++ src/LdapInterface.php | 13 ++++ src/Models/Concerns/HasPassword.php | 72 +++++++++++++++++++-- src/Models/Model.php | 12 ++++ src/Testing/ConnectionFake.php | 12 ++++ src/Testing/LdapFake.php | 8 +++ tests/Unit/Models/OpenLDAP/UserTest.php | 86 ++++++++++++++++++++++++- 8 files changed, 240 insertions(+), 8 deletions(-) diff --git a/src/Connection.php b/src/Connection.php index de1116891..ccdb67830 100644 --- a/src/Connection.php +++ b/src/Connection.php @@ -326,6 +326,35 @@ public function isolate(Closure $operation): mixed } } + /** + * Change a user's password using the RFC 3062 Password Modify extended operation. + * + * The change is performed on an isolated connection bound as the user itself, + * so the directory verifies the current password (a self-service change) + * without altering the bind state of the primary connection. + * + * @throws LdapRecordException + */ + public function changePassword(string $dn, string $oldPassword, string $newPassword): bool|string + { + return $this->isolate(function (Connection $connection) use ($dn, $oldPassword, $newPassword) { + $connection->initialize(); + + // Binding as the user with their current password proves knowledge + // of it. A failed bind (e.g. an incorrect current password) leaves + // the change unapplied and surfaces as an exception below. + $response = $connection->getLdapConnection()->bind($dn, $oldPassword); + + if (! $response->successful()) { + throw new LdapRecordException( + 'Unable to change password. The current password is incorrect or the directory rejected the bind.' + ); + } + + return $connection->getLdapConnection()->exopPasswd($dn, $oldPassword, $newPassword); + }); + } + /** * Attempt to get an exception for the cause of failure. */ diff --git a/src/Ldap.php b/src/Ldap.php index 09350e759..31ac1adbe 100644 --- a/src/Ldap.php +++ b/src/Ldap.php @@ -368,6 +368,22 @@ public function modifyBatch(string $dn, array $values): bool }); } + /** + * {@inheritdoc} + */ + public function exopPasswd(string $user = '', string $oldPassword = '', string $newPassword = '', ?array &$controls = null): bool|string + { + if (! function_exists('ldap_exop_passwd')) { + throw new LdapRecordException( + 'The function [ldap_exop_passwd] is unavailable. Ensure your PHP LDAP extension supports extended operations.' + ); + } + + return $this->executeFailableOperation(function () use ($user, $oldPassword, $newPassword, &$controls) { + return ldap_exop_passwd($this->connection, $user, $oldPassword, $newPassword, $controls); + }); + } + /** * {@inheritdoc} */ diff --git a/src/LdapInterface.php b/src/LdapInterface.php index 80b6d6eed..499aa8dcd 100644 --- a/src/LdapInterface.php +++ b/src/LdapInterface.php @@ -564,6 +564,19 @@ public function modify(string $dn, array $entry): bool; */ public function modifyBatch(string $dn, array $values): bool; + /** + * Modify a password using the RFC 3062 Password Modify extended operation. + * + * Returns the server-generated password when no new password is supplied, + * true on success when a new password is given, or false on failure. + * + * @see https://www.php.net/manual/en/function.ldap-exop-passwd.php + * @see https://www.rfc-editor.org/rfc/rfc3062 + * + * @throws LdapRecordException + */ + public function exopPasswd(string $user = '', string $oldPassword = '', string $newPassword = '', ?array &$controls = null): bool|string; + /** * Add attribute values to current attributes. * diff --git a/src/Models/Concerns/HasPassword.php b/src/Models/Concerns/HasPassword.php index 6b55ce2d9..7955cee65 100644 --- a/src/Models/Concerns/HasPassword.php +++ b/src/Models/Concerns/HasPassword.php @@ -2,6 +2,7 @@ namespace LdapRecord\Models\Concerns; +use Closure; use LdapRecord\ConnectionException; use LdapRecord\LdapRecordException; use LdapRecord\Models\Attributes\Password; @@ -10,6 +11,11 @@ /** @mixin Model */ trait HasPassword { + /** + * A password change deferred until the model is saved. + */ + protected ?Closure $pendingPasswordChange = null; + /** * Set the password on the user. * @@ -29,15 +35,23 @@ public function setPasswordAttribute(array|string $password): void // If the password given is an array, we can assume we // are changing the password for the current user. if (is_array($password)) { - if (in_array(strtolower($method), ['argon2i', 'argon2id'])) { - throw new LdapRecordException( - "Argon2 passwords cannot be changed using this method. Use the LDAP Password Modify extended operation instead." + [$oldPassword, $newPassword] = $password; + + // Argon2 hashes embed a random salt, so the currently stored hash + // cannot be reproduced to emit a REMOVE/ADD batch modification. + // Instead we defer a self-service RFC 3062 Password Modify + // extended operation until the model is saved. + if ($this->passwordChangeRequiresExop($method)) { + $this->pendingPasswordChange = fn () => $this->getConnection()->changePassword( + $this->getDn(), $oldPassword, $newPassword ); + + return; } $this->setChangedPassword( - $this->getHashedPassword($method, $password[0], $this->getPasswordSalt($method)), - $this->getHashedPassword($method, $password[1]), + $this->getHashedPassword($method, $oldPassword, $this->getPasswordSalt($method)), + $this->getHashedPassword($method, $newPassword), $this->getPasswordAttributeName() ); } @@ -125,6 +139,54 @@ protected function setChangedPassword(string $oldPassword, string $newPassword, ); } + /** + * Determine if changing a password hashed with the given method requires + * an extended operation rather than a batch modification. + */ + protected function passwordChangeRequiresExop(string $method): bool + { + return match (strtolower($method)) { + 'argon2i', 'argon2id' => true, + default => false, + }; + } + + /** + * Determine if the model has a password change deferred until save. + */ + public function hasPendingPasswordChange(): bool + { + return ! is_null($this->pendingPasswordChange); + } + + /** + * Flush any password change deferred until save. + * + * @throws LdapRecordException + */ + public function flushPendingPasswordChange(): void + { + if (! $change = $this->pendingPasswordChange) { + return; + } + + // Clear the pending change before executing it so a failed operation + // cannot be re-applied if the save is retried. + $this->pendingPasswordChange = null; + + $change(); + } + + /** + * Perform any operations deferred until the model is saved. + * + * @throws LdapRecordException + */ + protected function performDeferredOperations(): void + { + $this->flushPendingPasswordChange(); + } + /** * Set the password on the model. */ diff --git a/src/Models/Model.php b/src/Models/Model.php index 24d42f808..3e95d0855 100644 --- a/src/Models/Model.php +++ b/src/Models/Model.php @@ -955,6 +955,11 @@ public function save(array $attributes = []): void $this->exists ? $this->performUpdate() : $this->performInsert(); + // Some changes (such as argon2 password changes) cannot be expressed as + // batch modifications and are deferred until the model is saved. These + // run unconditionally here, even when no batch modifications exist. + $this->performDeferredOperations(); + $this->dispatch('saved'); $this->modifications = []; @@ -962,6 +967,13 @@ public function save(array $attributes = []): void $this->in = null; } + /** + * Perform any operations deferred until the model is saved. + * + * @throws LdapRecordException + */ + protected function performDeferredOperations(): void {} + /** * Inserts the model into the directory. * diff --git a/src/Testing/ConnectionFake.php b/src/Testing/ConnectionFake.php index 2c76a06d5..73e810434 100644 --- a/src/Testing/ConnectionFake.php +++ b/src/Testing/ConnectionFake.php @@ -33,6 +33,18 @@ public static function make(array $config = [], string $ldap = LdapFake::class): return $connection; } + /** + * Clone the connection, reusing the same fake LDAP connection. + * + * Sharing the underlying fake allows operations performed on an isolated + * connection (e.g. self-service password changes) to be asserted through + * the original fake's expectations. + */ + public function replicate(): static + { + return new static($this->configuration, $this->ldap); + } + /** * Set the user to authenticate as. */ diff --git a/src/Testing/LdapFake.php b/src/Testing/LdapFake.php index a1608e1ac..3d2dc6e69 100644 --- a/src/Testing/LdapFake.php +++ b/src/Testing/LdapFake.php @@ -458,6 +458,14 @@ public function modifyBatch(string $dn, array $values): bool return $this->resolveExpectation(__FUNCTION__, func_get_args()); } + /** + * {@inheritdoc} + */ + public function exopPasswd(string $user = '', string $oldPassword = '', string $newPassword = '', ?array &$controls = null): bool|string + { + return $this->resolveExpectation(__FUNCTION__, func_get_args()); + } + /** * {@inheritdoc} */ diff --git a/tests/Unit/Models/OpenLDAP/UserTest.php b/tests/Unit/Models/OpenLDAP/UserTest.php index 3e0b5541b..fa05176d5 100644 --- a/tests/Unit/Models/OpenLDAP/UserTest.php +++ b/tests/Unit/Models/OpenLDAP/UserTest.php @@ -7,6 +7,8 @@ use LdapRecord\LdapRecordException; use LdapRecord\Models\Attributes\Password; use LdapRecord\Models\OpenLDAP\User; +use LdapRecord\Testing\DirectoryFake; +use LdapRecord\Testing\LdapFake; use LdapRecord\Tests\TestCase; class UserTest extends TestCase @@ -76,18 +78,96 @@ public function test_algo_and_salt_is_automatically_detected_when_changing_a_use $this->assertEquals(Password::CRYPT_SALT_TYPE_SHA512, $newAlgo); } - public function test_changing_argon2_password_throws_exception() + public function test_changing_argon2_password_defers_a_pending_change_instead_of_queuing_modifications() { - $this->expectException(LdapRecordException::class); - $this->expectExceptionMessage('Argon2 passwords cannot be changed using this method.'); + $user = (new OpenLDAPUserTestStub)->setRawAttributes([ + 'dn' => ['cn=jdoe,dc=local,dc=com'], + 'userpassword' => [ + Password::argon2id('secret'), + ], + ]); + + $user->password = ['secret', 'new-secret']; + + // The change is performed via an extended operation on save, so no + // batch modifications are queued for it. + $this->assertEmpty($user->getModifications()); + $this->assertTrue($user->hasPendingPasswordChange()); + } + + public function test_resetting_argon2_password_still_queues_a_single_replace_modification() + { + $user = (new OpenLDAPUserTestStub)->setRawAttributes([ + 'dn' => ['cn=jdoe,dc=local,dc=com'], + 'userpassword' => [ + Password::argon2id('secret'), + ], + ]); + + $user->password = 'new-secret'; + + $modifications = $user->getModifications(); + + $this->assertFalse($user->hasPendingPasswordChange()); + $this->assertCount(1, $modifications); + $this->assertEquals(LDAP_MODIFY_BATCH_REPLACE, $modifications[0]['modtype']); + $this->assertEquals('ARGON2ID', Password::getHashMethod($modifications[0]['values'][0])); + } + + public function test_changing_argon2_password_performs_a_self_service_exop_on_save() + { + $ldap = DirectoryFake::setup()->getLdapConnection(); + + $ldap->expect([ + LdapFake::operation('bind')->once() + ->with('cn=jdoe,dc=local,dc=com', 'secret') + ->andReturnResponse(), + + LdapFake::operation('exopPasswd')->once() + ->with('cn=jdoe,dc=local,dc=com', 'secret', 'new-secret') + ->andReturnTrue(), + ]); $user = (new OpenLDAPUserTestStub)->setRawAttributes([ + 'dn' => ['cn=jdoe,dc=local,dc=com'], 'userpassword' => [ Password::argon2id('secret'), ], ]); $user->password = ['secret', 'new-secret']; + + $user->save(); + + $this->assertFalse($user->hasPendingPasswordChange()); + + // Guards against the change being silently skipped because no batch + // modifications exist (the early return in Model::performUpdate). + $ldap->assertMinimumExpectationCounts(); + } + + public function test_changing_argon2_password_throws_when_current_password_is_rejected() + { + $ldap = DirectoryFake::setup()->getLdapConnection(); + + $ldap->expect([ + LdapFake::operation('bind')->once() + ->with('cn=jdoe,dc=local,dc=com', 'wrong-secret') + ->andReturnResponse(49), + ]); + + $user = (new OpenLDAPUserTestStub)->setRawAttributes([ + 'dn' => ['cn=jdoe,dc=local,dc=com'], + 'userpassword' => [ + Password::argon2id('secret'), + ], + ]); + + $user->password = ['wrong-secret', 'new-secret']; + + $this->expectException(LdapRecordException::class); + + $user->save(); } public function test_correct_auth_identifier_is_returned() From 02746d32b1978bcc658909286691e67e536e7cef Mon Sep 17 00:00:00 2001 From: Bastien Wermeille Date: Tue, 7 Jul 2026 22:08:54 +0200 Subject: [PATCH 3/3] Fix {ARGON2} scheme handling and harden deferred password changes - Use the {ARGON2} scheme prefix for argon2i/argon2id hashes: OpenLDAP's argon2 module registers only {ARGON2}, the variant being carried by the PHC string that follows it. Values stored with the previous {ARGON2I}/{ARGON2ID} prefixes were never recognized by slapd, failing every subsequent bind. The variant is now detected back from stored {ARGON2} values when determining the hash method. - Guard the argon2 methods with defined() checks so PHP builds without argon2 support throw a catchable LdapRecordException instead of a fatal undefined-constant Error. - Connect as the user in Connection::changePassword() instead of initializing and binding directly, so the connection is upgraded with StartTLS (when configured) before any credentials are transmitted. - Store the pending password change as an [old, new] pair rather than a closure, keeping models serializable and the deferred state inspectable. - Clear the pending change when the password is reassigned, on discardChanges(), and on setRawAttributes(); clear it only after the operation succeeds so a transiently failed change is re-attempted on a retried save. - Require an existing model with a distinguished name to defer a password change. - Run deferred operations inside performUpdate() so exop-only changes fire the updating/updated events, and run them before batch modifications to fail early and avoid partial directory updates. - Add Password::hashMethodRequiresExop() alongside the existing hash-method capability helpers; models may override passwordChangeRequiresExop() to route other schemes through the extended operation. - Prevent replicated ConnectionFake instances from resetting the shared fake's state on disconnect. --- src/Connection.php | 17 +- src/Models/Attributes/Password.php | 34 +++- src/Models/Concerns/HasPassword.php | 71 +++++++-- src/Models/Model.php | 28 +++- src/Testing/ConnectionFake.php | 24 ++- tests/Unit/Models/Attributes/PasswordTest.php | 14 +- tests/Unit/Models/OpenLDAP/UserTest.php | 146 +++++++++++++++++- tests/Unit/Testing/ConnectionFakeTest.php | 19 +++ 8 files changed, 315 insertions(+), 38 deletions(-) diff --git a/src/Connection.php b/src/Connection.php index ccdb67830..61b98d765 100644 --- a/src/Connection.php +++ b/src/Connection.php @@ -338,18 +338,11 @@ public function isolate(Closure $operation): mixed public function changePassword(string $dn, string $oldPassword, string $newPassword): bool|string { return $this->isolate(function (Connection $connection) use ($dn, $oldPassword, $newPassword) { - $connection->initialize(); - - // Binding as the user with their current password proves knowledge - // of it. A failed bind (e.g. an incorrect current password) leaves - // the change unapplied and surfaces as an exception below. - $response = $connection->getLdapConnection()->bind($dn, $oldPassword); - - if (! $response->successful()) { - throw new LdapRecordException( - 'Unable to change password. The current password is incorrect or the directory rejected the bind.' - ); - } + // Connecting as the user proves knowledge of their current + // password, and upgrades the connection with StartTLS when + // configured, before any credentials are transmitted. A + // failed bind throws, leaving the change unapplied. + $connection->connect($dn, $oldPassword); return $connection->getLdapConnection()->exopPasswd($dn, $oldPassword, $newPassword); }); diff --git a/src/Models/Attributes/Password.php b/src/Models/Attributes/Password.php index f38ae95b2..8d62063c3 100644 --- a/src/Models/Attributes/Password.php +++ b/src/Models/Attributes/Password.php @@ -104,18 +104,36 @@ public static function md5(string $password): string /** * Make an argon2i password. + * + * OpenLDAP's argon2 module registers the single scheme {ARGON2} — + * the variant is carried by the PHC string that follows it. + * + * @throws LdapRecordException */ public static function argon2i(string $password): string { - return '{ARGON2I}'.password_hash($password, PASSWORD_ARGON2I); + if (! defined('PASSWORD_ARGON2I')) { + throw new LdapRecordException('Argon2i hashing is not supported by this PHP build.'); + } + + return '{ARGON2}'.password_hash($password, PASSWORD_ARGON2I); } /** * Make an argon2id password. + * + * OpenLDAP's argon2 module registers the single scheme {ARGON2} — + * the variant is carried by the PHC string that follows it. + * + * @throws LdapRecordException */ public static function argon2id(string $password): string { - return '{ARGON2ID}'.password_hash($password, PASSWORD_ARGON2ID); + if (! defined('PASSWORD_ARGON2ID')) { + throw new LdapRecordException('Argon2id hashing is not supported by this PHP build.'); + } + + return '{ARGON2}'.password_hash($password, PASSWORD_ARGON2ID); } /** @@ -244,6 +262,18 @@ public static function getSalt(string $encryptedPassword): string throw new LdapRecordException('Could not extract salt from encrypted password.'); } + /** + * Determine if passwords hashed with the given method can only be + * changed using the password modify extended operation (RFC 3062). + * + * These hashes embed a random salt that cannot be extracted, so the + * stored hash cannot be reproduced for a REMOVE/ADD batch modification. + */ + public static function hashMethodRequiresExop(string $method): bool + { + return in_array(strtolower($method), ['argon2', 'argon2i', 'argon2id'], true); + } + /** * Determine if the hash method requires a salt to be given. * diff --git a/src/Models/Concerns/HasPassword.php b/src/Models/Concerns/HasPassword.php index 7955cee65..8185f2e1e 100644 --- a/src/Models/Concerns/HasPassword.php +++ b/src/Models/Concerns/HasPassword.php @@ -2,7 +2,6 @@ namespace LdapRecord\Models\Concerns; -use Closure; use LdapRecord\ConnectionException; use LdapRecord\LdapRecordException; use LdapRecord\Models\Attributes\Password; @@ -12,19 +11,23 @@ trait HasPassword { /** - * A password change deferred until the model is saved. + * The [old, new] password pair of a change deferred until the model is saved. */ - protected ?Closure $pendingPasswordChange = null; + protected ?array $pendingPasswordChange = null; /** * Set the password on the user. * * @throws ConnectionException + * @throws LdapRecordException */ public function setPasswordAttribute(array|string $password): void { $this->assertSecureConnection(); + // Setting a password always supersedes a previously deferred change. + $this->pendingPasswordChange = null; + // Here we will attempt to determine the password hash method in use // by parsing the users hashed password (if it as available). If a // method is determined, we will override the default here. @@ -42,9 +45,13 @@ public function setPasswordAttribute(array|string $password): void // Instead we defer a self-service RFC 3062 Password Modify // extended operation until the model is saved. if ($this->passwordChangeRequiresExop($method)) { - $this->pendingPasswordChange = fn () => $this->getConnection()->changePassword( - $this->getDn(), $oldPassword, $newPassword - ); + if (! $this->exists || ! $this->getDn()) { + throw new LdapRecordException( + 'A password change requires an existing model with a distinguished name.' + ); + } + + $this->pendingPasswordChange = [$oldPassword, $newPassword]; return; } @@ -142,13 +149,13 @@ protected function setChangedPassword(string $oldPassword, string $newPassword, /** * Determine if changing a password hashed with the given method requires * an extended operation rather than a batch modification. + * + * Overriding this to return true routes any password change through the + * RFC 3062 extended operation, letting the server hash the password. */ protected function passwordChangeRequiresExop(string $method): bool { - return match (strtolower($method)) { - 'argon2i', 'argon2id' => true, - default => false, - }; + return Password::hashMethodRequiresExop($method); } /** @@ -170,11 +177,23 @@ public function flushPendingPasswordChange(): void return; } - // Clear the pending change before executing it so a failed operation - // cannot be re-applied if the save is retried. + [$oldPassword, $newPassword] = $change; + + $this->getConnection()->changePassword( + $this->getDn(), $oldPassword, $newPassword + ); + + // Cleared only once the operation has succeeded, so + // a failed change is re-attempted on a retried save. $this->pendingPasswordChange = null; + } - $change(); + /** + * Determine if the model has operations deferred until save. + */ + protected function hasDeferredOperations(): bool + { + return $this->hasPendingPasswordChange(); } /** @@ -187,6 +206,26 @@ protected function performDeferredOperations(): void $this->flushPendingPasswordChange(); } + /** + * Discard attribute changes and reset the attributes to their original state. + */ + public function discardChanges(): static + { + $this->pendingPasswordChange = null; + + return parent::discardChanges(); + } + + /** + * Set the model's attributes with raw LDAP result values. + */ + public function setRawAttributes(array $attributes = []): static + { + $this->pendingPasswordChange = null; + + return parent::setRawAttributes($attributes); + } + /** * Set the password on the model. */ @@ -278,6 +317,12 @@ public function determinePasswordHashMethod(): ?string return null; } + // The {ARGON2} scheme carries its variant inside the PHC + // string following the prefix, not in the scheme name. + if (strcasecmp($method, 'argon2') === 0) { + return str_contains($password, '$argon2i$') ? 'argon2i' : 'argon2id'; + } + if (! $hashAndAlgo = Password::getHashMethodAndAlgo($password)) { return $method; } diff --git a/src/Models/Model.php b/src/Models/Model.php index 3e95d0855..45a008bc0 100644 --- a/src/Models/Model.php +++ b/src/Models/Model.php @@ -955,11 +955,6 @@ public function save(array $attributes = []): void $this->exists ? $this->performUpdate() : $this->performInsert(); - // Some changes (such as argon2 password changes) cannot be expressed as - // batch modifications and are deferred until the model is saved. These - // run unconditionally here, even when no batch modifications exist. - $this->performDeferredOperations(); - $this->dispatch('saved'); $this->modifications = []; @@ -967,6 +962,14 @@ public function save(array $attributes = []): void $this->in = null; } + /** + * Determine if the model has operations deferred until save. + */ + protected function hasDeferredOperations(): bool + { + return false; + } + /** * Perform any operations deferred until the model is saved. * @@ -1028,13 +1031,24 @@ protected function performInsert(): void */ protected function performUpdate(): void { - if (! count($modifications = $this->getModifications())) { + $modifications = $this->getModifications(); + + // Deferred operations (such as argon2 password changes) cannot be + // expressed as batch modifications, but still constitute an update. + if (! count($modifications) && ! $this->hasDeferredOperations()) { return; } $this->dispatch('updating'); - $this->newQuery()->update($this->dn, $modifications); + // Deferred operations run first — they verify the user's current + // credentials, so they are more likely to be rejected than the + // batch modifications, and failing early avoids partial updates. + $this->performDeferredOperations(); + + if (count($modifications)) { + $this->newQuery()->update($this->dn, $modifications); + } $this->dispatch('updated'); diff --git a/src/Testing/ConnectionFake.php b/src/Testing/ConnectionFake.php index 73e810434..132a0692e 100644 --- a/src/Testing/ConnectionFake.php +++ b/src/Testing/ConnectionFake.php @@ -21,6 +21,11 @@ class ConnectionFake extends Connection */ protected bool $connected = false; + /** + * Whether the fake is a replica sharing another connection's fake LDAP connection. + */ + protected bool $replicated = false; + /** * Make a new fake LDAP connection instance. */ @@ -42,7 +47,24 @@ public static function make(array $config = [], string $ldap = LdapFake::class): */ public function replicate(): static { - return new static($this->configuration, $this->ldap); + $replica = new static($this->configuration, $this->ldap); + + $replica->replicated = true; + + return $replica; + } + + /** + * {@inheritdoc} + * + * Disconnecting a replica must not reset the state of + * the fake LDAP connection it shares with its parent. + */ + public function disconnect(): void + { + if (! $this->replicated) { + parent::disconnect(); + } } /** diff --git a/tests/Unit/Models/Attributes/PasswordTest.php b/tests/Unit/Models/Attributes/PasswordTest.php index 7340b90c5..d37279351 100644 --- a/tests/Unit/Models/Attributes/PasswordTest.php +++ b/tests/Unit/Models/Attributes/PasswordTest.php @@ -96,7 +96,7 @@ public function test_argon2i() { $password = Password::argon2i('password'); - $this->assertStringStartsWith('{ARGON2I}$argon2i$', $password); + $this->assertStringStartsWith('{ARGON2}$argon2i$', $password); $this->assertNotEquals($password, Password::argon2i('password')); } @@ -104,10 +104,20 @@ public function test_argon2id() { $password = Password::argon2id('password'); - $this->assertStringStartsWith('{ARGON2ID}$argon2id$', $password); + $this->assertStringStartsWith('{ARGON2}$argon2id$', $password); $this->assertNotEquals($password, Password::argon2id('password')); } + public function test_hash_method_requires_exop() + { + $this->assertTrue(Password::hashMethodRequiresExop('argon2')); + $this->assertTrue(Password::hashMethodRequiresExop('argon2i')); + $this->assertTrue(Password::hashMethodRequiresExop('ARGON2ID')); + + $this->assertFalse(Password::hashMethodRequiresExop('ssha')); + $this->assertFalse(Password::hashMethodRequiresExop('md5')); + } + // Unsalted Hash Tests. // public function test_sha() diff --git a/tests/Unit/Models/OpenLDAP/UserTest.php b/tests/Unit/Models/OpenLDAP/UserTest.php index fa05176d5..26dee9186 100644 --- a/tests/Unit/Models/OpenLDAP/UserTest.php +++ b/tests/Unit/Models/OpenLDAP/UserTest.php @@ -6,6 +6,8 @@ use LdapRecord\Container; use LdapRecord\LdapRecordException; use LdapRecord\Models\Attributes\Password; +use LdapRecord\Models\Events\Updated; +use LdapRecord\Models\Events\Updating; use LdapRecord\Models\OpenLDAP\User; use LdapRecord\Testing\DirectoryFake; use LdapRecord\Testing\LdapFake; @@ -111,7 +113,72 @@ public function test_resetting_argon2_password_still_queues_a_single_replace_mod $this->assertFalse($user->hasPendingPasswordChange()); $this->assertCount(1, $modifications); $this->assertEquals(LDAP_MODIFY_BATCH_REPLACE, $modifications[0]['modtype']); - $this->assertEquals('ARGON2ID', Password::getHashMethod($modifications[0]['values'][0])); + $this->assertEquals('ARGON2', Password::getHashMethod($modifications[0]['values'][0])); + $this->assertStringContainsString('$argon2id$', $modifications[0]['values'][0]); + } + + public function test_reassigning_a_password_clears_a_pending_password_change() + { + $user = (new OpenLDAPUserTestStub)->setRawAttributes([ + 'dn' => ['cn=jdoe,dc=local,dc=com'], + 'userpassword' => [ + Password::argon2id('secret'), + ], + ]); + + $user->password = ['secret', 'new-secret']; + + $this->assertTrue($user->hasPendingPasswordChange()); + + $user->password = 'reset-secret'; + + // The reset supersedes the deferred change entirely — only + // the replace modification may reach the directory on save. + $this->assertFalse($user->hasPendingPasswordChange()); + $this->assertCount(1, $user->getModifications()); + } + + public function test_discarding_changes_clears_a_pending_password_change() + { + $user = (new OpenLDAPUserTestStub)->setRawAttributes([ + 'dn' => ['cn=jdoe,dc=local,dc=com'], + 'userpassword' => [ + Password::argon2id('secret'), + ], + ]); + + $user->password = ['secret', 'new-secret']; + + $user->discardChanges(); + + $this->assertFalse($user->hasPendingPasswordChange()); + } + + public function test_a_pending_password_change_survives_serialization() + { + $user = (new OpenLDAPUserTestStub)->setRawAttributes([ + 'dn' => ['cn=jdoe,dc=local,dc=com'], + 'userpassword' => [ + Password::argon2id('secret'), + ], + ]); + + $user->password = ['secret', 'new-secret']; + + $restored = unserialize(serialize($user)); + + $this->assertTrue($restored->hasPendingPasswordChange()); + } + + public function test_changing_argon2_password_throws_when_model_does_not_exist() + { + $user = new OpenLDAPUserTestStub([ + 'userpassword' => Password::argon2id('secret'), + ]); + + $this->expectException(LdapRecordException::class); + + $user->password = ['secret', 'new-secret']; } public function test_changing_argon2_password_performs_a_self_service_exop_on_save() @@ -170,6 +237,83 @@ public function test_changing_argon2_password_throws_when_current_password_is_re $user->save(); } + public function test_changing_argon2_password_fires_update_events_on_save() + { + $ldap = DirectoryFake::setup()->getLdapConnection(); + + $ldap->expect([ + LdapFake::operation('bind')->once()->andReturnResponse(), + LdapFake::operation('exopPasswd')->once()->andReturnTrue(), + ]); + + $user = (new OpenLDAPUserTestStub)->setRawAttributes([ + 'dn' => ['cn=jdoe,dc=local,dc=com'], + 'userpassword' => [ + Password::argon2id('secret'), + ], + ]); + + $user->password = ['secret', 'new-secret']; + + $fired = []; + + $dispatcher = Container::getInstance()->getDispatcher(); + + $dispatcher->listen(Updating::class, function () use (&$fired) { + $fired[] = 'updating'; + }); + + $dispatcher->listen(Updated::class, function () use (&$fired) { + $fired[] = 'updated'; + }); + + $user->save(); + + $this->assertEquals(['updating', 'updated'], $fired); + } + + public function test_failed_argon2_password_change_is_retained_and_can_be_retried() + { + $ldap = DirectoryFake::setup()->getLdapConnection(); + + $ldap->expect([ + LdapFake::operation('bind')->once() + ->with('cn=jdoe,dc=local,dc=com', 'secret') + ->andReturnResponse(49), + + LdapFake::operation('bind')->once() + ->with('cn=jdoe,dc=local,dc=com', 'secret') + ->andReturnResponse(), + + LdapFake::operation('exopPasswd')->once() + ->with('cn=jdoe,dc=local,dc=com', 'secret', 'new-secret') + ->andReturnTrue(), + ]); + + $user = (new OpenLDAPUserTestStub)->setRawAttributes([ + 'dn' => ['cn=jdoe,dc=local,dc=com'], + 'userpassword' => [ + Password::argon2id('secret'), + ], + ]); + + $user->password = ['secret', 'new-secret']; + + try { + $user->save(); + + $this->fail('Expected the initial save to throw.'); + } catch (LdapRecordException) { + // A transiently failed change is retained for retry. + } + + $this->assertTrue($user->hasPendingPasswordChange()); + + $user->save(); + + $this->assertFalse($user->hasPendingPasswordChange()); + } + public function test_correct_auth_identifier_is_returned() { $entryUuid = 'foo'; diff --git a/tests/Unit/Testing/ConnectionFakeTest.php b/tests/Unit/Testing/ConnectionFakeTest.php index 9f4969609..8f04c527f 100644 --- a/tests/Unit/Testing/ConnectionFakeTest.php +++ b/tests/Unit/Testing/ConnectionFakeTest.php @@ -61,6 +61,25 @@ public function test_acting_as_with_dn() $this->assertTrue($fake->auth()->attempt('cn=John Doe,dc=local,dc=com', 'secret', $stayBound = true)); } + + public function test_replicated_connection_shares_the_fake_without_resetting_it_on_disconnect() + { + $fake = ConnectionFake::make(); + + $ldap = $fake->getLdapConnection(); + + $ldap->connect('localhost', 389); + + $replica = $fake->replicate(); + + $this->assertSame($ldap, $replica->getLdapConnection()); + + // Disconnecting the replica (as Connection::isolate() does) must + // not reset the state of the fake shared with the parent. + $replica->disconnect(); + + $this->assertTrue($ldap->isConnected()); + } } class ExtendedLdapFake extends LdapFake {}