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

[Jan Cholasta]

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.

-- 
Jan Cholasta