Fix unintended writes

.travis.yml

@@ -0,0 +1,9 @@
+language: python
+python:
+  - "3.3"
+  - "2.7"
+install:
+  - "sudo apt-get update -qq"
+  - "sudo apt-get install -y slapd libldap2-dev"
+  - "pip install -r requirements.txt"
+script: "python -m unittest -v tests"

ldapom/connection.py

@@ -24,6 +24,9 @@
 LDAP_OPT_X_TLS_TRY = libldap.LDAP_OPT_X_TLS_TRY
 
 
+def _filter_empty_attributes(attributes):
+    return {attr for attr in attributes if len(attr._values) > 0}
+
 def handle_ldap_error(err):
     """Given an LDAP error code, raise an error if needed.
 
@@ -161,6 +164,9 @@ def _fetch_attribute_types(self):
                     scope=libldap.LDAP_SCOPE_BASE,
                     search_filter="(objectClass=*)",
                     retrieve_attributes=["attributeTypes"]))
+        if len(result) == 0:
+            raise error.LDAPCouldNotFetchAttributeTypes
+
         # Decode the type definitions returned to strings
         attribute_type_definitions += map(compat._decode_utf8,
                 result[0][1][compat._encode_utf8("attributeTypes")])
@@ -203,7 +209,7 @@ def can_bind(self, bind_dn, bind_password):
         """
         try:
             self.__class__(self._uri, self._base, bind_dn, bind_password,
-                    self._cacertfile)
+                    self._cacertfile, enable_attribute_type_mapping=False)
         except error.LDAPInvalidCredentialsError:
             return False
         return True
@@ -394,17 +400,20 @@ def save(self, entry):
 
         if entry_exists:
             assert entry._fetched_attributes is not None
-            changed_attributes = entry._attributes - entry._fetched_attributes
+            # remove empty attributes to avoid unnecessary writes
+            attributes_before = _filter_empty_attributes(entry._fetched_attributes)
+            attributes_after = _filter_empty_attributes(entry._attributes)
+            changed_attributes = attributes_after - attributes_before
             # Deleted attributes are represented as empty attributes to the LDAP server.
-            deleted_attribute_names = (frozenset(a.name for a in entry._fetched_attributes)
-                    - frozenset(a.name for a in entry._attributes))
+            deleted_attribute_names = (frozenset(a.name for a in attributes_before)
+                    - frozenset(a.name for a in attributes_after))
             for deleted_name in deleted_attribute_names:
                 deleted_attribute_type = self.get_attribute_type(deleted_name)
                 changed_attributes.add(deleted_attribute_type(deleted_name))
         else:
             # Don't try to save empty attributes as this fails if the entry does
             # not exist on the server yet.
-            changed_attributes = set(filter(lambda attr: len(attr._values) > 0, entry._attributes))
+            changed_attributes = _filter_empty_attributes(entry._attributes)
 
         # Don't try to save an empty modification set
         if not changed_attributes:

ldapom/entry.py

@@ -87,10 +87,11 @@ def __getattr__(self, name):
                 return attribute.values
         else:
             if attribute_type.multi_value:
+                # keep reference, so changes are tracked
                 setattr(self, name, set())
                 return self.get_attribute(name).values
             else:
