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

[Stanislav Laznicka]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-slaznick-0058-2-Make-get_entries-not-ignore-its-limit-arguments.patch
Type: text/x-patch
Size: 1772 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160811/6ac2e8a5/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-slaznick-0062-fix-permission_find-fail-on-low-search-size-limit.patch
Type: text/x-patch
Size: 1831 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160811/6ac2e8a5/attachment-0001.bin>