Merge lp:~openerp-community/openobject-addons/stefan-therp_lp794584 into lp:openobject-addons

[Stefan Rijnhart (Opener)]

This branch breaks down LDAP authentication into a number of atomic functions so that additional modules can more easily override specific functionality.

[Olivier Dony (Odoo) (odo-openerp)]

Hello Stefan,

I agree with the idea of the bug report "wishlist", I wonder, isn't this use case sufficiently specific that it could be done in a separate module (e.g. 'ldap_import')?
Using the "template user" system alone is perhaps sufficient for the normal case?

Now, regardless of the above remark, some initial comments:

- the bind() call should now support anonymous login as provided by your other merge proposal ;-)
- DRY question: shouldn't the code of getting an ldap session, and the code of deciding/creating a user be refactored into separate functions? If we're starting to see it 2 or 3 times, it's time to think about it. Splitting these into separate functions would also allow them to be reused/extended into other module, such as perhaps an "ldap_import" one.

What do you think?

review: Needs Information

[Stefan Rijnhart (Opener) (stefan-opener)]

On 17-06-11 15:51, Olivier Dony (OpenERP) wrote:
> Review: Needs Information
> Hello Stefan,
>
> I agree with the idea of the bug report "wishlist", I wonder, isn't this use case sufficiently specific that it could be done in a separate module (e.g. 'ldap_import')?
> Using the "template user" system alone is perhaps sufficient for the normal case?
>
> Now, regardless of the above remark, some initial comments:
>
> - the bind() call should now support anonymous login as provided by your other merge proposal ;-)
> - DRY question: shouldn't the code of getting an ldap session, and the code of deciding/creating a user be refactored into separate functions? If we're starting to see it 2 or 3 times, it's time to think about it. Splitting these into separate functions would also allow them to be reused/extended into other module, such as perhaps an "ldap_import" one.
>
> What do you think?

Hi Olivier,

you hit the chicken right on the egg! If the basic functionality in the
module were sufficiently broken down, I would prefer to create a new
module for this particular use case. As it is however, I would have to
duplicate a large part of the basic functionality in the new module (I
had no problem doing so in the original module as this merely
demonstrates the need for refacturing).

I will therefore reuse this branch to suggest such a breakdown in
separate functions, and leave the added functionality for a separate
module of our own make.

Cheers,
Stefan.

--
Therp - Maatwerk in open ontwikkeling

Stefan Rijnhart - Ontwerp en implementatie

mail: <email address hidden>
tel: [phone]
web: http://therp.nl

[Olivier Dony (Odoo) (odo-openerp)]

On 06/20/2011 10:21 AM, Stefan Rijnhart (Therp) wrote:
> I will therefore reuse this branch to suggest such a breakdown in
> separate functions, and leave the added functionality for a separate
> module of our own make.

Sounds great, thanks! :-)

[Stefan Rijnhart (Opener) (stefan-opener)]

> On 06/20/2011 10:21 AM, Stefan Rijnhart (Therp) wrote:
> > I will therefore reuse this branch to suggest such a breakdown in
> > separate functions, and leave the added functionality for a separate
> > module of our own make.
>
> Sounds great, thanks! :-)

Hi Olivier,

this branch now features the refactured version of users_ldap. Here is an account of the changes I made:

[REM] reverted the populate functionality, moving to extra addon

As discussed, my team will publish a separate module with the populate functionality.

[REF] split up LDAP authentication in atomic methods

As discussed, res.company.ldap now has simple methods for retrieving the ldap configurations, connecting to the ldap server, query the subtree with the user accounts, authenticate dn/password combinations and compose a set of field values for creating res.users.

[IMP] leave it to res.users' defaults to select menu item for new users

Selecting the correct ir_action_action was broken in the module. After having installed several modules, a dashboard from one of these modules would be picked. I noticed that res.users assigned the correct default anyway and removed the corresponding code from users_ldap. As for the user's action_id, users_ldap used to set the menu action here as well which caused a continuous refreshing in the web client when such a user logs on. As res.users leaves this value empty by default, I removed this code as well.

[REF] remove unnecessary lambdas from defaults

[REF] improve pep8 compatibility

Respect 79 columns almost everywhere. Load system libraries first.

[REF] remove unnecessary conditionals when an exception is raised anyway

