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.r...@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

_______________________________________________
Zope-PAS mailing list
Zope-PAS@zope.org
https://mail.zope.org/mailman/listinfo/zope-pas

Reply via email to