Passwords: check and create hashes, modify passwords #7

markus-96 · 2024-03-01T17:19:21Z

The code in this PR mainly comes from SSP, and is a bit factorized by me.

I will tell you why the functions should be included here:

Function	can be used by SSP	can be used by SD	Why?
`make_xxx_password`	X	X	needed for `make_password()`
`check_xxx_password`	X	X	needed for `check_password()`
`make_password`	X	X	In SD, changing the Password is only possible using PPolicy. If you dont want this, you have to create the hash in SD
`check_password()`	X	X	can be to check if the current set password is the same as the new one (and is also part of a PR I have..)
`make_ad_password`	X	X	SD does not support AD yet, this could be a starting point
`get_hash_type`	X	X	same mechanism as in SSP: if you want to use `$hash = "auto"`
`set_samba_data`	X	X	SD does not provide support for Samba yet
`set_ad_data`	X	X	SD does not support AD yet, this could be a starting point
`set_shadow_data`	X	X	SD does not support shadow yet
`get_hashed_password`	X	X	if you want to use `$hash = "auto"`, you first have to get the current hash
`change_ad_password_as_user`	X		if `change_password_using_ppolicy()` and `change_password()` make they way to here, it should also be included
`change_password_with_exop`	X
`change_password_using_ppolicy`	X	X	currently, only password modifaction with enabled ppolicy is supported by SD. So, beeing able to parse ppolicy errors would be nice!
`change_password`	X	X	The default password modification

A starting point for following:

This is a proposal, maybe a starting point for a discussion what should and what should not be included here. For example, SD and WP share a big amount of similarity in there smarty templates. Maybe this can also be included here? Where is the limit?

markus-96 · 2024-03-01T17:20:04Z

At this point, the code is 100% untested.

davidcoutadeur

Having these new functions seems a good idea. Thanks for the proposal @markus-96

I have some remarks about renaming.

The main missing point is now to add a test for each function.

davidcoutadeur · 2024-03-13T14:49:37Z

src/Ltb/Ldap.php

