Code review comment for lp:~ibeardslee/openobject-addons/users_ldap-tls

Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

Ian, Stefan,

I've just merged this branch in trunk, so it will be included in v6.1.

After re-testing with a TLS-disabled LDAP server, I changed the default for the TLS flag to be off, and updated the module description and tooltips accordingly, for two main reasons:
 - When TLS is enabled but not supported by the LDAP server, all login attempts silently fail, with the diagnostics for the failure only visible in the server logs. This is fine, because end-users shouldn't be exposed to the technical reasons for their failed login, but will be a source of issues for users with non-TLS LDAP servers. It will for example prevent login for all existing LDAP users after an upgrade to 6.1 if they don't have TLS available (as it will be enabled automatically).
 - Most of the time the LDAP server is located within a restricted part of a company's network, so communication between OpenERP and the LDAP occurs on a relatively safe segment, mitigating the risk of not using TLS even when it is available.

Based on the above, I think having TLS as opt-in is better than opt-out, at least for 6.1. I hope you agree, or at least understand my point of view...

Thanks again for your great work!

« Back to merge proposal