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

[Martin Kosek]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-248-2-raise-proper-exception-when-ldap-limits-are-exceeded.patch
Type: text/x-patch
Size: 4434 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120417/e31c2c16/attachment.bin>