[Freeipa-devel] [PATCH] 248 Raise proper exception when LDAP limits are exceeded

[Rob Crittenden]

Martin Kosek wrote:
> On Mon, 2012-04-16 at 13:51 -0400, Rob Crittenden wrote:
>> Rob Crittenden wrote:
>>> Jan Cholasta wrote:
>>>> On 10.4.2012 10:57, Martin Kosek wrote:
>>>>> Few test hints are attached to the ticket.
>>>>>
>>>>> ---
>>>>>
>>>>> ldap2 plugin returns NotFound error for find_entries/get_entry
>>>>> queries when the server did not manage to return an entry
>>>>> due to time limits. This may be confusing for user when the
>>>>> entry he searches actually exists.
>>>>>
>>>>> This patch fixes the behavior in ldap2 plugin to return
>>>>> LimitsExceeded exception instead. This way, user would know
>>>>> that his time/size limits are set too low and can amend them to
>>>>> get correct results.
>>>>>
>>>>> https://fedorahosted.org/freeipa/ticket/2606
>>>>>
>>>>
>>>> ACK.
>>>>
>>>> Honza
>>>>
>>>
>>> Before pushing I'd like to look at this more. truncated is supposed to
>>> indicate a limits problem. I want to see if the caller should be
>>> responsible for returning a limits error instead.
>>>
>>> rob
>>
>> This is what I had in mind.
>>
>> diff --git a/ipaserver/plugins/ldap2.py b/ipaserver/plugins/ldap2.py
>> index 61341b0..447e738 100644
>> --- a/ipaserver/plugins/ldap2.py
>> +++ b/ipaserver/plugins/ldap2.py
>> @@ -754,7 +754,7 @@ class ldap2(CrudBackend, Encoder):
>>            except _ldap.LDAPError, e:
>>                _handle_errors(e)
>>
>> -        if not res:
>> +        if not res and not truncated:
>>                raise errors.NotFound(reason='no such entry')
>>
>>            if attrs_list and ('memberindirect' in attrs_list or '*' in
>> attrs_list)
>> :
>> @@ -801,7 +801,10 @@ class ldap2(CrudBackend, Encoder):
>>            if len(entries)>  1:
>>                raise errors.SingleMatchExpected(found=len(entries))
>>            else:
>> -            return entries[0]
>> +            if truncated:
>> +                raise errors.LimitsExceeded()
>> +            else:
>> +                return entries[0]
>>
>>        def get_entry(self, dn, attrs_list=None, time_limit=None,
>>                      size_limit=None, normalize=True):
>> @@ -811,10 +814,13 @@ class ldap2(CrudBackend, Encoder):
>>            Keyword arguments:
>>            attrs_list - list of attributes to return, all if None
>> (default None)
>>            """
>> -        return self.find_entries(
>> +        (entry, truncated) = self.find_entries(
>>                None, attrs_list, dn, self.SCOPE_BASE, time_limit=time_limit,
>>                size_limit=size_limit, normalize=normalize
>> -        )[0][0]
>> +        )
>> +        if truncated:
>> +            raise errors.LimitsExceeded()
>> +        return entry[0]
>>
>>        config_defaults = {'ipasearchtimelimit': [2],
>> 'ipasearchrecordslimit': [0]}
>>        def get_ipa_config(self, attrs_list=None):
>>
>
> Thanks for the patch. I had similar patch planned in my mind.
>
> We just need to be more careful, this patch will change an assumption
> that ldap2.find_entries will always either raise NotFound error or
> return at least one result.
>
> I checked all ldap2.find_entries calls and did few fixes, it should be
> OK now. No regression found in unit tests. The updated patch is
> attached.
>
> Martin

ACK, pushed to master and ipa-2-2

rob