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

[Standa Laznicka]

On 10/07/2016 08:31 AM, Jan Cholasta wrote:
> 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:
>
> ?
>
The length of entries list should not exceed max_entries if size_limit 
is properly implemented. Therefore the list you get from execute() 
should not have more then max_entries entries. You shouldn't also be 
able to append a legacy entry to entries list if this check is the first 
thing you do.

That being said, >= could be used but then some popping of entries from 
entries list would be necessary. But it's perhaps safer to do, although 
if there's a bug, it won't be that obvious :)