| Request | LDAP User Folder -- feature request -- by Wichert Akkerman |
| Posted on | Sep 4, 2007 6:08 am |
| Subscribe |
| Resolve by Jens Vagelpohl on Sep 4, 2007 12:18 pm | |
|
Thanks Wichert, that's great. The patch is now in Subversion: http://svn.dataflake.org/?view=rev&revision=1430 I just made a couple small changes: - moved the NotexistingUser class into the LDAPUser.py module - In the NotexistingUser constructor, you created a DateTime instance passing a time.time() value - but that's not necessary. Calling DateTime without any argument will already hand back the current time. |
|
|
|
| Comment by Wichert Akkerman on Sep 4, 2007 11:39 am | |
|
Patch below. I added a third cache for negative entries along with a dummy user type which can be put inside it. I also added a simple test that tests if the cache is filled and cleared properly. I am working on a CMF 2.1 instance which means I get a fair number of test failures, but none of those are related to my changes. Index: dtml/cache.dtml =================================================================== --- dtml/cache.dtml (revision 1420) +++ dtml/cache.dtml (working copy) @@ -62,6 +62,19 @@ </form> </td> </tr> + </tr><tr> + <td align="left" valign="top" class="form-text"> + Negative Cache + </td> + <td align="left" valign="top" class="form-element"> + <form action="setCacheTimeout"> + <input type="hidden" name="cache_type" value="negative" /> + <input type="text" size="6" name="timeout" + value="<dtml-var "getCacheTimeout('negative')">" /> + <input type="submit" value=" Change " /> + </form> + </td> + </tr> </table> <p> </p> Index: tests/test_LDAPUserFolder.py =================================================================== --- tests/test_LDAPUserFolder.py (revision 1420) +++ tests/test_LDAPUserFolder.py (working copy) @@ -60,6 +60,7 @@ ae(acl.getProperty('read_only'), not not dg('read_only')) ae(len(acl._cache('anonymous').getCache()), 0) ae(len(acl._cache('authenticated').getCache()), 0) + ae(len(acl._cache('negative').getCache()), 0) ae(len(acl.getSchemaConfig().keys()), 2) ae(len(acl.getSchemaDict()), 2) ae(len(acl._groups_store), 0) @@ -904,6 +905,16 @@ group_dn = group_dn.replace('\\', '') acl.manage_deleteGroups(dns=[group_dn]) self.failUnless(len(acl.getGroups()) == 0) + + def testNegativeCaching(self): + ae = self.assertEqual + acl = self.folder.acl_users + ae(len(acl._cache('negative').getCache()), 0) + ae(acl.getUser('missing'), None) + ae(len(acl._cache('negative').getCache()), 1) + acl.manage_addUser(REQUEST=None, kwargs=user) + ae(len(acl._cache('negative').getCache()), 0) + def test_suite(): Index: LDAPUserFolder.py =================================================================== --- LDAPUserFolder.py (revision 1420) +++ LDAPUserFolder.py (working copy) @@ -9,6 +9,7 @@ __version__='$Revision$'[11:-2] # General python imports +from DateTime import DateTime import logging import os import random @@ -50,6 +51,18 @@ EDIT_PERMISSION = 'Change user folder' +class NonexistingUser: + """Fake user we can use in our negative cache.""" + def __init__(self): + self.birth = DateTime(time.time()) + + def getCreationTime(self): + return self.birth + + def _getPassword(self): + return None + + class LDAPUserFolder(BasicUserFolder): """ LDAPUserFolder @@ -133,6 +146,9 @@ auth_timeout = self.getCacheTimeout('authenticated') self._cache('authenticated').setTimeout(auth_timeout) + negative_timeout = self.getCacheTimeout('negative') + self._cache('negative').setTimeout(negative_timeout) + # For older LDAPUserFolders which don't distinguish between uid # and login id attrs, provide a _uid_attr if not hasattr(self, '_uid_attr'): @@ -182,6 +198,7 @@ """ Clear all logs and caches for user-related information """ self._cache('anonymous').clear() self._cache('authenticated').clear() + self._cache('negative').clear() self._misc_cache().clear() security.declarePrivate('_lookupuserbyattr') @@ -644,9 +661,12 @@ """ if not value: return None - + cache_type = pwd and 'authenticated' or 'anonymous' if cache: + if self._cache('negative').get(value) is not None: + return None + cached_user = self._cache(cache_type).get(value, pwd) if cached_user: @@ -662,6 +682,7 @@ if user_dn is None: logger.debug('getUserByAttr: "%s=%s" not found' % (name, value)) + self._cache('negative').set(value, NonexistingUser()) return None if user_attrs is None: @@ -669,6 +690,7 @@ name, value ) logger.debug(msg) + self._cache('negative').set(value, NonexistingUser()) return None if user_roles is None or user_roles == self._roles: @@ -691,6 +713,7 @@ user_dn, self._login_attr ) logger.debug(msg) + self._cache('negative').set(value, NonexistingUser()) return None if self._uid_attr != 'dn' and len(uid) > 0: @@ -700,6 +723,7 @@ user_dn, self._uid_attr ) logger.debug(msg) + self._cache('negative').set(value, NonexistingUser()) return None user_obj = LDAPUser( uid @@ -1849,10 +1873,7 @@ security.declareProtected(manage_users, 'getCacheTimeout') def getCacheTimeout(self, cache_type='anonymous'): """ Retrieve the cache timout value (in seconds) """ - if cache_type == 'authenticated': - return getattr(self, '_authenticated_timeout', 600) - else: - return getattr(self, '_anonymous_timeout', 600) + return getattr(self, '_%s_timeout' % cache_type, 600) security.declareProtected(manage_users, 'setCacheTimeout') @@ -1867,10 +1888,7 @@ else: timeout = int(timeout) - if cache_type == 'authenticated': - self._authenticated_timeout = timeout - elif cache_type == 'anonymous': - self._anonymous_timeout = timeout + setattr(self, '_%s_timeout' % cache_type, timeout) self._cache(cache_type).setTimeout(timeout) |
|
|
|
| Comment by Jens Vagelpohl on Sep 4, 2007 8:24 am | |
|
No, I would welcome such a patch. Please don't forget that the LDAPUserFolder package has tests, so adding some tests for this feature would be highly appreciated. I would implement it similar to the other caches, where the admin can set the caching intrval via the ZMI and can see/purge the cache contents. |
|
|
|
| Initial Request by Wichert Akkerman on Sep 4, 2007 6:08 am | |
|
In a quest to get more performance out of LDAP using sites I am running into search queries resulting from non-LDAP users being used. If you login with a non-LDAP user on every request the user is looked up. This results in a LDAP search query for that userid (via acl_users.validate() -> authenticateCredentials -> LDAP getUser -> LDAP user search). If that user is not in LDAP this will be not be cached. Would you be averse to a modification that caches negative search results here? |
