Skip to content
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

[Bug?]: Standard dbAuth username is case sensitive causing inconsistencies #7787

Closed
1 task done
ched-dev opened this issue Mar 9, 2023 · 17 comments
Closed
1 task done
Assignees
Labels
bug/needs-info More information is needed for reproduction

Comments

@ched-dev
Copy link
Contributor

ched-dev commented Mar 9, 2023

What's not working?

Using the standard dbAuth strategy, username's are stored as they type in, causing possible duplicate accounts or failed logins because of mismatch.

Note: It is possible to implement some additional logic within my app to solve the issue for most cases. It will not be fixed for the duplicate user check which is handled by redwood - which could cause user to create duplicate users.

How do we reproduce the bug?

Using a redwoodjs project with standard dbAuth flow installed:

  • Sign up for an account with a case sensitive username (Ex: demoUser)
  • Log out, then log back in trying to use the username in all lowercase (Ex: demouser)
  • Error: Could not find account with matching username

What's your environment? (If it applies)

System:
    OS: macOS 12.0.1
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.13.2 - ../node
    Yarn: 3.2.0 - ../yarn
  Databases:
    SQLite: 3.36.0 - /usr/bin/sqlite3
  Browsers:
    Chrome: 110.0.5481.177
    Firefox: 110.0.1
    Safari: 15.1
  npmPackages:
    @redwoodjs/auth-dbauth-setup: 4.0.1 => 4.0.1
    @redwoodjs/core: ^4.0.1 => 4.0.1

Are you interested in working on this?

  • I'm interested in working on this
@ched-dev ched-dev added the bug/needs-info More information is needed for reproduction label Mar 9, 2023
@Tobbe
Copy link
Member

Tobbe commented Mar 14, 2023

usernames should probably be stored as the user types them in (i.e. case sensitive), but matched case insensitive, both for duplication checks and when logging in. @ched-dev if I can confirm that's the behavior we want, would you be interesting in trying to implement it?

@ched-dev
Copy link
Contributor Author

@Tobbe yes, I can work on this. I can probably put together a PR in the next couple weeks.

@Tobbe
Copy link
Member

Tobbe commented Mar 19, 2023

Perfect! @cannikin, what do you think?

@cannikin
Copy link
Member

usernames should probably be stored as the user types them in (i.e. case sensitive), but matched case insensitive

Yep, agree!

@Tobbe
Copy link
Member

Tobbe commented Mar 19, 2023

Great! @ched-dev please go ahead with that PR whenever you have time 🙂

@ched-dev
Copy link
Contributor Author

Looking into this and having some trouble.

If I try to add "case-insensitive" mode to the prisma query:

user = await this.dbAccessor.findUnique({
  where: {
    [this.options.authFields.username]: {
      equals: username,
      mode: 'insensitive'
    }
  },
})

I get an error when trying to run it:

Argument email: Got invalid value
{
equals: 'rob@redwoodjs.com',
mode: 'insensitive'
}
on prisma.findUniqueUser. Provided Json, expected String.

If I try to change the findUnique() to a findFirst() I get another error:

Unknown arg mode in where.email.mode for type StringFilter.

While prisma says it supports case-insensitive filtering, it doesn't seem to be working in this scenario.

Curious if you have any advice on how to proceed. I am a beginner prisma user, so not sure if I am approaching it wrong.

@ched-dev
Copy link
Contributor Author

ched-dev commented Mar 28, 2023

I've done some more digging and it seems the mode: 'insensitive' is only available if the database allows it since some databases are case-insensitive by default. So if I was to write code to handle this, I would have to write it based on the database being used. I believe I'm getting the errors now because I was using the default setup of SQLite.

Also, mode: 'insensitive' is only available on .findFirst() so I'd have to switch it from .findUnique(). Will that cause a noticeable performance hit?

@ageddesi
Copy link
Contributor

ageddesi commented Mar 29, 2023

As you say this depends on what db the user has chosen and its best to avoid logic based on different dbs.
As such, I think we should consider adding another prop to the User Db Object eg 'usernameInsensitive'
Then when we are doing the check-in '_createUser' we can do it on the 'usernameInsensitive' instead.

@thedavidprice
Copy link
Contributor

FYI that @Tobbe and @cannikin are traveling this week. Give them (me) a nudge if there's no reply by early next week.

@ched-dev
Copy link
Contributor Author

I will be AFK (away from keyboard) through Apr 6 too. @ageddesi if you have time try out a fix, feel free. otherwise, I will look at it again when I get back.

