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

[Maurits van Rees]

Op 04-01-13 22:03, Maurits van Rees schreef:
> 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?

Any reaction to this?

While working on adding some tests, I noticed that it may be better to 
not introduce this new IUpdateLoginNamePlugin plugin interface, but 
instead add the extra methods (updateUser and updateAllLoginNames) to 
the IUserEnumerationPlugin. That would mean changing an existing 
interface, which is why I initially did not do this, but I have already 
added the implementation to the ZODBUserManager class. The updateUser 
method has been there for a long time, it just was not part of any 
interface yet.

In updateLoginName(user_id, login_name) PAS first calls 
self.getUserById(user_id) in my implementation and does not even call 
updateUser(user_id, login_name) on an IUpdateLoginName plugin if the 
user is not found.  So any plugin that implements IUpdateLoginName is 
probably also going to implement IUserEnumeration.

Would you agree it is better to merge those two plugin interfaces? Added 
bonus would be that this voids the need for migration code, because no 
extra plugin interface needs to be registered and activated.

Again, my branch is here:
http://svn.zope.org/repos/main/Products.PluggableAuthService/branches/maurits-login-transform


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