-                raise AttributeError()
+                return None
 
     def __setattr__(self, name, value):
         """Set an attribute value.

ldapom/error.py

@@ -26,3 +26,6 @@ class LDAPServerDownError(LDAPError):
 
 class LDAPAttributeNameNotFoundError(LDAPomError):
     pass
+
+class LDAPCouldNotFetchAttributeTypes(LDAPomError):
+    pass

test_server/slapd.conf

@@ -29,19 +29,33 @@ pidfile   slapd.pid
 #		Allow authenticated users read access
 #		Allow anonymous users to authenticate
 #	Directives needed to implement policy:
-# access to dn.base="" by * read
-# access to dn.base="cn=Subschema" by * read
-# access to *
-#	by self write
-#	by users read
-#	by anonymous auth
-#
+access to dn.base="" by * read
+access to dn.base="cn=Subschema" by * read
+
 # if no access controls are present, the default policy
 # allows anyone and everyone to read anything but restricts
 # updates to rootdn.  (e.g., "access to * by * read")
 #
 # rootdn can always read and write EVERYTHING!
 
+# Users can change some of their own attributes
+access to attrs=userPassword
+    by self write
+    by anonymous auth
+    by dn="cn=admin,dc=example,dc=com" write
+    by * break
+
+# Daniel may not write to attribute givenName for test_entry_access_empty_attribute
+access to attrs=givenName
+    by dn="cn=daniel,dc=example,dc=com" read
+    by dn="cn=admin,dc=example,dc=com" write
+    by * break
+
+access to *
+    by dn="cn=daniel,dc=example,dc=com" write
+    by dn="cn=admin,dc=example,dc=com" write
+    by * break
+
 #######################################################################
 # BDB database definitions
 #######################################################################
@@ -59,11 +73,5 @@ rootpw		admin
 # Mode 700 recommended.
 directory	ldapdata
 
-# Users can change some of their own attributes (only if they are active)
-#access to attrs=userPassword
-#        by self write
-#        by anonymous auth
-#        by * break
-
 # Indices to maintain
 #index	objectClass	eq

test_server/testdata.ldif

@@ -12,6 +12,19 @@ gidNumber: 10001
 objectClass: posixGroup
 memberUid: luke
 
+# admin, example.com
+dn: cn=admin,dc=example,dc=com
+objectClass: person
+objectClass: posixAccount
+cn: admin
+gidNumber: 11000
+homeDirectory: /home/jack
+sn: admin
+uid: admin
+uidNumber: 11142
+userPassword: admin
+loginShell: /bin/bash
+
 # jack, example.com
 dn: cn=jack,dc=example,dc=com
 objectClass: person

tests.py

@@ -20,8 +20,8 @@ def setUp(self):
         self.ldap_connection = ldapom.LDAPConnection(
                 uri=self.ldap_server.ldapi_url(),
                 base='dc=example,dc=com',
-                bind_dn='cn=admin,dc=example,dc=com',
-                bind_password='admin')
+                bind_dn='cn=daniel,dc=example,dc=com',
+                bind_password='daniel')
 
     def tearDown(self):
         self.ldap_server.stop()
@@ -122,7 +122,7 @@ def test_delete_attribute(self):
 
         entry = self.ldap_connection.get_entry(
                 "cn=jack,dc=example,dc=com")
-        self.assertRaises(AttributeError, getattr, entry, "loginShell")
+        self.assertIsNone(entry.loginShell)
 
     def test_modify_single_value_attribute(self):
         entry = self.ldap_connection.get_entry(
@@ -135,11 +135,11 @@ def test_modify_single_value_attribute(self):
                 "cn=jack,dc=example,dc=com")
         self.assertEqual(entry.loginShell, "/bin/zsh")
         entry.loginShell = None
-        self.assertEqual(entry.loginShell, None)
+        self.assertIsNone(entry.loginShell)
         entry.save()
 
         entry.fetch()
-        self.assertRaises(AttributeError, getattr, entry, "loginShell")
+        self.assertIsNone(entry.loginShell)
 
     def test_modify_multi_value_attribute(self):
         entry = self.ldap_connection.get_entry(
@@ -182,7 +182,7 @@ def test_dn_computed_properties(self):
 
     def test_search(self):
         result = self.ldap_connection.search("cn=*n*")
-        self.assertEqual(set(["Noël", "daniel"]),
+        self.assertEqual(set(["Noël", "daniel", "admin"]),
                 set([next(iter(r.cn)) for r in result]))
 
     def test_search_empty_result(self):
@@ -191,12 +191,12 @@ def test_search_empty_result(self):
 
     def test_rename(self):
         entry = self.ldap_connection.get_entry(
-                "cn=daniel,dc=example,dc=com")
+                "cn=jack,dc=example,dc=com")
         entry.rename("cn=dieter,dc=example,dc=com")
         self.assertTrue(entry.exists())
 
         entry = self.ldap_connection.get_entry(
-                "cn=daniel,dc=example,dc=com")
+                "cn=jack,dc=example,dc=com")
         self.assertFalse(entry.exists())
 
         entry = self.ldap_connection.get_entry(
@@ -246,11 +246,23 @@ def test_entry_empty_multi_value_attribute(self):
         entry = self.ldap_connection.get_entry("cn=daniel,dc=example,dc=com")
         self.assertEqual({'superman'}, entry.description)
 
+    def test_entry_access_empty_attribute(self):
+        ldap_connection = ldapom.LDAPConnection(
+                uri=self.ldap_server.ldapi_url(),
+                base='dc=example,dc=com',
+                bind_dn='cn=daniel,dc=example,dc=com',
+                bind_password='daniel')
+        entry = ldap_connection.get_entry("cn=jack,dc=example,dc=com")
+        self.assertEqual(set(), entry.givenName)
+
+        # Ensure that saving doesn't trigger a write to unchanged and empty givenName
+        entry.save()
+        self.assertEqual(set(), entry.givenName)
+
     def test_entry_nonexistant_single_value_attribute(self):
         entry = self.ldap_connection.get_entry("cn=daniel,dc=example,dc=com")
         del entry.loginShell
-        with self.assertRaises(AttributeError):
-            entry.loginShell
+        self.assertIsNone(entry.loginShell)
 
     def test_retrieve_operational_attributes(self):
         # Test for https://github.com/HaDiNet/ldapom/issues/29