Makes sense. I also changed the doc string to reflect -1 as well. Updated
patch attached.

Thanks,

Gabe

On Thu, Sep 10, 2015 at 1:41 AM, Jan Cholasta <jchol...@redhat.com> wrote:

> On 4.9.2015 14:43, Gabe Alford wrote:
>
>> Bump for review.
>>
>> On Wed, Aug 12, 2015 at 9:32 AM, Gabe Alford <redhatri...@gmail.com
>> <mailto:redhatri...@gmail.com>> wrote:
>>
>>     On Tue, Aug 11, 2015 at 1:34 AM, Jan Cholasta <jchol...@redhat.com
>>     <mailto:jchol...@redhat.com>> wrote:
>>
>>         On 6.8.2015 21:43, Gabe Alford wrote:
>>
>>             Hello,
>>
>>             Updated patch attached.
>>
>>             - Time limit is -1 for unlimited. I found this
>>
>> https://www.redhat.com/archives/freeipa-devel/2011-January/msg00330.html
>>             in reference to keeping the time limit as -1 for unlimited.
>>
>>
>>         This patch does two conflicting things: it coerces time limit of
>>         0 to -1 and at the same time prohibits the user to use 0 for
>>         time limit. We should do just one of these and IMHO it should be
>>         the coercion of 0 to -1.
>>
>>             Sure enough, testing time limit at 0 did not work for
>>             unlimited as well
>>             as appeared to have negative effects on IPA.
>>
>>
>>         This is because the time limit read from ipa config is not
>>         converted to int in ldap2.find_entries(), so the coercion does
>>         not work. Fix this and 0 will work just fine.
>>
>>             Also, I believe that
>>
>> http://www.python-ldap.org/doc/html/ldap.html#ldap.LDAPObject.search_ext_s
>>             specifies unlimited for time limit as -1. (Please correct me
>>             if I am wrong.)
>>
>>
>>         python-ldap is layers below our API and should not determine
>>         what we use for unlimited time limit. I would prefer if we were
>>         self-consistent and use 0 for both time limit and size limit.
>>
>>
>>     A misunderstanding on my part as I thought it was higher up in the
>>     API for some reason. Updated patch attached.
>>
>
> Thanks, this is better, but it turns out I was wrong about coercing -1 to
> 0 in config-mod: in a topology with different versions of IPA servers,
> setting the limits in LDAP to 0 on a newer server with your patch will
> break older servers without your patch:
>
>     [user@old]$ ipa user-find
>     --------------
>     1 user matched
>     --------------
>       User login: admin
>       Last name: Administrator
>       Home directory: /home/admin
>       Login shell: /bin/bash
>       UID: 1364800000
>       GID: 1364800000
>       Account disabled: False
>       Password: True
>       Kerberos keys available: True
>     ----------------------------
>     Number of entries returned 1
>     ----------------------------
>
>     [user@new]$ ipa config-mod --searchtimelimit=0 --searchrecordslimit=0
>     ...
>
>     [user@old]$ ipa user-find
>     ---------------
>     0 users matched
>     ---------------
>     ----------------------------
>     Number of entries returned 0
>     ----------------------------
>
> To fix this, we actually need to do the opposite and store -1 in LDAP when
> 0 is specified in config-mod options.
>
> Honza
>
> --
> Jan Cholasta
>
From 715dfae42bbe9e1ca93dee902b100672d6dafc39 Mon Sep 17 00:00:00 2001
From: Gabe <redhatri...@gmail.com>
Date: Thu, 10 Sep 2015 07:51:58 -0600
Subject: [PATCH] Standardize minvalue for ipasearchrecordlimit and
 ipasesarchsizelimit for unlimited minvalue

https://fedorahosted.org/freeipa/ticket/4023
---
 install/ui/test/data/ipa_init_commands.json |  4 ++--
 install/ui/test/data/ipa_init_objects.json  |  4 ++--
 install/ui/test/data/json_metadata.json     |  2 +-
 ipalib/plugins/baseldap.py                  |  4 ++--
 ipalib/plugins/config.py                    | 19 ++++++++++++-------
 ipaserver/plugins/ldap2.py                  |  4 ++--
 6 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/install/ui/test/data/ipa_init_commands.json b/install/ui/test/data/ipa_init_commands.json
index 743f508e2a733b766008bdd21838454ef7df8c21..13e3cfe87549b0b58cb86db1e34a8f6e2cfbb7e8 100644
--- a/install/ui/test/data/ipa_init_commands.json
+++ b/install/ui/test/data/ipa_init_commands.json
@@ -2446,7 +2446,7 @@
                         "attribute": true,
                         "class": "Int",
                         "deprecated_cli_aliases": [],
-                        "doc": "Maximum amount of time (seconds) for a search (> 0, or -1 for unlimited)",
+                        "doc": "Maximum amount of time (seconds) for a search (-1 is unlimited)",
                         "flags": [
                             "nonempty"
                         ],
@@ -24018,4 +24018,4 @@
         "methods": {},
         "objects": {}
     }