@ageddesi
Copy link
Contributor

@ched-dev Hey, I will try to take a look before your back and let you know if I get somewhere.

@ageddesi
Copy link
Contributor

@ched-dev @thedavidprice
I managed to get some time to look into the code changes, which I have done here ageddesi#1
I need to just test this in a locally running project. The tests all pass.
Let me know if you think I am missing anything.
Once I have confirmed that I will point the PR to this branch.

@thedavidprice
Copy link
Contributor

@ageddesi Please open the PR! That's the best context for review and I can make sure the right people are looped in.

Huge thanks 🙏

cannikin added a commit to ageddesi/redwood that referenced this issue Apr 4, 2023
ageddesi added a commit to ageddesi/redwood that referenced this issue Apr 5, 2023
ageddesi added a commit to ageddesi/redwood that referenced this issue Apr 6, 2023
ageddesi added a commit to ageddesi/redwood that referenced this issue Apr 6, 2023
ageddesi added a commit to ageddesi/redwood that referenced this issue Apr 10, 2023
ageddesi added a commit to ageddesi/redwood that referenced this issue Apr 10, 2023
ageddesi added a commit to ageddesi/redwood that referenced this issue Apr 10, 2023
ageddesi added a commit to ageddesi/redwood that referenced this issue Apr 10, 2023
cannikin added a commit to ageddesi/redwood that referenced this issue Apr 10, 2023
ageddesi added a commit to ageddesi/redwood that referenced this issue Apr 11, 2023
cannikin added a commit to ageddesi/redwood that referenced this issue Apr 11, 2023
Tobbe added a commit to ageddesi/redwood that referenced this issue Apr 11, 2023
Tobbe added a commit that referenced this issue Apr 12, 2023
…th (#7979)

* fix(): Added userInsensitive for comparrison checks in dbAuth

* Revert "fix(): Added userInsensitive for comparrison checks in dbAuth"

This reverts commit 9ad34c7.

* #7787 - Added new usernameMatch to SignupFlowOptions for case insensitive check on db

* feat(#7787) - Added new usernameMatch to SignupFlowOptions for case insensitive check on db

* feature(#7787) - Fixed failing unit test

* feature(#7787) - Fix linting issues

* docs(#7787) - Added supporting documentation

* docs: correct Spellings in docs/docs/auth/dbauth.md

Co-authored-by: Rob Cameron <cannikin@fastmail.com>

---------

Co-authored-by: Rob Cameron <cannikin@fastmail.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
ageddesi added a commit to ageddesi/redwood that referenced this issue Apr 13, 2023
ageddesi added a commit to ageddesi/redwood that referenced this issue Apr 13, 2023
ageddesi added a commit to ageddesi/redwood that referenced this issue Apr 13, 2023
ageddesi added a commit to ageddesi/redwood that referenced this issue Apr 18, 2023
cannikin added a commit that referenced this issue Apr 18, 2023
… signups (#8045)

* fix(): Added userInsensitive for comparrison checks in dbAuth

* Revert "fix(): Added userInsensitive for comparrison checks in dbAuth"

This reverts commit 9ad34c7.

* #7787 - Added new usernameMatch to SignupFlowOptions for case insensitive check on db

* feat(#7787) - Added new usernameMatch to SignupFlowOptions for case insensitive check on db

* feature(#7787) - Fixed failing unit test

* feature(#7787) - Fix linting issues

* docs(#7787) - Added supporting documentation

* docs: correct Spellings in docs/docs/auth/dbauth.md

Co-authored-by: Rob Cameron <cannikin@fastmail.com>

* doc(#7787) - Fix spelling mistake in documentation

* fix(#7787) - updated check to findFirst in order to use mode checking

* fix(#7877): Updated tests for dbAuthHandler

---------

Co-authored-by: Rob Cameron <cannikin@fastmail.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@dennemark
Copy link
Contributor

Hi,
the PR by @ageddesi applies case insensitivity check on signup flow, but not on login. Is it possible to add it easily to login, too?

@ageddesi
Copy link
Contributor

This definitely could be a factor as well. If someone assigns a ticket to me i can add this as i know that area now.

@cannikin
Copy link
Member

@dennemark Can you create an issue for this and I'll assign to @ageddesi !

@Tobbe
Copy link
Member

Tobbe commented Oct 16, 2024

Closed by #7979 and #8045

@Tobbe Tobbe closed this as completed Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/needs-info More information is needed for reproduction
Projects
None yet
Development

No branches or pull requests

6 participants