[Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument

[Jan Cholasta]

On 17.8.2016 13:47, Stanislav Laznicka wrote:
> On 08/11/2016 02:59 PM, Stanislav Laznicka wrote:
>> On 08/11/2016 07:49 AM, Jan Cholasta wrote:
>>> On 2.8.2016 13:47, Stanislav Laznicka wrote:
>>>> On 07/19/2016 09:20 AM, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> On 14.7.2016 14:36, Stanislav Laznicka wrote:
>>>>>> Hello,
>>>>>>
>>>>>> This patch fixes https://fedorahosted.org/freeipa/ticket/5640.
>>>>>>
>>>>>> With not so much experience with the framework, it raises question
>>>>>> in my
>>>>>> head whether ipaldap.get_entries is used properly throughout the
>>>>>> system
>>>>>> - does it always assume that it gets ALL the requested entries or
>>>>>> just a
>>>>>> few of those as configured by the 'ipaSearchRecordsLimit'
>>>>>> attribute of
>>>>>> ipaConfig.etc which it actually gets?
>>>>>
>>>>> That depends. If you call get_entries() on the ldap2 plugin (which is
>>>>> usually the case in the framework), then ipaSearchRecordsLimit is
>>>>> used. If you call it on some arbitrary LDAPClient instance, the
>>>>> hardcoded default (= unlimited) is used.
>>>>>
>>>>>>
>>>>>> One spot that I know the get_entries method was definitely not used
>>>>>> properly before this patch is in the
>>>>>> baseldap.LDAPObject.get_memberindirect() method:
>>>>>>
>>>>>>  692             result = self.backend.get_entries(
>>>>>>  693                 self.api.env.basedn,
>>>>>>  694                 filter=filter,
>>>>>>  695                 attrs_list=['member'],
>>>>>>  696                 size_limit=-1, # paged search will get
>>>>>> everything
>>>>>> anyway
>>>>>>  697                 paged_search=True)
>>>>>>
>>>>>> which to me seems kind of important if the environment size_limit
>>>>>> is not
>>>>>> set properly :) The patch does not fix the non-propagation of the
>>>>>> paged_search, though.
>>>>>
>>>>> Why do you think size_limit is not used properly here?
>>>> AFAIU it is desired that the search is unlimited. However, due to the
>>>> fact that neither size_limit nor paged_search are passed from
>>>> ldap2.get_entries() to ldap2.find_entries() (methods inherited from
>>>> LDAPClient), only the number of records specified by
>>>> ipaSearchRecordsLimit is returned. That could eventually cause problems
>>>> should ipaSearchRecordsLimit be set to a low value as in the ticket.
>>>
>>> I see. This is *not* intentional, the **kwargs of get_entries()
>>> should be passed to find_entries(). This definitely needs to be fixed.
>>>
>>>>>
>>>>> Anyway, this ticket is not really easily fixable without more profound
>>>>> changes. Often, multiple LDAP searches are done during command
>>>>> execution. What do you do with the size limit then? Do you pass the
>>>>> same size limit to all the searches? Do you subtract the result size
>>>>> from the size limit after each search? Do you do something else with
>>>>> it? ... The answer is that it depends on the purpose of each
>>>>> individual LDAP search (like in get_memberindirect() above, we have to
>>>>> do unlimited search, otherwise the resulting entry would be
>>>>> incomplete), and fixing this accross the whole framework is a
>>>>> non-trivial task.
>>>>>
>>>> I do realize that the proposed fix for the permission plugin is not
>>>> perfect, it would probably be better to subtract the number of
>>>> currently
>>>> loaded records from the sizelimit, although in the end the number of
>>>> returned values will not be higher than the given size_limit. However,
>>>> it seems reasonable that if get_entries is passed a size limit, it
>>>> should apply it over current ipaSearchRecordsLimit rather than ignoring
>>>> it. Then, any use of get_entries could be fixed accordingly if someone
>>>> sees fit.
>>>>
>>>
>>> Right. Anyway, this is a different issue than above, so please put
>>> this into a separate commit.
>>>
>> Please see the attached patches, then.
>>
> Self-NACK, with Honza's help I found there was a mistake in the code. I
> also found an off-by-one bug which I hope I could stick to the other two
> patches (attaching only the modified and new patches).

Works for me, but this bit in patch 0064 looks suspicious to me:

+                        if max_entries > 0 and len(entries) == max_entries:

Shouldn't it rather be:

+                        if max_entries > 0 and len(entries) >= max_entries:

?

-- 
Jan Cholasta