openerp-community team mailing list archive

["Olivier Dony \(OpenERP\)" <odo@xxxxxxxxxxx>]

Review: Needs Information
Hello Stefan,

Thanks for the nice merge proposal!
Your changes make me wonder about a few things, listed below (disclaimer: I haven't tested this, and haven't read the full LDAP RFCs):

- It was my understanding that LDAP defines a lot of authentication mechanism (anonymous, user/pass, and even "unauthenticated" authentication [1]), and as a result prior authentication is always required before any operation can be done in an LDAP session. Your patch seems to remove the initial bind() operation in case "anonymous" is enabled, while I would have expected you would need to really perform an anonymous bind operation to conform to the protocol? (e.g. bind(who='', cred='') as described in [2]).
- The python-ldap documentation also seems to imply that some variation of bind() needs to be called before any operation can be done[3]. Based on this, it looks like the search() performed to verify the user's existence would fail with some LDAP servers, only because no bind() has been done before.
- Since LDAP also has other authentication modes, wouldn't it be better to instead display a selection of options, at least with the simple mechanisms: "Name/Password", "Anonymous", "Unauthenticated", implement them accordingly (passing empty who/cred to bind()) and set the appropriate required fields for each (as you did for anonymous).
- Out of curiosity, what LDAP server are you using, and with what kind of specific settings? Default settings?

Thanks!

[1] http://tools.ietf.org/html/rfc4513#section-5.1
[2] http://tools.ietf.org/html/rfc4513#section-5.1.1
[3] http://www.python-ldap.org/doc/html/ldap.html#ldap.LDAPObject.simple_bind_s
-- 
https://code.launchpad.net/~openerp-community/openobject-addons/stefan-therp_lp794450/+merge/63831
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-addons/stefan-therp_lp794450.