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.