From 0f028717d3aaabbde8dae1c33779bf945f32e49e Mon Sep 17 00:00:00 2001
From: Paul Marillonnet <pmarillonnet@entrouvert.com>
Date: Thu, 21 Jul 2022 16:08:54 +0200
Subject: [PATCH 1/2] ldap: perform extensive user lookup regardless of their
 origin (#67600)

---
 src/authentic2/backends/ldap_backend.py | 22 +++++++++++++---------
 tests/test_ldap.py                      | 19 +++++++++++--------
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/src/authentic2/backends/ldap_backend.py b/src/authentic2/backends/ldap_backend.py
index 11960226e..d760e888a 100644
--- a/src/authentic2/backends/ldap_backend.py
+++ b/src/authentic2/backends/ldap_backend.py
@@ -1359,15 +1359,18 @@ class LDAPBackend:
         return ' '.join(part for part in parts)
 
     def _lookup_user_queryset(self, block):
-        return LDAPUser.objects.prefetch_related('groups').exclude(userexternalid__source=block['realm'])
+        ueids = UserExternalId.objects.filter(source=block['realm'])
+        return User.objects.prefetch_related('groups').exclude(
+            id__in=ueids.values_list('user__id', flat=True)
+        )
 
     def _lookup_by_username(self, ou, block, username):
         try:
             log.debug('ldap: lookup using username %r', username)
             return self._lookup_user_queryset(block=block).get(ou=ou, username=username)
-        except LDAPUser.DoesNotExist:
+        except User.DoesNotExist:
             return None
-        except LDAPUser.MultipleObjectsReturned:
+        except User.MultipleObjectsReturned:
             log.warning(
                 'ldap: lookup using username %r, too many users with this username in ou "%s"', username, ou
             )
@@ -1384,9 +1387,9 @@ class LDAPBackend:
         try:
             log.debug('ldap: lookup using email %r', email)
             return self._lookup_user_queryset(block=block).get(ou=ou, email=email)
-        except LDAPUser.DoesNotExist:
+        except User.DoesNotExist:
             return None
-        except LDAPUser.MultipleObjectsReturned:
+        except User.MultipleObjectsReturned:
             log.warning('ldap: lookup using email %r, too many users with this email in ou "%s"', email, ou)
             return None
 
@@ -1583,9 +1586,10 @@ class LDAPBackend:
             user = LDAPUser(username=username)
             user._created = True
             user.set_unusable_password()
-        user.ldap_init(block, dn)
-        user.keep_password(password)
-        self.populate_user(user, dn, username, conn, block, attributes)
+        if isinstance(user, LDAPUser):
+            user.ldap_init(block, dn)
+            user.keep_password(password)
+            self.populate_user(user, dn, username, conn, block, attributes)
         if not user.pk or getattr(user, '_changed', False):
             user.save()
 
@@ -1652,7 +1656,7 @@ class LDAPBackend:
             if not user:
                 log.warning('unable to retrieve user for dn %s', dn)
                 continue
-            if user._changed or user._created:
+            if isinstance(user, LDAPUser) and (user._changed or user._created):
                 log.info(
                     '%s user %s (uuid %s) from %s',
                     'Created' if user._created else 'Updated',
diff --git a/tests/test_ldap.py b/tests/test_ldap.py
index 924effc43..5a34fc499 100644
--- a/tests/test_ldap.py
+++ b/tests/test_ldap.py
@@ -771,7 +771,7 @@ def test_get_users(slapd, settings, db, monkeypatch, caplog):
             'group_to_role_mapping': [
                 ['cn=unknown,o=dn', ['Role2']],
             ],
-            'lookups': ['external_id', 'username'],
+            'lookups': ['external_id', 'username', 'email'],
         }
     ]
     save = mock.Mock(wraps=ldap_backend.LDAPUser.save)
@@ -2324,15 +2324,16 @@ class TestLookup:
         return ldap_backend.LDAPBackend()
 
     def test_by_email(self, backend, slapd, settings, client, db):
-        user = User.objects.create(email=EMAIL, ou=get_default_ou())
+        user = User.objects.create(email=EMAIL, ou=get_default_ou(), username=USERNAME)
         assert backend.authenticate(None, username=USERNAME, password=PASS) == user
-        assert models.UserExternalId.objects.get(user=user, source='ldap', external_id=UID)
+        # user existed before ldap authn, we do not consider ldap as its source
+        assert not models.UserExternalId.objects.filter(user=user, source='ldap', external_id=UID).count()
         user.email = ''
         user.save()
-        # if email is changed
         auth_user = backend.authenticate(None, username=USERNAME, password=PASS)
+        # we do not modify pre-existing users
         assert auth_user == user
-        assert auth_user.email == EMAIL
+        assert auth_user.email == ''
 
     def test_by_email_only(self, backend, slapd, settings, client, db):
         settings.LDAP_AUTH_SETTINGS[0]['lookups'] = ['email']
@@ -2346,9 +2347,10 @@ class TestLookup:
         assert new_user.email == EMAIL
 
     def test_by_username(self, backend, slapd, settings, client, db):
-        user = User.objects.create(username=UID, ou=get_default_ou())
+        user = User.objects.create(username=UID, email=EMAIL, ou=get_default_ou())
         assert backend.authenticate(None, username=EMAIL, password=PASS) == user
-        assert models.UserExternalId.objects.get(user=user, source='ldap', external_id=UID)
+        # user existed before ldap authn, we do not consider ldap as its source
+        assert not models.UserExternalId.objects.filter(user=user, source='ldap', external_id=UID).count()
         user.username = ''
         user.save()
         auth_user = backend.authenticate(None, username=EMAIL, password=PASS)
@@ -2356,8 +2358,9 @@ class TestLookup:
         assert auth_user.username == ''
         settings.LDAP_AUTH_SETTINGS[0]['update_username'] = True
         auth_user = backend.authenticate(None, username=EMAIL, password=PASS)
+        # we do not modify pre-existing users
         assert auth_user == user
-        assert auth_user.username == f'{UID}@ldap'
+        assert auth_user.username == ''
 
     def test_by_guid_migration(self, backend, slapd, settings, client, db):
         settings.LDAP_AUTH_SETTINGS[0]['lookups'] = ['external_id']
-- 
2.36.1

