Don't catch all Exception

- add some logging
- fail properly when more than 1 user is found
- add unittest

review6411002

res.py

@@ -1,5 +1,7 @@
 #This file is part of Tryton.  The COPYRIGHT file at the top level of
 #this repository contains the full copyright notices and license terms.
+import logging
+
 import ldap
 from trytond.transaction import Transaction
 from trytond.pool import Pool, PoolMeta
@@ -7,6 +9,8 @@ from trytond.pool import Pool, PoolMeta
 __all__ = ['User']
 __metaclass__ = PoolMeta
 
+logger = logging.getLogger(__name__)
+
 
 class User:
     __name__ = 'res.user'
@@ -41,6 +45,8 @@ class User:
                     filter, attrs)
         if connection.active_directory:
             result = [x for x in result if x[0]]
+        if result and len(result) > 1:
+            logger.info('ldap_search_user found more than 1 user')
         return result
 
     @classmethod
@@ -62,8 +68,8 @@ class User:
                         con, connection, attrs=[]):
                     find = True
                     break
-        except Exception:
-            pass
+        except ldap.LDAPError, e:
+            logger.error('LDAPError: %s' % str(e))
         if find:
             cls.raise_user_error('set_passwd_ldap_user', (login.rec_name,))
 
@@ -101,16 +107,18 @@ class User:
                 if connection.bind_dn:
                     con.simple_bind_s(connection.bind_dn, connection.bind_pass)
                 user = cls(Transaction().user)
-                [(dn, attrs)] = cls.ldap_search_user(user.login, con,
+                users = cls.ldap_search_user(user.login, con,
                     connection, attrs=[str(connection.auth_uid)])
-                if con.simple_bind_s(dn, old_password):
-                    con.passwd_s(dn, old_password, values['password'])
-                    values = values.copy()
-                    del values['password']
-                else:
-                    cls.raise_user_error('wrong_password')
-            except Exception:
-                pass
+                if users and len(users) == 1:
+                    [(dn, attrs)] = users
+                    if con.simple_bind_s(dn, old_password):
+                        con.passwd_s(dn, old_password, values['password'])
+                        values = values.copy()
+                        del values['password']
+                    else:
+                        cls.raise_user_error('wrong_password')
+            except ldap.LDAPError, e:
+                logger.error('LDAPError: %s' % str(e))
         super(User, cls).set_preferences(values, old_password=old_password)
 
     @classmethod
@@ -128,20 +136,22 @@ class User:
                 con.start_tls_s()
             if connection.bind_dn:
                 con.simple_bind_s(connection.bind_dn, connection.bind_pass)
-            [(dn, attrs)] = cls.ldap_search_user(login, con, connection,
+            users = cls.ldap_search_user(login, con, connection,
                 attrs=[str(connection.auth_uid)])
-            if password and con.simple_bind_s(dn, password):
-                user_id, _ = cls._get_login(login)
-                if user_id:
-                    LoginAttempt.remove(login)
-                    return user_id
-                elif connection.auth_create_user:
-                    user, = cls.create([{
-                                'name': attrs.get(str(connection.auth_uid),
-                                    [login])[0],
-                                'login': login,
-                                }])
-                    return user.id
-        except Exception:
-            pass
+            if users and len(users) == 1:
+                [(dn, attrs)] = users
+                if password and con.simple_bind_s(dn, password):
+                    user_id, _ = cls._get_login(login)
+                    if user_id:
+                        LoginAttempt.remove(login)
+                        return user_id
+                    elif connection.auth_create_user:
+                        user, = cls.create([{
+                                    'name': attrs.get(str(connection.auth_uid),
+                                        [login])[0],
+                                    'login': login,
+                                    }])
+                        return user.id
+        except ldap.LDAPError, e:
+            logger.error('LDAPError: %s' % str(e))
         return super(User, cls).get_login(login, password)

setup.py

@@ -47,6 +47,8 @@ for dep in info.get('depends', []):
         requires.append(get_require_version('trytond_%s' % dep))
 requires.append(get_require_version('trytond'))
 
+tests_require = ['mock']
+
 setup(name=name,
     version=version,
     description='Tryton module to authenticate users through LDAP',
@@ -98,4 +100,5 @@ setup(name=name,
     """,
     test_suite='tests',
     test_loader='trytond.test_loader:Loader',
+    tests_require=tests_require,
     )

tests/test_ldap_authentication.py

@@ -1,8 +1,14 @@
 #This file is part of Tryton.  The COPYRIGHT file at the top level of
 #this repository contains the full copyright notices and license terms.
 import unittest
+
+from mock import patch
+import ldap
+
 import trytond.tests.test_tryton
 from trytond.tests.test_tryton import test_view, test_depends
+from trytond.tests.test_tryton import POOL, DB_NAME, USER, CONTEXT
+from trytond.transaction import Transaction
 
 
 class LDAPAuthenticationTestCase(unittest.TestCase):
@@ -19,6 +25,43 @@ class LDAPAuthenticationTestCase(unittest.TestCase):
         'Test depends'
         test_depends()
 
+    def test_user_get_login(self):
+        'Test User.get_login'
+        with Transaction().start(DB_NAME, USER, context=CONTEXT):
+            User = POOL.get('res.user')
+            Connection = POOL.get('ldap.connection')
+
+            connection = Connection(server='localhost',
+                auth_base_dn='dc=tryton,dc=org')
+            connection.save()
+
+            @patch.object(ldap, 'initialize')
+            @patch.object(User, 'ldap_search_user')
+            def get_login(login, password, find, ldap_search_user, initialize):
+                con = initialize.return_value
+                con.simple_bind_s.return_value = True
+                if find:
+                    ldap_search_user.return_value = [('dn', {})]
+                else:
+                    ldap_search_user.return_value = None
+                return User.get_login(login, password)
+
+            # Test existing user
+            self.assertEqual(get_login('admin', 'admin', False), USER)
+            self.assertEqual(get_login('admin', 'admin', True), USER)
+
+            # Test new user
+            self.assertFalse(get_login('foo', 'bar', False))
+            self.assertFalse(get_login('foo', 'bar', True))
+
+            # Test create new user
+            connection.auth_create_user = True
+            connection.save()
+            user_id = get_login('foo', 'bar', True)
+            foo, = User.search([('login', '=', 'foo')])
+            self.assertEqual(user_id, foo.id)
+            self.assertEqual(foo.name, 'foo')
+
 
 def suite():
     suite = trytond.tests.test_tryton.suite()