Skip to content

Case insensitive login & request password reset #7150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

vince1995
Copy link
Contributor

New Pull Request Checklist

Issue Description

Adds the possibility to login case insensitive with username / email and request password reset.

This is my first PR that changes the source code of parse-server. I hope that everything is correct, especially the tests.
Closes #7143.

TODOs before merging

  • Add test cases
  • Add entry to changelog
  • Add changes to documentation (guides, repository pages, in-code descriptions)

Sorry, something went wrong.

case insensitive login/verify password
case insensitive send password reset email
added tests
@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #7150 (376d4df) into master (f846dea) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7150   +/-   ##
=======================================
  Coverage   93.90%   93.91%           
=======================================
  Files         169      169           
  Lines       12535    12547   +12     
=======================================
+ Hits        11771    11783   +12     
  Misses        764      764           
Impacted Files Coverage Δ
src/Options/Definitions.js 100.00% <ø> (ø)
src/Options/index.js 100.00% <ø> (ø)
src/Routers/UsersRouter.js 95.62% <ø> (+1.25%) ⬆️
src/AccountLockout.js 97.56% <100.00%> (+0.19%) ⬆️
src/Config.js 91.22% <100.00%> (+0.26%) ⬆️
src/Controllers/UserController.js 95.27% <100.00%> (+0.15%) ⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.84% <0.00%> (-0.68%) ⬇️
src/RestWrite.js 93.51% <0.00%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f846dea...99bbc74. Read the comment docs.

@mtrezza mtrezza mentioned this pull request Feb 1, 2021
3 tasks
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work with the test cases, just some minor comments.

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2021

Any changes necessary for case insensitivity on email verification?

@vince1995
Copy link
Contributor Author

vince1995 commented Feb 1, 2021

Any changes necessary for case insensitivity on email verification?

I don't think so because the verification calls the _authenticateUserFromRequest function which is covered by the login test. And even without that test it's not necessary because the verification link contains the email that the user entered (case sensitive).

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2021

And even without that test it's not necessary because the verification link contains the email that the user entered (case sensitive).

There may be manipulation by the developer in between, for example when modifying the email verification link or parsing query parameters from the URL. A developer could also provide a custom process for resending the email verification for an expired token, for example require the user to re-enter their email address in a form, hence this is not a closed process. Do you think it makes sense to add a test case for case insensitive email verification and password reset where the casing is changed between the casing in the email link and the casing that is fed back to Parse Server?

@vince1995
Copy link
Contributor Author

  1. The email verification link can be modified? The parse-server validates immediately the email on click on the link and redirects to the success page. So I don't see there a scenario where this could fail.

  2. Ok, the verificationEmailRequest function needs to be modified and get a test case.

@mtrezza
Copy link
Member

mtrezza commented Feb 1, 2021

The email verification link can be modified?

Yes, at link generation before the email is sent, for example to append a custom query parameter.

For expired verification tokens, a developer could require the user to enter the email address on a website for additional validation.

Anyways, the general point is that case insensitivity should ideally apply to all routes for consistency.

@vince1995
Copy link
Contributor Author

You're right. I think my solution is the wrong approach. If you merge that, some queries are case insensitive, and another queries are not. Like, if you want to do a normal find query on the username.

My suggestion is now that all queries on the username/email are adjusted by default. It's really easy for postgres, the only line that has to be adjusted is this one(?).

For mongo it's not so easy. The transformQueryKeyValue function has to be edited and something like

case 'username':
      return {
        key: "username",
        value: {
          $regex: new RegExp("^" + value + "$", "i") // The value has to be escaped from regex
        }
      }

could be added. But that would slow down the database, a text search would be better. But that requires a new index.

My suggestion is now to close this PR, I think your skills are higher than mine and you won't need so much time to write that as me.

@mtrezza
Copy link
Member

mtrezza commented Feb 2, 2021

If you like let's go through to queries one by one and see if/how they need to be adapted. I think your PR is actually close to what we need. If it covers 100% of user facing endpoints it should be fine. And if it covers 80% of developer facing endpoints and the rest is for the developer to be mindful about, that would also be fine I think.

@vince1995 vince1995 closed this Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

case insensitive login
2 participants