collective / dexterity.membrane

enables dexterity content items to be used as users and groups in Plone sites
3 stars 14 forks source link

Approve & notify fails on mixed case emails #35

Closed agitator closed 6 years ago

agitator commented 6 years ago

Password reset doesn't work with emails like FirstnameLastname@domain.com

vedantc98 commented 6 years ago

Is it possible that this issue in Product.membrane could be causing this bug?

agitator commented 6 years ago

I'm planning to fix this with a https://pythonhosted.org/z3c.form/converter.html for the email field in https://github.com/collective/dexterity.membrane/blob/master/dexterity/membrane/content/member.py#L89 which would convert the value to lowercase.

Does anyone know how to register an adapter for this or got a better idea? @davisagli @mauritsvanrees ?

mauritsvanrees commented 6 years ago

Do my remarks in the issue that @vedantc98 points to maybe help?

Otherwise, I see a converter in plone.z3cform. Perhaps that one can server as an example.

Alternative could possibly be to register a subscriber for dexterity.membrane on an ObjectModified event.

agitator commented 6 years ago

Thanx @mauritsvanrees emaillogin is on

I gave the adapter a first shot in https://github.com/collective/dexterity.membrane/commit/a0dee9325797fa3fa917cab57680d686852db406 but it's not running into the pdb, any idea before doing it as a subscriber?

agitator commented 6 years ago

@ale-rt ping?

ale-rt commented 6 years ago

This is tricky. From what I can read googling the part of the email address before the @ is theoretically case sensitive.

Regarding the adapter I cannot see what's actually wrong, to understand that some diving in the z3c.form code using a pdb might help.

agitator commented 6 years ago

@ale-rt you're right https://en.wikipedia.org/wiki/Email_address and generally lowercasing or changing the email for dx.membrane is out of question.

so i dug a bit deeper... (at least) in case of dx.membrane the login/userid is uses acl_users in https://github.com/plone/Products.CMFPlone/blob/master/Products/CMFPlone/RegistrationTool.py#L69 to find the actual member.

What I tried:

I got three test users [<MembraneUser u'de-member@agitator.com'>, <MembraneUser u'fr-member@agitator.com'>, <MembraneUser u'en-member@agitator.com'>]

On the fr-user I get

(Pdb++) acl.searchUsers(name='fr-member@agitator.com')
({'title': u'fr-member@agitator.com', 'editurl': 'http://me:25080/Plone/members/member-fr/edit', 'principal_type': 'user', 'userid': '5cda9b4da8f6466c807723406b6adc3a', 'pluginid': 'membrane_users', 'login': u'fr-member@agitator.com', 'id': '5cda9b4da8f6466c807723406b6adc3a'},)

On the de-user with the email De-Member@agitator.com

(Pdb++) acl.searchUsers(name='de-member@agitator.com')
()
(Pdb++) acl.searchUsers(name='De-Member@agitator.com')
()

(Pdb++) de.getId()
'b3de55fa9e344661a4ee2a8688c31e33'
(Pdb++) de.getName()
'b3de55fa9e344661a4ee2a8688c31e33'
(Pdb++) de.getUserId()
'b3de55fa9e344661a4ee2a8688c31e33'

How do I get a similar representation for the de-user as for the fr-user to find out what could be wrong? Or how could I do a different lookup?

mauritsvanrees commented 6 years ago

In acl_users on the Properties tab, is there a 'Transform to apply to login name' with value 'lower'? If not: add it. If it is: remove. See if that makes a difference. Watch out though: I think this may take a while, as it needs to apply the transform immediately. Try out locally.

agitator commented 6 years ago

@mauritsvanrees that did the trick! De-Member@agitator.com is now found (but not de-member@agitator.com).

btw. it didn't take long to change that, I think it just changes the case for the query.

Still I have my doubts, I couldn't think of any user (besides users with "real" special characters in their email) that would really enter their email in some sort of titlecase.

I think I'll add a suscriber in my project that will lowercase the emails on save.

Raise your hands if something like this should go into dx.membrane in any form. Other opinions?

agitator commented 6 years ago

... and yes, problems like this show that there are obviosly reasons to not use the email as login ;-)