From 93f548a72569bfdf323827bb05c367381c8dedfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Piliszek?= Date: Fri, 28 Feb 2020 19:42:24 +0100 Subject: 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) --- keystone/identity/backends/ldap/common.py | 43 ++++++++++++++----------------- keystone/tests/unit/test_backend_ldap.py | 4 +-- 2 files changed, 21 insertions(+), 26 deletions(-) diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py index 041f0d68c..cef0d9bdf 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 bd6f5c7ab..4c8912c2c 100644 --- a/keystone/tests/unit/test_backend_ldap.py +++ b/keystone/tests/unit/test_backend_ldap.py @@ -2223,7 +2223,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, @@ -2232,7 +2232,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]) -- cgit v1.2.1