Case insensitive signup

[acinader]

todo:

• check if any consideration for linked account needs to be addressed
• ensure proper indexes for email and username at server start
• configurability
• document

fixes: #3990

[codecov]

Codecov Report

Merging #5634 into master will increase coverage by 67.27%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master    #5634       +/-   ##
===========================================
+ Coverage   16.25%   83.53%   +67.27%     
===========================================
  Files         166      169        +3     
  Lines       11632    11734      +102     
===========================================
+ Hits         1891     9802     +7911     
+ Misses       9741     1932     -7809

Impacted Files	Coverage Δ
src/Adapters/Storage/Mongo/MongoCollection.js	97.61% <100%> (+92.85%)	⬆️
src/Adapters/Files/GridStoreAdapter.js	46.37% <0%> (ø)
src/cli/definitions/parse-live-query-server.js	100% <0%> (ø)
src/LiveQuery/SessionTokenCache.js	86.95% <0%> (ø)
src/Controllers/index.js	96.66% <0%> (+1.11%)	⬆️
src/logger.js	100% <0%> (+9.09%)	⬆️
src/index.js	100% <0%> (+9.52%)	⬆️
src/Controllers/AdaptableController.js	96% <0%> (+16%)	⬆️
src/Adapters/Logger/WinstonLogger.js	100% <0%> (+19.14%)	⬆️
src/Adapters/MessageQueue/EventEmitterMQ.js	95% <0%> (+20%)	⬆️
... and 139 more

---

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 d5deca8...8227dca. Read the comment docs.

[acinader]

Now with a quick first pass.

1. I'll take a look at the query plan difference and post my findings here
2. I assume that logging in with facebook, twitter, etc likely has the same issue as anonymous. How to solve for that.
3. I have not yet, in any way contemplated the implications of including _auth_data_anonymous in specialQuerykeys

[acinader]

@dplewis

1. Any idea why postgress tests are failing? Probably this? https://github.com/parse-community/parse-server/pull/5634/files#diff-3646535e0655c134862bb00401c8097cR624

2. Any ideas on how I could systematically skip linked accounts in the username validation? The index on the username will still prevent any duplicates, so it's safe to ignore for this check.

FYI, to anyone following this, it is my goal to allow mixed case username's, but not to allow two usernames that would be equivalent in a case insensitive comparison.

[dplewis]

@acinader I should have sometime this week to look.

[acinader]

i should probably take care of email too....

[acinader]

@dplewis @davimacedo Hi Guys. I'd like to get this resolved.

@davimacedo any interest in reviewing this? Any questions?

@dplewis I could use your help on two things:

1. I think we can build the exclusions for registered auth adapters dynamically for the RestWrite username query. You've been in the auth adapter stuff, so I thought I'd bounce off you before I try and figure it out...

2. Can you look at the prostgres failed test? I don't run postgres and would really like to avoid it (i'd be more inclined to drop postgres support if maintaining it falls on me :)...especially cause there are already two languishing pr's for postgres (maybe there's someone we should be adding as a mainter on that front???))

[acinader]

@aprato i think i've addressed your concerns too.

[davimacedo]

@acinader The PR looks good to me for what it aims to do, but I have two questions:

1. I can't sign up davimacedo and DAVIMACEDO, but if I sign up DAVIMACEDO, I can't sign in davimacedo. It would be perhaps another issue but I think that sign in and sign up should follow the same rule.
2. I'm little bit afraid of the regex search that can degenerate the performance on a big users table. I was looking at Username/email uniqueness is case-sensitive #3990: don't you think that lowercasing everything when reading/writing would be better (it would also solve question 1)?

[davimacedo]

I've just found out why Postgres is failing. You are searching for _auth_data_anonymous and it does not exist in the User class (it works well for Mongo but does not work for Postgres). You just need to add the following line of code right after the line https://github.com/parse-community/parse-server/blob/master/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js#L965:

fields._auth_data_anonymous = { type: 'String' }; // Is it a String? I am not sure

[acinader]

• On your point 1: Case insensitive login

I'm starting with the assumption that if a user puts in a username of 'TomWFox', we need to preserve the case and cannot simply lowercase it on save.

I'd also want to check how Google and GitHub handle it. i.e. can @TomWFox login to GitHub with tomwfox? @TomWFox can you check and let us know?

In any event, I view that a separate, but related issue that is not exacerbated by fixing case sensitive sign up.

• On your point 2: After looking at it more closely, you are right and this solution is not acceptable from a performance standpoint. I will look into a solution using a case insensitive index on username.

• Another concern I haven't adequately addressed yet is how to exclude Auth Adapter created usernames from being rejected if they fail the case insensitive test when they are actually different when tested with case sensitive.

I'm hoping that I can get some guidance or a pointer to an existing test from @dplewis on how to cover the AuthAdapter cases for anonymous and things like facebook.

Addressing the AuthAdapter is also blocker.

...back to drawing board.

[acinader]

@davimacedo do you use postgress, or do you just use for testing with parse?

[TomWFox]

@acinader I can login as tomwfox.

