Raise an error when updating CIDict with duplicate keys

Updating a CIDict with data like {'A': 1, 'a': 2} would lead to data
loss since only one of the items would get to the CIDict.
This can result in non-obvious bugs similar to this one in python-ldap:
https://bugzilla.redhat.com/show_bug.cgi?id=1007820

Raise an error in this case; any resolution must be done by the caller.

2 files changed, 34 insertions, 3 deletions

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index c5b47f5b4..13d242792 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -455,8 +455,18 @@ class CIDict(dict):
     def __getitem__(self, key):
         return super(CIDict, self).__getitem__(key.lower())
 
-    def __setitem__(self, key, value):
+    def __setitem__(self, key, value, seen_keys=None):
+        """cidict[key] = value
+
+        The ``seen_keys`` argument is used by ``update()`` to keep track of
+        duplicate keys. It should be an initially empty set that is
+        passed to all calls to __setitem__ that should not set duplicate keys.
+        """
         lower_key = key.lower()
+        if seen_keys is not None:
+            if lower_key in seen_keys:
+                raise ValueError('Duplicate key in update: %s' % key)
+            seen_keys.add(lower_key)
         self._keys[lower_key] = key
         return super(CIDict, self).__setitem__(lower_key, value)
 
@@ -466,6 +476,14 @@ class CIDict(dict):
         return super(CIDict, self).__delitem__(lower_key)
 
     def update(self, new=None, **kwargs):
+        """Update self from dict/iterable new and kwargs
+
+        Functions like ``dict.update()``.
+
+        Neither ``new`` nor ``kwargs`` may contain two keys that only differ in
+        case, as this situation would result in loss of data.
+        """
+        seen = set()
         if new:
             try:
                 keys = new.keys
@@ -473,9 +491,10 @@ class CIDict(dict):
                 self.update(dict(new))
             else:
                 for key in keys():
-                    self[key] = new[key]
+                    self.__setitem__(key, new[key], seen)
+        seen = set()
         for key, value in kwargs.iteritems():
-            self[key] = value
+            self.__setitem__(key, value, seen)
 
     def __contains__(self, key):
         return super(CIDict, self).__contains__(key.lower())
diff --git a/ipatests/test_ipapython/test_ipautil.py b/ipatests/test_ipapython/test_ipautil.py
index a8a73edcd..04a43990e 100644
--- a/ipatests/test_ipapython/test_ipautil.py
+++ b/ipatests/test_ipapython/test_ipautil.py
@@ -243,6 +243,18 @@ class TestCIDict(object):
             'a': 'va', 'b': 'vb',
             'Key1': 'val1', 'key2': 'val2', 'KEY3': 'VAL3'}
 
+    def test_update_duplicate_values_dict(self):
+        with nose.tools.assert_raises(ValueError):
+            self.cidict.update({'a': 'va', 'A': None, 'b': 3})
+
+    def test_update_duplicate_values_list(self):
+        with nose.tools.assert_raises(ValueError):
+            self.cidict.update([('a', 'va'), ('A', None), ('b', 3)])
+
+    def test_update_duplicate_values_kwargs(self):
+        with nose.tools.assert_raises(ValueError):
+            self.cidict.update(a='va', A=None, b=3)
+
     def test_update_kwargs(self):
         self.cidict.update(b='vb', key2='val2')
         assert dict(self.cidict.items()) == {