Splitting out part of #1845004: Replace custom password hashing library with PHP password_hash() based on discussion with catch
Suggested commit message:
git commit -m 'Issue #2503083 by claudiu.cristea, neclimdul, and pwolanin: Simplify PasswordInterface so it'\''s not coupled to UserInterface'
Problem
\Drupal\Core\Password\PasswordInterface is coupled to Drupal\user\UserInterface which is not needed and makes the code more complex
Proposed resolution
Simplify the interface (make it more like Drupal 7) so \Drupal\Core\Password\PasswordInterface only cares about string passwords and hashes, not user objects
Remaining tasks
change record
API changes
Method signatures changes:
\Drupal\Core\Password\PasswordInterface::check
\Drupal\Core\Password\PasswordInterface::hash
This should have no real impact outside core at this point.
Beta phase evaluation
Issue category | Task to improve the interface so we avoid breaking changes in 8.1 to switch hashing algorithms |
---|---|
Issue priority | Normal, because it only lacks interoperability. |
Disruption | Disruptive for any existing contrib code calling core's password interface. This would be limited to modules providing additional authentication mechanisms. |
Comment | File | Size | Author |
---|---|---|---|
#15 | increment.txt | 1022 bytes | pwolanin |
#15 | 2503083-15.patch | 20.58 KB | pwolanin |
#9 | increment.txt | 4.63 KB | pwolanin |
#9 | 2503083-9.patch | 20.43 KB | pwolanin |
#5 | 2503083-5.patch | 21.4 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedwow the existing checks in core/tests/Drupal/Tests/Core/Password/PasswordHashingTest.php don't make sense - a lot of the test variables are never set!
I think I got the relevant parts from the other patch, plus fixes to that test to make it actually test something.
Comment #2
neclimdulpatch
errr... crap... we both made patches
Comment #3
pwolanin CreditAttribution: pwolanin at Acquia commenteduse the const PasswordInterface::PASSWORD_MAX_LENGTH instead of magic numbers.
Comment #4
pwolanin CreditAttribution: pwolanin at Acquia commentedOops - that cross-post blew up. Here's my #2 patch again so it's tested.
Comment #5
pwolanin CreditAttribution: pwolanin at Acquia commentedreposting the same patch w/ new name since the bot isn't getting it.
Comment #6
neclimdulI didn't include this since it was out of scope from the IS. Are we sure we want to include that in this?
These test the same thing. Also the first test is not testing md5 as implied by the comment. We should remove the first test.
Comment #7
pwolanin CreditAttribution: pwolanin at Acquia commentedIf we are modifying the interface, I think add PASSWORD_MAX_LENGTH is reasonable.
Also - look in detail at my test fixes. The test is now testing a hash over an md5 hash and validating that it works.
Comment #8
neclimdulNo, and I don't appreciate you repeatedly implying I haven't read your patch. I read this about 5 times before you pinged me in IRC to make sure I understood it and additionally twice after talking on IRC to make sure I wasn't mistaken. It is not testing MD5 it testing a lower hash count which we have an explict test for later in test suite.
Let me see if I can explain this again more clearly.
Here you set your hashes. The md5 hash was missing previously, that's a good catch.
Here you test the MD5 hash, same as before, but a bit shorter then you also check the PhPass hashed password with the lower count but the comment mentions MD5 hash.
Here we have an explicit test where we assert that we flag lower hash counts to be rehashed.
Comment #9
pwolanin CreditAttribution: pwolanin at Acquia commented@neclimdul - ok I see, yes - some of the changes are redundant. Sorry I misunderstood your comment.
Comment #14
neclimdulnit.
(int) (PasswordInterface::PASSWORD_MAX_LENGTH / 3)
will achieve the same thing and handle future changes better.Comment #15
pwolanin CreditAttribution: pwolanin at Acquia commentedSure - this change uses floor() and % to make the math robust.
Comment #16
pwolanin CreditAttribution: pwolanin at Acquia commentedComment #17
cilefen CreditAttribution: cilefen commentedIf it may be disruptive it needs a change record.
Comment #18
pwolanin CreditAttribution: pwolanin at Acquia commented@cilifen - yes, I put it in the list of remaining tasks
Comment #19
neclimdulPatch works for me.
Comment #20
claudiu.cristeaAs you already touched the interface docs, why not cleaning up all the docs in that file? I already did that in #1845004-70: Replace custom password hashing library with PHP password_hash().
Comment #21
claudiu.cristeaAh, sorry, my bad. The non-standard docs were not in interface but in
PhpassHashedPassword
. We'll clean that after #1845004: Replace custom password hashing library with PHP password_hash() will go in.The patch looks good to me.
Comment #22
alexpottStill need a change record.
Comment #23
neclimdulMan its hard to find the link to create a change record... I think I've got it done.
Comment #24
cilefen CreditAttribution: cilefen commentedBack to RTBC with the change record.
Comment #26
alexpottThe lack of interoperability is a regression from D7 and disentangling PasswordInterace from UserInterface reduces fragility and makes it more useful. Committed fd2bc62 and pushed to 8.0.x. Thanks!
Fixed unnecessary use added by the patch on commit.
Comment #27
JeroenTThis issue already has a change record.