zopefoundation / Products.PluggableAuthService

Pluggable Zope authentication / authorization framework
Other
9 stars 18 forks source link

Feature Request: allow "strip" for login transform #94

Closed perrinjerome closed 3 years ago

perrinjerome commented 3 years ago

PAS allows to configure a "Transform to apply to login name" and come with two built in transforms: lower and upper. Both also use value.strip():

https://github.com/zopefoundation/Products.PluggableAuthService/blob/d77e403ca53b8b915d47c5a72549e1a3cde84934/src/Products/PluggableAuthService/PluggableAuthService.py#L1035-L1053

I have an application where I want to keep case sensitive names, but I'm interested in stripping the white spaces.

Would it make sense to introduce a new transform strip ? I can send a patch if that makes sense.

I found out I can use a python script in a parent folder and it will be used thanks to acquisition, but that seems better to do that in non-restricted python and without relying on acquisition.

d-maurer commented 3 years ago

Jérome Perrin wrote at 2021-4-28 02:11 -0700:

... Would it make sense to introduce a new transform strip ? I can send a patch if that makes sense.

It looks too specialized. If we change something, then _get_login_transform_method should use an adapter to determine the transform method. This way, applications could define any transformation they need via configuration.

I found out I can use a python script in a parent folder and it will be used thanks to acquisition, but that seems better to do that in non-restricted python

You could use an "External Method"; this way, the transform would be unrestricted. Of course, strip can as easily be implemented in restricted mode.

and without relying on acquisition.

You could put the callable object implementing the transform directly on acl_users itself, thus avoidung acquisition. You would need to put this object programmatically into acl_users, because the ZMI restricts the type for objects which can be put (via the ZMI) into acl_users.

perrinjerome commented 3 years ago

Thanks for feedback.

This follows the style of the existing lower and upper transforms, but the login transform does not really follow the way of how the rest of PAS can be customized. Are you also thinking about something like an ILoginTransformationPlugin with a method applyTransform and then we could configure this with a plugin ?

Thanks also for the tips.

d-maurer commented 3 years ago

Jérome Perrin wrote at 2021-4-28 03:16 -0700:

Are you also thinking about something like an ILoginTransformationPlugin with a method applyTransform and then we could configure this with a plugin ?

Not exactly. My thought was for the use of a ZTK (= "Zope Tool Kit") adapter. But your thought would provide similar flexibility and could be more in line with the PAS philosophy.

perrinjerome commented 3 years ago

Thanks. I need to think more about all this. This login transform functionality operate globally at PAS level, but I start to feel that conceptually this should operate only at the level of IUserEnumerationPlugin - at least in my case I have multiple user sources, but stripping white space might not be OK for all the user sources. For my use case, I'm going to try implementing the transform in my custom IUserEnumerationPlugin plugin.

( Edit: clarify that I'm planning to modify my custom code for now, not the ZODBUserManager )

icemac commented 3 years ago

@perrinjerome Is this issue solved for you, so we can close it or is there still something open for you?

perrinjerome commented 3 years ago

Yes, thanks again for feedback. There are better solutions than adding new transformation methods directly in PAS like I initially thought.