On Fri, 2012-02-03 at 15:45 +0100, Jan Zelený wrote:
Please note that I haven't fully tested this yet, the LDAP server configuration needed for this is a little bit twisted ;-) I will perform more testing during the weekend. Consider this patch being preliminary and don't push it until it's tested.
Nack.
You need to update the IPA LDAP options as well: Running suite(s): ipa_ldap_opt 50%: Checks: 2, Failures: 1, Errors: 0 /home/sgallagh/workspace/sssd/src/tests/ipa_ldap_opt-tests.c:79:F:ipa_ldap_opt:test_check_num_opts:0: Failure 'IPA_OPTS_BASIC_TEST != SDAP_OPTS_BASIC' occured
Just for aesthetic reasons, please don't use the prefix sdap_ldap_*. "sdap" is already shorthand for SSSD LDAP. sdap_ldap_result() is a special case, since it's specifically a handler for "ldap_result" structures. In general, I'd prefer that you call it something like sdap_modify_shadow_lastchange_send().
Why are you changing the user's password? That's not only out of scope, but you're setting their password in plaintext on the server, if I'm reading this correctly (you're modifying the entry with the raw text of the new password). We ONLY support password modification via password-change extended operation.
The scope of https://fedorahosted.org/sssd/ticket/1019 is ONLY to update the last change time. We absolutely don't want people using shadow passwords; they're extremely insecure (highly vulnerable to offline dictionary attack).
Please modify the manpage for ldap_pwd_policy in sssd-ldap(5) as "Note that the current version of sssd cannot update this attribute during a password change." is no longer accurate.
Please don't conflate LDAP errors and errno errors in the "ret" variable in sdap_ldap_modify_passwd_done(). If for some reason openldap changed the value of LDAP_SUCCESS, this would break under our noses.
The logic of the functions looks good.