The old code checks the result of the user authentication against the ldap server. However, an unsuccesful authentication raises an exception which was handled already, rendering the conditional meaningless.

[IMP] do not log redundant exception ldap.INVALID_CREDENTIALS

An invalid login is already logged by WEB-SERVICE.

[IMP] replaced generic exceptions by ldap subclass

[FIX] update timestamp when LDAP user logs in
[FIX] prevent inactive LDAP users from logging in

Looking for guidance in res.users, I encounted these aspects of logins and I remembered coming across lp:784501. It seemed artificial to make an extra effort to preserve the original behaviour, but admittedly this is the only area in which the behaviour of the module has actually changed.

[FIX] use ldap.search_st() which actually does take the timeout argument

The old code uses ldap.search() with a timeout argument, but this function takes no such argument. In fact, ldap.search() is an asynchronous function which the old code does not anticipate so it seems to me that ldap.search_st() was the intended function. Also, the new code replaces ldap.open(), which is deprecated, by ldap.initialize().

Cheers,
Stefan.

review: Needs Resubmitting

[Olivier Dony (Odoo) (odo-openerp)]

Hello Stefan,

Wow, very impressive work, thanks!!

Here are a few comments (line numbers are within current diff):

- we're trying to unify out docstring formats (at least for new ones, as indeed the old ones are still a mess), and trying to use RST docstrings, so that they will be readily rendered within our online doc, with the help of sphinx's autodoc module. The format is not very different, you can have a look at examples [1][2] for the format, as well as the sphinx reference [3].

- to improve the signal-to-noise ratio of the documentation, we are trying to remove all redundant documentation of the 'cr', 'uid' and 'ids' parameters. Existing docstrings have a lot of these, but we're trying to stop/remove that. 'context' can be documented when a specific key/value in it could impact the behavior of the function, otherwise it's useless too.

- any particular reason you dropped the comments about the anonymous authentication ?

- l.94, l.278, l.339: not sure if this was done on purpose, but when logging from within an except block, you can simply use logger.exception('foo') to automatically log the full exception info and trace (it will use the current exception). If you want to achieve the same with a different logging level, you can use the exc_info kwarg, as in `logger.warning('foo', exc_info=True)`. This is usually better than trying to format the exception yourself, unless you really meant it. In general, it's also best to avoid doing the string format yourself with '%', the logger will do it (and will also avoid it if no message is finally output due to the current logger level)[4].

- l.183: the explicit test on empty passwords at the start of login() is really needed, because login() does not behave like check() and does not raise in case of failure. Therefore we can't distinguish super().login() returning False because of an empty password or because of a wrong password => we need to test this case explicitly, otherwise we'll perform an anonymous auth of the _user_, something that must be prevented by applications, as discussed previously! ;-)

- l.264: 'name' seems unused now?

- l.257-226 and 324-333 seem mostly identical, how about abstracting them out also in their own overridable function, e.g. auth_user() or similar, with only the code handling the result/exception being different?

Everything else looks quite excellent, well done and thanks a lot!

[1] Simple (outdated) example: http://bazaar.launchpad.net/~openerp/openobject-server/trunk/view/head:/openerp/osv/orm.py#L2031
[2] Complicated example, showing many options http://bazaar.launchpad.net/~openerp/openobject-server/trunk/view/head:/openerp/osv/fields.py#L755
[3] Sphinx reference: http://sphinx.pocoo.org/domains.html#info-field-lists
[4] http://docs.python.org/library/logging.html#logging.Logger.debug

review: Needs Fixing (technical)

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Olivier,

glad you like our work! Thank you for your valuable comments. I have tried to honour all of them in my latest commit. Additional comments:

> - l.94, l.278, l.339: not sure if this was done on purpose, but when logging
> from within an except block, you can simply use logger.exception('foo') to
> automatically log the full exception info and trace

Did not think that the trace is very interesting in these cases, as the exceptions most likely occur due to a configuration error or an error on the side of the LDAP server. I did change the code so as to leave the formatting to the logger though.

> - l.183: the explicit test on empty passwords at the start of login() is
> really needed

