On 08/07/2014 10:45 AM, Pavel Březina wrote:
>On 08/06/2014 06:36 PM, Pavel Reichl wrote:
>>
>>On 08/04/2014 02:37 PM, Pavel Březina wrote:
>>>On 08/01/2014 07:48 PM, Pavel Reichl wrote:
>>>>Hello,
>>>>
>>>>please see attached patches. Every patch is documented in its commit
>>>>message.
>>>>
>>>>Generally speaking the first 6 patches are preparation for patch #8.
>>>>
>>>>Thanks,
>>>>
>>>>Pavel Reichl
>>>>
>>>>PS: I also attached output of my testing to make it more obvious how
>>>>the
>>>>patches are supposed to work.
>>>>
>>>>------------------------------------------------------------------------
>>>>
>>>># john, people,
example.com
>>>>dn: uid=john,ou=people,dc=example,dc=com
>>>>pwdAccountLockedTime: 000001010000Z
>>>>
>>>># max, people,
example.com
>>>>dn: uid=max,ou=people,dc=example,dc=com
>>>>
>>>># dick, people,
example.com
>>>>dn: uid=dick,ou=people,dc=example,dc=com
>>>>pwdAccountLockedTime: 20140801115742Z
>>>>--------------------------------------------------------------------------
>>>>
>>>>
>>>>$ ssh -l john@openldap `hostname`
>>>>Connection closed by UNKNOWN
>>>>
>>>>$ ssh -l max@openldap `hostname`
>>>>Last login: Fri Aug 1 15:16:21 2014 from sssd.dev.work
>>>>
>>>>$ ssh -l dick@openldap `hostname`
>>>>Last login: Fri Aug 1 12:57:33 2014
>>>
>>>Patch #1:
>>>Tentative ack, although you don't have to use tmp_ctx in
>>>sdap_save_user_cache_bool(). You can just allocate attrs on NULL since
>>>it is the only talloc context you use there.
>>>
>>>Patch #2:
>>>Nack. I see it is safe here but please avoid initializing ret to EOK.
>>>Set it to EOK at the end of the function instead.
>>>
>>>Patch #3:
>>>Ack.
>>>
>>>Patch #4:
>>>Typo in commit message "Also remove *sdap_access_filter_recv()* as it
>>>is replaced by *sdap_access_filter_recv()*."
>>>
>>>But it's nack from me anyway.
>>>
>>>Tevent naming convention seems to be broken all around this code and
>>>this patch actually makes it a little bit better. But you shouldn't
>>>use function named "sdap_access_recv" inside
"sdap_access_done".
>>>
>>>I see that sdap_access_send can invoke two different requests
>>>depending on the configuration. Ideally both should have custom _recv
>>>and you should use some kind of switch to determine which one should
>>>be called.
>>>
>>>I would be willing to tentatively ack a patch with let's say
>>>"sdap_access_check_recv" or "sdap_access_subreq_recv" or
similar used
>>>in sdap_access_done.
>>>
>>>Patch #5:
>>>Ack.
>>>
>>>Patch #6:
>>>Ack.
>>>
>>>Patch #7:
>>>Ack. But please swap this ticket with #8. The feature should be first
>>>implemented then documented.
>>>
>>>Patch #8:
>>>I believe the following comment is copy & paste? It shouldn't say
>>>filter request :)
>>>+ /* Connection to LDAP succeeded
>>>+ * Send filter request
>>>+ */
>>>
>>>+ if (strcasecmp(pwdAccountLockedTime,
>>>+ PERMANENTLY_LOCKED_ACCOUNT) == 0) {
>>>+ locked = true;
>>>+ } else {
>>>+ DEBUG(SSSDBG_TRACE_FUNC,
>>>+ "Account of: %s is beeing blocked by password
>>>policy, "
>>>+ "but value: [%s] value is ignored by
SSSD.\n",
>>>+ state->username, pwdAccountLockedTime);
>>>+ }
>>>
>>>Why do we ignore the value? Isn't the value a time? Shouldn't we
check
>>>if lock_time < machine_time?
>>Yes, it's time, but we care only if its the special value
>>'000001010000Z' otherwise we grant the access.
>
>Why?
Because that's how it's described in BZ
https://bugzilla.redhat.com/show_bug.cgi?id=1099290 - or do you understand
it differently than me?
Can you please check with the openldap maintainer if that value indeed
has some special meaning?