[Freeipa-devel] [PATCH] 1018 enforce sizelimit when searching for permissions

[Martin Kosek]

On Fri, 2012-05-18 at 10:17 -0400, Rob Crittenden wrote:
> Rob Crittenden wrote:
> > Martin Kosek wrote:
> >> On Thu, 2012-05-17 at 16:11 -0400, Rob Crittenden wrote:
> >>> We do two searches when looking for permissions. One within the
> >>> permission object itself and a second in the ACIs. We weren't enforcing
> >>> a sizelimit on either search.
> >>>
> >>> rob
> >>
> >> This returns the right result, but I don't think it is right with
> >> respect to "truncated" flag because of several reasons:
> >>
> >> 1) You manipulate and set "truncated" flag in post_callback but this
> >> won't affect the flag in the returned result because the new value is
> >> not propagated outside of the post_callback function. I.e. truncated
> >> flag will be set correctly only when it was raised during original
> >> permission_find.
> >
> > Truncated is still honored as expected. I even added a test case for this.

Yes, but it only works when the truncated flag is raised by the base
LDAP search, i.e. the search for permission objects (which is a case of
your unit test). If the search does not raise the flag and it is set
later in post callback, it is never propagated to the user. Please check
the attached (crappy) test that shows this issue:

======================================================================
FAIL: test_permission[20]: permission_find: Search for permissions by
attr with a limit of 1 (truncated)
----------------------------------------------------------------------
...
AssertionError: assert_deepequal: expected != got.
  test_permission[20]: permission_find: Search for permissions by attr
with a limit of 1 (truncated)
  expected = True
  got = False
  path = ('truncated',)

I am not sure how to solve this right, we may need to add a new return
attribute (truncated) to all LDAPSearch post callbacks so that the post
callback can really modify it - something similar we already do with pre
callbacks which are able to change LDAP search filter, scope etc.

> >
> >> 2) The part with "ind" is strange:
> >>
> >> + # enforce --sizelimit
> >> + if len(entries) == max_entries:
> >> + if ind + 1< len(results):
> >> + truncated = True
> >> + break
> >>
> >> I think it would be much easier to just do
> >>
> >> ...
> >> if (dn, permission) not in entries:
> >> if len(entries)< max_entries:
> >> entries.append((dn, permission))
> >> else:
> >> truncated = True
> >> break
> >>
> >> Otherwise you would rise "truncated" even when the rest of "results"
> >> does not contain relevant entries that would have not been added anyway.
> >
> > Yes, that makes sense.
> 
> And now updated patch.

We can now also remove the "enumerate" part, "ind" is no longer needed.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test-aci_find-truncated.patch
Type: text/x-patch
Size: 1833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120522/0dd3bff5/attachment.bin>