Refactor some ldap code to implement TODOs

This implements TODOs added in [1], as promised in [2].
The first TODO is realised only partially because most ldap code
actually relies on having two connections obtained from the pool.

This optimizes mixin code by removing extra ldap calls.
There is no change in the observed behaviour of integration.

This also removes some duplication and refactors names to avoid
some confusion related to dn/object_id.

Backport to: Train, Stein (with [1]&[3]), Rocky (with [1]&[3]),
             Queens (with [1]&[3])

[1] c7fae97d87
[2] https://review.opendev.org/683303
[3] 19d4831daa

Change-Id: I22f3bce647182996dfc06084ee6d4989449e3d2d

keystone/identity/backends/ldap/common.py

@@ -1807,16 +1807,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,
@@ -1827,34 +1830,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)
@@ -1879,7 +1874,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):
@@ -1891,7 +1886,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:

keystone/tests/unit/test_backend_ldap.py

@@ -2224,7 +2224,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,
@@ -2233,7 +2233,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])