riverrun / phauxth

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

Phauxth 1.2, Bad error message for password reset #87

Closed guidotripaldi closed 5 years ago

guidotripaldi commented 5 years ago

If the user try to reset the password clicking again on the same link that was mailed to him for resetting the password after the link was already used for successfully resetting his password, the error message displayed is "Invalid Credentials", that is misinformative. The same when he click the link for the first time, but after the max_age was expired.

This because in Phauxth.Confirm.Report the case is managed by this function

  def report(%{reset_sent_at: nil}, :pass_reset, meta) do
    Log.warn(%Log{message: "no reset token found", meta: meta})
    {:error, Config.user_messages().default_error()}
  end

that call the generic Config.user_messages().default_error() that issue the "Invalid Credential" string.

I think that instead should be better to call something like {:error, Config.user_messages().invalid_token()}, defined as:

defmodule Phauxth.UserMessages.Base do

  @callback invalid_token() :: String.t()
  ...

  defmacro __using__(_) do
    quote do
      @behaviour Phauxth.UserMessages.Base

      ...
      def invalid_token, do: "Invalid token"

      defoverridable Phauxth.UserMessages.Base
    end
  end
end

defmodule Phauxth.UserMessages do
  use Phauxth.UserMessages.Base
end

If you agree with this solution, I can submit a PR

riverrun commented 5 years ago

My only concern with this change is that we will reveal too much information if we are using "invalid token" for invalid tokens and "invalid credentials" when there is no user.

Let me think about this, and then I will get back to you soon.

guidotripaldi commented 5 years ago

Yes I understand you concern. The problem is that in production, when there are thousand users that forget their password, giving them support is a big problem if the error messages are not enough informative.

I think it would be better if we could have completely distinct messages for every single different error conditions, and let the developers choose the level of transparency to give externally to the public, having always all the information to debug users actions and errors on the private side.

This way they could, for example, make a page reserved to the customer support with the exact informations regarding the errors, while returning to the user a more generic error.

riverrun commented 5 years ago

Could you send me a PR for this? I think we can handle the concerns I have by updating the documentation - but we can do that later.

riverrun commented 5 years ago

Fixed in v1.2 and v2.0.0