Rob Crittenden wrote:
Martin Kosek wrote:
On Fri, 2012-05-18 at 10:17 -0400, Rob Crittenden wrote:
Rob Crittenden wrote:
Martin Kosek wrote:
On Thu, 2012-05-17 at 16:11 -0400, Rob Crittenden wrote:
We do two searches when looking for permissions. One within the
permission object itself and a second in the ACIs. We weren't
enforcing
a sizelimit on either search.
rob
This returns the right result, but I don't think it is right with
respect to "truncated" flag because of several reasons:
1) You manipulate and set "truncated" flag in post_callback but this
won't affect the flag in the returned result because the new value is
not propagated outside of the post_callback function. I.e. truncated
flag will be set correctly only when it was raised during original
permission_find.
Truncated is still honored as expected. I even added a test case for
this.
Yes, but it only works when the truncated flag is raised by the base
LDAP search, i.e. the search for permission objects (which is a case of
your unit test). If the search does not raise the flag and it is set
later in post callback, it is never propagated to the user. Please check
the attached (crappy) test that shows this issue:
======================================================================
FAIL: test_permission[20]: permission_find: Search for permissions by
attr with a limit of 1 (truncated)
----------------------------------------------------------------------
...
AssertionError: assert_deepequal: expected != got.
test_permission[20]: permission_find: Search for permissions by attr
with a limit of 1 (truncated)
expected = True
got = False
path = ('truncated',)
I am not sure how to solve this right, we may need to add a new return
attribute (truncated) to all LDAPSearch post callbacks so that the post
callback can really modify it - something similar we already do with pre
callbacks which are able to change LDAP search filter, scope etc.
2) The part with "ind" is strange:
+ # enforce --sizelimit
+ if len(entries) == max_entries:
+ if ind + 1< len(results):
+ truncated = True
+ break
I think it would be much easier to just do
...
if (dn, permission) not in entries:
if len(entries)< max_entries:
entries.append((dn, permission))
else:
truncated = True
break
Otherwise you would rise "truncated" even when the rest of "results"
does not contain relevant entries that would have not been added
anyway.
Yes, that makes sense.
And now updated patch.
We can now also remove the "enumerate" part, "ind" is no longer needed.
Martin
You're right, I'd have sworn I tested that...
The only solution is going to be to have the post_callback return
truncated. This is going to be a rather intrusive change.
rob
Updated patch against master that adds a return value to post_callback.
I also included Martin's test. It relies on the ordering of data in LDAP
but it is better than nothing right now.
rob
>From 69dee075e305f73ec460fb9a60448762864996ae Mon Sep 17 00:00:00 2001
From: Rob Crittenden <rcrit...@redhat.com>
Date: Wed, 23 May 2012 11:00:24 -0400
Subject: [PATCH] Enforce sizelimit in permission-find, post_callback returns
truncated
We actually perform two searches in permission-find. The first looks
for matches within the permission object itself. The second looks at
matches in the underlying aci.
We need to break out in two places. The first is if we find enough
matches in the permission itself. The second when we are appending
matches from acis.
The post_callback() definition needed to be modified to return
the truncated value so a plugin author can modify that value.
https://fedorahosted.org/freeipa/ticket/2322
---
ipalib/plugins/baseldap.py | 6 +--
ipalib/plugins/entitle.py | 1 +
ipalib/plugins/host.py | 4 +-
ipalib/plugins/hostgroup.py | 3 +-
ipalib/plugins/permission.py | 18 ++++++-
ipalib/plugins/pwpolicy.py | 2 +
ipalib/plugins/selinuxusermap.py | 3 +-
ipalib/plugins/service.py | 3 +-
ipalib/plugins/user.py | 3 +-
tests/test_xmlrpc/test_permission_plugin.py | 74 +++++++++++++++++++++++++++
10 files changed, 107 insertions(+), 10 deletions(-)
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 2851f0f270d9e2bdba4780cc7bf308a76e180fd2..f278fa39ce21d6fd3fcaa20f1c7a1049cace683d 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -1849,9 +1849,9 @@ class LDAPSearch(BaseLDAPCommand, crud.Search):
for callback in self.POST_CALLBACKS:
if hasattr(callback, 'im_self'):
- callback(ldap, entries, truncated, *args, **options)
+ truncated = callback(ldap, entries, truncated, *args, **options)
else:
- callback(self, ldap, entries, truncated, *args, **options)
+ truncated = callback(self, ldap, entries, truncated, *args, **options)
if self.sort_result_entries:
if self.obj.primary_key:
@@ -1876,7 +1876,7 @@ class LDAPSearch(BaseLDAPCommand, crud.Search):
return (filters, base_dn, scope)
def post_callback(self, ldap, entries, truncated, *args, **options):
- pass
+ return truncated
def exc_callback(self, args, options, exc, call_func, *call_args, **call_kwargs):
raise exc
diff --git a/ipalib/plugins/entitle.py b/ipalib/plugins/entitle.py
index 6ade854c3bb44877c9b8cb3664e2ded480f5ffc0..5ccdb65108e9e1fa4cc3317e9f67eb74f17f9f64 100644
--- a/ipalib/plugins/entitle.py
+++ b/ipalib/plugins/entitle.py
@@ -461,6 +461,7 @@ class entitle_find(LDAPSearch):
def post_callback(self, ldap, entries, truncated, *args, **options):
if len(entries) == 0:
raise errors.NotRegisteredError()
+ return truncated
api.register(entitle_find)
diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py
index 662cff3114be5aae399ff4c5fb43ca3d7e46c703..96b73cc5594335ad02dd43f87e7e011ab84157a1 100644
--- a/ipalib/plugins/host.py
+++ b/ipalib/plugins/host.py
@@ -769,7 +769,7 @@ class host_find(LDAPSearch):
def post_callback(self, ldap, entries, truncated, *args, **options):
if options.get('pkey_only', False):
- return
+ return truncated
for entry in entries:
(dn, entry_attrs) = entry
set_certificate_attrs(entry_attrs)
@@ -785,6 +785,8 @@ class host_find(LDAPSearch):
output_sshpubkey(ldap, dn, entry_attrs)
+ return truncated
+
api.register(host_find)
diff --git a/ipalib/plugins/hostgroup.py b/ipalib/plugins/hostgroup.py
index 2a9a0a53342e08a73c89e572a368dec2eaece58f..b68c45842178f5d98ed51ab767cbe19c649cecd5 100644
--- a/ipalib/plugins/hostgroup.py
+++ b/ipalib/plugins/hostgroup.py
@@ -182,10 +182,11 @@ class hostgroup_find(LDAPSearch):
def post_callback(self, ldap, entries, truncated, *args, **options):
if options.get('pkey_only', False):
- return
+ return truncated
for entry in entries:
(dn, entry_attrs) = entry
self.obj.suppress_netgroup_memberof(dn, entry_attrs)
+ return truncated
api.register(hostgroup_find)
diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index 18fdcdddfbda769cf7e42384cf4bdfee7c0b565c..a484ff640820c2ba93d21b8dbfcbd2c66698e513 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -351,7 +351,7 @@ class permission_find(LDAPSearch):
def post_callback(self, ldap, entries, truncated, *args, **options):
if options.pop('pkey_only', False):
- return
+ return truncated
for entry in entries:
(dn, attrs) = entry
try:
@@ -363,6 +363,15 @@ class permission_find(LDAPSearch):
attrs[attr] = aci[attr]
except errors.NotFound:
self.debug('ACI not found for %s' % attrs['cn'][0])
+ if truncated:
+ # size/time limit met, no need to search acis
+ return truncated
+
+ if 'sizelimit' in options:
+ max_entries = options['sizelimit']
+ else:
+ config = ldap.get_ipa_config()[1]
+ max_entries = config['ipasearchrecordslimit']
# Now find all the ACIs that match. Once we find them, add any that
# aren't already in the list along with their permission info.
@@ -398,7 +407,12 @@ class permission_find(LDAPSearch):
dn = permission['dn']
del permission['dn']
if (dn, permission) not in entries:
- entries.append((dn, permission))
+ if len(entries) < max_entries:
+ entries.append((dn, permission))
+ else:
+ truncated = True
+ break
+ return truncated
api.register(permission_find)
diff --git a/ipalib/plugins/pwpolicy.py b/ipalib/plugins/pwpolicy.py
index 330f9f7e539c8dd5303c1ca9e2c5343ca6feb6e3..fe46b8091a6d1cec7611053a42bd5a8a76c6e778 100644
--- a/ipalib/plugins/pwpolicy.py
+++ b/ipalib/plugins/pwpolicy.py
@@ -496,5 +496,7 @@ class pwpolicy_find(LDAPSearch):
except KeyError:
pass
+ return truncated
+
api.register(pwpolicy_find)
diff --git a/ipalib/plugins/selinuxusermap.py b/ipalib/plugins/selinuxusermap.py
index e33e1016192d62312aa5f4f0dcdbafea23327216..a3c6d7be47897369688c47d069c342167891b190 100644
--- a/ipalib/plugins/selinuxusermap.py
+++ b/ipalib/plugins/selinuxusermap.py
@@ -318,10 +318,11 @@ all=True)['result']
def post_callback(self, ldap, entries, truncated, *args, **options):
if options.get('pkey_only', False):
- return
+ return truncated
for entry in entries:
(dn, attrs) = entry
self.obj._convert_seealso(ldap, attrs, **options)
+ return truncated
api.register(selinuxusermap_find)
diff --git a/ipalib/plugins/service.py b/ipalib/plugins/service.py
index 333d5bbf35d86f6de68c8616d6ddf3ec1be7580e..24a0a0f870c110c237d4c8284e849ac0b3c176db 100644
--- a/ipalib/plugins/service.py
+++ b/ipalib/plugins/service.py
@@ -399,11 +399,12 @@ class service_find(LDAPSearch):
def post_callback(self, ldap, entries, truncated, *args, **options):
if options.get('pkey_only', False):
- return
+ return truncated
for entry in entries:
(dn, entry_attrs) = entry
self.obj.get_password_attributes(ldap, dn, entry_attrs)
set_certificate_attrs(entry_attrs)
+ return truncated
api.register(service_find)
diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py
index 3bea7af6f25bdab4f544922d9305b5474fcea7a8..2391f235fd2d273b297927aa5056d29565e78aa0 100644
--- a/ipalib/plugins/user.py
+++ b/ipalib/plugins/user.py
@@ -625,13 +625,14 @@ class user_find(LDAPSearch):
def post_callback(self, ldap, entries, truncated, *args, **options):
if options.get('pkey_only', False):
- return
+ return truncated
for entry in entries:
(dn, attrs) = entry
self.obj._convert_manager(attrs, **options)
self.obj.get_password_attributes(ldap, dn, attrs)
convert_nsaccountlock(attrs)
output_sshpubkey(ldap, dn, attrs)
+ return truncated
msg_summary = ngettext(
'%(count)d user matched', '%(count)d users matched', 0
diff --git a/tests/test_xmlrpc/test_permission_plugin.py b/tests/test_xmlrpc/test_permission_plugin.py
index 28db7dc2f588f40d94276db3b3096a9ed5e5db0d..d8ff14903d582a7c880d348ae28f0c0061437542 100644
--- a/tests/test_xmlrpc/test_permission_plugin.py
+++ b/tests/test_xmlrpc/test_permission_plugin.py
@@ -387,6 +387,80 @@ class test_permission(Declarative):
dict(
+ desc='Search for %r with a limit of 1 (truncated)' % permission1,
+ command=('permission_find', [permission1], dict(sizelimit=1)),
+ expected=dict(
+ count=1,
+ truncated=True,
+ summary=u'1 permission matched',
+ result=[
+ {
+ 'dn': lambda x: DN(x) == permission1_dn,
+ 'cn': [permission1],
+ 'member_privilege': [privilege1],
+ 'type': u'user',
+ 'permissions': [u'write'],
+ },
+ ],
+ ),
+ ),
+
+
+ dict(
+ desc='Search for %r with a limit of 2' % permission1,
+ command=('permission_find', [permission1], dict(sizelimit=2)),
+ expected=dict(
+ count=2,
+ truncated=False,
+ summary=u'2 permissions matched',
+ result=[
+ {
+ 'dn': lambda x: DN(x) == permission1_dn,
+ 'cn': [permission1],
+ 'member_privilege': [privilege1],
+ 'type': u'user',
+ 'permissions': [u'write'],
+ },
+ {
+ 'dn': lambda x: DN(x) == permission2_dn,
+ 'cn': [permission2],
+ 'type': u'user',
+ 'permissions': [u'write'],
+ },
+ ],
+ ),
+ ),
+
+
+ # This tests setting truncated to True in the post_callback of
+ # permission_find(). The return order in LDAP is not guaranteed
+ # but in practice this is the first entry it finds. This is subject
+ # to change.
+ dict(
+ desc='Search for permissions by attr with a limit of 1 (truncated)',
+ command=('permission_find', [], dict(attrs=u'ipaenabledflag',
+ sizelimit=1)),
+ expected=dict(
+ count=1,
+ truncated=True,
+ summary=u'1 permission matched',
+ result=[
+ {
+ 'dn': lambda x: DN(x) == DN(('cn', 'Modify HBAC rule'),
+ api.env.container_permission,api.env.basedn),
+ 'cn': [u'Modify HBAC rule'],
+ 'member_privilege': [u'HBAC Administrator'],
+ 'permissions' : [u'write'],
+ 'attrs': [u'servicecategory', u'sourcehostcategory', u'cn', u'description', u'ipaenabledflag', u'accesstime', u'usercategory', u'hostcategory', u'accessruletype', u'sourcehost'],
+ 'subtree' : u'ldap:///ipauniqueid=*,cn=hbac,%s' % api.env.basedn,
+ 'memberindirect': [u'cn=hbac administrator,cn=privileges,cn=pbac,%s' % api.env.basedn, u'cn=it security specialist,cn=roles,cn=accounts,%s' % api.env.basedn],
+ },
+ ],
+ ),
+ ),
+
+
+ dict(
desc='Update %r' % permission1,
command=(
'permission_mod', [permission1], dict(permissions=u'read', memberof=u'ipausers')
--
1.7.10.1
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
