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

Tue Dec 6 10:37:24 UTC 2016

On 21.11.2016 17:08, Standa Laznicka wrote:
> On 10/10/2016 08:47 AM, Standa Laznicka wrote:
>> On 10/10/2016 07:53 AM, Jan Cholasta wrote:
>>> 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.
>>>
>> An assert seems like a very good idea, attached is an asserting patch.
>>
>>
>>
> Attached is the patch rebased on the current master. Renumbered it a bit
> as previous numbers could've been confusing so I also omitted the
> revision number.

ACK. Removed a stray **kwargs from find_entries() in patch 0065 (as 
agreed with Standa offline) and pushed to master: 
2663a966dad138160a850e6af97c38a88ec83f93

-- 
Jan Cholasta