On 07/07/2014 10:12 PM, Jakub Hrozek wrote:
On Wed, Oct 23, 2013 at 06:58:16PM +0200, Jakub Hrozek wrote:
> On Wed, Oct 23, 2013 at 01:21:48PM +0200, Jakub Hrozek wrote:
>> Hi,
>>
>> this bug was reported on #sssd by a user. He was running some flavor of
>> IBM Tivoli where the entries only had an "ID", not separate
"UID" and
>> "GID". But due to a bug in sssd he couldn't use the same value for
both,
>> this configuration:
>>
>> ldap_user_uid_number = idAttribute
>> ldap_user_gid_number = idAttribute
>>
>> only saved the ID into UID and left GID empty. It appears we have a long
>> standing bug in sdap_parse where we only consider first match. If this
>> patch is accepted, I would also like to refactor sdap_parse in master
>> because currently it is a 250-lines long function with multiple
>> branches..
>
> self-nack, this patch breaks parsing of rootDSE. I will prepare a new
> version.
After a slight delay I'm attaching a revised patchset. I know we're getting
close to the release of 1.12.0 and this patch changes a critical function,
so I also prepared a unit test. I think only patches #1 to #3 are important
for 1.12.0, so I'm fine with postponing the other patches.
[PATCH 1/7] PROVIDERS: Add ldap_common.h to opts.h of each provider
Trivial header file amendment, please see the commit message for details.
Ack.
[PATCH 2/7] TESTS: Add a unit test for the sdap.c module
As said earlier, a unit test was added. I haven't covered only the range
extensions.
Ack. I mostly reviewed this by reversing the tests and checking
if they are going to fail or not. They failed every time they
had to. Btw. it is good to see new unit test :)
[PATCH 3/7] LDAP: Try all attributes when saving an entry
https://fedorahosted.org/sssd/ticket/2184
I tested by setting the 'telephoneNumber' attribute to a numeric value
and ensuring that SSSD was able to use the value for both UID and GID.
Ack.
And now the less important patches, mostly just changes I did while
I
was changing that area of code:
[PATCH 4/7] SDAP: Fix DEBUG message priorities in sdap_parse_entry
Amends log levels of DEBUG messages.
Ack.
[PATCH 5/7] LDAP: Remove unused output parameter _dn from
sdap_parse_entry
Does what the commit message suggests.
Ack.
[PATCH 6/7] SDAP: Remove unused function sdap_get_msg_dn
This function was unused since 2009
Could you also remove the comment.
/* =Get-DN-from-message==============
[PATCH 7/7] SDAP: Free bervals on failure in sdap_parse_entry
I wasn't able to test this patch, so review would only be with visual
inspection, I think..
I think the ldap_value_free_len call should be here as well
to free the memory allocated for the one NULL pointer:
447 } else {
448 if (!vals[0]) {
449 DEBUG(SSSDBG_CRIT_FAILURE,
450 "Missing value after ldap_get_values()
??\n");
451 ret = EINVAL;
452 goto done;
453 }