search

plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
240 stars 183 forks source link

RegistrationTool fails with email containing a + #3968

Closed jensens closed 3 weeks ago

jensens commented 1 month ago

BUG/PROBLEM REPORT (OR OTHER COMMON ISSUE)

What I did:

Register with a user emial containing an add like foo+123@example.com

What I expect to happen:

Works

What actually happened:

fails with ValueError "The login name you selected is already in use or is not valid. Please choose another."

What version of Plone/ Addons I am using:

6.0.11.1

Found issue:

The regex in RegistrationTool does not allow a + in https://github.com/plone/Products.CMFPlone/blob/ab84546173490cf2ba722a89a57a8b7a92c4e2bc/Products/CMFPlone/RegistrationTool.py#L110

stevepiercy commented 1 month ago

Suggest using this validation for email: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/email#basic_validation

jensens commented 1 month ago

Suggest using this validation for email: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/email#basic_validation

It's not only email, it could be an uuid as well. AFAIK primary this has to be a valid Zope ID.

stevepiercy commented 1 month ago

What's the spec for Zope ID? We can use regex alternation to use both patterns.

davisagli commented 1 month ago

Why does the login name need to be a valid Zope id? I don't think that's right.

jensens commented 1 month ago

Why does the login name need to be a valid Zope id? I don't think that's right.

This is 14year old code - I have no idea why.

rohnsha0 commented 3 weeks ago

@jensens i found adding "+" in the email creates some issues with UI?

https://github.com/plone/Products.CMFPlone/blob/a180978c54f112d6eeaefeae44e7e5b6a46b46e7/Products/CMFPlone/tests/testEmailLogin.py#L82-L87

jensens commented 3 weeks ago

@rohnsha0 good catch, looks like this is by purpose.

Question now is: Do we want it like that?

rohnsha0 commented 3 weeks ago

Question now is: Do we want it like that?

@jensens I think there are two points to consider before making a decision about whether to allow "+" in email addresses:

  1. If people want to use disposable email addresses, then allowing "+" can be beneficial and provide ease of use.
  2. If "+" is allowed, it may increase spam, as users might create various spammy suffix email addresses (e.g., user+spam1@example.com, user+spam2@example.com).
stevepiercy commented 3 weeks ago

I would want to know what are the exact "problems in some parts of the UI" from using a plus sign. Unfortunately the old site for tickets is dead.

The last comment was by @mauritsvanrees at https://github.com/plone/Products.CMFPlone/blame/a180978c54f112d6eeaefeae44e7e5b6a46b46e7/Products/CMFPlone/tests/testEmailLogin.py#L82-L87

Previous to that, there's a mention of PLIP9214, and I think the author was @davisagli in fall of 2009, after I went spelunking in the mail list archive: https://github.com/plone/Products.CMFPlone/blame/25c25605a56548b09806f071798e7f8b79f049e5/Products/CMFPlone/tests/testEmailLogin.py#L59-L61

rohnsha0 commented 3 weeks ago

I doubt @mauritsvanrees would remember the cause of the comment made around 12 years from now!

jensens commented 3 weeks ago
  1. If people want to use disposable email addresses, then allowing "+" can be beneficial and provide ease of use.

The + is a valid delimiter in an email address. It is not about disposable email addresses, even if some services may use it for this purpose too. For details read RFC 5233 or the section about sub-addressing at Wikipedia: https://en.wikipedia.org/wiki/Email_address#Sub-addressing

If "+" is allowed, it may increase spam, as users might create various spammy suffix email addresses (e.g., user+spam1@example.com, user+spam2@example.com).

This can be used, true. We may want to modify our check for unique email-addresses to filter out dups by removing the sub-address. However, allowing free registration on a site has disadvantages and needs anti spam and moderation measures anyway. So, I do not see an increased problem level here.

rohnsha0 commented 3 weeks ago

The + is a valid delimiter in an email address. It is not about disposable email addresses, even if some services may use it for this purpose too. For details read RFC 5233 or the section about sub-addressing at Wikipedia: https://en.wikipedia.org/wiki/Email_address#Sub-addressing

I meant the same thing, though, but I used the wrong word. It would be sub-addressing (but not disposable).

mauritsvanrees commented 3 weeks ago

I doubt @mauritsvanrees would remember the cause of the comment made around 12 years from now!

The comment was: "A plus sign in the id gives problems in some parts of the UI, so we do not allow it." I don't know what those problems would have been. I would say we can try it out: allow the plus sign, then create some users with a plus in their id/login/email, and see if it goes wrong anywhere: for example on the author page, users/groups panel, sharing page.

rohnsha0 commented 3 weeks ago

I would say we can try it out: allow the plus sign, then create some users with a plus in their id/login/email, and see if it goes wrong anywhere: for example on the author page, users/groups panel, sharing page.

can be done!