<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 10/10/2016 08:47 AM, Standa Laznicka
wrote:<br>
</div>
<blockquote
cite="mid:c552597b-df9f-0ce0-34b5-44b9c3c9d334@redhat.com"
type="cite">On 10/10/2016 07:53 AM, Jan Cholasta wrote:
<br>
<blockquote type="cite">On 7.10.2016 12:23, Standa Laznicka wrote:
<br>
<blockquote type="cite">On 10/07/2016 08:31 AM, Jan Cholasta
wrote:
<br>
<blockquote type="cite">On 17.8.2016 13:47, Stanislav Laznicka
wrote:
<br>
<blockquote type="cite">On 08/11/2016 02:59 PM, Stanislav
Laznicka wrote:
<br>
<blockquote type="cite">On 08/11/2016 07:49 AM, Jan
Cholasta wrote:
<br>
<blockquote type="cite">On 2.8.2016 13:47, Stanislav
Laznicka wrote:
<br>
<blockquote type="cite">On 07/19/2016 09:20 AM, Jan
Cholasta wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
On 14.7.2016 14:36, Stanislav Laznicka wrote:
<br>
<blockquote type="cite">Hello,
<br>
<br>
This patch fixes
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/5640">https://fedorahosted.org/freeipa/ticket/5640</a>.
<br>
<br>
With not so much experience with the framework,
it raises question
<br>
in my
<br>
head whether ipaldap.get_entries is used
properly throughout the
<br>
system
<br>
- does it always assume that it gets ALL the
requested entries or
<br>
just a
<br>
few of those as configured by the
'ipaSearchRecordsLimit'
<br>
attribute of
<br>
ipaConfig.etc which it actually gets?
<br>
</blockquote>
<br>
That depends. If you call get_entries() on the
ldap2 plugin
<br>
(which is
<br>
usually the case in the framework), then
ipaSearchRecordsLimit is
<br>
used. If you call it on some arbitrary LDAPClient
instance, the
<br>
hardcoded default (= unlimited) is used.
<br>
<br>
<blockquote type="cite">
<br>
One spot that I know the get_entries method was
definitely not used
<br>
properly before this patch is in the
<br>
baseldap.LDAPObject.get_memberindirect() method:
<br>
<br>
692 result =
self.backend.get_entries(
<br>
693 self.api.env.basedn,
<br>
694 filter=filter,
<br>
695 attrs_list=['member'],
<br>
696 size_limit=-1, # paged
search will get
<br>
everything
<br>
anyway
<br>
697 paged_search=True)
<br>
<br>
which to me seems kind of important if the
environment size_limit
<br>
is not
<br>
set properly :) The patch does not fix the
non-propagation of the
<br>
paged_search, though.
<br>
</blockquote>
<br>
Why do you think size_limit is not used properly
here?
<br>
</blockquote>
AFAIU it is desired that the search is unlimited.
However, due to the
<br>
fact that neither size_limit nor paged_search are
passed from
<br>
ldap2.get_entries() to ldap2.find_entries() (methods
inherited from
<br>
LDAPClient), only the number of records specified by
<br>
ipaSearchRecordsLimit is returned. That could
eventually cause
<br>
problems
<br>
should ipaSearchRecordsLimit be set to a low value
as in the ticket.
<br>
</blockquote>
<br>
I see. This is *not* intentional, the **kwargs of
get_entries()
<br>
should be passed to find_entries(). This definitely
needs to be fixed.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<br>
Anyway, this ticket is not really easily fixable
without more
<br>
profound
<br>
changes. Often, multiple LDAP searches are done
during command
<br>
execution. What do you do with the size limit
then? Do you pass the
<br>
same size limit to all the searches? Do you
subtract the result size
<br>
from the size limit after each search? Do you do
something else with
<br>
it? ... The answer is that it depends on the
purpose of each
<br>
individual LDAP search (like in
get_memberindirect() above, we
<br>
have to
<br>
do unlimited search, otherwise the resulting entry
would be
<br>
incomplete), and fixing this accross the whole
framework is a
<br>
non-trivial task.
<br>
<br>
</blockquote>
I do realize that the proposed fix for the
permission plugin is not
<br>
perfect, it would probably be better to subtract the
number of
<br>
currently
<br>
loaded records from the sizelimit, although in the
end the number of
<br>
returned values will not be higher than the given
size_limit.
<br>
However,
<br>
it seems reasonable that if get_entries is passed a
size limit, it
<br>
should apply it over current ipaSearchRecordsLimit
rather than
<br>
ignoring
<br>
it. Then, any use of get_entries could be fixed
accordingly if
<br>
someone
<br>
sees fit.
<br>
<br>
</blockquote>
<br>
Right. Anyway, this is a different issue than above,
so please put
<br>
this into a separate commit.
<br>
<br>
</blockquote>
Please see the attached patches, then.
<br>
<br>
</blockquote>
Self-NACK, with Honza's help I found there was a mistake
in the code. I
<br>
also found an off-by-one bug which I hope I could stick to
the other two
<br>
patches (attaching only the modified and new patches).
<br>
</blockquote>
<br>
Works for me, but this bit in patch 0064 looks suspicious to
me:
<br>
<br>
+ if max_entries > 0 and
len(entries) ==
<br>
max_entries:
<br>
<br>
Shouldn't it rather be:
<br>
<br>
+ if max_entries > 0 and
len(entries) >=
<br>
max_entries:
<br>
<br>
?
<br>
<br>
</blockquote>
The length of entries list should not exceed max_entries if
size_limit
<br>
is properly implemented. Therefore the list you get from
execute()
<br>
should not have more then max_entries entries. You shouldn't
also be
<br>
able to append a legacy entry to entries list if this check is
the first
<br>
thing you do.
<br>
</blockquote>
<br>
That's a lot of shoulds :-) I would expect at least an assert
statement to make sure everything is right.
<br>
<br>
<blockquote type="cite">
<br>
That being said, >= could be used but then some popping of
entries from
<br>
entries list would be necessary. But it's perhaps safer to do,
although
<br>
if there's a bug, it won't be that obvious :)
<br>
</blockquote>
<br>
OK, nevermind then, just add the assert please, to make bugs
*very* obvious.
<br>
<br>
</blockquote>
An assert seems like a very good idea, attached is an asserting
patch.
<br>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
<p>Attached is the patch rebased on the current master. Renumbered
it a bit as previous numbers could've been confusing so I also
omitted the revision number.<br>
</p>
</body>
</html>