Don't hash the password while keeping a database transaction open

[axelson]

Currently the hash_password function is called in a prepare_changes block. This means that the password hashing function will run while keeping a database transaction open. I can understand this sometimes being desirable/intentional as the 0.2.0 changelog says:

Run User.maybe_hash_password/1 in a prepare_changes/1 hook instead of every time a changeset function is called. This decreases load on the server when a changeset function is being called multiple times like with a LiveView-based form.

I think that the trade-off should be made more explicit to the end-user, either via documentation or a generated comment.

[josevalim]

Good point. I think we should move it out of prepare_changes but make it the last step in our pipeline and only do so if our changeset is valid.

The move to prepare_changes was for convenience purposes after all.

[josevalim]

This is the change I did in my project, all tests pass:

diff --git a/apps/bytepack/lib/bytepack/accounts/user.ex b/apps/bytepack/lib/bytepack/accounts/user.ex
index ee2032f..3d3fec1 100644
--- a/apps/bytepack/lib/bytepack/accounts/user.ex
+++ b/apps/bytepack/lib/bytepack/accounts/user.ex
@@ -40,15 +40,19 @@ defmodule Bytepack.Accounts.User do
     changeset
     |> validate_required([:password])
     |> validate_length(:password, min: 12, max: 80)
-    |> prepare_changes(&hash_password/1)
+    |> hash_password()
   end

   defp hash_password(changeset) do
     password = get_change(changeset, :password)

-    changeset
-    |> put_change(:hashed_password, Bcrypt.hash_pwd_salt(password))
-    |> delete_change(:password)
+    if password && changeset.valid? do
+      changeset
+      |> put_change(:hashed_password, Bcrypt.hash_pwd_salt(password))
+      |> delete_change(:password)
+    else
+      changeset
+    end
   end

   @doc """

[aaronrenner]

@josevalim Thanks for posting these changes you made. One thing I've been thinking about is these change hash the password and clear the password field every time this changeset function is run. If we were to use this changeset with a LiveView-based form, every time the user enters a valid password I believe it would disappear on the next keystroke. Instead, I'm thinking we only want to hash the password on Accounts.register_user/1 and Accounts.update_user_password/3. Since Accounts.change_user_password and Accounts.change_user_registration are only about generating changesets, they don't need the additional password hashing and clearing logic.

I put together some updates to the generated code that accomplishes this goal and I am curious what you think.

https://github.com/aaronrenner/phx_gen_auth_password_hashing_proposal/commit/51ee03696342eb6304d398fde7d4d4a1519f6eca

Being able to fully support LiveView based user settings would take additional work that we probably don't want to do now, but I am thinking this might help the developer be one step closer, if they wanted to take that journey. WDYT?

[josevalim]

I love it, awesome job! :heart: My only nitpick is to indent the options list with two spaces, to follow markdown. :)