Fixing dn_to_id function for cases were id is not in the DN

The more common scenario to return the uid as part of the RDN in a DN,
However, it's a valid case to not have the uid in the RDN, so we need to
search in the LDAP based on the DN and return the uid in the entire object.

Also, we do not support multivalued attribute id on DN, so the test case
covering this case, it was adjusted for raise NotFound.

Closes-Bug: 1782922
Change-Id: I87a3bfa94b5907ce4c6b4eb8e124ec948b390bf2
(cherry picked from commit a1dc21f3d34ae34bc6a5c9acebc0eb752495ae7a)

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'.