[Zope-PAS] PAS: authenticateCredentials: check lowercase too?

Fri Jan 4 21:03:43 UTC 2013

Op 28-12-12 17:57, Wichert Akkerman schreef:
> On Dec 28, 2012, at 16:51 , Maurits van Rees <m.van.rees at zestsoftware.nl> wrote:
>
>> I have started a branch for this:
>> http://svn.zope.org/repos/main/Products.PluggableAuthService/branches/maurits-login-transform/
>>
>> The only commit message I did so far should be pretty clear:
>>
>> ================================================
>> Add possibility to transform the login name.
>>
>> The BasePlugin now has a property 'login_transform' and a method 'applyTransform'.
>> In proper places, plugins can call 'login_name = self.applyTransform(login_name)'.
>> When 'login_transform' is 'lower', this method will return 'self.lower(login_name)'.
>> Care is taken to not fail when the method does not exist.  The original login_name
>> is then returned.
>> A use case is to transform all login names to lowercase if you want to use the email
>> address as login name in Plone.
>> The ZODBUserManager and CookieAuthHelper plugins use this now,
>> though it is not strictly needed for the last one.
>> More may follow.
>> ================================================
>>
>> It has extra tests and they pass. I will have a look at which other plugins may need this.
>>
>> Wichert, you seem to suggest adding this in the main acl_users object. With my current implementation it is probably best to keep this an option in each plugin, with the base code (property and method) available in the BasePlugin class.
> There is a risk here in that multiple plugin types use the login name: All of IExtractionPlugin, IAuthenticationPlugin, ICredentialsUpdatePlugin, IUserAdderPlugin, IUserFactoryPlugin and IUserEnumerationPlugin all use the login name. If you only do the transform in one of them you are likely to get strange results: for example a user may show up multiple times in a user listings of he has logged in with different casing for his email address and multiple property sheets were created, or the authentication cookie might not match the login name user by the user, etc.  For that reason I am reasonably sure this needs to be done either in PAS itself at every API point where a login name is accepted.

I have basically reverted my earlier changes on that branch and have now 
done this in PAS itself instead of the plugins.  PAS now has a 
login_transform property, a helper method _get_login_transform_method 
and an applyTransform(login_name) method.  PAS calls that method in all 
spots that I could discover that need it.

One tricky thing is that the ZODBUserManager plugin has an 
updateUser(user_id, login_name) method that needs to call applyTransform 
itself, because there is no code in PAS where ZODBUserManager.updateUser 
is called.

That makes me think we could use a new plugin type for updateUser. It is 
currently not in any of the interfaces.  So on my branch I have added an 
IUpdateLoginNamePlugin plugin interface:

class IUpdateLoginNamePlugin( Interface ):
     """ Update the login name of a user.
     """

     def updateUser( user_id, login_name ):
         """ Update the login name of the user with id user_id.
         """

     def updateAllLoginNames(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.
         """

In a standard Plone context, the source_users object would be the only 
plugin that has this plugin interface activated.

I have added three related methods in IPluggableAuthService:

     def updateLoginName(user_id, login_name):
         """Update login name of user.
         """

     def updateOwnLoginName(login_name):
         """Update own login name of authenticated user.
         """

     def updateAllLoginNames(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.
         """

Those methods call the related methods of any activated 
IUpdateLoginNamePlugin plugins.
Note that I have overridden the _setPropValue method in PAS to make sure 
updateAllLoginNames is called when the login_transform method is changed 
and is not empty.

I must add more tests, but other than that: does this look good?

If I create a fresh Plone 4.2 site with these changes all seems to work 
fine.  But for current installations some migration is needed, because 
the new plugin type is not known yet, so you run into tracebacks like this:

Traceback (innermost last):
   Module ZPublisher.Publish, line 126, in publish
   Module ZPublisher.mapply, line 77, in mapply
   Module ZPublisher.Publish, line 46, in call_object
   Module OFS.PropertyManager, line 305, in manage_editProperties
   Module OFS.PropertyManager, line 210, in _updateProperty
   Module Products.PluggableAuthService.PluggableAuthService, line 1113, 
in _setPropValue
   Module Products.PluggableAuthService.PluggableAuthService, line 1321, 
in updateAllLoginNames
   Module Products.PluginRegistry.PluginRegistry, line 106, in listPlugins
   Module Products.PluginRegistry.PluginRegistry, line 385, in _getPlugins
KeyError: <InterfaceClass 
Products.PluggableAuthService.interfaces.plugins.IUpdateLoginNamePlugin>

If you never change the login_transform property you should never 
encounter this error, but then you cannot use the new feature.

In Plone I can probably create a GenericSetup upgrade step in PlonePAS 
and/or plone.app.upgrade. But is there a way to write migration code for 
PAS without Plone?  Is that needed?  There are two GS profiles in PAS, 
which are used when adding a 'Configured PAS' in the ZMI.  On Plone 4.2 
this fails for several reasons. Is there any way to automatically update 
an existing manually added PAS outside of Plone?

This would be the xml that you could add in the Export/Import tab of the 
plugins page, where the line with plugin id source_users is optional, 
but when I try it in Plone 4.2 it has no effect:

  <plugin-type id="IUpdateLoginNamePlugin" title="update_login_name"
      description="Login name updater plugins allow to set a new login 
name for a user."
interface="Products.PluggableAuthService.interfaces.plugins.IUpdateLoginNamePlugin" 
 >
   <plugin id="source_users" />
  </plugin-type>

Anyway, here is the branch url again, please have a look:

http://svn.zope.org/repos/main/Products.PluggableAuthService/branches/maurits-login-transform/

>> For example, if you have an LDAP plugin, you probably do not want to transform the logins there, though I am not sure if LDAP/AD servers are normally case sensitive.  In that case it would also be best to not transform the login in the extractCredentials plugin, otherwise someone with an upper or mixed case login name in LDAP would not be able to login.  For the ZODBUserManager it does not really matter if the passed credentials have already been transformed or not.
> And perhaps you might want to transform them by doing a case insensitive search in LDAP/AD to find the canonical spelling for the login name and use that. Which makes you wonder if it makes sense to define a new plugin type to handle this.

Come to think of it, if you have an LDAP/AD setup that expects login 
names in mixed or upper case and a different plugin for which you want 
it all lowercase, you should probably simply not do that in one 
Plone/Zope site.  So never mind this argument from me for the original 
implementation.

-- 
Maurits van Rees: http://maurits.vanrees.org/
Zest Software: http://zestsoftware.nl