Bootstrap error class not working

[joepstender]

I use a Bootstrap error helper to display error messages in forms correctly:

  <div class="form-group">
    <%= label f, :email %>
    <%= text_input f, :email, class: add_error_class(f, :email, "form-control") %>
    <%= error_tag f, :email %>
  </div>

@doc """
  Appends Bootstrap error class to input if there's an error.
  """
  def add_error_class(form, field, class) do
    if Keyword.has_key?(form.errors, field) do
      class <> " is-invalid"
    else
      class
    end
  end

But somehow this won't work when I use a custom Session Controller and login form. I checked the generated changeset errors and they are exactly like the ones from a changeset in "normal" Phoenix forms.

#Ecto.Changeset<
  action: nil,
  changes: %{email: "test"},
  errors: [
    password_hash: {"can't be blank", [validation: :required]},
    password: {"can't be blank", [validation: :required]},
    email: {"has invalid format",
     [
       validator: &Pow.Ecto.Schema.Changeset.validate_email/1,
       reason: "invalid format"
     ]}
  ],
  data: #MyAppWeb.Accounts.User<>,
  valid?: false
>

Any suggestions how to fix this?

[danschultzer]

It’s because :action is nil, but for login the changeset errors doesn’t have any meaningful messages. A flash message is shown, with a generic error message per OWASP recommendation, and the changeset is only used to fill out the user id field with what was submitted.

[joepstender]

Ah ok, so it is "by design"? I would rather not have the user guessing what they did wrong but provide meaningful error messages. Is there a way to get around this?

[danschultzer]

Yeah, it's by design. What message would you like to show? The limitation with changeset errors is that it's used for inserting or updating the row, so it doesn't really make sense to rely on the errors for authentication since the data doesn't change.

Edit: A good example is the "invalid format" error for email in the changeset. This doesn't really make sense for auth, since the format requirement may have changed but you can still have an user that was created before the requirement changed. It's probably not right to use the changeset method in the first place though, it really should just put the params and nothing else.

[joepstender]

Well something like "The password doesn’t correspond to your email.", or even "that email is unknown". (Checking email availability could also be done by trying to register with said email, so we are not giving any secret away that we otherwise wouldn't). But I guess these would need other checks than those in the changeset.
The "invalid format" would still be useful when someone uses their user name instead of their email, I then assume the requirements won't change (also: can't think of a situation where the email format has to change?).

[danschultzer]

even "that email is unknown". (Checking email availability could also be done by trying to register with said email, so we are not giving any secret away that we otherwise wouldn't)

Yeah it has been discussed before in #238 (comment) when discussing how to prevent timing attacks. I think it's best to follow the best practices by OWASP. Another point is that registration can be disabled.

The password doesn’t correspond to your email. is great since it can mean that either e-mail or password was wrong. You can override the default message using a custom messages module:

defmodule MyAppWeb.Pow.Messages do
  use Pow.Phoenix.Messages
  use Pow.Extension.Phoenix.Messages,
    extensions: [...]

  import MyAppWeb.Gettext

  def invalid_credentials(_conn), do: gettext("The password doesn’t correspond to your email.")
end

Would this be sufficient? Remember to set messages_backend: MyAppWeb.Pow.Messages in config.

I guess these would need other checks than those in the changeset.

Yeah, this is what happens: https://github.com/danschultzer/pow/blob/v1.0.15/lib/pow/phoenix/controllers/session_controller.ex#L46-L51

  def respond_create({:error, conn}) do
    conn
    |> assign(:changeset, Plug.change_user(conn, conn.params["user"]))
    |> put_flash(:error, messages(conn).invalid_credentials(conn))
    |> render("new.html")
  end

So there isn't really anything to the changeset other than just the params being passed in. The controller only gets the information whether the combined credentials worked or not.

The "invalid format" would still be useful when someone uses their user name instead of their email

As above, the changeset isn't used for anything during auth. However, if it was used to verify e.g. correct format, then you'll have this issue if you change the format in the future. Users who previously have registered with a user id that doesn't conform to the new format would get an error message that isn't correct since their user does exist already.

can't think of a situation where the email format has to change?

Previously a RegExp email validation was used but changed to support unicode emails. It can be that a mistake is made for the validation which permit some invalid emails, and that gets patched in later release. It would potentially cause the above issue, though in both cases it's very unlikely.

[joepstender]

Great, thanks for the detailed answers!