Add case-sensitive email option for User model

[richardpringle]

Have not created a test file for this yet. @ritch could you review this please (if you get a chance)?

Fix #1150
Connect to #1150

[superkhau]

if (this.settings.caseSensitiveEmail)

[superkhau]

Actually, can't you just set this default to true somewhere before? ie. When configs are read in, etc.

[richardpringle]

I couldn't change the setting in a boot script if I set it to true in user.js. Maybe I'm just trying to set it in the wrong place? Do you have a line number you could refer me to?

[superkhau]

Added comments. Also, can you write a test that fails and then make it pass?

[richardpringle]

@superkhau, I actually had some issues with npm test. Who should I talk to about that?

[bajtos]

Please remove.

[bajtos]

I am not very comfortable with modifying the email values to achieve case-insensitive comparison. It may lead to confusing situations, e.g.

await User.create({ email: 'mbajtos@cz.ibm.com', password: 'pass' });
var list = await User.find({ where: { email: 'mbajtos@cz.ibm.com' });
// no user found

IMO, we should use case-insensitive query operator instead. IIRC it was you @superkhau who implemented it?

[ritch]

It may lead to confusing situations, e.g.

As long as this is clear in the documentation, I think its fine.

I want to preserve the casing of the user input for email!

.... said no one ever.

[superkhau]

IMO, we should use case-insensitive query operator instead. IIRC it was you @superkhau who implemented it?

Yeah, we have the regex operator, which can take a a /i flag for case-insensitive regexes. However, the impl for each connector varies and the feature may not be supported, but you'll get a warning if it doesn't.

[superkhau]

I actually had some issues with npm test. Who should I talk to about that?

Not sure who the repo expert is for this particular repo. @ritch?

[richardpringle]

@superkhau @bajtos, can you guys review my changes to this test file?

[bajtos]

It may lead to confusing situations, e.g.

As long as this is clear in the documentation, I think its fine.

Most people don't read documentation. I personally wouldn't even know where (in which part of the documentation) to look for this information.

I want to preserve the casing of the user input for email!

.... said no one ever.

I think this is subjective, but let's say you are right and people won't mind LoopBack sanitising the email address on input.

In that case I would like LoopBack to also sanitise email addresses in all queries, so that my example keeps working:

await User.create({ email: 'MBajtos@cz.ibm.com', password: 'pass' });
  // email is stored as "mbajtos@cz.ibm.com"
var list = await User.find({ where: { email: 'MBajtos@cz.ibm.com' });
  // the query is modified to email: "mbajtos@cz.ibm.com"

[bajtos]

Please use this form: if (err) return done(err). It produces stack traces pointing to the original error, as opposed to assert(!err) which points to this line in tests. Please apply this rule to all new tests you are adding in this patch (no need to change existing code).

[richardpringle]

I followed the same conventions at the other tests in that describe block. Should I replace assert(!err) in all cases then?

[bajtos]

I followed the same conventions at the other tests in that describe block.

It's an old convention that I would like us to eventually get rid of.

Should I replace assert(!err) in all cases then?

I don't mind either. However, if you decide to fix existing tests, then make it in a standalone commit. This will make future inspection (code archaeology) much easier.

[bajtos]

@richardpringle can you guys review my changes to this test file?

You are on the right track 👍 , I left few comments on what should be improved.

[superkhau]

This case is confusing. You're trying to log in with validCaseInsenstiveEmailCredentials and expect an error?

[bajtos]

Good catch, @superkhau. I think the name validCaseInsensitiveEmailCredentials is confusing. The variable contains credentials that are invalid by default but valid when case-insensitive check is made.

[richardpringle]

@slnode test please

[bajtos]

@richardpringle could you please add an access hook to convert email fields to lower case in queries too, as I proposed in #1804 (comment)?

[richardpringle]

test please

[bajtos]

@slnode test please

[richardpringle]

@bajtos

If you have ACLs enabled then GET /api/users will be rejected. Let's try Users.find().then(...) instead.

I'm not quite sure what you mean. Make another remote method?

[bajtos]

@slnode test please

[bajtos]

I'm not quite sure what you mean. Make another remote method?

Sorry for the confusion. Just check that when you disable caseSensitiveEmail and call User.find() (with no query), your access hook does not crash on accessing a property of undefined.

[richardpringle]

@bajtos, does this look good? Wasn't too sure about case description...

[richardpringle]

@elkorep, Could you please take a look at the failed checks here? Everything passes on my machine...

[bajtos]

@richardpringle LGTM 👍 , please squash the commits to a single one.

[elkorep]

@richardpringle I am using this as one of my investigations. Look here for some reports on failed checks https://github.com/strongloop-internal/scrum-loopback/issues/630

[richardpringle]

Thanks @elkorep, looks like most (if not all) of the issues are concerning the npm registry or the modules within. Is that accurate?

[elkorep]

@richardpringle it seems like the failures are caused by npm and the ones with Error status are being aborted for some reason from what I have investigated so far.

[bajtos]

@slnode test please

[bajtos]

@slnode test please

[bajtos]

I am going to ignore failing downstream builds and land this now.

[bajtos]

@richardpringle could you please work with @crandmck to get this new feature described in the documentation (docs or apidocs or both)?