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

[Martin Kosek]

On Wed, 2012-05-23 at 13:55 -0400, Rob Crittenden wrote:
> Rob Crittenden wrote:
> > Martin Kosek wrote:
> >> 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
> >
> > You're right, I'd have sworn I tested that...
> >
> > The only solution is going to be to have the post_callback return
> > truncated. This is going to be a rather intrusive change.
> >
> > rob
> 
> Updated patch against master that adds a return value to post_callback.
> 
> I also included Martin's test. It relies on the ordering of data in LDAP 
> but it is better than nothing right now.
> 
> rob

Yeah, returning a value in LDAPSearch post_callback is OK. I just see
you missed post_callback in dnsrecord_find function, this makes
"dnsrecord-find" command and also other DNS command in the unit tests
crash.

Martin