Phx_gen_auth user creation

Phoenix Forum Questions / Help
,
1

I am going through the phx_gen_auth output in more details today…

… and another thing caught a bit of my attention:

  def create(conn, %{"user" => user_params}) do
    case Accounts.register_user(user_params) do
      {:ok, user} ->
        {:ok, _} =
          Accounts.deliver_user_confirmation_instructions(
            user, &Routes.user_confirmation_url(conn, :confirm, &1)
          )

        conn
        |> put_flash(:info, "User created successfully.")
        |> UserAuth.log_in_user(user)

      {:error, %Ecto.Changeset{} = changeset} ->
        render(conn, "new.html", changeset: changeset)
    end
  end

is an action in user_registration_controller.ex. It first creates (inserts) the user via Accounts.register_user() and later on creates plus inserts the “confirmation” token and delivers notification via Accounts.deliver_user_confirmation_instructions(). And here’s the doubt: I can easily imagine situation where inserting the token fails but the user is already inserted. Or the notification fails. And the user cannot be re-registered because the email is already taken, and so on.

So… shouldn’t these three be rather run transactionally?

2

In case of any of the unlikely scenarios happen: the token fails, the email fails or is simply lost, a new token and instruction can be requested at any time.

3

Noticed that too. The question is more of “why not wrap it in a transaction and have such scenarios covered?” Is there a reason other than simply “nobody felt a need to”?

As for requesting new token and instruction, there are two aspects that raise a bit of concern to my un-phoenix-trained eye too:

  • said requesting does not seem to require any authorisation (like after authentication/logging-in first for example)
  • upon requesting new token, all the previous ones remain stored in the database. Until the account is “confirmed” (removing of them is nicely transactional, BTW).

Doesn’t that (the latter especially) mean that even a third party can easily flood the database with tons of records, potentially leading to a DOS in an extreme case?

1 Like
4

Accounts.deliver_user_confirmation_instructions is unlikely to be transactional. Anything you do to “send” the instructions – short of inserting a job for later execution into the same db – will not comply with transactional guarantees anyways.

5

True that. Also, if that’s an e-mail, then it is later never guaranteed to be properly transmitted/received etc. I am rather about exceptions that may be risen upon invoking said function, when we’d immediately know that it was unsuccessful.

6

Yes, putting it in a transaction could be an improvement. But since the email delivery can fail anyway, we have safe retry mechanisms, and the token creation failing being extremely unlikely, so I don’t worry it.

About emailing new tokens, that’s on purpose, cause the token is encrypted, so we can’t recover old ones, and we don’t want to make them stale (as you can receive a late email first).

People flooding others inbox is a possibility. Not much on confirmation, cause it requires an unconfirmed account, but definitely on password recovery. A large app likely requires rate limiting, not only on those screens but also on login, but it has been out of scope so far. :slight_smile: Another idea is to require another information, which is app specific such as date of birth, before resetting the password.

That’s to say, your auditing has been on point. :slight_smile: Perhaps improvements for the future. If you do implement rate limiting, then consider writing a blog post, I am sure it will be helpful.

2 Likes
7

This isn’t really a point-to-be-made but rather sharing my experience (and blog post below) with phx_gen_auth's design decisions as they relate to my work with Metamorphic.

phx_gen_auth and rate limiting
I implemented a simple session-based rate limiting for log in based on Chris McCord’s post on GenServers and ETS. This also helped me practice and familiarize myself with the whole GenServer/ETS process a bit before employing ETS to handle temporary storage of people’s images on Metamorphic.

Here’s the friend link for free viewing of the post on Medium.

The rate limiting was super easy to implement into my design thanks to phx_gen_auth's design.

phx_gen_auth and password reset
In regard to password reset, I had a slightly different problem of creating a “zero-knowledge” encrypted system for people’s data.

If someone were to reset their password they would lose access to any data they had previously encrypted. Rather than allow that with ample warnings, I opted to just remove the password reset option altogether (you can still change your password from within your account), because the other options required things that I felt could ultimately lead to an attacker (albeit motivated) creating havoc with a person’s account (one thought was to allow you to reset your password if you use the same machine as when you first registered your account, but all these kinds of solutions come with other security, privacy, and design tradeoffs).

