[Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find

[Tomas Babej]

On 11/19/2014 02:03 PM, Jan Cholasta wrote:
> Dne 19.11.2014 v 13:44 Tomas Babej napsal(a):
>>
>> On 11/19/2014 12:51 PM, Martin Kosek wrote:
>>> On 11/19/2014 12:41 PM, Tomas Babej wrote:
>>>> On 11/19/2014 12:24 PM, Martin Kosek wrote:
>>>>> On 11/19/2014 12:03 PM, Tomas Babej wrote:
>>>>>> Hi,
>>>>>>
>>>>>> When constructing a parent DN in LDAPSearch, we should always
>>>>>> check that the parent object exists (hence use get_dn_if_exists),
>>>>>> rather than search on unexistant containers (which can happen
>>>>>> with get_dn).
>>>>>>
>>>>>> Replaces get_dn calls with get_dn_if_exists in *-find commands
>>>>>> and makes sure proper error message is raised.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/4659
>>>>> Doesn't it produce extra LDAP search thus making all our search
>>>>> commands
>>>>> slower? Is that what we want?
>>>> No it does not make all of our LDAP search slower. It only happens for
>>>> the objects that have parent objects, such as idoverrides or
>>>> dnsrecords.
>>> ... and makes them slower.
>>
>> What I was pointing out here is that this is not a issue for ALL *-find
>> commands (e.g user-find, group-find will not suffer from it), as you
>> incorrectly stated.
>>
>>>
>>>>> Wouldn't it be better to distinguish between LDAP
>>>>> search with no results and LDAP search with missing parent DN? The
>>>>> reply looks
>>>>> different, at least in CLI:
>>>> Up to discussion. We would probably need to introduce a new exception,
>>>> like ParentObjectNotFound.
>>>>
>>>>> # search result
>>>>> search: 4
>>>>> result: 0 Success
>>>>>
>>>>> # search result
>>>>> search: 4
>>>>> result: 32 No such object
>>>>> matchedDN: cn=accounts,dc=mkosek-f20,dc=test
>>>>>
>>>>> Also, I do not think you can just stop using get_dn(), some
>>>>> commands override
>>>>> this call to get more complex searches (like host-find searching
>>>>> for shortname).
>>>> Look into the get_dn_if_exists, it just wraps around get_dn, so no
>>>> issue
>>>> here. Any custom behaviour is preserved.
>>> Ah, ok, thanks for info.
>>>
>>>> To sum up, I think this is worth changing this behaviour by default,
>>>> ignoring a non-matching value of the parent object is not a correct
>>>> general approach in my opinion.
>>> Well, that's the question. Whether we would leave DS to validate the
>>> search
>>> itself or do all the pre-check ourselves. To me, doing just one LDAP
>>> search and
>>> processing the error correctly looks better. But I can live even
>>> with your
>>> version then, I will leave the framework guardians like Honza or
>>> Petr3 to decide.
>
> +1 on single LDAP search and proper error processing.
>
>>
>> I see now what you're trying to suggest. However, the reason boils
>> down to ipaldap.find_entries method not differentiating between a
>> LDAP search that returns error code 32 (No such object) and LDAP
>> search returning error code 0 (Success), but returning no results.
>>
>> In both cases errors.NotFound is raised.
>>
>> The reason I did not go this way is that changing the find_entries
>> method
>> is quite more invasive as this is the method subsenqently called by
>> almost
>> any command.
>
> You can always derive the new error (ParentNotFound or whatever) on
> NotFound, so old code won't break.
>

Thanks for the suggestsions.

Attached is a new patch which hooks into find_entries method and
differentiates between the cases.

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0286-2-baseldap-Handle-missing-parent-objects-properly-in-f.patch
Type: text/x-patch
Size: 4193 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141119/96386cd3/attachment.bin>