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

[Rob Crittenden]

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