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

[Stanislav Laznicka]

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).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-slaznick-0062-2-fix-permission_find-fail-on-low-search-size-limit.patch
Type: text/x-patch
Size: 1597 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160817/4fe1ebc0/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-slaznick-0064-permission-find-fix-a-sizelimit-off-by-one-bug.patch
Type: text/x-patch
Size: 2183 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160817/4fe1ebc0/attachment-0001.bin>