-}
\ No newline at end of file
+}
diff --git a/install/ui/test/data/ipa_init_objects.json b/install/ui/test/data/ipa_init_objects.json
index c8c836926d94dd4c1903aa9a62fa91c11a238e75..3333e6b00f0335295d9c1ecfecf90bf470c63511 100644
--- a/install/ui/test/data/ipa_init_objects.json
+++ b/install/ui/test/data/ipa_init_objects.json
@@ -498,7 +498,7 @@
                     {
                         "class": "Int",
                         "deprecated_cli_aliases": [],
-                        "doc": "Maximum amount of time (seconds) for a search (> 0, or -1 for unlimited)",
+                        "doc": "Maximum amount of time (seconds) for a search (-1 is unlimited)",
                         "flags": [],
                         "label": "Search time limit",
                         "maxvalue": 2147483647,
@@ -9158,4 +9158,4 @@
             }
         }
     }
-}
\ No newline at end of file
+}
diff --git a/install/ui/test/data/json_metadata.json b/install/ui/test/data/json_metadata.json
index a9e66230f94875f7da7efa73141e244b8201a5cc..082dc6c2f52e32d904c7da1b5033b542e77866ab 100644
--- a/install/ui/test/data/json_metadata.json
+++ b/install/ui/test/data/json_metadata.json
@@ -2596,7 +2596,7 @@
                         "cli_name": "searchtimelimit",
                         "cli_short_name": null,
                         "default": null,
-                        "doc": "Max. amount of time (sec.) for a search (> 0, or -1 for unlimited)",
+                        "doc": "Max. amount of time (sec.) for a search (-1 is unlimited)",
                         "exclude": null,
                         "flags": [],
                         "hint": null,
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 36a5d5f4918828a97b0b30d4613819f777722de8..b4b14c7f01b78d4f6e3885dcac0d80f81f068804 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -1917,14 +1917,14 @@ class LDAPSearch(BaseLDAPCommand, crud.Search):
     takes_options = (
         Int('timelimit?',
             label=_('Time Limit'),
-            doc=_('Time limit of search in seconds'),
+            doc=_('Time limit of search in seconds (-1 is unlimited)'),
             flags=['no_display'],
             minvalue=0,
             autofill=False,
         ),
         Int('sizelimit?',
             label=_('Size Limit'),
-            doc=_('Maximum number of entries returned'),
+            doc=_('Maximum number of entries returned (-1 is unlimited)'),
             flags=['no_display'],
             minvalue=0,
             autofill=False,
diff --git a/ipalib/plugins/config.py b/ipalib/plugins/config.py
index 6267313d5e9af2d97f45f987115de143d7aa7915..9a090953a1b80ff27acce6487238ebbd038d60de 100644
--- a/ipalib/plugins/config.py
+++ b/ipalib/plugins/config.py
@@ -78,11 +78,6 @@ EXAMPLES:
 
 register = Registry()
 
-def validate_searchtimelimit(ugettext, limit):
-    if limit == 0:
-        raise ValidationError(name='ipasearchtimelimit', error=_('searchtimelimit must be -1 or > 1.'))
-    return None
-
 @register()
 class config(LDAPObject):
     """
@@ -150,10 +145,10 @@ class config(LDAPObject):
             label=_('Default e-mail domain'),
             doc=_('Default e-mail domain'),
         ),
-        Int('ipasearchtimelimit', validate_searchtimelimit,
+        Int('ipasearchtimelimit',
             cli_name='searchtimelimit',
             label=_('Search time limit'),
-            doc=_('Maximum amount of time (seconds) for a search (> 0, or -1 for unlimited)'),
+            doc=_('Maximum amount of time (seconds) for a search (-1 is unlimited)'),
             minvalue=-1,
         ),
         Int('ipasearchrecordslimit',
@@ -267,6 +262,16 @@ class config_mod(LDAPUpdate):
                             name=k, error=_('attribute "%s" not allowed') % a
                         )
 
+        # Set ipasearchrecordslimit to -1 if 0 is used
+        if 'ipasearchrecordslimit' in entry_attrs:
+            if entry_attrs['ipasearchrecordslimit'] is 0:
+                 entry_attrs['ipasearchrecordslimit'] = -1
+
+        # Set ipasearchtimelimit to -1 if 0 is used
+        if 'ipasearchtimelimit' in entry_attrs:
+            if entry_attrs['ipasearchtimelimit'] is 0:
+                 entry_attrs['ipasearchtimelimit'] = -1
+
         for (attr, obj) in (('ipauserobjectclasses', 'user'),
                             ('ipagroupobjectclasses', 'group')):
             if attr in entry_attrs:
diff --git a/ipaserver/plugins/ldap2.py b/ipaserver/plugins/ldap2.py
index 68feee4f09eb12e50867dfbe3c482a359838aa82..c12191cf049e93913bf4573504544873b0e6fbc2 100644
--- a/ipaserver/plugins/ldap2.py
+++ b/ipaserver/plugins/ldap2.py
@@ -182,8 +182,8 @@ class ldap2(CrudBackend, LDAPClient):
             """Get configured global limits, caching them for more calls"""
             if not _lims:
                 config = self.get_ipa_config()
-                _lims['time'] = config.get('ipasearchtimelimit', [None])[0]
-                _lims['size'] = config.get('ipasearchrecordslimit', [None])[0]
+                _lims['time'] = int(config.get('ipasearchtimelimit', [None])[0])
+                _lims['size'] = int(config.get('ipasearchrecordslimit', [None])[0])
             return _lims
         _lims = {}
 
-- 
1.8.3.1

-- 
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