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.
From 5e54c0d49ab680b75af872031d9c2bd4a3852d27 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/3] 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 | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index bbfc6f6190045fd4d3cc9d893eefbdc695953f8f..39719521255d06817c240c8755294c38235fb1c6 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -1322,7 +1322,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:
@@ -1337,7 +1338,7 @@ 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, paged_search=False):
+ size_limit=None, 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 f3c190f64178ca99bbb4f744ff084518b9c7f513 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 11 Aug 2016 14:09:22 +0200
Subject: [PATCH 2/3] 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 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipaserver/plugins/permission.py b/ipaserver/plugins/permission.py
index 130c99c294c1fe4bfa38a56335030c35d6dbb01a..af0cfea5962dbae5686349c0963ed6efaa7c24f9 100644
--- a/ipaserver/plugins/permission.py
+++ b/ipaserver/plugins/permission.py
@@ -1307,10 +1307,10 @@ class permission_find(baseldap.LDAPSearch):
if options.get('all'):
attrs_list.append('*')
try:
- legacy_entries = ldap.get_entries(
+ legacy_entries, truncated = ldap.find_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:
--
2.7.4
From 1262c284b3e04f26e74afa3583db12497bef47df Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Wed, 17 Aug 2016 13:35:04 +0200
Subject: [PATCH 3/3] permission-find: fix a sizelimit off-by-one bug
permission-find: sizelimit option set to number of permissions -1
could return all permissions anyway
https://fedorahosted.org/freeipa/ticket/5640
---
ipaserver/plugins/permission.py | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/ipaserver/plugins/permission.py b/ipaserver/plugins/permission.py
index af0cfea5962dbae5686349c0963ed6efaa7c24f9..0bd75b09fabc7b7ade69cbf8eeef18d5e13fdd2e 100644
--- a/ipaserver/plugins/permission.py
+++ b/ipaserver/plugins/permission.py
@@ -1296,6 +1296,10 @@ class permission_find(baseldap.LDAPSearch):
else:
max_entries = self.api.Backend.ldap2.size_limit
+ if max_entries > 0:
+ # should we get more entries than current sizelimit, fail
+ assert len(entries) <= max_entries
+
filters = ['(objectclass=ipaPermission)',
'(!(ipaPermissionType=V2))']
if 'name' in options:
@@ -1320,15 +1324,6 @@ class permission_find(baseldap.LDAPSearch):
for entry in legacy_entries:
if entry.single_value['cn'] in nonlegacy_names:
continue
- if max_entries > 0 and len(entries) > max_entries:
- # We've over the limit, pop the last entry and set
- # truncated flag
- # (this is easier to do than checking before adding
- # the entry to results)
- # (max_entries <= 0 means unlimited)
- entries.pop()
- truncated = True
- break
self.obj.upgrade_permission(entry, output_only=True,
cached_acientry=root_entry)
# If all given options match, include the entry
@@ -1354,6 +1349,11 @@ class permission_find(baseldap.LDAPSearch):
for value in values):
break
else:
+ if max_entries > 0 and len(entries) == max_entries:
+ # We've reached the limit, set truncated flag
+ # (max_entries <= 0 means unlimited)
+ truncated = True
+ break
entries.append(entry)
for entry in 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
