Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Align behavior with objects raising in __getattr__ #157

Merged
merged 3 commits into from
Oct 9, 2024

Conversation

perrinjerome
Copy link
Contributor

@perrinjerome perrinjerome commented Sep 24, 2024

The observed problem was a behavior different between C and python
implementation on python 3, happening with Zope python script. When the
context can not be accessed by the current user, Zope binds a
Shared.DC.Scripts.Bindings.UnauthorizedBinding, a class that raises an
Unauthorized error when the context is actually accessed, in order to
postpone the Unauthorized if something is actually accessed. This class
does implements this by raising Unauthorized in __getattr__.

The python implementation of rolesForPermissionOn used hasattr and
hasattr has changed between python2 and python3, on python2 it was
ignoring all exceptions, including potential Unauthorized errors and
just returning False, but on python3 these errors are now raised.
This change of behavior of python causes rolesForPermissionOn to
behave differently: when using python implementation on python2 or when
using C implementation, such Unauthorized errors were gracefully handled
and caused checkPermission to return False, but on python3 the
Unauthorized is raised.

The C implementation of rolesForPermissionOn uses a construct
equivalent to the python2 version of hasattr. For consistency - and
because ignoring errors is usually not good - we also want to change it
to be have like the python3 implementation.

This change make this scenario behave the same between python and C
implementations:

  • Unauthorized errors raised in __getattr__ are supported on py3.
  • Other errors than AttributeError and Unauthorized raised in
    __getattr__ are no longer ignored in the C implementation.

@perrinjerome
Copy link
Contributor Author

Some more notes:

  • guarded_getattr also has the same logic of explicitly tolerating Unauthorized errors
    def guarded_hasattr(object, name):
    try:
    guarded_getattr(object, name)
    except (AttributeError, Unauthorized, TypeError):
    return 0
    return 1
  • The test reproduces a simplified version of the problem I described, it happens when calling a zope python script with proxy role, when running with security manager with "ownerous" set to false. By defining __ac_local_roles__ = {} like in the test class, it uses the same code path.
  • In production the problem I observed is probably a rare case, but it seemed OK to make the code python3 implementation behaves same as python2 and C regarding this.

Copy link
Member

@dataflake dataflake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't claim to understand the implications 100% but it looks like a sensible change.

@perrinjerome perrinjerome force-pushed the perrinjerome/getattr_unauthorized branch from 27d88c4 to 2db5b9f Compare October 9, 2024 08:42
@perrinjerome perrinjerome changed the title checkPermission: align behavior with objects raising in __getattr__ Align behavior with objects raising in __getattr__ Oct 9, 2024
@perrinjerome
Copy link
Contributor Author

I have updated the changes here to make the C implementation behave same as the python3 implementation: only AttributeError and Unauthorized exceptions are allowed when getting the permission attribute on objects and other exceptions are re-raised. I have also adjusted the commit message of the commit changing the behavior of __getattr__ to describe the behavior.

For the testing, I have changed the test of PermissionRole to test both the python and C implementations and implemented some more low level tests there. This is another seperate preparatory commit.

The observed problem was a behavior different between C and python
implementation on python 3, happening with Zope python script. When the
context can not be accessed by the current user, Zope binds a
`Shared.DC.Scripts.Bindings.UnauthorizedBinding`, a class that raises an
Unauthorized error when the context is actually accessed, in order to
postpone the Unauthorized if something is actually accessed. This class
does implements this by raising Unauthorized in `__getattr__`.

The python implementation of `rolesForPermissionOn` used `hasattr` and
`hasattr` has changed between python2 and python3, on python2 it was
ignoring all exceptions, including potential Unauthorized errors and
just returning False, but on python3 these errors are now raised.
This change of behavior of python causes `rolesForPermissionOn` to
behave differently: when using python implementation on python2 or when
using C implementation, such Unauthorized errors were gracefully handled
and caused `checkPermission` to return False, but on python3 the
Unauthorized is raised.

The C implementation of `rolesForPermissionOn` uses a construct
equivalent to the python2 version of `hasattr`. For consistency - and
because ignoring errors is usually not good - we also want to change it
to be have like the python3 implementation.

This change make this scenario behave the same between python and C
implementations:
 - `Unauthorized` errors raised in `__getattr__` are supported on py3.
 - Other errors  than `AttributeError` and `Unauthorized` raised in
    `__getattr__` are no longer ignored in the C implementation.
@perrinjerome perrinjerome force-pushed the perrinjerome/getattr_unauthorized branch from 2db5b9f to 52994b3 Compare October 9, 2024 08:45
CHANGES.rst Outdated Show resolved Hide resolved
Co-authored-by: Dieter Maurer <[email protected]>
@perrinjerome perrinjerome enabled auto-merge (squash) October 9, 2024 11:16
@perrinjerome perrinjerome merged commit 87079f7 into master Oct 9, 2024
47 checks passed
@perrinjerome perrinjerome deleted the perrinjerome/getattr_unauthorized branch October 9, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants