Make dict views behave like their unrestricted versions

CHANGES.rst

@@ -8,6 +8,13 @@ For changes before version 3.0, see ``HISTORY.rst``.
 
 - Nothing changed yet.
 
+- Make dict views (`.keys()`, `.items()` and `.values()`) behave like their
+  unrestricted versions.
+  (`#147 <https://github.com/zopefoundation/AccessControl/pull/147>`_)
+
+- Make `.items()` validate each keys and values, like `.keys()` and
+  `.values()` do.
+
 
 6.3 (2023-11-20)
 ----------------

src/AccessControl/ZopeGuards.py

@@ -12,6 +12,7 @@
 ##############################################################################
 
 
+import collections.abc
 import math
 import random
 import string
@@ -127,13 +128,18 @@ def guarded_pop(key, default=_marker):
     return guarded_pop
 
 
-def get_iter(c, name):
-    iter = getattr(c, name)
+def get_mapping_view(c, name):
 
-    def guarded_iter():
-        return SafeIter(iter(), c)
+    view_class = {
+        'keys': SafeKeysView,
+        'items': SafeItemsView,
+        'values': SafeValuesView,
+    }
 
-    return guarded_iter
+    def guarded_mapping_view():
+        return view_class[name](c)
+
+    return guarded_mapping_view
 
 
 def get_list_pop(lst, name):
@@ -153,18 +159,15 @@ def guarded_pop(index=-1):
     'copy': 1,
     'fromkeys': 1,
     'get': get_dict_get,
-    'items': 1,
+    'items': get_mapping_view,
+    'keys': get_mapping_view,
     'pop': get_dict_pop,
     'popitem': 1,
     'setdefault': 1,
     'update': 1,
+    'values': get_mapping_view,
 }
 
-_dict_white_list.update({
-    'keys': get_iter,
-    'values': get_iter,
-})
-
 
 def _check_dict_access(name, value):
     # Check whether value is a dict method
@@ -272,6 +275,40 @@ def __next__(self):
     next = __next__
 
 
+# The following three view classes are used only for mappings of type `dict`
+# (not subclasses). Therefore, the mapping does not have security assertions
+# and cannot acquire ones. As a consequence, the `guard` calls used in their
+# methods verify that the checked key or value is accessible based solely on
+# its own virtues, i.e. either because it is public or has its own security
+# assertions allowing access.
+class _SafeMappingView:
+    __allow_access_to_unprotected_subobjects__ = 1
+
+    def __iter__(self):
+        for e in super().__iter__():
+            guard(self._mapping, e)
+            yield e
+
+
+class SafeKeysView(_SafeMappingView, collections.abc.KeysView):
+    pass
+
+
+class SafeValuesView(_SafeMappingView, collections.abc.ValuesView):
+    pass
+
+
+class SafeItemsView(_SafeMappingView, collections.abc.ItemsView):
+    def __iter__(self):
+        for k, v in super().__iter__():
+            guard(self._mapping, k)
+            # When checking the value, we check the guard with index=None,
+            # not with index=k, the key name does not matter. guard just
+            # needs to verify that the value itself can be accessed.
+            guard(self._mapping, v)
+            yield k, v
+
+
 def _error(index):
     raise Unauthorized('unauthorized access to element')
 

src/AccessControl/tests/actual_python.py

@@ -123,6 +123,39 @@ def f7():
         access = getattr(d, meth)
         result = sorted(access())
         assert result == expected[kind], (meth, kind, result, expected[kind])
+        assert len(access()) == len(expected[kind]), (meth, kind, "len")
+        iter_ = access()  # iterate twice on the same view
+        assert list(iter_) == list(iter_)
+
+        assert sorted([k for k in getattr(d, meth)()]) == expected[kind]
+        assert sorted(k for k in getattr(d, meth)()) == expected[kind]
+    assert {k: v for k, v in d.items()} == d
+
+    assert 1 in d
+    assert 1 in d.keys()
+    assert 2 in d.values()
+    assert (1, 2) in d.items()
+
+    assert d
+    assert d.keys()
+    assert d.values()
+    assert d.items()
+
+    empty_d = {}
+    assert not empty_d
+    assert not empty_d.keys()
+    assert not empty_d.values()
+    assert not empty_d.items()
+
+    smaller_d = {1: 2}
+    for m, _ in methods:
+        assert getattr(d, m)() != getattr(smaller_d, m)()
+        assert not getattr(d, m)() == getattr(smaller_d, m)()
+        if m != 'values':
+            assert getattr(d, m)() > getattr(smaller_d, m)()
+            assert getattr(d, m)() >= getattr(smaller_d, m)()
+            assert getattr(smaller_d, m)() < getattr(d, m)()
+            assert getattr(smaller_d, m)() <= getattr(d, m)()
 
 
 f7()

src/AccessControl/tests/testZopeGuards.py

@@ -258,42 +258,58 @@ def test_pop_validates(self):
         self.assertTrue(sm.calls)
 
     def test_keys_empty(self):
-        from AccessControl.ZopeGuards import get_iter
-        keys = get_iter({}, 'keys')
+        from AccessControl.ZopeGuards import get_mapping_view
+        keys = get_mapping_view({}, 'keys')
         self.assertEqual(list(keys()), [])
 
+    def test_kvi_len(self):
+        from AccessControl.ZopeGuards import get_mapping_view
+        for attr in ("keys", "values", "items"):
+            with self.subTest(attr):
+                view = get_mapping_view({'a': 1}, attr)
+                self.assertEqual(len(view()), 1)
+
     def test_keys_validates(self):
         sm = SecurityManager()
         old = self.setSecurityManager(sm)
         keys = guarded_getattr({GuardTestCase: 1}, 'keys')
         try:
-            next(keys())
+            next(iter(keys()))
         finally:
             self.setSecurityManager(old)
         self.assertTrue(sm.calls)
 
+    def test_items_validates(self):
+        sm = SecurityManager()
+        old = self.setSecurityManager(sm)
+        items = guarded_getattr({GuardTestCase: GuardTestCase}, 'items')
+        try:
+            next(iter(items()))
+        finally:
+            self.setSecurityManager(old)
+        self.assertEqual(len(sm.calls), 2)
+
     def test_values_empty(self):
-        from AccessControl.ZopeGuards import get_iter
-        values = get_iter({}, 'values')
+        from AccessControl.ZopeGuards import get_mapping_view
+        values = get_mapping_view({}, 'values')
         self.assertEqual(list(values()), [])
 
     def test_values_validates(self):
         sm = SecurityManager()
         old = self.setSecurityManager(sm)
         values = guarded_getattr({GuardTestCase: 1}, 'values')
         try:
-            next(values())
+            next(iter(values()))
         finally:
             self.setSecurityManager(old)
         self.assertTrue(sm.calls)
 
     def test_kvi_iteration(self):
-        from AccessControl.ZopeGuards import SafeIter
         d = dict(a=1, b=2)
         for attr in ("keys", "values", "items"):
             v = getattr(d, attr)()
-            si = SafeIter(v)
-            self.assertEqual(next(si), next(iter(v)))
+            si = guarded_getattr(d, attr)()
+            self.assertEqual(next(iter(si)), next(iter(v)))
 
 
 class TestListGuards(GuardTestCase):