fix: fault tolerant email login and user creation

[dieselftw]

Proposed changes (including videos or screenshots)

This PR introduces fault tolerance to email fields while logging in and creating users (as administrator).

2024-01-08.23-02-29.mp4

Issue(s)

fixes #31397

Steps to test or reproduce

Admin mistake

1. Go to /admin/users/new
2. Create a new account with the E-Mail set to " test@example.com" or "test@example.com ".
3. Logout.
4. Try to login using "test@example.com" afterwards.

User mistake

1. Go to /admin/users/new
2. Create a new account with the E-Mail set to "test@example.com".
   Logout.
3. Try to login using " test@example.com" or "test@example.com " afterwards.

Further comments

Bonus documentation grammar fix :)
I feel like there should be some unit tests for this? Let me know.

[changeset-bot]

⚠️ No Changeset found

Latest commit: cc9d19b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[hugocostadev]

Would be better to do this on the API level, since both endpoints can be used with an API

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.58%. Comparing base (acd9178) to head (cc9d19b).
Report is 2041 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #31399      +/-   ##
===========================================
+ Coverage    54.52%   54.58%   +0.05%     
===========================================
  Files         2290     2286       -4     
  Lines        50315    50249      -66     
  Branches     10271    10258      -13     
===========================================
- Hits         27435    27427       -8     
+ Misses       20385    20336      -49     
+ Partials      2495     2486       -9     

Flag	Coverage Δ
e2e	43.67% <ø> (-9.83%)	⬇️
e2e-api	21.31% <ø> (-18.67%)	⬇️
unit	71.34% <ø> (-5.12%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[hugocostadev]

Hey @hardikbhatia777 thanks a lot for your contribution!

Another contributor did also made a contribution that's is better and fix around others endpoints/frontend with a simple solution, so we are going to keep with his solution

#31400 (review)

[dieselftw]

Hey @hardikbhatia777 thanks a lot for your contribution!

Another contributor did also made a contribution that's is better and fix around others endpoints/frontend with a simple solution, so we are going to keep with his solution

#31400 (review)

@hugocostadev I respect your decision but I'm a bit confused. The issue specifically asked for fault tolerance in the login field, like Google has (it doesn't give an error if you have white spaces, instead it trims it down for you). This is a quality-of-life feature, where you won't have to worry about copying a few extra white spaces along with the email address.

Modifying the regex isn't fault tolerant and not good for the end-user since they'll have to spend an extra few seconds every time to delete the white spaces and be extra careful while copying.

The API endpoints can be adjusted to support trimming as well.

This is just a suggestion of course, I do still respect your decision! Thanks.

[david-uhlig]

I agree with @hardikbhatia777. Raising an error creates unnecessary overhead. The user might not even realize their mistake because enclosing white spaces are not clearly visible. The resulting error message is not helpful ("Invalid e-mail address"). The issue can easily happen on mobile devices with an auto-completion app that automatically adds a white space after words.

I would prefer a QoL solution that gracefully trims enclosing white space without letting the user know.

[hugocostadev]

Thanks for both feedbacks @hardikbhatia777 and @david-uhlig , I'm going to check internally and get back to you guys... We can reopen this if necessary. Thanks

[hugocostadev]

Hey guys, so...

We would suggest doing this in the backend, the methods that uses email ...

In this file apps/meteor/app/api/server/v1/users.ts there are several endpoints that could be trimmed.
Another important file is: apps/meteor/app/2fa/client/TOTPPassword.js

Check another files if uses something with email too...

Would be very nice to add API tests for it too, if possible.

Thanks again for your feedback, this makes a strong community 🚀

[dieselftw]

@hugocostadev I've made the necessary changes in all the files I could find the email field in. I'm unsure about how to write API tests though (I have written basic unit tests before, but I'm unsure of how to do it here). I'd appreciate your help in that regard :) Thanks!

[Gustrb]

@hardikbhatia777 you can add api tests by going into the tests file, in this case the path would be: apps/meteor/tests/end-to-end/api/01-users.js.
You can add a test in the [/users.create] describe block, just create a new user with an email having trailing spaces (as it is done in the should create a new user with custom fields test case. And then validate the e-mail that got back.
Remember to delete the user afterwards

[dieselftw]

@hardikbhatia777 you can add api tests by going into the tests file, in this case the path would be: apps/meteor/tests/end-to-end/api/01-users.js. You can add a test in the [/users.create] describe block, just create a new user with an email having trailing spaces (as it is done in the should create a new user with custom fields test case. And then validate the e-mail that got back. Remember to delete the user afterwards

@Gustrb Thanks a lot for your guidance. Another issue was posted by the same person who posted this issue. I want to draw your attention there since it's very much relevant and related to this issue. It has to do with streamlining and normalizing the email parser for all endpoints. I'd appreciate if you could have the internal team look into it. #31518

Since this issue is almost fixed, I'll go ahead and add the tests so it's fixed and closed :)

Thanks!

[Gustrb]

@hardikbhatia777 yea sure, I will take a look into the issue with the team, thanks for the heads up 🚀

[dieselftw]

Hey @Gustrb, I've added an API test for this in the file that you mentioned. Can you please review the changes and let me know if everything is in order? Thanks!

[dieselftw]

Hey @Gustrb @hugocostadev would you mind following up on this please? Thanks!

[dieselftw]

PS. Please excuse the ugly commit history, I switched PCs and ended up messing up my fork. Apologies for that.