diff --git a/CHANGES.rst b/CHANGES.rst index 3ade612..c8a14b3 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -5,7 +5,13 @@ History 1.5.2 (unreleased) ------------------ -- No changes yet. +- Avoid ``KeyError`` in ``getPropertiesForUser`` when ``self.users`` is None. [maurits] + +- Check if interface is active when calling a method. [maurits] + +- Skip matches that don't have our required login attribute. [fredvd] + +- Merge Asko's speed optimizations. [asko] 1.5.1 (2016-10-18) diff --git a/setup.py b/setup.py index c22f487..d28585e 100644 --- a/setup.py +++ b/setup.py @@ -40,7 +40,7 @@ install_requires=[ 'AccessControl>=3.0', 'Acquisition', - 'bda.cache', + 'bda.cache[pylibmc]', 'five.globalrequest', 'node', 'node.ext.ldap>=1.0b2', diff --git a/src/pas/plugins/ldap/plugin.py b/src/pas/plugins/ldap/plugin.py index ea4f516..e5dc646 100644 --- a/src/pas/plugins/ldap/plugin.py +++ b/src/pas/plugins/ldap/plugin.py @@ -9,8 +9,9 @@ from Products.PluggableAuthService.permissions import ManageGroups from Products.PluggableAuthService.permissions import ManageUsers from Products.PluggableAuthService.plugins.BasePlugin import BasePlugin -from node.ext.ldap.base import decode_utf8 -from node.ext.ldap.base import encode_utf8 +from Products.statusmessages.interfaces import IStatusMessage +from node.utils import decode +from node.utils import encode from node.ext.ldap.interfaces import ILDAPGroupsConfig from node.ext.ldap.interfaces import ILDAPProps from node.ext.ldap.interfaces import ILDAPUsersConfig @@ -20,6 +21,7 @@ from pas.plugins.ldap.interfaces import VALUE_NOT_CACHED from pas.plugins.ldap.sheet import LDAPUserPropertySheet from zope.interface import implementer +from plone.memoize import ram import ldap import logging import os @@ -96,6 +98,17 @@ def _wrapper(self, *args, **kwargs): return _decorator +def cacheKey(func, timeout, *args, **kwargs): + key = __file__ + key += ':' + func.__name__ + key += ':' + str(int(time.time() // timeout)) + for arg in ([args] + list(kwargs.values())): + if isinstance(arg, unicode): + arg = arg.encode('utf-8') + key += ':' + str(arg) + return key + + @implementer( ILDAPPlugin, pas_interfaces.IAuthenticationPlugin, @@ -103,13 +116,7 @@ def _wrapper(self, *args, **kwargs): pas_interfaces.IGroupsPlugin, pas_interfaces.IPropertiesPlugin, pas_interfaces.IUserEnumerationPlugin, - plonepas_interfaces.capabilities.IDeleteCapability, - plonepas_interfaces.capabilities.IGroupCapability, - plonepas_interfaces.capabilities.IPasswordSetCapability, - plonepas_interfaces.group.IGroupManagement, - plonepas_interfaces.group.IGroupIntrospection, - plonepas_interfaces.plugins.IMutablePropertiesPlugin, - plonepas_interfaces.plugins.IUserManagement) + plonepas_interfaces.group.IGroupIntrospection) class LDAPPlugin(BasePlugin): """Glue layer for making node.ext.ldap available to PAS. """ @@ -129,6 +136,12 @@ def __init__(self, id, title=None, **kw): self.settings = OOBTree.OOBTree() self.plugin_caching = True + @security.private + def is_plugin_active(self, iface): + pas = self._getPAS() + ids = pas.plugins.listPluginIds(iface) + return self.getId() in ids + @property @security.private def groups_enabled(self): @@ -197,19 +210,22 @@ def authenticateCredentials(self, credentials): o If the credentials cannot be authenticated, return None. """ + default = None + if not self.is_plugin_active(pas_interfaces.IAuthenticationPlugin): + return default login = credentials.get('login') pw = credentials.get('password') if not (login and pw): - return None + return default logger.debug('credentials: %s' % credentials) users = self.users if not users: - return None + return default userid = users.authenticate(login, pw) if userid: logger.info('logged in %s' % userid) return (userid, login) - return None + return default # ## # pas_interfaces.IGroupEnumerationPlugin @@ -217,6 +233,7 @@ def authenticateCredentials(self, credentials): # Allow querying groups by ID, and searching for groups. # @security.private + @ram.cache(lambda f, o, *args, **kwargs: cacheKey(f, 10, *args, **kwargs)) def enumerateGroups(self, id=None, exact_match=False, sort_by=None, max_results=None, **kw): """ -> ( group_info_1, ... group_info_N ) @@ -258,10 +275,21 @@ def enumerateGroups(self, id=None, exact_match=False, sort_by=None, o Insufficiently-specified criteria may have catastrophic scaling issues for some implementations. """ + default = () + if not self.is_plugin_active(pas_interfaces.IGroupEnumerationPlugin): + return default groups = self.groups if not groups: - return () - if id: + return default + if id and exact_match: + if id in self.getGroupIds(): + return ({ + 'id': id, + 'pluginid': self.getId() + }) + else: + return default + elif id: kw['id'] = id if not kw: # show all matches = groups.ids @@ -270,21 +298,40 @@ def enumerateGroups(self, id=None, exact_match=False, sort_by=None, cookie = None while True: try: - batch_matches, cookie = groups.search( - criteria=kw, + res = groups.search( + criteria=encode(kw), + attrlist=None, exact_match=exact_match, page_size=self._ldap_props.page_size, cookie=cookie, ) + if isinstance(res, tuple): + batch_matches, cookie = res + else: + batch_matches, cookie = res, '' except ValueError: - return tuple() + return default matches += batch_matches if not cookie: break + if len(matches) >= 100: + msg = 'Too many search results. Please, narrow your search.' + IStatusMessage(self.REQUEST).add(msg, type='warning') + return default if sort_by == 'id': matches = sorted(matches) pluginid = self.getId() - ret = [dict(id=encode_utf8(_id), pluginid=pluginid) for _id in matches] + ret = list() + if exact_match: + for id, attrs in matches: + ret.append({ + 'id': id, + 'pluginid': pluginid}) + else: + for id in matches: + ret.append({ + 'id': id, + 'pluginid': pluginid}) if max_results and len(ret) > max_results: ret = ret[:max_results] return ret @@ -294,6 +341,7 @@ def enumerateGroups(self, id=None, exact_match=False, sort_by=None, # # Determine the groups to which a user belongs. @security.private + @ram.cache(lambda f, o, *args, **kwargs: cacheKey(f, 10, args[0])) def getGroupsForPrincipal(self, principal, request=None): """principal -> ( group_1, ... group_N ) @@ -302,21 +350,26 @@ def getGroupsForPrincipal(self, principal, request=None): o May assign groups based on values in the REQUEST object, if present """ - users = self.users - if not users: - return tuple() - try: - _principal = self.users[principal.getId()] - except KeyError: + default = tuple() + if not self.is_plugin_active(pas_interfaces.IGroupsPlugin): + return default + groups = self.groups + if not groups: + return default + + _id = principal.getId() + + if _id in self.getGroupIds(): # getGroupIds is cached groups.ids # XXX: that's where group in group will happen, but so far # group nodes do not provide membership info so we just # return if there is no user - return tuple() - if self.groups: - # XXX: provide group_ids function in UGM! Way too calculation-heavy - # now - return [_.id for _ in _principal.groups] - return tuple() + return default + users = self.users + try: + return users and users[_id].group_ids or [] + except KeyError: + pass + return default # ## # pas_interfaces.IUserEnumerationPlugin @@ -325,6 +378,7 @@ def getGroupsForPrincipal(self, principal, request=None): # @ldap_error_handler('enumerateUsers') @security.private + @ram.cache(lambda f, o, *args, **kwargs: cacheKey(f, 10, *args, **kwargs)) def enumerateUsers(self, id=None, login=None, exact_match=False, sort_by=None, max_results=None, **kw): """-> ( user_info_1, ... user_info_N ) @@ -367,6 +421,9 @@ def enumerateUsers(self, id=None, login=None, exact_match=False, o Insufficiently-specified criteria may have catastrophic scaling issues for some implementations. """ + default = tuple() + if not self.is_plugin_active(pas_interfaces.IUserEnumerationPlugin): + return default # TODO: sort_by in node.ext.ldap if login: if not isinstance(login, basestring): @@ -385,34 +442,82 @@ def enumerateUsers(self, id=None, login=None, exact_match=False, kw['id'] = id users = self.users if not users: - return tuple() + return default + if not exact_match: + for value in users.principal_attrmap.values(): + kw[value] = kw.values()[0] matches = [] cookie = None while True: try: - batch_matches, cookie = users.search( - criteria=kw, - attrlist=('login',), + res = users.search( + criteria=encode(kw), + attrlist=exact_match and + users.context.search_attrlist or ['login'], exact_match=exact_match, - page_size=self._ldap_props.page_size, + or_search=not exact_match, + page_size=not exact_match and + self._ldap_props.page_size or None, cookie=cookie, ) + if isinstance(res, tuple): + batch_matches, cookie = res + else: + batch_matches, cookie = res, '' except ValueError: - return tuple() + return default matches += batch_matches if not cookie: break + if len(matches) >= 100: + msg = 'Too many search results. Please, narrow your search.' + IStatusMessage(self.REQUEST).add(msg, type='warning') + return default pluginid = self.getId() ret = list() for id, attrs in matches: + if 'login' not in attrs: + continue ret.append({ - 'id': encode_utf8(id), + 'id': id, 'login': attrs['login'][0], 'pluginid': pluginid}) if max_results and len(ret) > max_results: ret = ret[:max_results] return ret + @security.private + def updateUser(self, user_id, login_name): + """ Update the login name of the user with id user_id. + + The plugin must return True (or any truth value) to indicate a + successful update, also when no update was needed. + + When updating a login name makes no sense for a plugin (most + likely because it does not actually store login names) and it + does not do anything, it must return None or False. + """ + # XXX + # if not self.is_plugin_active(pas_interfaces.IUserEnumerationPlugin): + # return default + return False + + @security.private + def updateEveryLoginName(self, quit_on_first_error=True): + """Update login names of all users to their canonical value. + + This should be done after changing the login_transform + property of PAS. + + You can set quit_on_first_error to False to report all errors + before quitting with an error. This can be useful if you want + to know how many problems there are, if any. + """ + # XXX + # if not self.is_plugin_active(pas_interfaces.IUserEnumerationPlugin): + # return default + return + # ## # plonepas_interfaces.group.IGroupManagement # @@ -481,6 +586,7 @@ def removePrincipalFromGroup(self, principal_id, group_id): # which case the properties are not persistently mutable). # @security.private + @ram.cache(lambda f, o, *args, **kwargs: cacheKey(f, 10, args[0])) def getPropertiesForUser(self, user_or_group, request=None): """User -> IMutablePropertySheet || {} @@ -492,13 +598,17 @@ def getPropertiesForUser(self, user_or_group, request=None): o May assign properties based on values in the REQUEST object, if present """ + default = {} + if not self.is_plugin_active(pas_interfaces.IPropertiesPlugin): + return default ugid = user_or_group.getId() + users = self.users try: - if self.enumerateUsers(id=ugid) or self.enumerateGroups(id=ugid): + if ugid in self.getGroupIds() or users and users[ugid]: return LDAPUserPropertySheet(user_or_group, self) except KeyError: pass - return {} + return default @security.private def setPropertiesForUser(self, user, propertysheet): @@ -603,10 +713,13 @@ def getGroupById(self, group_id): Returns the portal_groupdata-ish object for a group corresponding to this id. None if group does not exist here! """ - group_id = decode_utf8(group_id) + default = None + if not self.is_plugin_active( + plonepas_interfaces.group.IGroupIntrospection): + return default groups = self.groups - if not groups or group_id not in groups.keys(): - return None + if not groups or group_id not in self.getGroupIds(): + return default ugmgroup = self.groups[group_id] title = ugmgroup.attrs.get('title', None) group = PloneGroup(ugmgroup.id, title).__of__(self) @@ -619,7 +732,7 @@ def getGroupById(self, group_id): data = propfinder.getPropertiesForUser(group, None) if not data: continue - group.addPropertysheet(propfinder_id, data) + group.addPropertysheet(propfinder_id, decode(data)) # add subgroups group._addGroups(pas._getGroupsForPrincipal(group, None, plugins=plugins)) @@ -637,23 +750,59 @@ def getGroups(self): """ Returns an iteration of the available groups """ + # Checking self.is_plugin_active( + # plonepas_interfaces.group.IGroupIntrospection) + # is done in self.getGroupIds() already. return map(self.getGroupById, self.getGroupIds()) + @ram.cache(lambda f, o: cacheKey(f, 300)) def getGroupIds(self): """ Returns a list of the available groups (ids) """ - return self.groups and self.groups.ids or [] + default = [] + if not self.is_plugin_active( + plonepas_interfaces.group.IGroupIntrospection): + return default + groups = self.groups + if not groups: + return default + matches = [] + cookie = None + while True: + try: + batch_matches, cookie = groups.search( + attrlist=['id'], + page_size=self._ldap_props.page_size, + cookie=cookie, + ) + except ValueError: + return default + matches += batch_matches + if not cookie: + break + return [group[1]['id'][0] for group in matches] def getGroupMembers(self, group_id): """ return the members of the given group """ + default = () + if not self.is_plugin_active( + plonepas_interfaces.group.IGroupIntrospection): + return default try: group = self.groups[group_id] except (KeyError, TypeError): - return () - return tuple(group.member_ids) + return default + member_ids = group.member_ids + if len(member_ids) <= 500: + return tuple(member_ids) + else: + msg = ('Group "{0:s}" has over 500 members. ' + 'Members cannot be listed.'.format(group_id)) + IStatusMessage(self.REQUEST).add(msg, type='error') + return default # ## # plonepas_interfaces.capabilities.IPasswordSetCapability diff --git a/src/pas/plugins/ldap/properties.py b/src/pas/plugins/ldap/properties.py index fa2a8f3..6e3c6a8 100644 --- a/src/pas/plugins/ldap/properties.py +++ b/src/pas/plugins/ldap/properties.py @@ -174,7 +174,10 @@ def propproxy(ckey): def _getter(context): value = context.plugin.settings.get(ckey, DEFAULTS[ckey]) - return value + if isinstance(value, unicode): + return value.encode('utf-8') + else: + return value def _setter(context, value): context.plugin.settings[ckey] = value @@ -219,7 +222,7 @@ def memcached(self, value): recordProvider = queryUtility(ICacheSettingsRecordProvider) if recordProvider is not None: record = recordProvider() - record.value = value.decode('utf8') + record.value = value else: return u'feature not available' diff --git a/src/pas/plugins/ldap/properties.yaml b/src/pas/plugins/ldap/properties.yaml index 3915acd..1c6eb40 100644 --- a/src/pas/plugins/ldap/properties.yaml +++ b/src/pas/plugins/ldap/properties.yaml @@ -188,6 +188,7 @@ widgets: label: Memcached Server to use help: global - same server for all ldap plugins field.class: memcached field + datatype: unicode - timeout: factory: '#field:number' value: expr:context.props.timeout diff --git a/src/pas/plugins/ldap/sheet.py b/src/pas/plugins/ldap/sheet.py index 22599c0..b1e3dbb 100644 --- a/src/pas/plugins/ldap/sheet.py +++ b/src/pas/plugins/ldap/sheet.py @@ -1,7 +1,9 @@ # -*- coding: utf-8 -*- from Acquisition import aq_base -from Products.PlonePAS.interfaces.propertysheets import IMutablePropertySheet from Products.PluggableAuthService.UserPropertySheet import UserPropertySheet +from Products.PluggableAuthService.interfaces.propertysheets import IPropertySheet # noqa +from node.utils import decode +from node.utils import encode from node.ext.ldap.interfaces import ILDAPGroupsConfig from node.ext.ldap.interfaces import ILDAPUsersConfig from zope.globalrequest import getRequest @@ -12,7 +14,7 @@ logger = logging.getLogger('pas.plugins.ldap') -@implementer(IMutablePropertySheet) +@implementer(IPropertySheet) class LDAPUserPropertySheet(UserPropertySheet): def __init__(self, principal, plugin): @@ -46,8 +48,11 @@ def __init__(self, principal, plugin): request['_ldap_props_reloaded'] = 1 for key in self._attrmap: self._properties[key] = ldapprincipal.attrs.get(key, '') + if 'fullname' in self._properties: + self._properties['fullname'] = ' '.join(map( + str.strip, reversed(self._properties['fullname'].split(',')))) UserPropertySheet.__init__(self, principal.getId(), schema=None, - **self._properties) + **decode(self._properties)) def _get_ldap_principal(self): """returns ldap principal @@ -76,7 +81,8 @@ def setProperties(self, obj, mapping): assert(id in self._properties) ldapprincipal = self._get_ldap_principal() for id in mapping: - self._properties[id] = ldapprincipal.attrs[id] = mapping[id] + self._properties[id] = ldapprincipal.attrs[id] = encode( + mapping[id]) try: ldapprincipal.context() except Exception, e: