[Openerp-community] [Merge] lp:~openerp-community/openobject-addons/stefan-therp_lp794450 into lp:openobject-addons
The proposal to merge lp:~openerp-community/openobject-addons/stefan-therp_lp794450 into lp:openobject-addons has been updated. Status: Needs review = Merged For more details, see: https://code.launchpad.net/~openerp-community/openobject-addons/stefan-therp_lp794450/+merge/63831 -- 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. ___ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-addons/stefan-therp_lp794450 into lp:openobject-addons
On 10-06-11 23:46, Olivier Dony (OpenERP) wrote: One more thing occurs to me: since anonymous authentication basically means empty name and password, why not simplify the code and the patch by just removing the required flag on these fields? With clear tooltips on the fields, indicating that they should be left empty for anonymous binding, it should be pretty intuitive, don't you think? This also removes the need for an explicit boolean field that is a bit orthogonal with the binddn/password fields. And before calling bind() you can test ldap_binddn instead of the flag... Thoughts? Good suggestion, thanks. I have updated the code accordingly. Usability-wise, it seems common to many applications to imply an anonymous connection when the bind DN and password are left unconfigured. Ironically in the light of our previous discussion, an anonymous bind is now performed as I resorted to simply replacing a None value for the bind DN and password with the empty string. You can now even have a harmless unauthenticated authentication if your ldap server allows it by only filling in the bind DN ;-) Cheers, Stefan. -- Therp - Maatwerk in open ontwikkeling Stefan Rijnhart - Ontwerp en implementatie mail: ste...@therp.nl tel: +31 (0) 614478606 web: http://therp.nl 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. ___ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-addons/stefan-therp_lp794450 into lp:openobject-addons
Review: Approve Excellent! Simple, minimalist, obvious: that's how I like patches served :-) -- 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. ___ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-addons/stefan-therp_lp794450 into lp:openobject-addons
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. ___ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-addons/stefan-therp_lp794450 into lp:openobject-addons
On 06/10/2011 10:57 PM, Stefan Rijnhart (Therp) wrote: LDAPv3 allows for unauthorized access when no bind is performed [1]. Although [2] describes how compatibility with other versions of the protocol can be improved by anonymous binding, the only candidate LDAP version 2 was retired in 2003. Following rfc4513, I leave the code to query the LDAP server in the implied unauthorized state without performing the bind. Thanks for the details, it definitely looks like the default binding is indeed anonymous, and calling explicitly bind() is not required. And if this works with OpenLDAP, we've got all the confirmation we need. I guess the python-ldap doc is simply inaccurate. With regards to Unauthenticated Authentication, this is provided for trace purposes only and is considered something which client application developers should protect themselves against using, as it is too easy to mistake such a bind featuring the actual user name for a succesful authentication [3] Indeed we've been bitten by this in OpenERP before, and have an explicit check for blank passwords now on the user authentication. So you're right, unauthenticated mode is irrelevant. Of course, there is SASL as an honourable authentication mechanism but it is somewhat out of scope for this particular effort. I certainly agree. I will therefore not display a selection of options, but leave the checkbox. One more thing occurs to me: since anonymous authentication basically means empty name and password, why not simplify the code and the patch by just removing the required flag on these fields? With clear tooltips on the fields, indicating that they should be left empty for anonymous binding, it should be pretty intuitive, don't you think? This also removes the need for an explicit boolean field that is a bit orthogonal with the binddn/password fields. And before calling bind() you can test ldap_binddn instead of the flag... Thoughts? -- 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. ___ Mailing list: https://launchpad.net/~openerp-community Post to : openerp-community@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
[Openerp-community] [Merge] lp:~openerp-community/openobject-addons/stefan-therp_lp794450 into lp:openobject-addons
Stefan Rijnhart (Therp) has proposed merging
lp:~openerp-community/openobject-addons/stefan-therp_lp794450 into
lp:openobject-addons.
Requested reviews:
OpenERP Core Team (openerp)
Related bugs:
Bug #794450 in OpenERP Addons: [users_ldap] Wishlist: allow anonymous bind
https://bugs.launchpad.net/openobject-addons/+bug/794450
For more details, see:
https://code.launchpad.net/~openerp-community/openobject-addons/stefan-therp_lp794450/+merge/63831
--
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.
=== modified file 'users_ldap/users_ldap.py'
--- users_ldap/users_ldap.py 2011-04-29 11:22:51 +
+++ users_ldap/users_ldap.py 2011-06-08 09:43:25 +
@@ -37,14 +37,15 @@
ondelete='cascade'),
'ldap_server': fields.char('LDAP Server address', size=64, required=True),
'ldap_server_port': fields.integer('LDAP Server port', required=True),
-'ldap_binddn': fields.char('LDAP binddn', size=64, required=True),
-'ldap_password': fields.char('LDAP password', size=64, required=True),
+'ldap_binddn': fields.char('LDAP binddn', size=64),
+'ldap_password': fields.char('LDAP password', size=64),
'ldap_filter': fields.char('LDAP filter', size=64, 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),
'create_user': fields.boolean('Create user',
help=Create the user if not in database),
+'anonymous': fields.boolean('Anonymous bind'),
}
_defaults = {
'ldap_server': lambda *a: '127.0.0.1',
@@ -75,14 +76,15 @@
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
+ ldap_filter, ldap_base, user, create_user, anonymous
FROM res_company_ldap
-WHERE ldap_server != '' and ldap_binddn != '' ORDER BY sequence)
+WHERE ldap_server != '' and anonymous = TRUE or ldap_binddn != '' 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'])
-if l.simple_bind_s(res_company_ldap['ldap_binddn'], res_company_ldap['ldap_password']):
+if (res_company_ldap['anonymous'] or
+l.simple_bind_s(res_company_ldap['ldap_binddn'], res_company_ldap['ldap_password'])):
base = res_company_ldap['ldap_base']
scope = ldap.SCOPE_SUBTREE
filter = filter_format(res_company_ldap['ldap_filter'], (login,))
@@ -150,8 +152,9 @@
for res_company_ldap in user.company_id.ldaps:
try:
l = ldap.open(res_company_ldap.ldap_server, res_company_ldap.ldap_server_port)
-if l.simple_bind_s(res_company_ldap.ldap_binddn,
-res_company_ldap.ldap_password):
+if (res_company_ldap.anonymous or
+l.simple_bind_s(res_company_ldap.ldap_binddn,
+res_company_ldap.ldap_password)):
base = res_company_ldap.ldap_base
scope = ldap.SCOPE_SUBTREE
filter = filter_format(res_company_ldap.ldap_filter, (user.login,))
=== modified file 'users_ldap/users_ldap_view.xml'
--- users_ldap/users_ldap_view.xml 2011-01-14 00:11:01 +
+++ users_ldap/users_ldap_view.xml 2011-06-08 09:43:25 +
@@ -12,8 +12,16 @@
form string=LDAP Configuration
field name=ldap_server/
field name=ldap_server_port/
-field name=ldap_binddn/
-field name=ldap_password/
+field name=ldap_binddn attrs={
+ 'required': [('anonymous', '!=', True)],
+ 'readonly': [('anonymous', '=', True)],
+ }/
+field name=ldap_password attrs={
+ 'required': [('anonymous', '!=', True)],
+ 'readonly': [('anonymous', '=', True)],
+ }/
+field name=anonymous/
+ newline/
field name=ldap_base/
field name=ldap_filter/
field name=create_user/
___
Mailing list: https://launchpad.net/~openerp-community
Post to : openerp-community@lists.launchpad.net
Unsubscribe :
