100% coverage for proxy.py

- The implementation of __getattribute__/__getattr__ now behaves like
  C and will not call a target's version of those functions more than
  once if they raise an AttributeError.

2 files changed, 83 insertions, 8 deletions

diff --git a/src/zope/security/proxy.py b/src/zope/security/proxy.py
index da71363..8f6d7c3 100644
--- a/src/zope/security/proxy.py
+++ b/src/zope/security/proxy.py
@@ -54,10 +54,9 @@ def _fmt_address(obj):
     # directly (and ctypes seems like overkill).
     if sys.platform != 'win32':
         return '0x%0x' % id(obj)
-    elif sys.maxsize < 2**32:
+    if sys.maxsize < 2**32: # pragma: no cover
         return '0x%08X' % id(obj)
-    else:
-        return '0x%016X' % id(obj)
+    return '0x%016X' % id(obj) # pragma: no cover
 
 
 class ProxyPy(PyProxyBase):
@@ -117,10 +116,29 @@ class ProxyPy(PyProxyBase):
         return checker.proxy(val)
 
     def __getattr__(self, name):
+        # We only get here if __getattribute__ has already raised an
+        # AttributeError (we have to implement this because the super
+        # class does). We expect that we will also raise that same
+        # error, one way or another---either it will be forbidden by
+        # the checker or it won't exist. However, if the underlying
+        # object is playing games in *its*
+        # __getattribute__/__getattr__, and we call getattr() on it,
+        # (maybe there are threads involved), we might actually
+        # succeed this time.
+
+        # The C implementation *does not* do two checks; it only does
+        # one check, and raises either the ForbiddenAttribute or the
+        # underlying AttributeError, *without* invoking any defined
+        # __getattribute__/__getattr__ more than once. So we
+        # explicitly do the same. The consequence is that we lose a
+        # good stack trace if the object implemented its own methods
+        # but we're consistent. We would provide a better error
+        # message or even subclass of AttributeError, but that's liable to break
+        # (doc)tests.
         wrapped = super(ProxyPy, self).__getattribute__('_wrapped')
         checker = super(ProxyPy, self).__getattribute__('_checker')
         checker.check_getattr(wrapped, name)
-        return checker.proxy(getattr(wrapped, name))
+        raise AttributeError(name)
 
     def __setattr__(self, name, value):
         if name in ('_wrapped', '_checker'):
@@ -353,7 +371,7 @@ _c_available = not PURE_PYTHON
 if _c_available:
     try:
         from zope.security._proxy import _Proxy
-    except (ImportError, AttributeError): #pragma NO COVER PyPy / PURE_PYTHON
+    except (ImportError, AttributeError): # pragma: no cover PyPy / PURE_PYTHON
         _c_available = False
 
 
diff --git a/src/zope/security/tests/test_proxy.py b/src/zope/security/tests/test_proxy.py
index 077023f..8f52e17 100644
--- a/src/zope/security/tests/test_proxy.py
+++ b/src/zope/security/tests/test_proxy.py
@@ -57,6 +57,7 @@ class AbstractProxyTestBase(object):
         checker = DummyChecker()
         proxy = self._makeOne(target, checker)
         self.assertEqual(proxy.bar, 'Bar')
+        self.assertEqual(getattr(proxy, 'bar'), 'Bar')
         self.assertEqual(checker._checked, 'bar')
         self.assertEqual(checker._proxied, 'Bar')
 
@@ -77,9 +78,29 @@ class AbstractProxyTestBase(object):
         target = Foo()
         checker = DummyChecker(ForbiddenAttribute)
         proxy = self._makeOne(target, checker)
-        self.assertRaises(ForbiddenAttribute, getattr, proxy, 'bar')
+
+        with self.assertRaises(ForbiddenAttribute):
+            getattr(proxy, 'bar')
         self.assertEqual(checker._checked, 'bar')
 
+    def test__getattr__w_checker_ok_dynamic_attribute_called_once(self):
+        class Dynamic(object):
+            count = 0
+            def __getattr__(self, name):
+                self.count += 1
+                if self.count == 1:
+                    # Called from __getattribute__
+                    raise AttributeError(name)
+                raise AssertionError("We should not be called more than once")
+
+        target = Dynamic()
+        checker = DummyChecker()
+        proxy = self._makeOne(target, checker)
+
+        with self.assertRaisesRegexp(AttributeError, "name"):
+            getattr(proxy, 'name')
+        self.assertEqual(1, target.count)
+
     def test___setattr___w_checker_ok(self):
         class Foo(object):
             bar = 'Bar'
@@ -1290,7 +1311,7 @@ class AbstractProxyTestBase(object):
             pass
         class Get(object):
             def __getitem__(self, x):
-                raise Missing('__getitem__')
+                raise Missing('__getitem__') # pragma: no cover (only py3)
             def __getslice__(self, start, stop):
                 raise Missing("__getslice__")
         target = Get()
@@ -1355,7 +1376,7 @@ class AbstractProxyTestBase(object):
             pass
         class Set(object):
             def __setitem__(self, k, v):
-                raise Missing('__setitem__')
+                raise Missing('__setitem__') # pragma: no cover (only py3)
             def __setslice__(self, start, stop, value):
                 raise Missing("__setslice__")
         target = Set()
@@ -1470,6 +1491,16 @@ class ProxyPyTests(AbstractProxyTestBase,
         self.assertRaises(AttributeError, getattr, proxy, '_wrapped')
         self.assertRaises(AttributeError, getattr, proxy, '_checker')
 
+    def test_access_checker_from_subclass(self):
+        target = object()
+        checker = DummyChecker()
+        class Sub(self._getTargetClass()):
+            def get_checker(self):
+                return self._checker
+
+        sub = Sub(target, checker)
+        self.assertIs(checker, sub.get_checker())
+
     def test_ctor_w_checker(self):
         from zope.security.proxy import getObjectPy, getCheckerPy
         # Can't access '_wrapped' / '_checker' in C version
@@ -1535,6 +1566,32 @@ class ProxyPyTests(AbstractProxyTestBase,
         finally:
             zope.security.proxy._builtin_isinstance = orig_builtin_isinstance
 
+    def test_getObjectPy_other_object(self):
+        # If it's not a proxy, return it
+        from zope.security.proxy import getObjectPy
+        self.assertIs(self, getObjectPy(self))
+
+    def test_get_reduce(self):
+        class Reduce(object):
+            def __reduce__(self):
+                return 1
+
+            def __reduce_ex__(self, prot):
+                return prot
+
+        reduce_ = Reduce()
+        proxy = self._makeOne(reduce_, DummyChecker())
+        self.assertEqual(1, proxy.__reduce__())
+        self.assertEqual(2, proxy.__reduce_ex__(2))
+
+    def test__module__(self):
+        class WithModule(object):
+            __module__ = 'foo'
+
+        module = WithModule()
+        proxy = self._makeOne(module, DummyChecker())
+        self.assertEqual(WithModule.__module__, proxy.__module__)
+
 class DummyChecker(object):
     _proxied = _checked = None
     def __init__(self, raising=None, allowed=()):