| Request | LDAPMultiPlugins -- bug report -- by Marcin Davies |
| Posted on | Jul 24, 2006 11:17 am |
| Subscribe |
| Resolve by Jens Vagelpohl on Oct 3, 2006 5:53 pm | |
|
Alright, calling it "resolved" then. |
|
|
|
| Comment by John Hannon on Oct 3, 2006 5:50 pm | |
|
Just installed the latest from the trunk and verified that it works, and is recursing groups. Thanks Jens! John |
|
|
|
| Comment by Jens Vagelpohl on Oct 3, 2006 5:25 pm | |
|
> I apologize for the earlier faulty patch. The patch below is > written against the latest from the svn trunk, and inculdes the > filter_format call. You're probably about fed up with me by now, > but I thought I ought to post this anyway in case anyone can use it. Nah, not fed up, thanks for the final patch. I refactored it a little bit to keep actual changes to a minimum (there were a couple unneeded changes) and checked it in. Could you check out the current code and tell me if it works as desired for you? |
|
|
|
| Comment by John Hannon on Oct 3, 2006 5:05 pm | |
|
Jens, I apologize for the earlier faulty patch. The patch below is written against the latest from the svn trunk, and inculdes the filter_format call. You're probably about fed up with me by now, but I thought I ought to post this anyway in case anyone can use it. John 142a143 > {'id':'group_recurse_depth', 'type':'int', 'mode':'w'}, 148a150 > group_recurse_depth = 1 151c153 < grouptitle_attr='cn', group_class='group', group_recurse=1): --- > grouptitle_attr='cn', group_class='group', group_recurse=1, group_recurse_depth=1): 158a161 > self.group_recurse_depth = group_recurse_depth 213c216 < def _recurseGroups(self, ldap_results, temp=None, seen=None): --- > def _recurseGroups(self, ldap_results, temp=None, seen=None, depth=0): 222a226,228 > acl = self.acl_users > delegate = acl._delegate > base = acl.groups_base 224a231 > 229,230c236,242 < < filt_bits.append(filter_format('(memberOf=%s)', (dn,))) --- > > if result.has_key('memberOf'): > for parent_dn in result['memberOf']: > filt = filter_format('(distinguishedName=%s)', (parent_dn,)) > if filt in filt_bits: > continue > filt_bits.append(filt) 235,237c247,249 < acl = self.acl_users < delegate = acl._delegate < R = delegate.search(acl.groups_base, acl.groups_scope, filter=filt) --- > > R = delegate.search(base, acl.groups_scope, filter=filt) > 240c252,253 < self._recurseGroups(R['results'], temp, seen) --- > if depth < self.group_recurse_depth: > self._recurseGroups(R['results'], temp, seen, depth + 1) |
|
|
|
| Comment by Jens Vagelpohl on Oct 3, 2006 4:05 pm | |
|
Well, I'll leave the issue deferred. John's patch is unworkable. If someone wants to prepare a patch that works against what's in Subversion I'll look at it again. |
|
|
|
| Comment by John Hannon on Sep 28, 2006 5:55 pm | |
|
>Thanks very much for taking the time to get into this, I >appreciate it. I'm still a little puzzled, did you create >this diff against the current SVN trunk? Specifically, the >code on trunk uses the "filter_format" function, which is >not used here. I did use the current SVN trunk, but copied lines from my orginal hack, and neglected to add back in the filter_format call. Sorry 'bout that--it works from me without that function, but possibly only because I haven't hit a group with a character that needed escaping. John |
|
|
|
| Comment by Jens Vagelpohl on Sep 28, 2006 5:39 pm | |
|
> Sorry, I'm pretty much a newb at cooperative development. I went > and got your latest ActiveDirectoryMultiPlugins.py from svn, and > hacked (and tested) that. Here's a diff. Let me know if that will > do. Thanks very much for taking the time to get into this, I appreciate it. I'm still a little puzzled, did you create this diff against the current SVN trunk? Specifically, the code on trunk uses the "filter_format" function, which is not used here. |
|
|
|
| Comment by John Hannon on Sep 26, 2006 2:11 pm | |
|
Hi Jens, Sorry, I'm pretty much a newb at cooperative development. I went and got your latest ActiveDirectoryMultiPlugins.py from svn, and hacked (and tested) that. Here's a diff. Let me know if that will do. John 142a143 > {'id':'group_recurse_depth', 'type':'int', 'mode':'w'}, 148a150 > group_recurse_depth = 1 151c153 < grouptitle_attr='cn', group_class='group', group_recurse=1): --- > grouptitle_attr='cn', group_class='group', group_recurse=1, group_recurse_depth=1): 158a161 > self.group_recurse_depth = group_recurse_depth 213c216 < def _recurseGroups(self, ldap_results, temp=None, seen=None): --- > def _recurseGroups(self, ldap_results, temp=None, seen=None, depth=0): 222a226,228 > acl = self.acl_users > delegate = acl._delegate > base = acl.groups_base 224a231 > 229,230c236,242 < < filt_bits.append(filter_format('(memberOf=%s)', (dn,))) --- > > if result.has_key('memberOf'): > for parent_dn in result['memberOf']: > filt = '(distinguishedName=%s)' % parent_dn > if filt in filt_bits: > continue > filt_bits.append(filt) 235,237c247,249 < acl = self.acl_users < delegate = acl._delegate < R = delegate.search(acl.groups_base, acl.groups_scope, filter=filt) --- > > R = delegate.search(base, acl.groups_scope, filter=filt) > 240c252,253 < self._recurseGroups(R['results'], temp, seen) --- > if depth < self.groups_recurse_depth: > self._recurseGroups(R['results'], temp, seen, depth + 1) 243c256 < --- > |
|
|
|
| Comment by Jens Vagelpohl on Sep 26, 2006 3:43 am | |
|
> I've hacked ActiveDirectoryMultiPlugin.py so that it works with > AD. I've also added a property to control depth of recursion (for > performance). This has been tested with an actual MS AD domain. > Changes are limited to the recurseGroups method and the > groups_recurse_depth property. > > John John, I wouldn't mind giving this a try, but I can't use this code: - it is not a patch, but raw code. Well, I could work around that. However... - it changes an older version of the code which has seen changes significant enough, especially in _recurseGroup, to make merging impossible. If there was a way to produce a working patch that can patch the current SVN trunk I'd merge and release it and just see if anyone screams about the change in behavior... |
|
|
|
| Comment by John Hannon on Sep 25, 2006 4:53 pm | |
|
I've hacked ActiveDirectoryMultiPlugin.py so that it works with AD. I've also added a property to control depth of recursion (for performance). This has been tested with an actual MS AD domain. Changes are limited to the recurseGroups method and the groups_recurse_depth property. John ... ############################################################################# # ActiveDirectoryMultiPlugin Shim to use the LDAPUserFolder with th # PluggableAuthenticationService w/ A # ############################################################################## __doc__ = """ ActiveDirectoryUserFolder shim module """ __version__ = '$Revision: 1341 $'[11:-2] # General Python imports import logging import os from urllib import quote_plus # Zope imports from Acquisition import aq_base, aq_parent from Globals import InitializeClass from Globals import DTMLFile from Globals import package_home from AccessControl import ClassSecurityInfo from Products.LDAPUserFolder import manage_addLDAPUserFolder from Products.PluggableAuthService.interfaces.plugins import \ IUserEnumerationPlugin, IGroupsPlugin, IGroupEnumerationPlugin, \ IRoleEnumerationPlugin from Products.PluggableAuthService.utils import classImplements from Products.PluggableAuthService.utils import implementedBy from LDAPPluginBase import LDAPPluginBase logger = logging.getLogger('event.LDAPMultiPlugin') _dtmldir = os.path.join(package_home(globals()), 'dtml') addActiveDirectoryMultiPluginForm = DTMLFile('addActiveDirectoryMultiPlugin', _dtmldir) def manage_addActiveDirectoryMultiPlugin( self, id, title, LDAP_server , login_attr , uid_attr, users_base, users_scope, roles , groups_base, groups_scope, binduid, bindpwd , binduid_usage=1, rdn_attr='cn', local_groups=0 , use_ssl=0 , encryption='SHA', read_only=0 , REQUEST=None ): """ Factory method to instantiate a ActiveDirectoryMultiPlugin """ # Make sure we really are working in our container (the # PluggableAuthService object) self = self.this() # Instantiate the folderish adapter object lmp = ActiveDirectoryMultiPlugin(id, title=title) self._setObject(id, lmp) lmp = getattr(aq_base(self), id) lmp_base = aq_base(lmp) # Put the "real" LDAPUserFolder inside it manage_addLDAPUserFolder(lmp) luf = getattr(lmp_base, 'acl_users') host_elems = LDAP_server.split(':') host = host_elems[0] if len(host_elems) > 1: port = host_elems[1] else: port = '389' luf.manage_addServer(host, port=port) luf.manage_edit( title , login_attr , uid_attr , users_base , users_scope , roles , groups_base , groups_scope , binduid , bindpwd , binduid_usage=binduid_usage , rdn_attr=rdn_attr , local_groups=local_groups , encryption=encryption , read_only=read_only , REQUEST=None ) # clean out the __allow_groups__ bit because it is not needed here # and potentially harmful if hasattr(lmp_base, '__allow_groups__'): del lmp_base.__allow_groups__ uf = lmp.acl_users uf._ldapschema = { 'cn' : { 'ldap_name' : 'cn' , 'friendly_name' : 'Canonical Name' , 'multivalued' : '' , 'public_name' : '' } , 'sn' : { 'ldap_name' : 'sn' , 'friendly_name' : 'Last Name' , 'multivalued' : '' , 'public_name' : 'last_name' } } uf.manage_addLDAPSchemaItem('dn', 'Distinguished Name', public_name='dn') uf.manage_addLDAPSchemaItem('sAMAccountName', 'Windows Login Name', public_name='windows_login_name') uf.manage_addLDAPSchemaItem('objectGUID', 'AD Object GUID', public_name='objectGUID') uf.manage_addLDAPSchemaItem('givenName', 'First Name', public_name='first_name') uf.manage_addLDAPSchemaItem('sn', 'Last Name', public_name='last_name') uf.manage_addLDAPSchemaItem('memberOf', 'Group DNs', public_name='memberOf', multivalued=True) if REQUEST is not None: REQUEST.RESPONSE.redirect('%s/manage_main' % self.absolute_url()) class ActiveDirectoryMultiPlugin(LDAPPluginBase): """ The adapter that mediates between the PAS and the LDAPUserFolder """ security = ClassSecurityInfo() meta_type = 'ActiveDirectory Multi Plugin' _properties = LDAPPluginBase._properties + ( {'id':'groupid_attr', 'type':'string', 'mode':'w'}, {'id':'grouptitle_attr', 'type':'string', 'mode':'w'}, {'id':'group_class', 'type':'string', 'mode':'w'}, {'id':'group_recurse', 'type':'int', 'mode':'w'}, {'id':'groups_recurse_depth', 'type':'int', 'mode':'w'}, ) groupid_attr = 'objectGUID' grouptitle_attr = 'cn' group_class = 'group' group_recurse = 1 groups_recurse_depth = 1 def __init__(self, id, title='', groupid_attr='objectGUID', grouptitle_attr='cn', group_class='group', group_recurse=1, groups_recurse_depth=1): """ Initialize a new instance """ self.id = id self.title = title self.groupid_attr = groupid_attr self.grouptitle_attr = grouptitle_attr self.group_class = group_class self.group_recurse = group_recurse self.groups_recurse_depth = groups_recurse_depth security.declarePublic('getGroupsForPrincipal') def getGroupsForPrincipal(self, user, request=None, attr=None): """ Fulfill GroupsPlugin requirements """ if attr is None: attr = self.groupid_attr acl = self._getLDAPUserFolder() if acl is None: return () view_name = self.getId() + '_getGroupsForPrincipal' criteria = {'user_id':user.getId(), 'attr':attr} cached_info = self.ZCacheable_get(view_name = view_name, keywords = criteria, default = None) if cached_info is not None: logger.debug('returning cached results from getGroupsForPrincipal') return cached_info unmangled_userid = self._demangle(user.getId()) if unmangled_userid is None: return () ldap_user = acl.getUserById(unmangled_userid) if ldap_user is None: return () cns = [ x.split(',')[0] for x in (ldap_user.memberOf or []) ] if not cns: return () cns = [ '(%s)' % x for x in cns ] filt = '(&(objectClass=%s)(|%s))' % (self.group_class, ''.join(cns)) delegate = acl._delegate R = delegate.search(acl.groups_base, acl.groups_scope, filter=filt) if R['exception']: raise RuntimeError, R['exception'] if self.group_recurse: groups = self._recurseGroups(R['results']) else: groups = R['results'] results = [ x[attr][0] for x in groups] self.ZCacheable_set(results, view_name=view_name, keywords=criteria) return results def _recurseGroups(self, ldap_results, temp=None, seen=None, depth=0): """ Given a set of LDAP result data for a group search, return the recursive group memberships for each group: arbitrarily expensive """ if seen is None: seen = {} if temp is None: temp = [] # Build a single filter so we can do it with a single search. filt_bits = [] acl = self.acl_users delegate = acl._delegate base = acl.groups_base for result in ldap_results: dn = result['dn'] if seen.has_key(dn): continue temp.append(result) seen[dn] = 1 if result.has_key('memberOf'): for parent_dn in result['memberOf']: filt = '(distinguishedName=%s)' % parent_dn if filt in filt_bits: continue filt_bits.append(filt) if filt_bits: bits_s = ''.join(filt_bits) filt = "(&(objectClass=%s)(|%s))" % (self.group_class, bits_s) R = delegate.search(base, acl.groups_scope, filter=filt) if R['exception']: raise RuntimeError, R['exception'] if depth < self.groups_recurse_depth: self._recurseGroups(R['results'], temp, seen, depth + 1) return temp security.declarePrivate('enumerateUsers') def enumerateUsers( self , id=None , login=None , exact_match=0 , sort_by=None , max_results=None , **kw ): """ Fulfill the UserEnumerationPlugin requirements """ view_name = self.getId() + '_enumerateUsers' criteria = {'id':id, 'login':login, 'exact_match':exact_match, 'sort_by':sort_by, 'max_results':max_results} criteria.update(kw) cached_info = self.ZCacheable_get(view_name = view_name, keywords = criteria, default = None) if cached_info is not None: logger.debug('returning cached results from enumerateUsers') return cached_info result = [] acl = self._getLDAPUserFolder() login_attr = acl.getProperty('_login_attr') uid_attr = acl.getProperty('_uid_attr') plugin_id = self.getId() edit_url = '%s/%s/manage_userrecords' % (plugin_id, acl.getId()) if login_attr in kw: login = kw[login_attr] del kw[login_attr] if uid_attr in kw: id = kw[uid_attr] del kw[uid_attr] if acl is None: return () if exact_match: if id: ldap_user = acl.getUserById(id) elif login: ldap_user = acl.getUser(login) else: msg = 'Exact Match specified but no ID or Login given' raise ValueError, msg if ldap_user is not None: qs = 'user_dn=%s' % quote_plus(ldap_user.getUserDN()) result.append( { 'id' : ldap_user.getId() , 'login' : ldap_user.getProperty(login_attr) , 'pluginid' : plugin_id , 'title': ldap_user.getProperty(login_attr) , 'editurl' : '%s?%s' % (edit_url, qs) } ) elif id or login or kw: l_results = [] seen = [] attrs = (uid_attr, login_attr) if id: l_results.extend(acl.findUser(uid_attr, id, attrs=attrs)) if login: l_results.extend(acl.findUser(login_attr, login, attrs=attrs)) for key, val in kw.items(): l_results.extend(acl.findUser(key, val, attrs=attrs)) for l_res in l_results: if l_res['dn'] not in seen: l_res['id'] = l_res[uid_attr] l_res['login'] = l_res[login_attr] l_res['pluginid'] = plugin_id quoted_dn = quote_plus(l_res['dn']) l_res['editurl'] = '%s?user_dn=%s' % (edit_url, quoted_dn) result.append(l_res) seen.append(l_res['dn']) if sort_by is not None: result.sort(lambda a, b: cmp( a.get(sort_by, '').lower() , b.get(sort_by, '').lower() ) ) if isinstance(max_results, int) and len(result) > max_results: result = result[:max_results-1] else: result = [] for uid, name in acl.getUserIdsAndNames(): tmp = {} tmp['id'] = uid tmp['login'] = name tmp['pluginid'] = plugin_id tmp['editurl'] = None result.append(tmp) if sort_by is not None: result.sort(lambda a, b: cmp( a.get(sort_by, '').lower() , b.get(sort_by, '').lower() ) ) if isinstance(max_results, int) and len(result) > max_results: result = result[:max_results-1] result = tuple(result) self.ZCacheable_set(result, view_name=view_name, keywords=criteria) return result security.declarePrivate('enumerateGroups') def enumerateGroups( self , id=None , exact_match=0 , sort_by=None , max_results=None , **kw ): """ Fulfill the RoleEnumerationPlugin requirements """ view_name = self.getId() + '_enumerateGroups' criteria = {'id':id, 'exact_match':exact_match, 'sort_by':sort_by, 'max_results':max_results} criteria.update(kw) cached_info = self.ZCacheable_get(view_name = view_name, keywords = criteria, default = None) if cached_info is not None: logger.debug('returning cached results from enumerateGroups') return cached_info acl = self._getLDAPUserFolder() if acl is None: return () if id is None and exact_match != 0: raise ValueError, 'Exact Match requested but no id provided' elif id is None: id = '' test_id = id.lower() plugin_id = self.getId() filt = ['(objectClass=%s)' % self.group_class] if not test_id: filt.append('(%s=*)' % self.groupid_attr) elif exact_match: filt.append('(%s=%s)' % (self.groupid_attr, test_id)) elif test_id: filt.append('(%s=*%s*)' % (self.groupid_attr, test_id)) filt = '(&%s)' % ''.join(filt) delegate = acl._delegate R = delegate.search(acl.groups_base, acl.groups_scope, filter=filt) if R['exception']: raise RuntimeError, R['exception'] groups = R['results'] results = [] for group in groups: tmp = {} tmp['title'] = '(Group) ' + group[self.grouptitle_attr][0] id = tmp['id'] = group[self.groupid_attr][0] tmp['pluginid'] = plugin_id results.append(tmp) if sort_by is not None: results.sort(lambda a, b: cmp( a.get(sort_by, '').lower() , b.get(sort_by, '').lower() ) ) if isinstance(max_results, int) and len(results) > max_results: results = results[:max_results+1] results = tuple(results) self.ZCacheable_set(results, view_name=view_name, keywords=criteria) return results security.declarePrivate('enumerateRoles') def enumerateRoles( self , id=None , exact_match=0 , sort_by=None , max_results=None , **kw ): """ Fulfill the RoleEnumerationPlugin requirements """ return [] classImplements( ActiveDirectoryMultiPlugin , IUserEnumerationPlugin , IGroupsPlugin , IGroupEnumerationPlugin , IRoleEnumerationPlugin , *implementedBy(LDAPPluginBase) ) InitializeClass(ActiveDirectoryMultiPlugin) |
|
|
|
| Defer by Jens Vagelpohl on Sep 24, 2006 9:10 am | |
|
I'm sorry for the long silence, I've looked at the issue a few times but every single time I'm coming back with the feeling that I'm too afraid to touch it because this would completely turn around the logic of that method. I'm going to defer the issue for right now. If someone else comes to me with the same problem I can look at it again - so far you have been the only user to comment on it. |
|
|
|
| Comment by Marcin Davies on Aug 1, 2006 6:22 am | |
|
OK, so I stepped through getGroupsForPrincipal and _recurseGroups with pdb and I think I have isolated the problem. The logic behind the _recurseGroups method seems to cause the wrong behavior. I assumed that it would recurse through all the memberOf links to enumerate a complete list of groups (similarly as you have written in your previous follow-up). Instead, it searches the LDAP tree for groups that have a memberOf attribute that contains the original (i.e. direct) groups of the user (line 223). This is a quite opposite behavior. If a user belongs to group A and group A is a member of group B, then group A has a memberOf atrribute containing the DN of group B (this is also what AD does, and I think - without having access to it - OpenLDAP behaves the same way). _recurseGroups, however, searches the directory for groups that have a memberOf that contain the DN of A. Thus, it searches for groups that are members (i.e. childs) of the original group A, and not groups that are parents of group A (which is the idea of nesting). However, this would be perfectly OK with a minimal change, namely if the search would be performed for the member atrribute not the memberOf. Thus, I changed line 215 to filt = '(member=%s)',% (dn,) and everything seems to work. The search is then performed for groups that have the original/direct groups of the user as a member, and this is IMO the way it should be. Hence, I would propose either changing memberOf to member or change the logic of the whole method. Please think about this proposal - I will continue testing in the meantime. If you agree, I can also commit the change to SVN (if you provide me with an account). Thanks and best regards, Marcin |
|
|
|
| Comment by Jens Vagelpohl on Jul 25, 2006 8:10 am | |
|
I had to check back myself, the ActiveDirectoryMultiPlugin was not written by me and I haven't had the chance to do any testing against AD so far, so I'm never sure if those bits really work. Looking at the code it seems the group recursion should work if this groups containment works using the AD idiom of assigning a "memberOf" attribute to the user record which points to a group record. The group record itself can also have a "memberOf" attribute, which implies the original user will also be considered part of the original group as well as all groups the original group's "memberOf" attribute is pointing to, and so forth. I don't know if AD allows any variations on that theme, but that should work when looking at the code. If I had access to a AD instance that shows the problem I could debug it further. Otherwise I can only ask you to step through the code in the getGroupsForPrincipal method and investigate... |
|
|
|
| Comment by Marcin Davies on Jul 25, 2006 3:14 am | |
| Thanks for the quick reply. If you say that AD nested groups are not supported, why is the group_recurse option mentioned at all (under special features of the Active Directory Multi Plugin)? Thanks! |
|
|
|
| Reject by Jens Vagelpohl on Jul 24, 2006 11:23 am | |
|
Sorry, AD's strange nested groups setup is currently not supported. The behavior is correct in the light of this missing "feature". |
|
|
|
| Initial Request by Marcin Davies on Jul 24, 2006 11:17 am | |
|
Hi, I observe a weird behaviour with the enumeration of nested groups for a user. I'm using LDAPMultiPlugins 1.2, LDAPUserFolder 2.7-beta1, PluggableAuthService 1.2 and Plone 2.5 (PlonePAS 2.1). Connection to our MS AD (Windows Server 2003 SP1) is working fine, but when I turn the group_recurse option on, the result is not as expected. The following example should illustrate the issue: User A is in Group G1. Group G1 also contains Groups G2 and G3 (nested groups). User B is a member of G2, User C is a member of G3. To my understanding, User B should be a member of G2 and G1, User C of G3 and G1. However, both are only members of their direct groups, i.e. G2 and G3, not of G1. In addition, User A (who should only be in G1) is a member of G1 AND G2 AND G3. The wrong memberships are reflected in the wrong mapping of roles (i have group to role mappings set up in portal_role_manager). LDAPUserFolder shows the (direct) group memberships correctly, however. Thus, I assume the erroneous behavior happening when the groups are unfolded. This is a reproducible behavior. Any help would be greatly appreciated. Thanks! |
