Merge "Fixing dn_to_id function for cases were id is not in the DN" into stable/rocky

4 files changed, 97 insertions, 10 deletions

diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py
index e54c8d166..b873d6e0e 100644
--- a/keystone/identity/backends/ldap/common.py
+++ b/keystone/identity/backends/ldap/common.py
@@ -1293,9 +1293,38 @@ class BaseLdap(object):
         else:
             return self._id_to_dn_string(object_id)
 
-    @staticmethod
-    def _dn_to_id(dn):
-        return ldap.dn.str2dn(dn)[0][0][1]
+    def _dn_to_id(self, dn):
+        # Check if the naming attribute in the DN is the same as keystone's
+        # configured 'id' attribute'.  If so, extract the ID value from the DN
+        if self.id_attr == utf8_decode(
+                ldap.dn.str2dn(utf8_encode(dn))[0][0][0].lower()):
+            return utf8_decode(ldap.dn.str2dn(utf8_encode(dn))[0][0][1])
+        else:
+            # The 'ID' attribute is NOT in the DN, so we need to perform an
+            # LDAP search to look it up from the user entry itself.
+            with self.get_connection() as conn:
+                search_result = conn.search_s(dn, ldap.SCOPE_BASE)
+
+            if search_result:
+                try:
+                    id_list = search_result[0][1][self.id_attr]
+                except KeyError:
+                    message = ('ID attribute %(id_attr)s not found in LDAP '
+                               'object %(dn)s.') % ({'id_attr': self.id_attr,
+                                                     'dn': search_result})
+                    LOG.warning(message)
+                    raise exception.NotFound(message=message)
+                if len(id_list) > 1:
+                    message = ('In order to keep backward compatibility, in '
+                               'the case of multivalued ids, we are '
+                               'returning the first id %(id_attr) in the '
+                               'DN.') % ({'id_attr': id_list[0]})
+                    LOG.warning(message)
+                return id_list[0]
+            else:
+                message = _('DN attribute %(dn)s not found in LDAP') % (
+                    {'dn': dn})
+                raise exception.NotFound(message=message)
 
     def _ldap_res_to_model(self, res):
         # LDAP attribute names may be returned in a different case than
diff --git a/keystone/identity/backends/ldap/core.py b/keystone/identity/backends/ldap/core.py
index 32cd97602..212f89e73 100644
--- a/keystone/identity/backends/ldap/core.py
+++ b/keystone/identity/backends/ldap/core.py
@@ -311,8 +311,11 @@ class UserApi(common_ldap.EnabledEmuMixIn, common_ldap.BaseLdap):
         return obj
 
     def get_filtered(self, user_id):
-        user = self.get(user_id)
-        return self.filter_attributes(user)
+        try:
+            user = self.get(user_id)
+            return self.filter_attributes(user)
+        except ldap.NO_SUCH_OBJECT:
+            raise self.NotFound(user_id=user_id)
 
     def get_all(self, ldap_filter=None, hints=None):
         objs = super(UserApi, self).get_all(ldap_filter=ldap_filter,
diff --git a/keystone/tests/unit/test_backend_ldap.py b/keystone/tests/unit/test_backend_ldap.py
index 25dae27ae..564678014 100644
--- a/keystone/tests/unit/test_backend_ldap.py
+++ b/keystone/tests/unit/test_backend_ldap.py
@@ -1816,7 +1816,33 @@ class LDAPIdentity(BaseLDAPIdentity):
         self.assertEqual(self.user_foo['email'], user_ref['id'])
 
     @mock.patch.object(common_ldap.BaseLdap, '_ldap_get')
-    def test_get_id_from_dn_for_multivalued_attribute_id(self, mock_ldap_get):
+    def test_get_multivalued_attribute_id_from_dn(self,
+                                                  mock_ldap_get):
+        driver = PROVIDERS.identity_api._select_identity_driver(
+            CONF.identity.default_domain_id)
+        driver.user.id_attr = 'mail'
+
+        # make 'email' multivalued so we can test the error condition
+        email1 = uuid.uuid4().hex
+        email2 = uuid.uuid4().hex
+        # Mock the ldap search results to return user entries with
+        # user_name_attribute('sn') value has emptyspaces, emptystring
+        # and attibute itself is not set.
+        mock_ldap_get.return_value = (
+            'cn=users,dc=example,dc=com',
+            {
+                'mail': [email1, email2],
+            }
+        )
+
+        # This is not a valid scenario, since we do not support multiple value
+        # attribute id on DN.
+        self.assertRaises(exception.NotFound,
+                          PROVIDERS.identity_api.get_user, email1)
+
+    @mock.patch.object(common_ldap.BaseLdap, '_ldap_get')
+    def test_raise_not_found_dn_for_multivalued_attribute_id(self,
+                                                             mock_ldap_get):
         driver = PROVIDERS.identity_api._select_identity_driver(
             CONF.identity.default_domain_id)
         driver.user.id_attr = 'mail'
@@ -1833,10 +1859,29 @@ class LDAPIdentity(BaseLDAPIdentity):
             }
         )
 
-        user_ref = PROVIDERS.identity_api.get_user(email1)
-        # make sure we get the ID from DN (old behavior) if the ID attribute
-        # has multiple values
-        self.assertEqual('nobodycares', user_ref['id'])
+        # This is not a valid scenario, since we do not support multiple value
+        # attribute id on DN.
+        self.assertRaises(exception.NotFound,
+                          PROVIDERS.identity_api.get_user, email1)
+
+    @mock.patch.object(common_ldap.BaseLdap, '_ldap_get')
+    def test_get_id_not_in_dn(self,
+                              mock_ldap_get):
+        driver = PROVIDERS.identity_api._select_identity_driver(
+            CONF.identity.default_domain_id)
+        driver.user.id_attr = 'sAMAccountName'
+
+        user_id = uuid.uuid4().hex
+        mock_ldap_get.return_value = (
+            'cn=someuser,dc=example,dc=com',
+            {
+                'cn': 'someuser',
+                'sn': [uuid.uuid4().hex],
+                'sAMAccountName': [user_id],
+            }
+        )
+        user_ref = PROVIDERS.identity_api.get_user(user_id)
+        self.assertEqual(user_id, user_ref['id'])
 
     @mock.patch.object(common_ldap.BaseLdap, '_ldap_get')
     def test_id_attribute_not_found(self, mock_ldap_get):
diff --git a/releasenotes/notes/bug-1782922-db822fda486ac773.yaml b/releasenotes/notes/bug-1782922-db822fda486ac773.yaml
new file mode 100644
index 000000000..d8f5e26d7
--- /dev/null
+++ b/releasenotes/notes/bug-1782922-db822fda486ac773.yaml
@@ -0,0 +1,10 @@
+---
+fixes:
+  - |
+    [`bug 1782922 <https://bugs.launchpad.net/keystone/+bug/1782922>`_]
+    Fixed the problem where Keystone indiscriminately return the first RDN
+    as the user ID, regardless whether it matches the configured
+    'user_id_attribute' or not. This will break deployments where
+    'group_members_are_ids' are set to False and 'user_id_attribute' is not
+    in the DN. This patch will perform a lookup by DN if the first RND does
+    not match the configured 'user_id_attribute'.