In message <20241023164057.k7m2yvmftzgdq55o@runtux.com>,
Ralf Schlatterbeck writes:
>On Tue, Oct 22, 2024 at 05:08:53PM +0000, John Rouillard wrote:
>> 
>> In message <20241021162755.4s6uag2wukr6va22@runtux.com>,
>> Ralf Schlatterbeck writes:
>> >The latest version is pushed to branch permission-performance.
>> 
>> I am just starting to look at these changes. What were you seeing
>> that prompted:
>> 
>>    changeset:   8131:8e9181dfc9fa
>>    Fix searchable checks in roundup/security.py
>> 
>> You moved the test:
>> 
>>         if self.check:
>>             return 0
>> 
>> from the last test in the searchable() method to before the property
>> checks. From the original method description, this seems correct.
>> 
>> Then you added a test:
>> 
>>       if self.properties is None:
>>           return 1
>> 
>> that used to be handled by fall through to the return 1 at the end of
>> the method. I think "not self._properties_dict[property]" would be
>> False for every property if self.properties is None.
>
>The check should not fail if the permission does not define *any*
>properties. In that case the _properties_dict is {None: True} and the
>test
>    if not self._properties_dict[property]
>fails. I have no idea why this ever worked.

_properties_dict should be a TruthDict. How are you getting a regular
dictionary? It should be a TruthDict instance.

  class Permission:
    def __init__():
        ...
        self.properties = properties
        self._properties_dict = support.TruthDict(properties)
        self.check = check

When TruthDict() is passed an empty properties list, the returned
object doesn't define a keys attribute. This causes
TruthDict.__getitem__(key) or TruthDict[key] to return True for
any/all keys. (It took me a while to figure out how it worked. Had to
run it under the debugger.)

>> Also, I think a search permissions like:
>> 
>>   addPermission("Search", "user", properties=("username", "id"),
>>                 check=somecheck)
>> 
>> would have returned 1 with the original code when called as:
>> 
>>    permission.searchable("Search", "user", "username")
>> 
>> But would return 0 with this changeset.
>
>Yes, this is a bug-fix. A Search permission must never have a check
>method. It doesn't make sense because we don't have any items to check
>against *before* we start searching.

Ah good point. That needs to be documented somewhere....

>> I don't see this documented anywhere in the permissions section of
>> doc/references.txt. (Granted that section is kind of light on
>> details/examples.) No mention in customizing.txt either.
>
>Hmm, what would be the semantics of a check function for search? IMO
>there is None.

I agree, but it's not documented anywhere that Search is special.
I'll find a suitable spot to document that.