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.

From 75d8cf9c3708b68c9b3a9ba999b3d034b1ddc33a Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 11 Aug 2016 14:08:33 +0200
Subject: [PATCH 1/2] Make get_entries() not ignore its limit arguments

get_entries() wouldn't pass some arguments deeper to find_entries()
function it wraps. This would cause unexpected behavior in some
cases throughout the framework where specific (non-)limitations
are expected.

https://fedorahosted.org/freeipa/ticket/5640
---
 ipapython/ipaldap.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 704e71a9471c27430328a8c7c6a319aa72a9d482..a3f0a5668616f66ba744c336832064269882eb8b 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -1298,7 +1298,8 @@ class LDAPClient(object):
         for their description.
         """
         entries, truncated = self.find_entries(
-            base_dn=base_dn, scope=scope, filter=filter, attrs_list=attrs_list)
+            base_dn=base_dn, scope=scope, filter=filter, attrs_list=attrs_list,
+            **kwargs)
         try:
             self.handle_truncated_result(truncated)
         except errors.LimitsExceeded as e:
@@ -1313,7 +1314,8 @@ class LDAPClient(object):
 
     def find_entries(self, filter=None, attrs_list=None, base_dn=None,
                      scope=ldap.SCOPE_SUBTREE, time_limit=None,
-                     size_limit=None, search_refs=False, paged_search=False):
+                     size_limit=None, search_refs=False, paged_search=False,
+                     **kwargs):
         """
         Return a list of entries and indication of whether the results were
         truncated ([(dn, entry_attrs)], truncated) matching specified search
-- 
2.7.4

From d08a31f8992a1f21ae2adc42d2aaef047148296b Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 11 Aug 2016 14:09:22 +0200
Subject: [PATCH] fix permission_find fail on low search size limit

permission_find() method would have failed if size_limit in config is too
small caused by a search in post_callback. This search should also
respect the passed sizelimit or the sizelimit from ipa config if no
sizelimit is passed.

https://fedorahosted.org/freeipa/ticket/5640
---
 ipaserver/plugins/permission.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ipaserver/plugins/permission.py b/ipaserver/plugins/permission.py
index 830773ae7a09f0197da702e4ec31b0b58f1214dd..0564c6593e5fe9c297e3aaf28a5a338628665654 100644
--- a/ipaserver/plugins/permission.py
+++ b/ipaserver/plugins/permission.py
@@ -1308,12 +1308,15 @@ class permission_find(baseldap.LDAPSearch):
                 legacy_entries = ldap.get_entries(
                     base_dn=DN(self.obj.container_dn, self.api.env.basedn),
                     filter=ldap.combine_filters(filters, rules=ldap.MATCH_ALL),
-                    attrs_list=attrs_list)
+                    attrs_list=attrs_list, size_limit=max_entries)
                 # Retrieve the root entry (with all legacy ACIs) at once
                 root_entry = ldap.get_entry(DN(api.env.basedn), ['aci'])
             except errors.NotFound:
                 legacy_entries = ()
                 cached_root_entry = None
+            except errors.LimitsExceeded:
+                # there were more results than max_entries
+                pass
             self.log.debug('potential legacy entries: %s', len(legacy_entries))
             nonlegacy_names = {e.single_value['cn'] for e in entries}
             for entry in legacy_entries:
-- 
2.7.4

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to