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

[Jan Cholasta]

Dne 21.11.2014 v 11:28 Tomas Babej napsal(a):
>
> On 11/20/2014 04:01 PM, Jan Cholasta wrote:
>> Dne 19.11.2014 v 15:12 Tomas Babej napsal(a):
>>>
>>> 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.
>>>
>>
>> Why are you special casing base scope search?
>>
>
> Since base search is performed only on the entry specified by the DN,
> there is no need to differentiate between ContainerNotFound and NotFound.
>
> So the logic is as follows:
>
> subtree search - ContainerNotFound is raised when container does not
> exist, NotFound if search provided no results
> onelevel search - the same
> base search - NotFound is raised if the specified DN does not exist
>

There is a difference between a search on a non-existent entry and a 
search on an existent entry with a non-matching filter. This difference 
exists on LDAP level and applies to all search scopes, not just the base 
scope.

I don't think hiding this difference is useful at all. Remember that 
this bug exists because we were hiding the difference in the first place.

Also, after giving this some thought, I think it would be better to 
create a new error for the case where there is no match instead of the 
case where the entry does not exist. NotFound pretty much corresponds to 
LDAP's NO_SUCH_OBJECT, something like NoMatchingEntry or EmptyResult 
would fit the no-match result better.

-- 
Jan Cholasta