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] c7fae97d873f72068ca65538ec5b5919c0ac7d5a
[2] https://review.opendev.org/683303
[3] 19d4831daa3991bed48fb364fa05927740c96445

Change-Id: I22f3bce647182996dfc06084ee6d4989449e3d2d
(cherry picked from commit a6bb81146ff2126f055834459f428cf97080466f)

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])