Ouch, that hurts :-( Thank you for saving me there! I put the check right in the heart of the authorization method of the ldap class, so there is no escaping it now.

Regards,
Stefan.

review: Needs Resubmitting

[Olivier Dony (Odoo) (odo-openerp)]

Excellent work, Stefan, looks good for merging.
Nice touch with the RFC reference in the docstrings. I didn't know the :rfc: RST role yet :-)

Thanks a lot for another great contribution!

review: Approve

[Olivier Dony (Odoo) (odo-openerp)]

Just merged after re-testing and replacing UID 1 with the new constant meant for that.

Thanks for your great work on this!

I will proceed with Ian's dependent branch next.

=== modified file 'users_ldap/users_ldap.py'
--- users_ldap/users_ldap.py	2011-06-16 16:02:19 +0000
+++ users_ldap/users_ldap.py	2011-07-27 22:12:47 +0000
@@ -18,19 +18,181 @@
 #
 ##############################################################################
 
+import ldap
+from ldap.filter import filter_format
 from osv import fields, osv
 import pooler
 import tools
 import logging
 from service import security
-import ldap
-from ldap.filter import filter_format
-
 
 class CompanyLDAP(osv.osv):
     _name = 'res.company.ldap'
     _order = 'sequence'
     _rec_name = 'ldap_server'
+
+    def get_ldap_dicts(self, cr, ids=None):
+        """ 
+        Retrieve res_company_ldap resources from the database in dictionary
+        format.
+
+        :param list ids: Valid ids of model res_company_ldap. If not \
+        specified, process all resources (unlike other ORM methods).
+        :return: ldap configurations
+        :rtype: list of dictionaries
+        """
+
+        if ids:
+            id_clause = 'AND id IN (%s)'
+            args = [tuple(ids)]
+        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
+            FROM res_company_ldap
+            WHERE ldap_server != '' """ + id_clause + """ ORDER BY sequence
+        """, args)
+        return cr.dictfetchall()
+
+    def connect(self, conf):
+        """ 
+        Connect to an LDAP server specified by an ldap
+        configuration dictionary.
+
+        :param dict conf: LDAP configuration
+        :return: an LDAP object
+        """
+
+        uri = 'ldap://%s:%d' % (conf['ldap_server'],
+                                conf['ldap_server_port'])
+        return ldap.initialize(uri)
+
+    def authenticate(self, conf, login, password):
+        """
+        Authenticate a user against the specified LDAP server.
+
+        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
+        :return: LDAP entry of authenticated user or False
+        :rtype: dictionary of attributes
+        """
+
+        if not password:
+            return False
+
+        entry = False
+        filter = filter_format(conf['ldap_filter'], (login,))
+        try:
+            results = self.query(conf, filter)
+            if results and len(results) == 1:
+                dn = results[0][0]
+                conn = self.connect(conf)
+                conn.simple_bind_s(dn, password)
+                conn.unbind()
+                entry = results[0]
+        except ldap.INVALID_CREDENTIALS:
+            return False
+        except ldap.LDAPError, e:
+            logger = logging.getLogger('orm.ldap')
+            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.
+
+        Allow for all authentication methods of the simple authentication
+        method:
+
+        - authenticated bind (non-empty binddn + valid password)
+        - anonymous bind (empty binddn + empty password)
+        - unauthenticated authentication (non-empty binddn + empty password)
+
+        .. seealso::
+           :rfc:`4513#section-5.1` - LDAP: Simple Authentication Method.
+
+        :param dict conf: LDAP configuration
+        :param filter: valid LDAP filter
+        :param list retrieve_attributes: LDAP attributes to be retrieved. \
+        If not specified, return all attributes.
+        :return: ldap entries
+        :rtype: list of tuples (dn, attrs)
+
+        """
+
+        results = []
+        logger = logging.getLogger('orm.ldap')
+        try:
+            conn = self.connect(conf)
+            conn.simple_bind_s(conf['ldap_binddn'] or '',
+                               conf['ldap_password'] or '')
+            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)
+        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)
+        :return: parameters for a new resource of model res_users
+        :rtype: dict
+        """
+
+        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):
+        """
+        Retrieve an active resource of model res_users with the specified
+        login. Create the user if it is not initially found.
+
+        :param dict conf: LDAP configuration
+        :param login: the user's login
+        :param tuple ldap_entry: single LDAP result (dn, attrs)
+        :return: res_users id
+        :rtype: int
+        """
+        
+        user_id = False
+        login = tools.ustr(login)
+        cr.execute("SELECT id, active FROM res_users WHERE login=%s", (login,))
+        res = cr.fetchone()
+        if res:
+            if res[1]:
+                user_id = res[0]
+        elif conf['create_user']:
+            logger = logging.getLogger('orm.ldap')
+            logger.debug("Creating new OpenERP user \"%s\" from LDAP" % login)
+            user_obj = self.pool.get('res.users')
+            values = self.map_ldap_attributes(cr, uid, conf, login, ldap_entry)
+            if conf['user']:
+                user_id = user_obj.copy(cr, 1, conf['user'],
+                                        default={'active': True})
+                user_obj.write(cr, 1, user_id, values)
+            else:
+                user_id = user_obj.create(cr, 1, values)
+        return user_id
+
     _columns = {
         'sequence': fields.integer('Sequence'),
         'company': fields.many2one('res.company', 'Company', required=True,
@@ -43,7 +205,7 @@
         'ldap_password': fields.char('LDAP password', size=64,
             help=("The password of the user account on the LDAP server that is "
                   "used to query the directory.")),
-        'ldap_filter': fields.char('LDAP filter', size=64, required=True),
+        'ldap_filter': fields.char('LDAP filter', size=256, required=True),
         'ldap_base': fields.char('LDAP base', size=64, required=True),
         'user': fields.many2one('res.users', 'Model User',
             help="Model used for user creation"),
@@ -51,10 +213,10 @@
             help="Create the user if not in database"),
     }
     _defaults = {
-        'ldap_server': lambda *a: '127.0.0.1',
-        'ldap_server_port': lambda *a: 389,
-        'sequence': lambda *a: 10,
-        'create_user': lambda *a: True,
+        'ldap_server': '127.0.0.1',
+        'ldap_server_port': 389,
+        'sequence': 10,
+        'create_user': True,
     }
 
 CompanyLDAP()
@@ -63,87 +225,32 @@
 class res_company(osv.osv):
     _inherit = "res.company"
     _columns = {
-        'ldaps': fields.one2many('res.company.ldap', 'company', 'LDAP Parameters'),
+        'ldaps': fields.one2many(
+            'res.company.ldap', 'company', 'LDAP Parameters'),
     }
 res_company()
 
+
 class users(osv.osv):
     _inherit = "res.users"
     def login(self, db, login, password):
-
-        if not password:
-            # empty passwords are disallowed for obvious security reasons
-            return False
-
-        ret = super(users,self).login(db, login, password)
-        if ret:
-            return ret
-        logger = logging.getLogger('orm.ldap')
-        pool = pooler.get_pool(db)
+        user_id = super(users, self).login(db, login, password)
+        if user_id:
+            return user_id
         cr = pooler.get_db(db).cursor()
-        action_obj = pool.get('ir.actions.actions')
-        cr.execute("""
-            SELECT id, company, ldap_server, ldap_server_port, ldap_binddn, ldap_password,
-                   ldap_filter, ldap_base, "user", create_user
-            FROM res_company_ldap
-            WHERE ldap_server != '' ORDER BY sequence""")
-        for res_company_ldap in cr.dictfetchall():
-            logger.debug(res_company_ldap)
-            try:
-                l = ldap.open(res_company_ldap['ldap_server'], res_company_ldap['ldap_server_port'])
-                # An empty binddn means anonymous auth, so it should be replaced w/ an empty string
-                # See LDAP RFC 4513, Section 5.1.1
-                if l.simple_bind_s(res_company_ldap['ldap_binddn'] or '',
-                                   res_company_ldap['ldap_password'] or ''):
-                    base = res_company_ldap['ldap_base']
-                    scope = ldap.SCOPE_SUBTREE
-                    filter = filter_format(res_company_ldap['ldap_filter'], (login,))
-                    retrieve_attributes = None
-                    result_id = l.search(base, scope, filter, retrieve_attributes)
-                    timeout = 60
-                    result_type, result_data = l.result(result_id, timeout)
-                    if not result_data:
-                        continue
-                    if result_type == ldap.RES_SEARCH_RESULT and len(result_data) == 1:
-                        dn = result_data[0][0]
-                        logger.debug(dn)
-                        name = result_data[0][1]['cn'][0]
-                        if l.bind_s(dn, password):
-                            l.unbind()
-                            cr.execute("SELECT id FROM res_users WHERE login=%s",(tools.ustr(login),))
-                            res = cr.fetchone()
-                            logger.debug(res)
-                            if res:
-                                cr.close()
-                                return res[0]
-                            if not res_company_ldap['create_user']:
-                                continue
-                            action_id = action_obj.search(cr, 1, [('usage', '=', 'menu')])[0]
-                            if res_company_ldap['user']:
-                                res = self.copy(cr, 1, res_company_ldap['user'],
-                                        default={'active': True})
-                                self.write(cr, 1, res, {
-                                    'name': name,
-                                    'login': login.encode('utf-8'),
-                                    'company_id': res_company_ldap['company'],
-                                    })
-                            else:
-                                res = self.create(cr, 1, {
-                                    'name': name,
-                                    'login': login.encode('utf-8'),
-                                    'company_id': res_company_ldap['company'],
-                                    'action_id': action_id,
-                                    'menu_id': action_id,
-                                    })
-                            cr.commit()
-                            cr.close()
-                            return res
-                    l.unbind()
-            except Exception:
-                logger.warning("Cannot auth", exc_info=True)
-                continue
+        ldap_obj = pooler.get_pool(db).get('res.company.ldap')
+        for conf in ldap_obj.get_ldap_dicts(cr):
+            entry = ldap_obj.authenticate(conf, login, password)
+            if entry:
+                user_id = ldap_obj.get_or_create_user(
+                    cr, 1, conf, login, entry)
+                if user_id:
+                    cr.execute('UPDATE res_users SET date=now() WHERE '
+                               'login=%s', (tools.ustr(login),))
+                    cr.commit()
+                    break
         cr.close()
-        return False
+        return user_id
 
     def check(self, db, uid, passwd):
         try:
@@ -151,44 +258,19 @@
         except security.ExceptionNoTb: # AccessDenied
             pass
 
-        if not passwd:
-            # empty passwords disallowed for obvious security reasons
-            raise security.ExceptionNoTb('AccessDenied')
-
         cr = pooler.get_db(db).cursor()
-        user = self.browse(cr, 1, uid)
-        logger = logging.getLogger('orm.ldap')
-        if user and user.company_id.ldaps:
-            for res_company_ldap in user.company_id.ldaps:
-                try:
-                    l = ldap.open(res_company_ldap.ldap_server, res_company_ldap.ldap_server_port)
-                    # An empty binddn means anonymous auth, so it should be replaced w/ an empty string
-                    # See LDAP RFC 4513, Section 5.1.1
-                    if l.simple_bind_s(res_company_ldap.ldap_binddn or '',
-                                       res_company_ldap.ldap_password or ''):
-                        base = res_company_ldap.ldap_base
-                        scope = ldap.SCOPE_SUBTREE
-                        filter = filter_format(res_company_ldap.ldap_filter, (user.login,))
-                        retrieve_attributes = None
-                        result_id = l.search(base, scope, filter, retrieve_attributes)
-                        timeout = 60
-                        result_type, result_data = l.result(result_id, timeout)
-                        if result_data and result_type == ldap.RES_SEARCH_RESULT and len(result_data) == 1:
-                            dn = result_data[0][0]
-                            # some LDAP servers allow anonymous binding with blank passwords,
-                            # but these have been rejected above, so we're safe to use bind()
-                            if l.bind_s(dn, passwd):
-                                l.unbind()
-                                self._uid_cache.setdefault(db, {})[uid] = passwd
-                                cr.close()
-                                return True
-                        l.unbind()
-                except Exception:
-                    logger.warning('cannot check', exc_info=True)
-                    pass
+        cr.execute('SELECT login FROM res_users WHERE id=%s AND active=TRUE',
+                   (int(uid),))
+        res = cr.fetchone()
+        if res:
+            ldap_obj = pooler.get_pool(db).get('res.company.ldap')
+            for conf in ldap_obj.get_ldap_dicts(cr):
+                if ldap_obj.authenticate(conf, res[0], passwd):
+                    self._uid_cache.setdefault(db, {})[uid] = passwd
+                    cr.close()
+                    return True
         cr.close()
         raise security.ExceptionNoTb('AccessDenied')
         
 users()
 # vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4:
-