If the very first call to removeSecurityProxy was given a proxy, the results would be wrong under PyPy. See zopefoundation/zope.pagetemplate#4

3 files changed, 31 insertions, 12 deletions

diff --git a/CHANGES.rst b/CHANGES.rst
index cfe3bb9..71bfffc 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -4,7 +4,10 @@ Changes
 4.0.2 (unreleased)
 ------------------
 
-- Compatibility with ``zope.proxy`` 4.1.5 under PyPy.
+- Fixed compatibility with ``zope.proxy`` 4.1.5 under PyPy.
+
+- Fixed the very first call to ``removeSecurityProxy`` returning
+  incorrect results if given a proxy under PyPy.
 
 4.0.1 (2014-03-19)
 ------------------
diff --git a/src/zope/security/proxy.py b/src/zope/security/proxy.py
index 87b3f22..f6055f0 100644
--- a/src/zope/security/proxy.py
+++ b/src/zope/security/proxy.py
@@ -315,9 +315,10 @@ for name in ['__iadd__',
 def getCheckerPy(proxy):
     return super(PyProxyBase, proxy).__getattribute__('_checker')
 
+_builtin_isinstance = __builtins__.isinstance
+
 def getObjectPy(proxy):
-    isinstance_func = builtin_isinstance or isinstance
-    if not isinstance_func(proxy, ProxyPy):
+    if not _builtin_isinstance(proxy, ProxyPy):
         return proxy
     return super(PyProxyBase, proxy).__getattribute__('_wrapped')
 
@@ -341,18 +342,11 @@ def getTestProxyItems(proxy):
     return sorted(checker.get_permissions.items())
 
 
-builtin_isinstance = None
 def isinstance(object, cls):
     """Test whether an object is an instance of a type.
 
-    This works even if the object is security proxied:
+    This works even if the object is security proxied.
     """
-    global builtin_isinstance
-    if builtin_isinstance is None:
-        if PYPY:
-            builtin_isinstance = getattr(__builtins__, 'isinstance')
-        else:
-            builtin_isinstance = __builtins__['isinstance']
     # The removeSecurityProxy call is OK here because it is *only*
     # being used for isinstance
-    return builtin_isinstance(removeSecurityProxy(object), cls)
+    return _builtin_isinstance(removeSecurityProxy(object), cls)
diff --git a/src/zope/security/tests/test_proxy.py b/src/zope/security/tests/test_proxy.py
index fbe4964..aaed8e6 100644
--- a/src/zope/security/tests/test_proxy.py
+++ b/src/zope/security/tests/test_proxy.py
@@ -1389,6 +1389,28 @@ class ProxyPyTests(unittest.TestCase, ProxyTestBase):
         # pow(i, j, proxy(k)) will fail with a TypeError
         self.assertRaises(TypeError, pow, (x, y, proxy))
 
+    def test_getObjectPy_initial_conditions(self):
+        # Once upon a time, we dynamically set _builtin_isinstance
+        # in z.s.proxy.isinstance itself. And at that time getObjectPy
+        # (aka removeSecurityProxy) called z.s.proxy.isinstance if
+        # _builtin_isinstance was not set...which recursively calls
+        # getObjectPy. The net result was that the very first call
+        # to getObjectPy would falsely return the proxy object if passed
+        # a proxy, not the wrapped object!
+        # This test makes sure we're not dynamically setting that attribute
+        # any more.
+        import zope.security.proxy
+
+        target = object()
+        checker = object()
+        proxy = self._makeOne(target, checker)
+
+        orig_builtin_isinstance = zope.security.proxy._builtin_isinstance
+        zope.security.proxy._builtin_isinstance = None
+        try:
+            self.assertRaises(TypeError, zope.security.proxy.getObjectPy, proxy)
+        finally:
+            zope.security.proxy._builtin_isinstance = orig_builtin_isinstance
 
 class DummyChecker(object):
     _proxied = _checked = None