fix(#7787): Added userInsensitive for comparrison checks in dbAuth

[ageddesi]

For #7787

What it does:

Fixes an issue in the dbAuth user validation where the check is not case insensitive. This would result in duplicate users if they had different casing.

Approach

I have taken the approach we discussed in the ticket of adding an additional db line item.

Outstanding Questions

David in the Discord channel https://discord.com/channels/679514959968993311/1091099927041556661 has suggested an alternative to this approach (below), I am happy to explore this idea as well if we decide in this PR to do that.

Out of curiosity, why do you need a new field? Why can’t the insensitive rule be a config option and the query to find the user query for the user accordingly?

I also have an outstanding Q on my current implementation. I was wondering do I need to define a db migration for existing apps that use the dbAuth or does it only work for new installs of dbAuth. If I do can you point me to the correct place in the code please.

[cannikin]

Hmm, I don't love the idea of having an additional field in the database for this functionality.

What if we add a config option to api/src/functions/auth.js so that you can tell dbAuth to use an insensitive lookup, and it's on the user to know what database they have and whether this option is supported? Something like:

  const loginOptions = {
+   usernameMatch: 'insensitive',
    handler: (user) => {
      return user
    },
    errors: {
      usernameOrPasswordMissing: 'Both username and password are required',
      usernameNotFound: 'Username ${username} not found',
      incorrectPassword: 'Incorrect password for ${username}',
    },
    expires: 60 * 60 * 24 * 365 * 10,
  }

And we document which database support insensitive matching. If we really wanted to get fancy we could have a check in the dbAuth code that makes sure the actual database in use supports the option that they passed in and throw our own error if there's a mismatch.

[replay-io]

15 replays were recorded for 3e92455.

0 Failed

15 Passed

• useAuth hook, auth redirects checks
• Check that a specific blog post is prerendered
• Check that about is prerendered
• Check that homepage is prerendered
• Check that meta-tags are rendering the correct dynamic data
• Check that you can navigate from home page to specific blog post
• Waterfall prerendering (nested cells)
• RBAC: Admin user should be able to delete contacts
• RBAC: Should not be able to delete contact as non-admin user
• Smoke test with dev server
• Smoke test with rw serve
• Loads Cell mocks when Cell is nested in another story
• Loads Cell Stories
• Loads MDX Stories
• Mocks current user, and updates UI while dev server is running

View test run on Replay ↗︎

[ageddesi]

Hi @cannikin
I am happy to make the change to do this instead.

[ageddesi]

@cannikin Here is what I found about the differences for the db's

https://www.prisma.io/docs/concepts/components/prisma-client/case-sensitivity

• Postgres = 'insensitive' : Default : 'default'
• MySQL = 'case-insensitive' by default, so not required
• MongoDB = 'insensitive' : Default : 'default'
• SQLit = Not supported, individual columns need to be defined per column.
• Microsoft SQL Server provider = 'case-insensitive' by default, so not required

Shall I make it so that it only attempts to add that usernameMatch if you have one defined and ignore if null.

[cannikin]

Yes I like that idea: if you don't provide the option at all then it behaves just the way it does now, and only if you provide a usernameMatch option does it forward that value to Prisma. That keeps backwards compatibility and avoids having to wait for a major version revision to release this.

I suggested above that the code could actually compare the value for the option against the database you're using, and warn if the two aren't compatible (like SQLite not supporting an option at all) but that's probably overkill. I would say if the option is present, just give to Prisma, and let Prisma worry about throwing an error if it's invalid.

[arackley-tribal]

@cannikin Brilliant I will get on this. Might be just after the easter break now as got family round :)

[Tobbe]

Hmm, I don't love the idea of having an additional field in the database for this functionality.

I actually prefer the additional field. username and lowercaseUsername.

It works in all databases
No extra config for users to worry about
No need to transform the username on every call
It's simple and easy to understand

[ageddesi]

@cannikin merged in your changes. I never was good at spelling 👍

[ageddesi]

Hey @cannikin @Tobbe , I have not checked this works yet from hooking up to an instance of the app running as mentioned above. I will try take a look at this tonight just to make sure nothing is missing as I see you have merged it in. 👍

[Tobbe]

My bad! Trigger happy! 🔫 🤠
Let us know how it goes

[ageddesi]

Hey @Tobbe #8045
Here is a PR with some final changes after I got the app running locally to check against.