On 03/21/2016 12:25 PM, Jan Cholasta wrote:
On 21.3.2016 10:17, Petr Spacek wrote:
On 18.3.2016 13:49, Rob Crittenden wrote:
Martin Babinsky wrote:
These patches implement behavior agreed upon during discussion of
https://fedorahosted.org/freeipa/ticket/5677
However I'm not sure if we want to push them into 4-3 branch (the
ticket
is triaged into 4.3.2 milestone) since they modify the framework
behavior quite a bit.
If there is no need to have it there (CC'ing Milan since he is the
reporter), I would retriage it into 4.4 milestone.
+ desc="while getting entries (search base: '{}',"
+ "filter: {})".format(base_dn, filter))
This is going to expose parts of the DIT in an error message to
users. We have
tried in the past to hide the implementation. I'd propose logging the
error
and making the exception less verbose.
I agree with Rob here, we shouldn't expose internal stuff in error
messages for users.
In this particular case, even if we included internal stuff in the error
message, it should be the error message returned by the server rather
than this ad-hoc message.
IMHO it actually helps to print the DN. At very least the user can see
if the
error is happening always with the same DN or if it keeps changing.
In other words, for user it is not that important to understand
meaning of the
DN but it might be important to see if it is the same or not.
I can't imagine a situation where it would actually be useful for the
user (as opposed to the admin, who has access to logs) to know the base
DN of some arbitrary LDAP search operation. Could you give an example?
Right, attaching updated patches.
--
Martin^3 Babinsky
From 8201c09d465020b2e8fd61ece5a69ed771224b6d Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Fri, 18 Mar 2016 09:49:41 +0100
Subject: [PATCH 1/2] differentiate between limit types when LDAP search
exceeds configured limits
When LDAP search fails on exceeded limits, we should raise an specific
exception for the type of limit raised (size, time, administrative) so that
the consumer can distinguish between e.g. searches returning too many entries
and those timing out.
https://fedorahosted.org/freeipa/ticket/5677
---
install/tools/ipa-httpd-kdcproxy | 6 ++--
install/tools/ipactl | 6 ++--
ipalib/errors.py | 28 +++++++++++++++++
ipalib/plugins/automount.py | 6 ++--
ipalib/plugins/baseldap.py | 14 +++------
ipapython/ipaldap.py | 68 +++++++++++++++++++++++++++++-----------
ipaserver/plugins/ldap2.py | 5 +--
7 files changed, 92 insertions(+), 41 deletions(-)
diff --git a/install/tools/ipa-httpd-kdcproxy b/install/tools/ipa-httpd-kdcproxy
index 5e67f61a6e2b3fe26532323d773bd502ac52f454..c14b3a721ca739c3412aaaeef0b0d5c71b0e0cf2 100755
--- a/install/tools/ipa-httpd-kdcproxy
+++ b/install/tools/ipa-httpd-kdcproxy
@@ -97,10 +97,8 @@ class KDCProxyConfig(object):
def _find_entry(self, dn, attrs, filter, scope=IPAdmin.SCOPE_BASE):
"""Find an LDAP entry, handles NotFound and Limit"""
try:
- entries, truncated = self.con.find_entries(
- filter, attrs, dn, scope, time_limit=self.time_limit)
- if truncated:
- raise errors.LimitsExceeded()
+ entries = self.con.get_entries(
+ dn, scope, filter, attrs, time_limit=self.time_limit)
except errors.NotFound:
self.log.debug('Entry not found: %s', dn)
return None
diff --git a/install/tools/ipactl b/install/tools/ipactl
index fb1e890ea16af492e1dc4956d40a3fb6287d5837..b41b10c8a65fc8d4f2f4c169e08b2c89125909a4 100755
--- a/install/tools/ipactl
+++ b/install/tools/ipactl
@@ -160,14 +160,12 @@ def get_config(dirsrv):
wait_for_open_ports(host, [int(port)], timeout=api.env.startup_timeout)
con = IPAdmin(ldap_uri=api.env.ldap_uri)
con.do_external_bind()
- res, truncated = con.find_entries(
+ res = con.get_entries(
+ base,
filter=srcfilter,
attrs_list=attrs,
- base_dn=base,
scope=con.SCOPE_SUBTREE,
time_limit=10)
- if truncated:
- raise errors.LimitsExceeded()
except errors.NetworkError:
# LSB status code 3: program is not running
raise IpactlError("Failed to get list of services to probe status:\n" +
diff --git a/ipalib/errors.py b/ipalib/errors.py
index 52b770027081448827007d8af00143046d59de0a..2507e13dcdc6c11b04a9ea1d50d3356f4e21986a 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -1612,6 +1612,34 @@ class TaskTimeout(DatabaseError):
format = _("%(task)s LDAP task timeout, Task DN: '%(task_dn)s'")
+class TimeLimitExceeded(LimitsExceeded):
+ """
+ **4214** Raised when time limit for the operation is exceeded.
+ """
+
+ errno = 4214
+ format = _('Configured time limit exceeded')
+
+
+class SizeLimitExceeded(LimitsExceeded):
+ """
+ **4215** Raised when size limit for the operation is exceeded.
+ """
+
+ errno = 4215
+ format = _('Configured size limit exceeded')
+
+
+class AdminLimitExceeded(LimitsExceeded):
+ """
+ **4216** Raised when server limit imposed by administrative authority was
+ exceeded
+ """
+
+ errno = 4216
+ format = _('Configured administrative server limit exceeded')
+
+
class CertificateError(ExecutionError):
"""
**4300** Base class for Certificate execution errors (*4300 - 4399*).
diff --git a/ipalib/plugins/automount.py b/ipalib/plugins/automount.py
index 7dc00224e845e12a699ea263abbb0edd45ea4bc0..c0a55d5dabec72ace99ab178ff3dec7b15ede744 100644
--- a/ipalib/plugins/automount.py
+++ b/ipalib/plugins/automount.py
@@ -803,12 +803,10 @@ class automountkey(LDAPObject):
('cn', parent_keys[0]), self.container_dn,
api.env.basedn)
attrs_list = ['*']
- entries, truncated = ldap.find_entries(
- sfilter, attrs_list, basedn, ldap.SCOPE_ONELEVEL)
+ entries = ldap.get_entries(
+ basedn, ldap.SCOPE_ONELEVEL, sfilter, attrs_list)
if len(entries) > 1:
raise errors.NotFound(reason=_('More than one entry with key %(key)s found, use --info to select specific entry.') % dict(key=pkey))
- if truncated:
- raise errors.LimitsExceeded()
dn = entries[0].dn
return dn
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index ffc0008a65d3ea52a084870591b867648c8dea36..0eae45dbd3cce0e5113c07cf27ea1da6c1218d83 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -684,14 +684,12 @@ class LDAPObject(Object):
filter = self.backend.combine_filters(
('(member=*)', mo_filter), self.backend.MATCH_ALL)
try:
- result, truncated = self.backend.find_entries(
- base_dn=self.api.env.basedn,
+ result = self.backend.get_entries(
+ self.api.env.basedn,
filter=filter,
attrs_list=['member'],
size_limit=-1, # paged search will get everything anyway
paged_search=True)
- if truncated:
- raise errors.LimitsExceeded()
except errors.NotFound:
result = []
@@ -709,12 +707,10 @@ class LDAPObject(Object):
filter = self.backend.make_filter(
{'member': dn, 'memberuser': dn, 'memberhost': dn})
try:
- result, truncated = self.backend.find_entries(
- base_dn=self.api.env.basedn,
+ result = self.backend.get_entries(
+ self.api.env.basedn,
filter=filter,
attrs_list=[''])
- if truncated:
- raise errors.LimitsExceeded()
except errors.NotFound:
result = []
@@ -2105,7 +2101,7 @@ class LDAPSearch(BaseLDAPCommand, crud.Search):
result = dict(
result=entries,
count=len(entries),
- truncated=truncated,
+ truncated=bool(truncated),
)
if truncated:
diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 2965ba4a5509eb16589bc9dde9485147d9114032..5347b30296cd34a204fa7ecdb2a1b3af7126e88e 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -60,6 +60,11 @@ AUTOBIND_AUTO = 1
AUTOBIND_ENABLED = 2
AUTOBIND_DISABLED = 3
+TRUNCATED_SIZE_LIMIT = object()
+TRUNCATED_TIME_LIMIT = object()
+TRUNCATED_ADMIN_LIMIT = object()
+
+
def unicode_from_utf8(val):
'''
val is a UTF-8 encoded string, return a unicode object.
@@ -971,11 +976,11 @@ class LDAPClient(object):
except ldap.OBJECT_CLASS_VIOLATION:
raise errors.ObjectclassViolation(info=info)
except ldap.ADMINLIMIT_EXCEEDED:
- raise errors.LimitsExceeded()
+ raise errors.AdminLimitExceeded()
except ldap.SIZELIMIT_EXCEEDED:
- raise errors.LimitsExceeded()
+ raise errors.SizeLimitExceeded()
except ldap.TIMELIMIT_EXCEEDED:
- raise errors.LimitsExceeded()
+ raise errors.TimeLimitExceeded()
except ldap.NOT_ALLOWED_ON_RDN:
raise errors.NotAllowedOnRDN(attr=info)
except ldap.FILTER_ERROR:
@@ -1003,6 +1008,20 @@ class LDAPClient(object):
'Unhandled LDAPError: %s: %s' % (type(e).__name__, str(e)))
raise errors.DatabaseError(desc=desc, info=info)
+ @staticmethod
+ def handle_truncated_result(truncated):
+ if not truncated:
+ return
+
+ if truncated is TRUNCATED_ADMIN_LIMIT:
+ raise errors.AdminLimitExceeded()
+ elif truncated is TRUNCATED_SIZE_LIMIT:
+ raise errors.SizeLimitExceeded()
+ elif truncated is TRUNCATED_TIME_LIMIT:
+ raise errors.TimeLimitExceeded()
+ else:
+ raise errors.LimitsExceeded()
+
@property
def schema(self):
"""schema associated with this LDAP server"""
@@ -1249,7 +1268,7 @@ class LDAPClient(object):
return self.combine_filters(flts, rules)
def get_entries(self, base_dn, scope=ldap.SCOPE_SUBTREE, filter=None,
- attrs_list=None):
+ attrs_list=None, **kwargs):
"""Return a list of matching entries.
:raises: errors.LimitsExceeded if the list is truncated by the server
@@ -1260,13 +1279,21 @@ class LDAPClient(object):
:param scope: search scope, see LDAP docs (default ldap2.SCOPE_SUBTREE)
:param filter: LDAP filter to apply
:param attrs_list: ist of attributes to return, all if None (default)
-
- Use the find_entries method for more options.
+ :param kwargs: additional keyword arguments. See find_entries method
+ for their description.
"""
entries, truncated = self.find_entries(
base_dn=base_dn, scope=scope, filter=filter, attrs_list=attrs_list)
- if truncated:
- raise errors.LimitsExceeded()
+ try:
+ self.handle_truncated_result(truncated)
+ except errors.LimitsExceeded as e:
+ self.log.debug(
+ "{} while getting entries (base DN: {}, filter: {})".format(
+ e, base_dn, filter
+ )
+ )
+ raise
+
return entries
def find_entries(self, filter=None, attrs_list=None, base_dn=None,
@@ -1357,6 +1384,15 @@ class LDAPClient(object):
break
else:
cookie = ''
+ except ldap.ADMINLIMIT_EXCEEDED:
+ truncated = TRUNCATED_ADMIN_LIMIT
+ break
+ except ldap.SIZELIMIT_EXCEEDED:
+ truncated = TRUNCATED_SIZE_LIMIT
+ break
+ except ldap.TIMELIMIT_EXCEEDED:
+ truncated = TRUNCATED_TIME_LIMIT
+ break
except ldap.LDAPError as e:
# If paged search is in progress, try to cancel it
if paged_search and cookie:
@@ -1402,15 +1438,13 @@ class LDAPClient(object):
search_kw = {attr: value, 'objectClass': object_class}
filter = self.make_filter(search_kw, rules=self.MATCH_ALL)
- (entries, truncated) = self.find_entries(filter, attrs_list, base_dn)
+ entries = self.get_entries(
+ base_dn, filter=filter, attrs_list=attrs_list)
if len(entries) > 1:
raise errors.SingleMatchExpected(found=len(entries))
- else:
- if truncated:
- raise errors.LimitsExceeded()
- else:
- return entries[0]
+
+ return entries[0]
def get_entry(self, dn, attrs_list=None, time_limit=None,
size_limit=None):
@@ -1423,13 +1457,11 @@ class LDAPClient(object):
assert isinstance(dn, DN)
- (entries, truncated) = self.find_entries(
- None, attrs_list, dn, self.SCOPE_BASE, time_limit=time_limit,
+ entries = self.get_entries(
+ dn, self.SCOPE_BASE, None, attrs_list, time_limit=time_limit,
size_limit=size_limit
)
- if truncated:
- raise errors.LimitsExceeded()
return entries[0]
def add_entry(self, entry):
diff --git a/ipaserver/plugins/ldap2.py b/ipaserver/plugins/ldap2.py
index a1f1e1982980f1a913d7f5f40ee4e7c93cff64e7..d50ae1698176c9ec59f4e09461e7377e90ed72e4 100644
--- a/ipaserver/plugins/ldap2.py
+++ b/ipaserver/plugins/ldap2.py
@@ -230,12 +230,13 @@ class ldap2(CrudBackend, LDAPClient):
# Not in our context yet
pass
try:
+ # use find_entries here lest we hit an infinite recursion when
+ # ldap2.get_entries tries to determine default time/size limits
(entries, truncated) = self.find_entries(
None, attrs_list, base_dn=dn, scope=self.SCOPE_BASE,
time_limit=2, size_limit=10
)
- if truncated:
- raise errors.LimitsExceeded()
+ self.handle_truncated_result(truncated)
config_entry = entries[0]
except errors.NotFound:
config_entry = self.make_entry(dn)
--
2.5.0
From 02152de954ae5b2966333be0dcdcc97b1f7a9e9f Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Fri, 18 Mar 2016 09:56:08 +0100
Subject: [PATCH 2/2] specify type of exceeded limit when warning about
truncated search results
API commands inheriting from LDAPSearch should mention which limit was
exceeded in the warning message sent with truncated results.
https://fedorahosted.org/freeipa/ticket/5677
---
ipalib/messages.py | 2 +-
ipalib/plugins/baseldap.py | 7 +++++--
ipatests/test_xmlrpc/test_old_permission_plugin.py | 8 ++++----
ipatests/test_xmlrpc/test_permission_plugin.py | 8 ++++----
4 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/ipalib/messages.py b/ipalib/messages.py
index 5cd0ea1769920c076c62729ed5fe359cd1680723..2c9d0660d7364840c04b9ad4933371629e896590 100644
--- a/ipalib/messages.py
+++ b/ipalib/messages.py
@@ -338,7 +338,7 @@ class SearchResultTruncated(PublicMessage):
errno = 13017
type = "warning"
- format = _("Search result has been truncated to configured search limit.")
+ format = _("Search result has been truncated: %(reason)s")
class BrokenTrust(PublicMessage):
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 0eae45dbd3cce0e5113c07cf27ea1da6c1218d83..dc61fe45438260cb6286bcfd0935cb1a610366a1 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -2104,8 +2104,11 @@ class LDAPSearch(BaseLDAPCommand, crud.Search):
truncated=bool(truncated),
)
- if truncated:
- add_message(options['version'], result, SearchResultTruncated())
+ try:
+ ldap.handle_truncated_result(truncated)
+ except errors.LimitsExceeded as e:
+ add_message(options['version'], result, SearchResultTruncated(
+ reason=e))
return result
diff --git a/ipatests/test_xmlrpc/test_old_permission_plugin.py b/ipatests/test_xmlrpc/test_old_permission_plugin.py
index 09f43fee8c60e4e97d233d256f046fef44d31acf..1af76f6a65ba7dc477fb4dc3d1341c8df3735078 100644
--- a/ipatests/test_xmlrpc/test_old_permission_plugin.py
+++ b/ipatests/test_xmlrpc/test_old_permission_plugin.py
@@ -525,8 +525,8 @@ class test_old_permission(Declarative):
},
],
messages=({
- 'message': (u'Search result has been truncated to '
- 'configured search limit.'),
+ 'message': (u'Search result has been truncated: '
+ u'Configured size limit exceeded'),
'code': 13017,
'type': u'warning',
'name': u'SearchResultTruncated'
@@ -585,8 +585,8 @@ class test_old_permission(Declarative):
api.env.basedn)) and
'ipapermission' in res['objectclass']],
messages=({
- 'message': (u'Search result has been truncated to '
- 'configured search limit.'),
+ 'message': (u'Search result has been truncated: '
+ u'Configured size limit exceeded'),
'code': 13017,
'type': u'warning',
'name': u'SearchResultTruncated'
diff --git a/ipatests/test_xmlrpc/test_permission_plugin.py b/ipatests/test_xmlrpc/test_permission_plugin.py
index 8026e84366e3b9056ed6e2ff6d76c58bdc95140e..df5498a61003667843e582a04e0c23d0c93f90c4 100644
--- a/ipatests/test_xmlrpc/test_permission_plugin.py
+++ b/ipatests/test_xmlrpc/test_permission_plugin.py
@@ -818,8 +818,8 @@ class test_permission(Declarative):
],
messages=(
{
- 'message': (u'Search result has been truncated to '
- 'configured search limit.'),
+ 'message': (u'Search result has been truncated: '
+ u'Configured size limit exceeded'),
'code': 13017,
'type': u'warning',
'name': u'SearchResultTruncated'
@@ -882,8 +882,8 @@ class test_permission(Declarative):
'ipapermission' in res['objectclass']],
messages=(
{
- 'message': (u'Search result has been truncated to '
- 'configured search limit.'),
+ 'message': (u'Search result has been truncated: '
+ u'Configured size limit exceeded'),
'code': 13017,
'type': u'warning',
'name': u'SearchResultTruncated'
--
2.5.0
--
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
