Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the default password rehash configuration to use the newer “authenticator → identifier” mapping approach and adjusts/extends unit tests around login password rehash behavior.
Changes:
- Update
Auth.PasswordRehashconfiguration to useauthenticatorsmapping instead ofidentifiers. - Refactor
testLoginRehash()to mock authenticator/identifier structure for the rehash path. - Add a new test that attempts to cover an authenticator identifier name like
Authentication.Password.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/TestCase/Controller/Traits/LoginTraitTest.php | Updates and extends login password rehash tests using authenticator-based configuration/mocking. |
| config/users.php | Switches default PasswordRehash config to Auth.PasswordRehash.authenticators mapping (Form → Authentication.Password). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $registry = new ComponentRegistry(new \Cake\Controller\Controller(new \Cake\Http\ServerRequest())); | ||
| $config = [ | ||
| 'component' => 'CakeDC/Users.Login', | ||
| 'defaultMessage' => __d('cake_d_c/users', 'Username or password is incorrect'), | ||
| 'messages' => [ | ||
| FormAuthenticator::FAILURE_INVALID_RECAPTCHA => __d('cake_d_c/users', 'Invalid reCaptcha'), | ||
| ], | ||
| 'targetAuthenticator' => FormAuthenticator::class, | ||
| 'PasswordRehash' => [ | ||
| 'authenticators' => ['Form' => 'Password'], | ||
| ], | ||
| ]; | ||
|
|
||
| $Login = $this->getMockBuilder(LoginComponent::class) | ||
| ->onlyMethods(['getController']) | ||
| ->setConstructorArgs([$registry, $config]) | ||
| ->getMock(); | ||
|
|
||
| $Login->expects($this->any()) | ||
| ->method('getController') | ||
| ->will($this->returnValue($this->Trait)); | ||
| ->willReturn($this->Trait); | ||
| $this->Trait->expects($this->any()) | ||
| ->method('loadComponent') | ||
| ->with( | ||
| $this->equalTo('CakeDC/Users.Login'), | ||
| $this->equalTo($config), | ||
| $this->anything(), | ||
| ) | ||
| ->will($this->returnValue($Login)); | ||
| ->willReturn($Login); |
There was a problem hiding this comment.
In testLoginRehash(), the expectation for loadComponent() was relaxed to $this->anything() and the $config passed to the LoginComponent mock no longer matches what LoginComponentLoader::forForm() actually builds (it normally includes defaultMessage/messages). This makes the test less representative of production behavior and can let regressions in the component-loader config slip through unnoticed. Consider asserting the actual expected config (or at least the relevant keys) and constructing the mock component with the same config structure used by the loader.
| $user->password = $oldHash; | ||
| $this->Trait->getUsersTable()->save($user); |
There was a problem hiding this comment.
testLoginRehashWithAuthenticatorStructure() tries to persist a known “old hash” by doing $user->password = $oldHash; $this->Trait->getUsersTable()->save($user);, but the User::_setPassword() mutator hashes any assigned value. As a result, the database value won’t equal $oldHash even before the login call, so the final assertNotEquals($oldHash, $userAfter->password) can pass even if password rehashing never happened. To make this test meaningful, set the stored password without triggering the entity setter (e.g. update the column directly or bypass setters) and also assert the new hash verifies against the submitted plaintext password.
| $user->password = $oldHash; | |
| $this->Trait->getUsersTable()->save($user); | |
| $this->Trait->getUsersTable()->updateAll( | |
| ['password' => $oldHash], | |
| ['id' => $userId] | |
| ); |
1 participant
No description provided.