aaronrenner / phx_gen_auth

An authentication system generator for Phoenix 1.5 applications.
774 stars 56 forks source link

Case sensitiveness #89

Closed ruudk closed 3 years ago

ruudk commented 3 years ago

First of all, thanks for this amazing generator. Great work 🙌

I was wondering why the case sensitiveness is handed off to the database layer instead of handling this in the application. In my opinion it would be better to always lowercase the username (email) before persisting it in the database. The same should be done every time the database is queried. First lowercase in the application and then send it to the database.

The reason why I ask is because on MySQL this will pass:

    test "returns the user with accent" do
      %{id: id} = user = user_fixture(email: "rúüd@example.com")
      assert %User{id: ^id} = Accounts.get_user_by_email("ruud@example.com")

      %{id: id} = user = user_fixture(email: "ruud@example.com")
      assert %User{id: ^id} = Accounts.get_user_by_email("rúüd@example.com")
    end

While on PostgreSQL it will not pass:

  1) test get_user_by_email/1 returns the user with accent (PostgresBinaryId.AccountsTest)
     test/postgres_binary_id/accounts_test.exs:19
     match (=) failed
     The following variables were pinned:
       id = "f658e0f9-0d84-4287-bd4e-f52fb7e4152e"
     code:  assert %User{id: ^id} = Accounts.get_user_by_email("ruud@example.com")
     left:  %PostgresBinaryId.Accounts.User{id: ^id}
     right: nil
     stacktrace:
       test/postgres_binary_id/accounts_test.exs:21: (test)

The issue on MySQL is with the _ci collation. It sees accent characters like ú the same as u. But it will store the accent char.

I think it would be safer if the email column was stored in _bin.

For PostgreSQL I don't really understand why a citext extension is installed for this purpose. If you go through all this trouble, why not simply do in the app?

"RÚüd@example.com" |> String.downcase()
"rúüd@example.com"
josevalim commented 3 years ago

Sorry but I don't see how this is related to downcasing. Even if we downcased it in the app, the database will handle collation differently, which means you will still have a difference in behaviour.

For PostgreSQL I don't really understand why a citext extension is installed for this purpose. If you go through all this trouble, why not simply do in the app?

Because it is much easier to do it at the database level. It is two lines of code. If you do it in the app, you need to downcase when you insert, when you run queries, etc... probably by implementing a custom type.

josevalim commented 3 years ago

Oh, and thank you for the nice words and the discussion! :heart:

josevalim commented 3 years ago

Closing this as it has stalled.