Merge "Refactor some ldap code to implement TODOs" into stable/stein

2 files changed, 21 insertions, 26 deletions

diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py
index f79c123d9..4bcebd939 100644
--- a/keystone/identity/backends/ldap/common.py
+++ b/keystone/identity/backends/ldap/common.py
@@ -1818,16 +1818,19 @@ class EnabledEmuMixIn(BaseLdap):
                            naming_rdn[1])
         self.enabled_emulation_naming_attr = naming_attr
 
-    # TODO(yoctozepto): methods below use _id_to_dn which requests another
-    # LDAP connection - optimize it
-
-    def _get_enabled(self, object_id, conn):
+    def _id_to_member_attribute_value(self, object_id):
+        """Convert id to value expected by member_attribute."""
         if self.group_members_are_ids:
-            dn = object_id
-        else:
-            dn = self._id_to_dn(object_id)
+            return object_id
+        return self._id_to_dn(object_id)
+
+    def _is_id_enabled(self, object_id, conn):
+        member_attr_val = self._id_to_member_attribute_value(object_id)
+        return self._is_member_enabled(member_attr_val, conn)
+
+    def _is_member_enabled(self, member_attr_val, conn):
         query = '(%s=%s)' % (self.member_attribute,
-                             ldap.filter.escape_filter_chars(dn))
+                             ldap.filter.escape_filter_chars(member_attr_val))
         try:
             enabled_value = conn.search_s(self.enabled_emulation_dn,
                                           ldap.SCOPE_BASE,
@@ -1838,34 +1841,26 @@ class EnabledEmuMixIn(BaseLdap):
             return bool(enabled_value)
 
     def _add_enabled(self, object_id):
-        if self.group_members_are_ids:
-            dn = object_id
-        else:
-            dn = self._id_to_dn(object_id)
+        member_attr_val = self._id_to_member_attribute_value(object_id)
         with self.get_connection() as conn:
-            # TODO(yoctozepto): _get_enabled potentially calls
-            # _id_to_dn 2nd time - optimize it
-            if not self._get_enabled(object_id, conn):
+            if not self._is_member_enabled(member_attr_val, conn):
                 modlist = [(ldap.MOD_ADD,
                             self.member_attribute,
-                            [dn])]
+                            [member_attr_val])]
                 try:
                     conn.modify_s(self.enabled_emulation_dn, modlist)
                 except ldap.NO_SUCH_OBJECT:
                     attr_list = [('objectClass', [self.group_objectclass]),
                                  (self.member_attribute,
-                                  [dn]),
+                                  [member_attr_val]),
                                  self.enabled_emulation_naming_attr]
                     conn.add_s(self.enabled_emulation_dn, attr_list)
 
     def _remove_enabled(self, object_id):
-        if self.group_members_are_ids:
-            dn = object_id
-        else:
-            dn = self._id_to_dn(object_id)
+        member_attr_val = self._id_to_member_attribute_value(object_id)
         modlist = [(ldap.MOD_DELETE,
                     self.member_attribute,
-                    [dn])]
+                    [member_attr_val])]
         with self.get_connection() as conn:
             try:
                 conn.modify_s(self.enabled_emulation_dn, modlist)
@@ -1890,7 +1885,7 @@ class EnabledEmuMixIn(BaseLdap):
             ref = super(EnabledEmuMixIn, self).get(object_id, ldap_filter)
             if ('enabled' not in self.attribute_ignore and
                     self.enabled_emulation):
-                ref['enabled'] = self._get_enabled(object_id, conn)
+                ref['enabled'] = self._is_id_enabled(object_id, conn)
             return ref
 
     def get_all(self, ldap_filter=None, hints=None):
@@ -1902,7 +1897,7 @@ class EnabledEmuMixIn(BaseLdap):
                         if x[0] != self.enabled_emulation_dn]
             with self.get_connection() as conn:
                 for obj_ref in obj_list:
-                    obj_ref['enabled'] = self._get_enabled(
+                    obj_ref['enabled'] = self._is_id_enabled(
                         obj_ref['id'], conn)
             return obj_list
         else:
diff --git a/keystone/tests/unit/test_backend_ldap.py b/keystone/tests/unit/test_backend_ldap.py
index d2b8c7db9..48fea00ec 100644
--- a/keystone/tests/unit/test_backend_ldap.py
+++ b/keystone/tests/unit/test_backend_ldap.py
@@ -2205,7 +2205,7 @@ class LDAPIdentityEnabledEmulation(LDAPIdentity, unit.TestCase):
         # Override the tree_dn, it's used to build the enabled member filter
         mixin_impl.tree_dn = sample_dn
 
-        # The filter that _get_enabled is going to build contains the
+        # The filter, which _is_id_enabled is going to build, contains the
         # tree_dn, which better be escaped in this case.
         exp_filter = '(%s=%s=%s,%s)' % (
             mixin_impl.member_attribute, mixin_impl.id_attr, object_id,
@@ -2214,7 +2214,7 @@ class LDAPIdentityEnabledEmulation(LDAPIdentity, unit.TestCase):
         with mixin_impl.get_connection() as conn:
             m = self.useFixture(
                 fixtures.MockPatchObject(conn, 'search_s')).mock
-            mixin_impl._get_enabled(object_id, conn)
+            mixin_impl._is_id_enabled(object_id, conn)
             # The 3rd argument is the DN.
             self.assertEqual(exp_filter, m.call_args[0][2])