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.

> Is this just an optimization to not hit the final return 1 in the
> method?  If so is that final "return 1" ever hit? Maybe add a
> log.warning to see if it is called and under what circumstances?
> 
> 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.

> Is this correct? If so we need a section in upgrading.txt because this
> is an incompatible change.

No, a Search permission with a check method doesn't make sense. The
check method would never have been called.

> Any idea why Search permissions can not have a check function? The
> docstring for it indicates checks are not allowed.

The check function gets an item. We don't have one before we start
searching.

> 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.

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com