That really didn’t have much to do with phx_gen_auth but I include it because the beauty, in my opinion, of phx_gen_auth is that it understands that people may have quite different needs for their authentication. By laying just the strong foundation, it becomes really easy to quickly customize and adapt the system to fit your needs (be it “more” secure, or remove things altogether).

I think the decisions that were made about phx_gen_auth, and reasoning behind them, were made with a lot of foresight and wisdom from past experiences. And my experience using it has been very positive because of it. Thank you!

1 Like
8

Thank you for your response. Right, as I mentioned in the other thread, I strive not to leave any pages requiring no authorisation w/o additional measures so I did implement a trivial/naive rate limiting for resending “confirmation instructions” and “password reset instructions”. Login has IP blacklisting too. The rate limiting is trivial in the sense that it prevents re-requesting another e-mail earlier than a predefined amount of time since the last one. For “the last one” I took the created_at of the latest token in given context. But I also

  • pass the email through session, after logging in rather than leaving it an open POST param
  • invalidate/remove earlier tokens for the same user/context once a new one is generated

If you believe this makes a topic for a useful blog post I might try to find a time slot for it

1 Like
9

This looks like a more “proper”, generic rate limiting implementation, as opposed to my “naive” approach. Thank you for sharing.

1 Like
10

@josevalim Thank you :slight_smile: There’s one more thing that raised my attention in phx_gen_auth, which I seem to have an issue with:

  @doc """
  Resets the user password.

  ## Examples

      iex> reset_user_password(user, %{password: "new long password", password_confirmation: "new long password"})
      {:ok, %User{}}

      iex> reset_user_password(user, %{password: "valid", password_confirmation: "not the same"})
      {:error, %Ecto.Changeset{}}

  """
  def reset_user_password(user, attrs) do
    Ecto.Multi.new()
    |> Ecto.Multi.update(:user, User.password_changeset(user, attrs))
    |> Ecto.Multi.delete_all(:tokens, UserToken.user_and_contexts_query(user, :all))
    |> Repo.transaction()
    |> case do
      {:ok, %{user: user}} -> {:ok, user}
      {:error, :user, changeset, _} -> {:error, changeset}
    end
  end

namely this line:

|> Ecto.Multi.delete_all(:tokens, UserToken.user_and_contexts_query(user, :all))

and even more specifically the :all instead of particular context in question. This allows situation like:

  1. User creates an account but does not “activate” aka “confirm” her account yet
  2. User resets her password
  3. Boom - user can no longer activate her account with the activation/confirmation link she received
11

Correct. They would have to request a new email. The same happens for changing the email.

Generally speaking, the recommendation is to expire existing tokens whenever emails/passwords are updated. An alternative could be to ask the user if they want to sign out existing sessions (with the default set to yes) - this way you can at least know if the changes are being motivated for security reasons. Although I think the flow you described is a bit unlikely to happen.

12

Oh, our confirmation route does not sign the user in, so it does not need to be treated as a session. I guess we could delete all except confirmation by replacing the atom :all by :except_confirm and then changing the token schema module accordingly. Would you like to send a PR?

13

Yes - I still have one or two more small things scheduled already so I’ll add it to my list. This should go against 1.6, right? Still - generally I would delete only the directly relevant tokens. The reason being that I perceive the token mechanism introduced in phx_auth_gen as a generic one, which can be used to safeguard other actions in a larger application context. In such case – as a user – I wouldn’t be too happy if I couldn’t authenticate those actions out of a sudden. I mentally probably wouldn’t even connect my password reset with other things not working as they should. IOW I’d rather whitelist the contexts instead of blacklisting them. WDYT?

14

The flip side is that there are then some tokens that should be discarded and now you forgot about them, opening up a potential security flaw. :slight_smile: Plus the “change:…” tokens would require some sort of prefix query. So I think we should explicit list the ones we want to keep.

15

Roger that.

What exactly do you mean here?

16

The change email tokens have “change:#{email}” as context.