Added seeUserPasswordDoesNotNeedRehash function #29
Conversation
|
I don't understand the context fully. |
|
Sure. This method is useful after persisting entities that implement the With this function you can verify that the password is correctly encrypted with the best available algorithm. (that is, the listener or event that you wrote to encode the user's password is working correctly.) I will update the docblock to include the context where it is intended to be used. |
src/Codeception/Module/Symfony.php
Outdated
| @@ -1074,6 +1074,56 @@ public function seeCurrentActionIs($action) | |||
| $this->fail("Action '$action' does not exist"); | |||
| } | |||
|
|
|||
| /** | |||
| * Checks that the user's password is encrypted | |||
There was a problem hiding this comment.
I find it odd that Symfony decided to name this feature password encoding
but using word "encrypted" is worse, because encrypting passwords is bad practice, they must be hashed.
I looked at Symfony Security component and it hashes passwords, except PlaintextPasswordEncoder.
There was a problem hiding this comment.
It's called 'encode' for historical reasons [1], i agree that using 'encrypt' left even more room for improvement.
However in the new description there is no mention of that word, thank you for letting me know.
src/Codeception/Module/Symfony.php
Outdated
| $this->markTestIncomplete('User password needs rehash'); | ||
| return; | ||
| } | ||
| $this->assertFalse($needsRehash); |
There was a problem hiding this comment.
Your description says that this method Checks that the user's password is encrypted,
but I don't see that in the code.
If the second parameter is provided, this method checks if the password is correct.
otherwise it marks test as incomplete if password needs rehashing.
The behaviour of this method just doesn't match description,
I would replace it with 2 separate methods - seeUserPasswordIsValid and skipTestIfPasswordNeedsRehash.
There was a problem hiding this comment.
@Naktibalda Thank you!
Symfony does not provide a method to directly check if a password is encoded.
If for example, i have a password encoded with bcrypt, needsRehash would return true, because there are better algorithms like argon2... so i changed the name of the function.
Now there are no intermediate results with markTestIncomplete and i think that verifying that the password is encoded with the best possible algorithm is the only thing i want to assert.
TavoNiievez
changed the title
Method to verify that the user's password would not benefit from rehashing.
If the user is not specified it is taken from the session.