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

[Rob Crittenden]

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