+     * @param array $userdata the array, containing the new (hashed) password
+     * @return type
+     */
+    static function change_password($ldap, $dn, $userdata): array {


This function is making a generic modification described by $userdata. I see no reason for no reason for calling this function "change_password"... Also this function seems quite simple, not sure it is really helpful.

Thats true, and I thought much about including it here. The reason for including it was, as above, possible class abstraction.

davidcoutadeur · 2024-03-13T14:50:03Z

nbproject/project.properties

+src.dir=src
+tags.asp=false
+tags.short=false
+web.root=.


Please remove the metadata from your IDE

davidcoutadeur · 2024-03-13T14:50:12Z

nbproject/project.xml

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>


Please remove the metadata from your IDE

davidcoutadeur · 2024-03-13T14:50:46Z

src/Ltb/Ldap.php

+     * Gets the value of the password attribute
+     * @param \LDAP\Connection|array $ldap An LDAP\Connection instance, returned by ldap_connect()
+     * @param string $dn the dn of the user
+     * @param type $pwdattribute the Attriubte that contains the password


small typo: Attriubte

davidcoutadeur · 2024-03-13T14:55:14Z

src/Ltb/Ldap.php

+     * @param type $pwdattribute the Attriubte that contains the password
+     * @return string the value of $pwdattribute
+     */
+    static function get_hashed_password($ldap, $dn, $pwdattribute): string {


we should simply name this function get_password_value

At his step, we have no idea if the password is hashed or not. (it may be in cleartext)

We should clarify, and most importantly manage the returned values in these cases:

what is returned if the password cannot be read (no permission)?

what is returned if there is no password value at all?

davidcoutadeur · 2024-03-13T15:01:26Z

src/Ltb/Ldap.php

+        }
+        if ($exop_passwd === TRUE) {
+            # If password change works update other data
+            if (!empty($userdata)) {


So this function does not only change the password, it also modify some attributes defined in $userdata?

It think it is preferable to do atomic actions only. If the calling program needs to modify the password + some other attribute, it should call two different functions.

This comes from the fact that in SSP, you can also set some shadow, samba and ad data, see the appropriate functions set_xxx_data in Password.php. Of course, it can also be set in an additional ldap replace command, but I think one replace task is more appropriate.

davidcoutadeur · 2024-03-13T15:03:58Z

src/Ltb/Ldap.php

+        $error_msg = "";
+        $ctrls = array();
+        $ppolicy_error_code = "";
+        $ppolicy_replace = ldap_mod_replace_ext($ldap, $dn, $userdata, [['oid' => LDAP_CONTROL_PASSWORDPOLICYREQUEST]]);


This function does a generic modification, it should not be called change_password...
Maybe modify_attribute_using_ppolicy?

I thought about having an abstract class AbstractPasswordModify for password modification strategies and a concrete class for every strategy, ie PasswordModifyPPolicy, but this was what I ended up with.

Yes, in this state it should be called modify_attributes_using_ppolicy

davidcoutadeur · 2024-03-13T15:14:47Z

src/Ltb/Password.php

+                return check_md4_password($password, $hash);
+            default:
+                return $password == $hash;
+        }


I think there are some existing tests in ssp for verifying password functions.

Anyway, I think any of the function in ltb-ldap lib should have a unit test. This PR seems to be the opportunity to verify all the tests are done.

Checking/verifying passwords against a hash is not yet part of SSP, I have written them for my PR with the custom password fields ;)
I wrote tests for the PR, but I did not fully understand how the tests are made here so I did not include them here. See CheckHashAlgorithms: https://github.com/ltb-project/self-service-password/pull/751/files#diff-a1417d309e512c458344b37ea7144ad35645a4310866ff963332078770c5886b (I hope this link is correct)

davidcoutadeur · 2024-03-29T14:32:16Z

@markus-96 is this PR ready for you? If so, I am going to do a last review, and optionally fix last problems.

markus-96 · 2024-03-29T14:35:27Z

I am right now finding out how to add some testing for the added functions in Ldap.php, did not use Mockery yet. But if you want to, I can leave it as is.

davidcoutadeur · 2024-03-29T14:38:10Z

I am right now finding out how to add some testing for the added functions in Ldap.php, did not use Mockery yet. But if you want to, I can leave it as is.

Yes, no problem, I can take that in charge. Thanks for the last modifications you have done!

I'll merge the pr when it's done.

markus-96 · 2024-03-29T14:41:05Z

Ok, thanks a lot! Added the needed functions in PhpLDAP.php so that they can be mocked.

Will then see how to include these functions in SD

davidcoutadeur · 2024-04-02T16:49:14Z

Done in PR #12

bring code from ssp

f0c45bf

markus-96 mentioned this pull request Mar 6, 2024

feature: ability to change custom password fields ltb-project/self-service-password#751

Closed

coudot mentioned this pull request Mar 7, 2024

Improve code factorization ltb-project/self-service-password#707

Closed

davidcoutadeur mentioned this pull request Mar 13, 2024

ability to change custom password fields, developped by @markus-96 (#864) ltb-project/self-service-password#865

Merged

davidcoutadeur reviewed Mar 13, 2024

View reviewed changes

markus-96 and others added 2 commits March 16, 2024 13:56

Delete nbproject directory

6dd031c

renaming and function get_ppolicy_error_code

bcf5f61

davidcoutadeur mentioned this pull request Mar 29, 2024

new password functions #11

Closed

markus-96 and others added 3 commits March 29, 2024 12:34

Merge branch 'ltb-project:main' into password-change

97f5f5b

some testing, fix class Password

2b31b95

extend PhpLDAP with needed functions

2da96ea

... and use the functions in PhpLDAP

b5bc0b2

markus-96 marked this pull request as ready for review March 29, 2024 15:02

davidcoutadeur mentioned this pull request Apr 2, 2024

11 new password functions #12

Merged

davidcoutadeur closed this Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Passwords: check and create hashes, modify passwords #7

Passwords: check and create hashes, modify passwords #7

markus-96 commented Mar 1, 2024

markus-96 commented Mar 1, 2024

davidcoutadeur left a comment

davidcoutadeur Mar 13, 2024

markus-96 Mar 13, 2024

davidcoutadeur Mar 13, 2024

davidcoutadeur Mar 13, 2024

davidcoutadeur Mar 13, 2024

davidcoutadeur Mar 13, 2024

davidcoutadeur Mar 13, 2024

markus-96 Mar 13, 2024

davidcoutadeur Mar 13, 2024

markus-96 Mar 13, 2024

davidcoutadeur Mar 13, 2024

markus-96 Mar 13, 2024

davidcoutadeur commented Mar 29, 2024

markus-96 commented Mar 29, 2024

davidcoutadeur commented Mar 29, 2024

markus-96 commented Mar 29, 2024

davidcoutadeur commented Apr 2, 2024

Passwords: check and create hashes, modify passwords #7

Passwords: check and create hashes, modify passwords #7

Conversation

markus-96 commented Mar 1, 2024

markus-96 commented Mar 1, 2024

davidcoutadeur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidcoutadeur commented Mar 29, 2024

markus-96 commented Mar 29, 2024

davidcoutadeur commented Mar 29, 2024

markus-96 commented Mar 29, 2024

davidcoutadeur commented Apr 2, 2024