From fb813874b6f658fd1f4cac56682c70a60576ee23 Mon Sep 17 00:00:00 2001
From: Paul Marillonnet <pmarillonnet@entrouvert.com>
Date: Fri, 17 Jun 2022 10:23:40 +0200
Subject: [PATCH 1/2] ldap: return server error info on failed connection
 attempts (#65491)

---
 src/authentic2/backends/ldap_backend.py | 72 +++++++++++++------------
 src/authentic2/manager/views.py         |  2 +-
 tests/test_ldap.py                      |  5 +-
 3 files changed, 41 insertions(+), 38 deletions(-)

diff --git a/src/authentic2/backends/ldap_backend.py b/src/authentic2/backends/ldap_backend.py
index 2ba481409..26e15f300 100644
--- a/src/authentic2/backends/ldap_backend.py
+++ b/src/authentic2/backends/ldap_backend.py
@@ -362,7 +362,7 @@ class LDAPUser(User):
             return None
 
     def check_password(self, raw_password):
-        connection = self.ldap_backend.get_connection(self.block)
+        connection, _ = self.ldap_backend.get_connection(self.block)
         if connection:
             try:
                 connection.simple_bind_s(self.dn, raw_password)
@@ -380,7 +380,7 @@ class LDAPUser(User):
         # if the verify pass, we have the old password stored in self._current_password
         _current_password = getattr(self, '_current_password', None) or self.get_password_in_session()
         if _current_password != new_password:
-            conn = self.get_connection()
+            conn, dummy = self.get_connection()
             if not conn:
                 log.warning('ldap: set_password failed, could not get a connection')
                 return
@@ -414,7 +414,7 @@ class LDAPUser(User):
         cache_key = hashlib.md5(
             (force_text(str(self.pk)) + ';' + force_text(self.dn)).encode('utf-8')
         ).hexdigest()
-        conn = self.get_connection()
+        conn, dummy = self.get_connection()
         # prevents blocking on temporary LDAP failures
         if conn is not None:
             attributes = self.ldap_backend.get_ldap_attributes(self.block, conn, self.dn) or {}
@@ -599,7 +599,9 @@ class LDAPBackend:
         group_to_role_mapping = block.get('group_to_role_mapping')
         if not block.get('group_filter') or not group_to_role_mapping:
             return
-        for conn in cls.get_connections(block):
+        for conn, dummy in cls.get_connections(block):
+            if not conn:
+                continue
             existing_groups = cls.get_groups_dns(conn, block)
             for group_dn, dummy_role_slugs in group_to_role_mapping:
                 if group_dn in existing_groups:
@@ -647,7 +649,9 @@ class LDAPBackend:
                 return user
 
     def authenticate_block(self, request, block, username, password):
-        for conn in self.get_connections(block):
+        for conn, dummy in self.get_connections(block):
+            if not conn:
+                continue
             ldap_uri = conn.get_option(ldap.OPT_URI)
             authz_ids = []
             user_basedn = force_text(block.get('user_basedn') or block['basedn'])
@@ -1628,7 +1632,7 @@ class LDAPBackend:
     @classmethod
     def get_users_for_block(cls, block):
         log.info('Synchronising users from realm "%s"', block['realm'])
-        conn = cls.get_connection(block)
+        conn, dummy = cls.get_connection(block)
         if conn is None:
             log.warning('unable to synchronize with LDAP servers %s', force_text(block['url']))
             return
@@ -1682,7 +1686,7 @@ class LDAPBackend:
         from authentic2.manager.journal_event_types import ManagerUserDeactivation
 
         for block in cls.get_config():
-            conn = cls.get_connection(block)
+            conn, dummy = cls.get_connection(block)
             if conn is None:
                 continue
             ldap_uri = conn.get_option(ldap.OPT_URI)
@@ -1810,42 +1814,40 @@ class LDAPBackend:
             conn.set_option(ldap.OPT_REFERRALS, 1 if block['referrals'] else 0)
             # allow TLS options to be applied
             conn.set_option(ldap.OPT_X_TLS_NEWCTX, 0)
+            error = None
             try:
                 if not url.startswith('ldaps://') and block['use_tls']:
                     try:
                         conn.start_tls_s()
                     except ldap.CONNECT_ERROR:
-                        log.error(
-                            'connection to %r failed when activating TLS, did you forget to declare the TLS'
-                            ' certificate in /etc/ldap/ldap.conf ?',
-                            url,
-                        )
-                        continue
+                        error = (
+                            'connection to %r failed when activating TLS, did you forget to declare the TLS certificate'
+                            ' in /etc/ldap/ldap.conf ?'
+                        ) % url
             except ldap.TIMEOUT:
-                log.error('connection to %r timed out', url)
-                continue
+                error = 'connection to %r timed out' % url
             except ldap.CONNECT_ERROR:
-                log.error(
+                error = (
                     'connection to %r failed when activating TLS, did you forget to declare the TLS'
-                    ' certificate in /etc/ldap/ldap.conf ?',
-                    url,
-                )
-                continue
+                    ' certificate in /etc/ldap/ldap.conf ?'
+                ) % url
             except ldap.SERVER_DOWN:
-                if block['replicas']:
-                    log.warning('ldap %r is down', url)
-                else:
-                    log.error('ldap %r is down', url)
-                continue
+                error = 'ldap %r is down' % url
             user_credentials = block['connect_with_user_credentials'] and credentials
-            success, error = cls.bind(block, conn, credentials=user_credentials)
-            if success:
-                yield conn
+            if error:
+                log.error(error)
+                yield None, error
             else:
-                if block['replicas']:
-                    log.warning('admin bind failed on %s: %s', url, error)
+                success, binderror = cls.bind(block, conn, credentials=user_credentials)
+                if success:
+                    yield conn, None
                 else:
-                    log.error('admin bind failed on %s: %s', url, error)
+                    msg = 'admin bind failed on %s: %s' % (url, binderror)
+                    yield None, error
+                    if block['replicas']:
+                        log.warning(msg)
+                    else:
+                        log.error(msg)
 
     @classmethod
     def bind(cls, block, conn, credentials=()):
@@ -1903,9 +1905,9 @@ class LDAPBackend:
     @classmethod
     def get_connection(cls, block, credentials=()):
         '''Try to get at least one connection'''
-        for conn in cls.get_connections(block, credentials=credentials):
-            return conn
-        return None
+        for conn, err in cls.get_connections(block, credentials=credentials):
+            return conn, err
+        return None, None
 
     @classmethod
     def update_default(cls, block, validate=True):
@@ -2015,7 +2017,7 @@ class LDAPBackendPasswordLost(LDAPBackend):
                 if user_external_id.source != force_text(block['realm']):
                     continue
                 for external_id_tuple in map_text(block['external_id_tuples']):
-                    conn = self.ldap_backend.get_connection(block)
+                    conn, dummy = self.ldap_backend.get_connection(block)
                     if not conn:
                         log.warning('ldap: password-lost authenticate failed, could not get a connection')
                         continue
diff --git a/src/authentic2/manager/views.py b/src/authentic2/manager/views.py
index 6e205660b..103bad555 100644
--- a/src/authentic2/manager/views.py
+++ b/src/authentic2/manager/views.py
@@ -676,7 +676,7 @@ class TechnicalInformationView(TitleMixin, MediaMixin, TemplateView):
         kwargs['ldap_list'] = []
         for block in backend.get_config():
             config = block.copy()
-            conn = backend.get_connection(config)
+            conn, dummy = backend.get_connection(config)
             if not conn:
                 kwargs['error'] = True
             else:
diff --git a/tests/test_ldap.py b/tests/test_ldap.py
index 9c82e2ef7..6d7c0c1de 100644
--- a/tests/test_ldap.py
+++ b/tests/test_ldap.py
@@ -1532,7 +1532,8 @@ pwdSafeModify: FALSE
 
     time.sleep(pwdMaxAge * 3)
 
-    conn = ldap_backend.LDAPBackend.get_connection(settings.LDAP_AUTH_SETTINGS[0])
+    conn, errmsg = ldap_backend.LDAPBackend.get_connection(settings.LDAP_AUTH_SETTINGS[0])
+    assert errmsg is None
     attributes = ldap_backend.LDAPBackend.get_ppolicy_attributes(settings.LDAP_AUTH_SETTINGS[0], conn, DN)
     assert 'pwdchangedtime' in attributes
     assert attributes['pwdmaxage'] == [str(pwdMaxAge)]
@@ -2288,7 +2289,7 @@ def test_technical_info_ldap(app, admin, superuser, slapd, settings, monkeypatch
         assert opt in ldap_config_text
 
     # mock a buggy connection
-    monkeypatch.setattr(ldap_backend.LDAPBackend, 'get_connection', lambda x: None)
+    monkeypatch.setattr(ldap_backend.LDAPBackend, 'get_connection', lambda x: (None, None))
     resp = app.get(reverse('a2-manager-tech-info'))
     ldap_config_text = resp.pyquery('div#a2-manager-tech-info-ldap-list').text()
 
-- 
2.36.1

