CGUC / skybunk-server

The back-end application for Conrad Grebel University College students to stay connected
3 stars 9 forks source link

Case insensitive usernames #78

Closed mipollex closed 5 years ago

mipollex commented 5 years ago

This is a tiny one-line fix that makes username matching case insensitive.

As a side note: Should the username have some format validation? I've seen a few interesting usernames that use whitespace and emojis.

mipollex commented 5 years ago

In order to update the existing db entries run the below script in a mongo terminal:

db.users.find().forEach(
  function(e){
    db.users.update({_id: e._id}, {$set: {username: e.username.toLowerCase()}})
})
scholvat commented 5 years ago

@mipollex, in response to your command to update the existing db, I believe that works fine, but using the db.users.updateMany would be a cleaner solution.

picklechips commented 5 years ago

I'm not entirely convinced this works, have you tested it? Adding the lowercase field just converts the username to lowercase before saving, but there's nothing here which converts the inputted username to lowercase also. Are you missing a toLowerCase() on the inputted username?

mipollex commented 5 years ago

This does indeed work, which surprised me as well when I first tried it. I think that Mongo is internally converting all searches to lowercase.

scholvat commented 5 years ago

From the mongoose docs, lowercase: true configures it to call .tolowercase() on all transactions.

On Sun, Feb 10, 2019 at 4:33 PM mipollex notifications@github.com wrote:

This does indeed work, which surprised me as well when I first tried it. I think that Mongo is internally converting all searches to lowercase.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/CGUC/skybunk-server/pull/78#issuecomment-462176290, or mute the thread https://github.com/notifications/unsubscribe-auth/AJquduAv7pGQegKmp8R9q9qaYVVVJpDoks5vMJBHgaJpZM4afwNo .

picklechips commented 5 years ago

That's dope - lgtm.

Although unlikely, it may be worth verifying nobody has the same username with different casing first