Honor group_members_are_ids for user_enabled_emulation

Applied when group config is to be honored
(i.e. set user_enabled_emulation_use_group_config).
Conditionals follow usage of group_members_are_ids.

Added new test for the case with ids.
It fails without fix.
The original test expanded to ensure the change did not
break its internals either.
It passes without fix as well.

Additionally some TODOs are added for observed potential issues.

Backport amended with [1] to pass CI.

[1] 19d4831daa3991bed48fb364fa05927740c96445 (pep8)

Change-Id: I7874a70e6109219baee80309c3a27f8af9905a6d
Closes-Bug: #1839133
Signed-off-by: Radosław Piliszek <radoslaw.piliszek@gmail.com>
(cherry picked from commit c7fae97d873f72068ca65538ec5b5919c0ac7d5a)

3 files changed, 76 insertions, 4 deletions

diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py
index b9becea74..f79c123d9 100644
--- a/keystone/identity/backends/ldap/common.py
+++ b/keystone/identity/backends/ldap/common.py
@@ -1781,6 +1781,7 @@ class EnabledEmuMixIn(BaseLdap):
 
     DEFAULT_GROUP_OBJECTCLASS = 'groupOfNames'
     DEFAULT_MEMBER_ATTRIBUTE = 'member'
+    DEFAULT_GROUP_MEMBERS_ARE_IDS = False
 
     def __init__(self, conf):
         super(EnabledEmuMixIn, self).__init__(conf)
@@ -1797,9 +1798,11 @@ class EnabledEmuMixIn(BaseLdap):
         if not self.use_group_config:
             self.member_attribute = self.DEFAULT_MEMBER_ATTRIBUTE
             self.group_objectclass = self.DEFAULT_GROUP_OBJECTCLASS
+            self.group_members_are_ids = self.DEFAULT_GROUP_MEMBERS_ARE_IDS
         else:
             self.member_attribute = conf.ldap.group_member_attribute
             self.group_objectclass = conf.ldap.group_objectclass
+            self.group_members_are_ids = conf.ldap.group_members_are_ids
 
         if not self.enabled_emulation_dn:
             naming_attr_name = 'cn'
@@ -1815,8 +1818,14 @@ 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):
-        dn = self._id_to_dn(object_id)
+        if self.group_members_are_ids:
+            dn = object_id
+        else:
+            dn = self._id_to_dn(object_id)
         query = '(%s=%s)' % (self.member_attribute,
                              ldap.filter.escape_filter_chars(dn))
         try:
@@ -1829,24 +1838,34 @@ 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)
         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):
                 modlist = [(ldap.MOD_ADD,
                             self.member_attribute,
-                            [self._id_to_dn(object_id)])]
+                            [dn])]
                 try:
                     conn.modify_s(self.enabled_emulation_dn, modlist)
                 except ldap.NO_SUCH_OBJECT:
                     attr_list = [('objectClass', [self.group_objectclass]),
                                  (self.member_attribute,
-                                  [self._id_to_dn(object_id)]),
+                                  [dn]),
                                  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)
         modlist = [(ldap.MOD_DELETE,
                     self.member_attribute,
-                    [self._id_to_dn(object_id)])]
+                    [dn])]
         with self.get_connection() as conn:
             try:
                 conn.modify_s(self.enabled_emulation_dn, modlist)
diff --git a/keystone/tests/unit/test_backend_ldap.py b/keystone/tests/unit/test_backend_ldap.py
index aa7a50747..d2b8c7db9 100644
--- a/keystone/tests/unit/test_backend_ldap.py
+++ b/keystone/tests/unit/test_backend_ldap.py
@@ -2046,9 +2046,17 @@ class LDAPIdentityEnabledEmulation(LDAPIdentity, unit.TestCase):
             "Enabled emulation conflicts with enabled mask")
 
     def test_user_enabled_use_group_config(self):
+        # Establish enabled-emulation group name to later query its members
+        group_name = 'enabled_users'
+        driver = PROVIDERS.identity_api._select_identity_driver(
+            CONF.identity.default_domain_id)
+        group_dn = 'cn=%s,%s' % (group_name, driver.group.tree_dn)
+
         self.config_fixture.config(
             group='ldap',
             user_enabled_emulation_use_group_config=True,
+            user_enabled_emulation_dn=group_dn,
+            group_name_attribute='cn',
             group_member_attribute='uniqueMember',
             group_objectclass='groupOfUniqueNames')
         self.ldapdb.clear()
@@ -2064,6 +2072,46 @@ class LDAPIdentityEnabledEmulation(LDAPIdentity, unit.TestCase):
         user_ref = PROVIDERS.identity_api.get_user(user_ref['id'])
         self.assertIs(True, user_ref['enabled'])
 
+        # Ensure state matches the group config
+        group_ref = PROVIDERS.identity_api.get_group_by_name(
+            group_name, CONF.identity.default_domain_id)
+        PROVIDERS.identity_api.check_user_in_group(
+            user_ref['id'], group_ref['id'])
+
+    def test_user_enabled_use_group_config_with_ids(self):
+        # Establish enabled-emulation group name to later query its members
+        group_name = 'enabled_users'
+        driver = PROVIDERS.identity_api._select_identity_driver(
+            CONF.identity.default_domain_id)
+        group_dn = 'cn=%s,%s' % (group_name, driver.group.tree_dn)
+
+        self.config_fixture.config(
+            group='ldap',
+            user_enabled_emulation_use_group_config=True,
+            user_enabled_emulation_dn=group_dn,
+            group_name_attribute='cn',
+            group_member_attribute='memberUid',
+            group_members_are_ids=True,
+            group_objectclass='posixGroup')
+        self.ldapdb.clear()
+        self.load_backends()
+
+        # Create a user and ensure they are enabled.
+        user1 = unit.new_user_ref(enabled=True,
+                                  domain_id=CONF.identity.default_domain_id)
+        user_ref = PROVIDERS.identity_api.create_user(user1)
+        self.assertIs(True, user_ref['enabled'])
+
+        # Get a user and ensure they are enabled.
+        user_ref = PROVIDERS.identity_api.get_user(user_ref['id'])
+        self.assertIs(True, user_ref['enabled'])
+
+        # Ensure state matches the group config
+        group_ref = PROVIDERS.identity_api.get_group_by_name(
+            group_name, CONF.identity.default_domain_id)
+        PROVIDERS.identity_api.check_user_in_group(
+            user_ref['id'], group_ref['id'])
+
     def test_user_enabled_invert(self):
         self.config_fixture.config(group='ldap', user_enabled_invert=True,
                                    user_enabled_default='False')
diff --git a/releasenotes/notes/bug-1839133-24570c9fbacb530d.yaml b/releasenotes/notes/bug-1839133-24570c9fbacb530d.yaml
new file mode 100644
index 000000000..b6ed1556d
--- /dev/null
+++ b/releasenotes/notes/bug-1839133-24570c9fbacb530d.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    [`bug 1839133 <https://bugs.launchpad.net/keystone/+bug/1839133>`_]
+    Makes user_enabled_emulation_use_group_config honor group_members_are_ids.