From 1d1d2e6c64f3b47655ef1ff2d3b0932ba8f74f28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Krier?= Date: Wed, 9 Jul 2014 00:23:24 +0200 Subject: [PATCH] Don't catch all Exception - add some logging - fail properly when more than 1 user is found - add unittest review6411002 --- res.py | 62 ++++++++++++++++++------------- setup.py | 3 ++ tests/test_ldap_authentication.py | 43 +++++++++++++++++++++ 3 files changed, 82 insertions(+), 26 deletions(-) diff --git a/res.py b/res.py index 46e35ad4a8..876a9bd923 100644 --- a/res.py +++ b/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) diff --git a/setup.py b/setup.py index a5cfec175c..de9c700f8e 100644 --- a/setup.py +++ b/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, ) diff --git a/tests/test_ldap_authentication.py b/tests/test_ldap_authentication.py index b5dd1f6b28..6c163f284a 100644 --- a/tests/test_ldap_authentication.py +++ b/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()