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

[Jan Cholasta]

On 7.10.2016 12:23, Standa Laznicka wrote:
> 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's a lot of shoulds :-) I would expect at least an assert statement 
to make sure everything is right.

>
> 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 :)

OK, nevermind then, just add the assert please, to make bugs *very* obvious.

-- 
Jan Cholasta