[davimacedo]

@acinader

• I use Postgres only for testing the PRs.
• Most of web-sites I see:
  -- allow an account like DaviMacedo (and then show the acocunt like this)
  -- Does not allow a second account like davimacedo (case insensitive sign up)
  -- Allow to sign in as davimacedo (case insensitive sign in)
• What I think we could do:
  -- Store both the original username and the lowercase version of it
  -- Create two additional options to the Parse Server (enableInsensitiveSignUp and enableInsensitiveSignIn)

[acinader]

@davimacedo

• I agree 100% with your requirements:
  -- allow an account like DaviMacedo (and then show the acocunt like this)
  -- Does not allow a second account like davimacedo (case insensitive sign up)
  -- Allow to sign in as davimacedo (case insensitive sign in)

• I have an alternate design implementation from your suggestion that you can see the start of with bbc1d73 which I think proves out that it is doable in a performant way. I verified that with the proper index, the index is used and the key inspection is limited to the range of insensitive options.

db.getCollection('_User').createIndex({ username: 1 }, { collation: { strength: 2, locale: 'en_US' }, name: 'username_insensitive', background: true});

• My current design proposal:

  • At startup ensure a case insensitive index on the two fields
  • at signup verify with a case insensitive query if enableInsensitiveSignUp is true
  • at login find the user with a case insensitive query if enableInsensitiveSignIn is true
  • ensure that we don't interfere with auth providers

• Argument for case insensitive index vs. lower case field:

  1. We don't have a mechanism to apply migrations so there's no way for us to 'ensure' that existing installs will create the 'usernameLower' field or to lowercase the email addresses.

  2. An international user with a better understanding of collation for non-latin alphabets will be able to easily parameterize the collation variables.

I've only looked into collation in Postgres casually so far ( i did install it and am looking at how to migrate one of our large mongo db's into it so I can get real query statistics.

Unfortunately, it'll be a few days before I can turn back to this issue. Let me know what you think if you have the chance.

[davimacedo]

@acinader I love the approach you chose. Let me know if you need any help.

[acinader]

@dplewis I think from mongo perspective, this is ready. I'm confused about the postgres.

@TomWFox any thought on what, if any, additional documentation we would want for this beyond the bug and the changelog?

[TomWFox]

@acinader from what I can tell this adds behaviour that I think most devs would expect based on other platforms.

I can have a look to see if there are any points in the docs that would need changing based on this new behaviour but I would say the most important thing is making the change clear to existing devs in the changelog.

i think i've addressed

[acinader]

@dplewis i think this is ready for your review

[dplewis]

@acinader Everything looks good. Can you check and see what this does and should it be case insensitive?

parse-server/src/RestWrite.js

Line 1575 in 3ec5995

return this.config.database

[acinader]

Nice catch! I’ll address. Thanks

[acinader]

@dplewis I looked at that section of code and I have no idea how one would even get there!

I looked at git history for a bit and it looks to me that the code section you highlighted came about a year before we added unique constraints on username and password.

While it's possible that we should rip out the section, I'd don't want to do that as it really isn't part of this change I am trying to make.

My changes only affect username and email address validation. When checking validation (which isn't done for anonymous users, for example), the check is more strict, prohibiting both exact matches as well as case insensitive matches, but the database itself has to permit all but an exact match to handle the corner case of anonymous users.

In reviewing what you pointed out, I did want to make extra sure that I am not breaking anonymous users in the rare case that there is a case insensitive collision which I added with 201839a.

I don't know what to do about Postgres, but I think the mongo implementation here is solid.

[dplewis]

I honestly was thinking the same thing. I have no idea why its there or can we reach it. Maybe there is a test that runs against it that can reach it?

The changes look good to me!

[acinader]

no test: https://codecov.io/gh/parse-community/parse-server/compare/201839a6a67541bd251adf14730dbc49067f30f7...201839a6a67541bd251adf14730dbc49067f30f7/src/src/RestWrite.js#L1575

that was a good question!

interesting to see the stuff that isn't covered.

I'll merge this in tomorrow. I can either cook up a releasee tomorrow or wait a week as I'll be away from the keyboard next week.

thanks!

[oallouch]

Hi guys,

Any link with #4883 ?

:)

[acinader]

Hi @oallouch

I didn't implement it in a generic way

but you could look at the code changes i made here if you wanted to do it and i'd be willing to help you with it if i can find the time.

[rdhelms]

@acinader I'm having a bit of trouble understanding the end result here - did the new flags enableInsensitiveSignIn and enableInsensitiveSignUp that were discussed end up being implemented? Should we expect that a user with username Bob@mail.com should be able to login using bob@mail.com?

[acinader]

Hi @rdhelms

1. No, I did not implement the flags
2. This pull request really only did one thing: prevent successful registration of users with a case insensitive collision of username or email. Nothing else.

You should not expect a user with email Bob@mail.com to be able to login as bob@mail.com

That, of course, would be good and a logical next step from this pull request.

[rdhelms]

Thanks for the clarification @acinader 👍🏻