add drupal and seafile password hash methods #2796
Conversation
|
seems like some tests are failing. can you have another look? |
|
Sure, I'm going to take a look. |
|
Hum... scrutinizer suggests to remove Yet, this instruction has been there for years and that condition did not appear because of this PR. So I don't want to remove it without your confirmation. |
There was a problem hiding this comment.
LGTM overall. I don't like the previous design of the arguments of djangopbkdf2which is reused here, but I guess it's fine for this PR to be approved.
Two asserttrue needs to be changed, though in PHP it doesn't quite matter.
Also for the scrutinizer, maybe just ignore its warning...
inc/PassHash.class.php
Outdated
| * @return string Hashed password | ||
| * @throws Exception when PHP is missing support for the method/algo | ||
| */ | ||
| public function hash_seafilepbkdf2($clear, $salt=null, $opts=array()) { |
There was a problem hiding this comment.
$opts that I don't quite like, but it was introduced by hash_djangopbkdf2.
inc/PassHash.class.php
Outdated
| // have 'U' added as the first character and need an extra md5(). | ||
| $hash = substr($hash, 1); | ||
| $clear = md5($clear); | ||
| } |
There was a problem hiding this comment.
A minor thing, but less popular methods should be moved further down in the check (as long as it's possible, eg. before very broad checks that might match otherwise).
There was a problem hiding this comment.
Sure. However, this does not easily fit in the long if elseif elseif ... list of test. The added U char does not identify the hash function. It only indicate to use $clear=md5($clear). Very probably, when such a password is used, the hashing function will be Drupal's sha512. The problem is that I cannot be 100% sure of that. According to https://stackoverflow.com/a/5031807/1831273 the hash function should be Drupal's sha512. but this Drupal 8 code shows that Drupal can use other hashing functions. That's why this is done first, and the normal tests to determine the hashing function are run afterwards.
If you want, I can modify the test to check U+sha512 and move it down.
|
Opps, @schplurtz would you be able to resolve the merge conflict, or I can do it for you if you want? |
|
Sorry for the delay, I was offline. |
|
I'm sorry @phy25. I don't see what I can do to resolve the conflict. Resolve conflicts button is greyed for me. And what I see when trying to manually merge upstream and my version is argon2id introduced in commit 1f993c3 in master branch conflicts with nothing (no text between ===== and >>>>>) in my version of the file. This puzzles me. What could I do in my version if literally nothing conflicts with upstream. I must be missing something. So please, resolve the conflict |
phy25
force-pushed
the
drupal-and-seafile
branch
from
fe6a38d
to
b98368c
Compare
|
I rebased @schplurtz's branch (which can only be done locally), and will do a final check after CI is passed. |
Hi
This adds support for seafile and drupal (6,7 and hopefully 8) hash methods.
Only password verification is tested. Hash generation is untested. Should it be ?