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

[Martin Kosek]

On Tue, 2012-05-29 at 16:44 -0400, Rob Crittenden wrote:
> Martin Kosek wrote:
> > 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.
> 
> Yup, guess I didn't have DNS set up when I ran the tests. Fixed now.
> 
> rob

ACK. Pushed to master.

Martin