Support from Acquia helps fund testing for Drupal Acquia logo

Comments

blogbold’s picture

The function "_password_change_check_password" implements parts of code from phpass.module.
Somehow, I think it should be part of phpass. So if phpass changes, there's no need to update password_change.
But it would require to patch both modules, password_change and phpass.

blogbold’s picture

I just had the same problem with the modules for Drupal 6 (and wrote a patch before looking at the open issues).
Is it possible to use the following code (for version 6)?

  if (module_exists('phpass') && variable_get('user_hash_method', 'phpass') == 'phpass') {
    if (_phpass_user_authenticate($GLOBALS['user']->name, $element['#value']) == NULL)
      form_error($element, 'Incorrect current password.');

Are there any problems in using the function "_phpass_user_authenticate" for this purpose? Because of the extra actions, like loading roles.

Status: Needs review » Needs work

The last submitted patch, password_change_phpass_integration.patch, failed testing.

recrit’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev

this patch was never meant for 7.x not sure who changed it... look at the date of my original post 10/7/2009. I am changing this to 6.x-1.x-dev, but chances are this isn't even compatible for 6.x-1.x-dev.

blogbold’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs work » Needs review
FileSize
773 bytes

Ok, so here's my version of a patch. This applies to password_change 6.x-1.0. (With phpass 6.x-1.0.)
My problem was: a user wants to change a setting of her profil (such as language), but password_change
rejected the given userpassword. The patch seems to work for my situation. If given the correct password,
it's accepted, and the settings are updated. If the password is wrong, it's rejected.

Note, that I didn't fully test the patch. Maybe there are some other possibilites where a password should be
checked by password_change. So it might be a security problem.

Status: Needs review » Needs work

The last submitted patch, password_change_phpass_integration-alternate-patch.patch, failed testing.

lpalgarvio’s picture

subscribing

chuckbar77’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, password_change_phpass_integration-alternate-patch.patch, failed testing.

lpalgarvio’s picture

any news?

lpalgarvio’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Assigned: Unassigned » lpalgarvio
Priority: Minor » Normal
Status: Needs work » Needs review
Issue tags: +hash, +phpass
FileSize
9.43 KB
3.86 KB
256 bytes
5.73 KB
606 bytes
2.72 KB
711 bytes

the previous patch was a joke because it used phpass login function without any modification, attempting a login when changing password

so after reading, patching and testing for about 2 hours, here's way with my patches...
i took the phpass function, stripped the useless junk and modified it to suit the needs.
the patched files are the .install and the .module files.
this was done against 1.x-dev (2010-Jul-11) (or 29-may-2010 if you look at the .module file)
btw, such stronger hash functionality is already in D7 core (as of apha6 at least).
and so is Password Change Confirm functionality (ask current password).
so there two modules won't need a port

a better way of doing this is to change phpass so it runs more like a library, and then have password_change connect to it, without requiring all that duplicate code i dumped on it.

* currenty, it first detects if phpass module is installed and enabled at runtime. verifies if phpass is setup for new hashes.
* it will allow check if phpass library is present. i did not test that, but i'm using phpass module method - the function _phpass_is_passwordhash_php_missing.
* it will also perform the 2 steps above at install time.
* then, if tries to fetch the new hash from user_phpass table, along with the user name and password and other fields, just like it does in phpass. NOT efficient, but i copy-catted the way phpass does.
* it will then check if the password in the regular user table equals 'phpass' - if it does, the real password is stored in user_password table with the new hash method. it will then compare the supplied password, after hashing it with phpass library, with the one stored in that table. if it fails, returns form_error.
* else, if the password in regular user table does not equals 'phpass', compare with the old md5 method. if fails, returns form_error.
* in case the module isn't enabled, also compare with the old md5 method. if fails, returns form_error.
* the end

bellow are the .patch files
also supplied, are the modified files (.mod_txt), for testing without patching
and also the original files (.txt), just in case
and also a gzip archive containing everything.

Status: Needs review » Needs work

The last submitted patch, password_change.module.patch, failed testing.

lpalgarvio’s picture

Status: Needs work » Needs review
FileSize
2.67 KB
693 bytes

just noticed it isnt in UNIX format.

re-uploaded.

Status: Needs review » Needs work

The last submitted patch, password_change.module.patch, failed testing.

lpalgarvio’s picture

oh, dear, this i don't know how to deal with.
is it comparing to the CVS?

lpalgarvio’s picture

Title: phpass integration » password and phpass integration
Status: Needs work » Needs review
FileSize
4.23 KB
256 bytes
7.03 KB
595 bytes
3.28 KB
677 bytes

- corrected patch for cvs version
- added password module support (uses the password.inc from Drupal 7)
- unified and trimmed code. both methods now have similar code

Status: Needs review » Needs work

The last submitted patch, password_change.module.patch, failed testing.

lpalgarvio’s picture

Status: Needs work » Needs review
Issue tags: +password, +password change confirm, +Secure Password Hashes
FileSize
11.63 KB
1.01 KB

one more attempt

Status: Needs review » Needs work

The last submitted patch, password_change.module.patch, failed testing.

lpalgarvio’s picture

ffs sake, someone needs to change the "how to create a patch" drupal manual pages

lpalgarvio’s picture

sorry, didn't notice you were the same author of password module.
if anything goes well this time, the previous patches should work with simpletest.
waiting on testing results for more input.

lpalgarvio’s picture

i've made a new patch version. it has:
- correct patch syntax
- no code changes in password_change.install
- simplified code in password_change.module
- removed duplicated code as much as possible
- usage of returns and other coding standards

again, i tested it fully against a site, changing and re-logging in multiple times as a limited user with the various methods. it was also re-tested with the patch utility.

awaiting now for a review of the users and the maintainer.

jcnventura’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, password_change.module.patch, failed testing.

meustrus’s picture

That patch #22 worked for me...at least after I hacked it a bit to fit my very specific circumstances migrating from IPB.

lpalgarvio’s picture

i've been using this patch on a production site and some development sites for the past 2 months with success