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

[Rob Crittenden]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-1018-4-sizelimit.patch
Type: text/x-diff
Size: 13493 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120529/747fa4fc/attachment.bin>