From aba616ab690ec9dbd947ed9b189e10299c796843 Mon Sep 17 00:00:00 2001 From: "robin.keunen" Date: Mon, 25 Nov 2019 17:37:19 +0100 Subject: [PATCH 1/3] Improvements to auth_ldap cf https://github.com/odoo/odoo/commit/17ee9c74b1d9742cc6062ece385e7dc2f2e33b16\#diff-dec2a9d90d46ed6d5f4b1ba3662432d3 --- addons/auth_ldap/users_ldap.py | 204 ++++++++++++++++----------- addons/auth_ldap/users_ldap_view.xml | 3 +- 2 files changed, 122 insertions(+), 85 deletions(-) diff --git a/addons/auth_ldap/users_ldap.py b/addons/auth_ldap/users_ldap.py index c7506b9cd559c..7642852b16b0a 100644 --- a/addons/auth_ldap/users_ldap.py +++ b/addons/auth_ldap/users_ldap.py @@ -9,7 +9,10 @@ from openerp.osv import fields, osv from openerp import SUPERUSER_ID from openerp.modules.registry import RegistryManager + _logger = logging.getLogger(__name__) +import psycopg2 + class CompanyLDAP(osv.osv): _name = 'res.company.ldap' @@ -17,7 +20,7 @@ class CompanyLDAP(osv.osv): _rec_name = 'ldap_server' def get_ldap_dicts(self, cr, ids=None): - """ + """ Retrieve res_company_ldap resources from the database in dictionary format. @@ -33,17 +36,22 @@ def get_ldap_dicts(self, cr, ids=None): else: id_clause = '' args = [] - cr.execute(""" - SELECT id, company, ldap_server, ldap_server_port, ldap_binddn, - ldap_password, ldap_filter, ldap_base, "user", create_user, - ldap_tls - FROM res_company_ldap - WHERE ldap_server != '' """ + id_clause + """ ORDER BY sequence - """, args) - return cr.dictfetchall() + try: + cr.execute(""" + SELECT id, company, ldap_server, ldap_server_port, ldap_bind_suffix, ldap_pre_bind, ldap_binddn, + ldap_password, ldap_filter, ldap_base, "user", create_user, + ldap_tls + FROM res_company_ldap + WHERE ldap_server != '' """ + id_clause + """ ORDER BY sequence + """, args) + return cr.dictfetchall() + except psycopg2.ProgrammingError: + # Do not fail during upgrade, some fields may be missing + _logger.exception("Error in get_ldap_dicts") + return [] def connect(self, conf): - """ + """ Connect to an LDAP server specified by an ldap configuration dictionary. @@ -51,13 +59,16 @@ def connect(self, conf): :return: an LDAP object """ - uri = 'ldap://%s:%d' % (conf['ldap_server'], - conf['ldap_server_port']) - - connection = ldap.initialize(uri) if conf['ldap_tls']: - connection.start_tls_s() - return connection + protocol = "ldaps" + else: + protocol = "ldap" + + uri = '%s://%s:%d' % (protocol, conf['ldap_server'], + conf['ldap_server_port']) + + _logger.debug("Using LDAP URI: %s" % repr(uri)) + return ldap.initialize(uri) def authenticate(self, conf, login, password): """ @@ -66,7 +77,7 @@ def authenticate(self, conf, login, password): In order to prevent an unintended 'unauthenticated authentication', which is an anonymous bind with a valid dn and a blank password, check for empty passwords explicitely (:rfc:`4513#section-6.3.1`) - + :param dict conf: LDAP configuration :param login: username :param password: Password for the LDAP user @@ -78,41 +89,56 @@ def authenticate(self, conf, login, password): return False entry = False + filter = filter_format(conf['ldap_filter'], (login,)) + conn = self.connect(conf) + try: - filter = filter_format(conf['ldap_filter'], (login,)) - except TypeError: - _logger.warning('Could not format LDAP filter. Your filter should contain one \'%s\'.') - return False - try: - results = self.query(conf, filter.encode('utf-8')) - - # Get rid of (None, attrs) for searchResultReference replies - results = [i for i in results if i[0]] - if results and len(results) == 1: - dn = results[0][0] - conn = self.connect(conf) - conn.simple_bind_s(dn, password.encode('utf-8')) - conn.unbind() - entry = results[0] - except ldap.INVALID_CREDENTIALS: + if conf['ldap_pre_bind']: + if conf['ldap_binddn']: + bind_dn = "%s%s" % ( + conf['ldap_binddn'], conf['ldap_bind_suffix'] or '') + else: + bind_dn = '' + conn.simple_bind_s(bind_dn, + conf['ldap_password'] or '') + results = self.query(conn, conf['ldap_base'], filter) + + if len(results) != 1: + _logger.error("Filter %s on base %s returned %s entries" % ( + filter, conf['ldap_base'], len(results))) + return False + + user_dn = results[0][0] + conn.simple_bind_s(user_dn, password.encode('utf-8')) + _logger.info("Successful LDAP login for %s" % login) + else: + bind_dn = "%s%s" % (login, conf['ldap_bind_suffix'] or '') + conn.simple_bind_s(bind_dn, password.encode('utf-8')) + _logger.info("Successful LDAP login for %s" % login) + results = self.query(conn, conf['ldap_base'], filter) + + if len(results) != 1: + _logger.error("Filter %s on base %s returned %s entries" % ( + filter, conf['ldap_base'], len(results))) + return False + + entry = results[0] + except ldap.INVALID_CREDENTIALS, e: + _logger.debug( + "Invalid credentials for %s: %s" % (repr(bind_dn), repr(e))) return False - except ldap.LDAPError, e: - _logger.error('An LDAP exception occurred: %s', e) - return entry - - def query(self, conf, filter, retrieve_attributes=None): - """ - Query an LDAP server with the filter argument and scope subtree. + except ldap.LDAPError: + _logger.exception('An LDAP exception occurred') + finally: + conn.unbind() - Allow for all authentication methods of the simple authentication - method: + _logger.debug("LDAP result: %s" % repr(entry)) - - authenticated bind (non-empty binddn + valid password) - - anonymous bind (empty binddn + empty password) - - unauthenticated authentication (non-empty binddn + empty password) + return entry - .. seealso:: - :rfc:`4513#section-5.1` - LDAP: Simple Authentication Method. + def query(self, conn, base, filter, retrieve_attributes=None): + """ + Query an LDAP server with the filter argument and scope subtree. :param dict conf: LDAP configuration :param filter: valid LDAP filter @@ -124,25 +150,22 @@ def query(self, conf, filter, retrieve_attributes=None): """ results = [] - try: - conn = self.connect(conf) - ldap_password = conf['ldap_password'] or '' - ldap_binddn = conf['ldap_binddn'] or '' - conn.simple_bind_s(ldap_binddn.encode('utf-8'), ldap_password.encode('utf-8')) - results = conn.search_st(conf['ldap_base'], ldap.SCOPE_SUBTREE, - filter, retrieve_attributes, timeout=60) - conn.unbind() - except ldap.INVALID_CREDENTIALS: - _logger.error('LDAP bind failed.') - except ldap.LDAPError, e: - _logger.error('An LDAP exception occurred: %s', e) + results = conn.search_st(base, ldap.SCOPE_SUBTREE, + filter, retrieve_attributes, timeout=60) + + # Get rid of (None, attrs) for searchResultReference replies + results = [i for i in results if i[0]] + + _logger.debug("LDAP search base=%s filter=%s returned %s results" % ( + repr(base), repr(filter), len(results))) + return results def map_ldap_attributes(self, cr, uid, conf, login, ldap_entry): """ Compose values for a new resource of model res_users, based upon the retrieved ldap entry and the LDAP settings. - + :param dict conf: LDAP configuration :param login: the new user's login :param tuple ldap_entry: single LDAP result (dn, attrs) @@ -150,12 +173,12 @@ def map_ldap_attributes(self, cr, uid, conf, login, ldap_entry): :rtype: dict """ - values = { 'name': ldap_entry[1]['cn'][0], - 'login': login, - 'company_id': conf['company'] - } + values = {'name': ldap_entry[1]['cn'][0], + 'login': login, + 'company_id': conf['company'] + } return values - + def get_or_create_user(self, cr, uid, conf, login, ldap_entry, context=None): """ @@ -168,10 +191,11 @@ def get_or_create_user(self, cr, uid, conf, login, ldap_entry, :return: res_users id :rtype: int """ - + user_id = False login = tools.ustr(login.lower().strip()) - cr.execute("SELECT id, active FROM res_users WHERE lower(login)=%s", (login,)) + cr.execute("SELECT id, active FROM res_users WHERE lower(login)=%s", + (login,)) res = cr.fetchone() if res: if res[1]: @@ -191,25 +215,35 @@ def get_or_create_user(self, cr, uid, conf, login, ldap_entry, _columns = { 'sequence': fields.integer('Sequence'), 'company': fields.many2one('res.company', 'Company', required=True, - ondelete='cascade'), + ondelete='cascade'), 'ldap_server': fields.char('LDAP Server address', required=True), 'ldap_server_port': fields.integer('LDAP Server port', required=True), - 'ldap_binddn': fields.char('LDAP binddn', - help=("The user account on the LDAP server that is used to query " - "the directory. Leave empty to connect anonymously.")), + 'ldap_bind_suffix': fields.char('LDAP bind suffix', + help="""Suffix to append to the user login, useful for automatically + specifying a domain like @company.com. Only used when ldap_pre_bind + is turned off."""), + 'ldap_pre_bind': fields.boolean('Perform two-step bind', default=True, + help="""Traditionally Odoo binds two times: once anonymously or with + ldap_binddn, and a second time with the user login. Disable this + option to bind directly with the user-provided credentials."""), + 'ldap_binddn': fields.char('LDAP binddn', + help="""The user account on the LDAP server that is used to query + the directory for two-step bind. Leave empty to connect + anonymously or to use one-step bind."""), 'ldap_password': fields.char('LDAP password', - help=("The password of the user account on the LDAP server that is " - "used to query the directory.")), + help=( + "The password of the user account on the LDAP server that is " + "used to query the directory for two-step bind.")), 'ldap_filter': fields.char('LDAP filter', required=True), 'ldap_base': fields.char('LDAP base', required=True), 'user': fields.many2one('res.users', 'Template User', - help="User to copy when creating new users"), + help="User to copy when creating new users"), 'create_user': fields.boolean('Create user', - help="Automatically create local user accounts for new users authenticating via LDAP"), + help="Automatically create local user accounts for new users authenticating via LDAP"), 'ldap_tls': fields.boolean('Use TLS', - help="Request secure TLS/SSL encryption when connecting to the LDAP server. " - "This option requires a server with STARTTLS enabled, " - "otherwise all authentication attempts will fail."), + help="Request secure TLS/SSL encryption when connecting to the LDAP server. " + "This option requires a server with STARTTLS enabled, " + "otherwise all authentication attempts will fail."), } _defaults = { 'ldap_server': '127.0.0.1', @@ -219,24 +253,26 @@ def get_or_create_user(self, cr, uid, conf, login, ldap_entry, } - class res_company(osv.osv): _inherit = "res.company" _columns = { 'ldaps': fields.one2many( - 'res.company.ldap', 'company', 'LDAP Parameters', copy=True, groups="base.group_system"), + 'res.company.ldap', 'company', 'LDAP Parameters', copy=True, + groups="base.group_system"), } class users(osv.osv): _inherit = "res.users" + def _login(self, db, login, password): user_id = super(users, self)._login(db, login, password) if user_id: return user_id registry = RegistryManager.get(db) with registry.cursor() as cr: - cr.execute("SELECT id FROM res_users WHERE lower(login)=%s", (login,)) + cr.execute("SELECT id FROM res_users WHERE lower(login)=%s", + (login,)) res = cr.fetchone() if res: return False @@ -255,8 +291,9 @@ def check_credentials(self, cr, uid, password): super(users, self).check_credentials(cr, uid, password) except openerp.exceptions.AccessDenied: - cr.execute('SELECT login FROM res_users WHERE id=%s AND active=TRUE', - (int(uid),)) + cr.execute( + 'SELECT login FROM res_users WHERE id=%s AND active=TRUE', + (int(uid),)) res = cr.fetchone() if res: ldap_obj = self.pool['res.company.ldap'] @@ -264,4 +301,3 @@ def check_credentials(self, cr, uid, password): if ldap_obj.authenticate(conf, res[0], password): return raise - diff --git a/addons/auth_ldap/users_ldap_view.xml b/addons/auth_ldap/users_ldap_view.xml index 2aba97c5e209f..23b9dfda76f34 100644 --- a/addons/auth_ldap/users_ldap_view.xml +++ b/addons/auth_ldap/users_ldap_view.xml @@ -13,13 +13,14 @@ + + - From fba744d06e35b13d62330c02477cb2a609a9b5a9 Mon Sep 17 00:00:00 2001 From: "robin.keunen" Date: Mon, 25 Nov 2019 17:41:57 +0100 Subject: [PATCH 2/3] [FIX] auth_ldap: allow non-ascii ldap base cf https://github.com/OCA/OCB/commit/6f1c2919825c9e690ea9a69fb94e27a1e5dbd7cd#diff-9f4e25679802e4ca850d80986cea80fd --- addons/auth_ldap/users_ldap.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/addons/auth_ldap/users_ldap.py b/addons/auth_ldap/users_ldap.py index 7642852b16b0a..cfc06de594d87 100644 --- a/addons/auth_ldap/users_ldap.py +++ b/addons/auth_ldap/users_ldap.py @@ -150,7 +150,7 @@ def query(self, conn, base, filter, retrieve_attributes=None): """ results = [] - results = conn.search_st(base, ldap.SCOPE_SUBTREE, + results = conn.search_st(base.encode('utf-8'), ldap.SCOPE_SUBTREE, filter, retrieve_attributes, timeout=60) # Get rid of (None, attrs) for searchResultReference replies From afe6f289f1a36fde3c12c3d3767eb87f5717b8f5 Mon Sep 17 00:00:00 2001 From: "robin.keunen" Date: Fri, 27 Mar 2020 17:32:05 +0100 Subject: [PATCH 3/3] [FIX] auth_ldap: return false on LDAPError --- addons/auth_ldap/users_ldap.py | 1 + 1 file changed, 1 insertion(+) diff --git a/addons/auth_ldap/users_ldap.py b/addons/auth_ldap/users_ldap.py index cfc06de594d87..d1904a374c0f8 100644 --- a/addons/auth_ldap/users_ldap.py +++ b/addons/auth_ldap/users_ldap.py @@ -129,6 +129,7 @@ def authenticate(self, conf, login, password): return False except ldap.LDAPError: _logger.exception('An LDAP exception occurred') + return False finally: conn.unbind()