ability to change custom password fields, developped by @markus-96 (#864) #865
Conversation
There was a problem hiding this comment.
Here is a bunch of new remarks / comments:
-
basically, the code in
changecustompwdfield.phpis not working, there is much work to adapt it. For example, the checks on confirmpassword, newpassword and oldpassword are invalid, because you have replaced these fields by confirmcustompassword, newcustompassword and password. This is just an example, there are many other things to fix. Could you get the new branchchange-custom-password-fieldsand fix these problems? -
there is a big duplication of code in
changecustompwdfield.php. Basically, this file is a copy / paste of change.php. I think we should make much more factorization here. For now, I don't know exactly what is the best approach, so just ignore it for the moment. -
as you mentioned this before, some ppolicy checks are unusable, typically the pwd_no_reuse and pwd_diff_last_min_chars ones. The new ppolicy js code can be used for disabling these checks automatically. I will take this point in charge. The entropy of app-password could be displayed however. Why did you disable it?
-
the code in
lib/functions.inc.phpcan be factorized from Passwords: check and create hashes, modify passwords ltb-ldap#7 I'll give a look to this PR. -
check_password_strength and change_password signatures have changed, but there are still multiple places where these functions are called without the new arguments
davidcoutadeur
force-pushed
the
change-custom-password-fields
branch
2 times, most recently
from
22fdb3f
to
43ca4cf
Compare
davidcoutadeur
force-pushed
the
change-custom-password-fields
branch
2 times, most recently
from
1cb957e
to
a810f05
Compare
|
Basically, the feature seems to work on my side. I must do additional checks and especially give a look at the checkentropy modifications |
davidcoutadeur
force-pushed
the
change-custom-password-fields
branch
from
d69a90d
to
ac24473
Compare
|
Seems all good to me. Many small fixes here:
I have tested extensively the feature. The only bug I encounter sometimes is that the field that is modified is not the custompwdfield, but the userPassword. I suspect there is maybe a wrong redirection to index.php at some point, but I can't figure out where is the problem. (and can't reproduce) I write it here in case we reproduce one day the issue. @markus-96 could you do a last test of the feature, and tell me if everything is ok for you? If so, I'll squash and rebase the pull-request. Thank you for your multiple reviews and tests during this long-standing issue. |
davidcoutadeur
force-pushed
the
change-custom-password-fields
branch
from
da58deb
to
d42b9bc
Compare
htdocs/change.php
Outdated
| @@ -158,8 +158,8 @@ | |||
| #============================================================================== | |||
| # Check password strength | |||
| #============================================================================== | |||
| if ( !$result ) { | |||
| $result = check_password_strength( $newpassword, $oldpassword, $pwd_policy_config, $login, $entry_array ); | |||
| if ( $result === "" ) { | |||
There was a problem hiding this comment.
this has to be if ( !$result ) {.
Besides that, everything seams to be working as expected.
There was a problem hiding this comment.
Perfect, I have just fixed it in last commit. I am going to merge this
- fix change_password() call in changecustompwdfield - fix check_password call in check_password_strength function - improve custom password field documentation - add new custom password parameters in default configuration file - add missing parameters in custompwdfield doc - clean default values in changecustompwdfield - isolate policy parameter: pwd_unique_across_custom_password_fields - fix tests (new param pwd_unique_across_custom_password_fields) - add documentation for prehook / posthook in custompwdfield - improve code readibility - remove useless check for entropy page (#830, #864) - always consider checkentropy as an available action (#830, #864) - remove useless test (already done in ltb-ldap project) - adapt changecustompwdfield.tpl to bootstrap 5.3 upgrade - improve condition for testing check_password_strength
davidcoutadeur
force-pushed
the
change-custom-password-fields
branch
from
c410824
to
ea31808
Compare
Closes #864