riverrun / phauxth

Not actively maintained - Authentication library for Phoenix, and other Plug-based, web applications
409 stars 20 forks source link

[BUG] Argon 2 error - function depreciation leads to 500 #105

Closed dsignr closed 5 years ago

dsignr commented 5 years ago

Environment

Phauxth 2.1, Argon 2.0

Current behavior

Getting this error from inside my User model, specifically at the put_pass_hash function and throws out a 500 server error.

** (exit) an exception was raised:
    ** (FunctionClauseError) no function clause matching in Ecto.Changeset.change/2
        (ecto) lib/ecto/changeset.ex:322: Ecto.Changeset.change(#Ecto.Changeset<action: nil, changes: %{email: "hello@example.com", name: "Example", password: "12345678", phone: "12345678"}, errors: [], data: #MyApp.IAM.User<>, valid?: true>, :ok)
        (exampleApp) lib/example_web/graphql/resolvers/IAM/user.ex:20: ExampleWeb.GraphQL.Resolvers.IAM.User.create/2

On inspection, there is a warning right before the 500 error:

warning: Comeonin.Argon2.add_hash has been removed.
Add {:argon2_elixir, "~> 2.0"} to the deps in your mix.exs file,
and use Argon2.add_hash instead.

My user.ex:

def create_changeset(%__MODULE__{} = user, attrs) do
    user
    |> cast(attrs, [:email, :password])
    |> validate_required([:email, :password])
    |> unique_email
    |> validate_password(:password)
    |> put_pass_hash  # <--- this is where the error seems to come from
end

Expected behavior

There should be no errors and warnings.

Additional information

This is happening solely due to the put_pass_hash function in the user model. Downgrading to Phauxth 2.0 and Argon 1.2 is the current solution.

riverrun commented 5 years ago

Version 2.1 of Phauxth is now using version 5 of Comeonin, and the main change, as far as Phauxth is concerned, is that Comeonin is now not called directly.

The changes you will need to make are:

I added a note about this to the Gitter channel and updated the UPGRADE_v2 guide, but it's not always easy to let everyone know when there is a change.

dsignr commented 5 years ago

Thanks David, I missed that. Probably because I was already on 2.0.0 so, must have totally missed it. Ok, I fixed as you have mentioned and now I'm being hit with this:

Request: POST /api/core
** (exit) an exception was raised:
    ** (FunctionClauseError) no function clause matching in MyAppWeb.Auth.Login.report/2
        (myapp) lib/myapp_web/auth/login.ex:7: MyAppWeb.Auth.Login.report(:ok, [])

Inside my auth/login.ex:

defmodule MyAppWeb.Auth.Login do
  @moduledoc """
  Custom login module that checks if the user is confirmed before
  allowing the user to log in.
  """

  use Phauxth.Login.Base

  alias Comeonin.Argon2
  alias MyApp.IAM

  @impl true
  def authenticate(%{"password" => password} = params, _, opts) do
    case IAM.get_by(params) do
      nil -> {:error, "no user found"}
      %{confirmed_at: nil} -> {:error, "account unconfirmed"}
      user -> Argon2.check_pass(user, password, opts)
    end
  end
end

Seems like a struct mismatch within the report function. I can probably override it, but not sure it is a bug..

dsignr commented 5 years ago

Ok, I found out the issue. In this callback:

@impl true
 def authenticate(%{"password" => password} = params, _, opts) do
    case IAM.get_by(params) do
      nil -> {:error, "no user found"}
      %{confirmed_at: nil} -> {:error, "account unconfirmed"}
      user -> Argon2.check_pass(user, password, opts)
   end
end 

The success case returns just :ok, instead of the tuple {:ok, user} that the report function is expecting.

@impl true
 def authenticate(%{"password" => password} = params, _, opts) do
    case IAM.get_by(params) do
      nil -> {:error, "no user found"}
      %{confirmed_at: nil} -> {:error, "account unconfirmed"}
      user -> {Argon2.check_pass(user, password, opts), user} # <---  Changing it to this solved it
   end
end

Since it's a callback implementation, I don't think it's a bug per se, but then again, I'm upgrading from Phauxth 2.0.0 to 2.1.0, so my implementation hasn't changed, but seems like, as you have mentioned, the Argon function has changed.

riverrun commented 5 years ago

You need to remove the alias Comeonin.Argon2 line

riverrun commented 5 years ago

This commit shows the changes that I made to the example app to upgrade it to 2.1

dsignr commented 5 years ago

You need to remove the alias Comeonin.Argon2 line

That was it! I can confirm this was the issue. Thanks David. Should I close this issue now?

riverrun commented 5 years ago

Glad to hear it's now fixed. I will close this issue now. I have made a few changes to the UPGRADE guide to make things a little clearer.

dsignr commented 5 years ago

Thank you David!