Reduce cast coverage (#851)

* remove cast role in create

* remove cast in update

this will fail

* Revert "remove cast role in create"

This reverts commit 217c62170f1f09987c6adbf7a5b3f467dd84034c.

* remove role in generic cast

* separate role and sign in token out from generic update

* handle empty value in put_password_hash

* separate last signed in at

* remove usage of removed changeset function

* fix clear sign in token

* improve tests

* fix tests

* split update user

* require password change

* fix test

* remove unused env var

apps/fz_http/lib/fz_http/release.ex

@@ -45,7 +45,7 @@ defmodule FzHttp.Release do
 
     {:ok, _user} =
       Users.get_user!(email: email)
-      |> Users.update_user(params)
+      |> Users.admin_update_user(params)
   end
 
   defp repos do

apps/fz_http/lib/fz_http/users.ex

@@ -61,14 +61,18 @@ defmodule FzHttp.Users do
   def create_user_with_role(attrs, role) do
     attrs
     |> Enum.into(%{})
-    |> create_user(%{role: role})
+    |> create_user(role: role)
   end
 
-  def create_user(attrs, overwrites \\ %{}) do
-    result =
-      struct(User, sign_in_keys())
+  def create_user(attrs, overwrites \\ []) do
+    changeset =
+      User
+      |> struct(sign_in_keys())
       |> User.create_changeset(attrs)
-      |> User.update_changeset(overwrites)
+
+    result =
+      overwrites
+      |> Enum.reduce(changeset, fn {k, v}, cs -> put_change(cs, k, v) end)
       |> Repo.insert()
 
     case result do
@@ -90,9 +94,37 @@ defmodule FzHttp.Users do
     }
   end
 
-  def update_user(%User{} = user, attrs) do
+  def admin_update_user(%User{} = user, attrs) do
     user
-    |> User.update_changeset(attrs)
+    |> User.update_email(attrs)
+    |> User.update_password(attrs)
+    |> Repo.update()
+  end
+
+  def admin_update_self(%User{} = user, attrs) do
+    user
+    |> User.update_email(attrs)
+    |> User.update_password(attrs)
+    |> User.require_current_password(attrs)
+    |> Repo.update()
+  end
+
+  def unprivileged_update_self(%User{} = user, attrs) do
+    user
+    |> User.require_password_change(attrs)
+    |> User.update_password(attrs)
+    |> Repo.update()
+  end
+
+  def update_user_role(%User{} = user, role) do
+    user
+    |> User.update_role(%{role: role})
+    |> Repo.update()
+  end
+
+  def update_user_sign_in_token(%User{} = user, attrs) do
+    user
+    |> User.update_sign_in_token(attrs)
     |> Repo.update()
   end
 
@@ -102,7 +134,7 @@ defmodule FzHttp.Users do
   end
 
   def change_user(%User{} = user \\ struct(User)) do
-    User.changeset(user, %{})
+    change(user)
   end
 
   def new_user do
@@ -150,7 +182,12 @@ defmodule FzHttp.Users do
         m -> to_string(m)
       end
 
-    update_user(user, %{last_signed_in_at: DateTime.utc_now(), last_signed_in_method: method})
+    user
+    |> User.update_last_signed_in(%{
+      last_signed_in_at: DateTime.utc_now(),
+      last_signed_in_method: method
+    })
+    |> Repo.update()
   end
 
   def enable_vpn_connection(user, %{provider: :identity}), do: user
@@ -189,7 +226,7 @@ defmodule FzHttp.Users do
 
   def reset_sign_in_token(email) do
     with %User{} = user <- Repo.get_by(User, email: email),
-         {:ok, user} <- update_user(user, sign_in_keys()) do
+         {:ok, user} <- update_user_sign_in_token(user, sign_in_keys()) do
       # send email in a separate process so that the time this function takes
       # doesn't reflect whether a user exists or not
       Task.start(fn ->
@@ -234,7 +271,7 @@ defmodule FzHttp.Users do
   end
 
   defp clear_token(user) do
-    result = update_user(user, %{sign_in_token: nil, sign_in_token_created_at: nil})
+    result = update_user_sign_in_token(user, %{sign_in_token: nil, sign_in_token_created_at: nil})
 
     case result do
       {:ok, user} -> {:ok, user}

apps/fz_http/lib/fz_http/users/password_helpers.ex

@@ -18,6 +18,11 @@ defmodule FzHttp.Users.PasswordHelpers do
 
   def validate_password_equality(changeset), do: changeset
 
+  def put_password_hash(%Ecto.Changeset{changes: %{password: password}} = changeset)
+      when password in ["", nil] do
+    changeset
+  end
+
   def put_password_hash(
         %Ecto.Changeset{
           valid?: true,
@@ -25,7 +30,7 @@ defmodule FzHttp.Users.PasswordHelpers do
         } = changeset
       ) do
     changeset
-    |> change(password_hash: Argon2.hash_pwd_salt(password))
+    |> put_change(:password_hash, Argon2.hash_pwd_salt(password))
     |> delete_change(:password)
     |> delete_change(:password_confirmation)
   end

apps/fz_http/lib/fz_http/users/user.ex

@@ -38,7 +38,6 @@ defmodule FzHttp.Users.User do
   def create_changeset(user, attrs \\ %{}) do
     user
     |> cast(attrs, [
-      :role,
       :email,
       :password_hash,
       :password,
@@ -52,129 +51,67 @@ defmodule FzHttp.Users.User do
     |> put_password_hash()
   end
 
-  # Sign in token
-  # XXX: Map keys must be strings for this approach to work. Refactor to something that is key
-  # type agnostic.
-  # If password isn't being changed, remove it from list of attributes to validate
-  def update_changeset(
-        user,
-        %{
-          "password" => nil,
-          "password_confirmation" => nil,
-          "current_password" => nil
-        } = attrs
-      ) do
-    update_changeset(
-      user,
-      Map.drop(attrs, ["password", "password_confirmation", "current_password"])
-    )
-  end
-
-  # If password isn't being changed, remove it from list of attributes to validate
-  def update_changeset(
-        user,
-        %{"password" => "", "password_confirmation" => "", "current_password" => ""} = attrs
-      ) do
-    update_changeset(
-      user,
-      Map.drop(attrs, ["password", "password_confirmation", "current_password"])
-    )
-  end
-
-  # Password and other fields are being changed
-  def update_changeset(
-        user,
-        %{
-          "password" => _password,
-          "password_confirmation" => _password_confirmation,
-          "current_password" => _current_password
-        } = attrs
-      ) do
+  def require_current_password(user, attrs) do
     user
-    |> cast(attrs, [:role, :email, :password, :password_confirmation, :current_password])
-    |> validate_required([:email, :password, :password_confirmation, :current_password])
-    |> validate_format(:email, ~r/@/)
-    |> verify_current_password(user)
-    |> validate_length(:password, min: @min_password_length, max: @max_password_length)
+    |> cast(attrs, [:current_password])
+    |> validate_required([:current_password])
+    |> verify_current_password()
+  end
+
+  def update_password(user, attrs) do
+    user
+    |> cast(attrs, [:password, :password_confirmation])
+    |> then(fn
+      %{changes: %{password: _}} = changeset ->
+        validate_length(changeset, :password, min: @min_password_length, max: @max_password_length)
+
+      changeset ->
+        changeset
+    end)
     |> validate_password_equality()
     |> put_password_hash()
     |> validate_required([:password_hash])
   end
 
-  # Email updated from an admin
-  def update_changeset(
-        user,
-        %{
-          "email" => _email,
-          "password" => "",
-          "password_confirmation" => ""
-        } = attrs
-      ) do
-    update_changeset(user, Map.drop(attrs, ["password", "password_confirmation"]))
-  end
-
-  # Password updated from token or admin
-  def update_changeset(
-        user,
-        %{
-          "password" => _password,
-          "password_confirmation" => _password_confirmation
-        } = attrs
-      ) do
+  def require_password_change(user, attrs) do
     user
-    |> cast(attrs, [:email, :password, :password_confirmation])
-    |> validate_required([:email, :password, :password_confirmation])
-    |> validate_format(:email, ~r/@/)
-    |> validate_length(:password, min: @min_password_length, max: @max_password_length)
-    |> validate_password_equality()
-    |> put_password_hash()
-    |> validate_required([:password_hash])
+    |> cast(attrs, [:password, :password_confirmation])
+    |> validate_required([:password, :password_confirmation])
   end
 
-  # Only email being updated
-  def update_changeset(user, %{"email" => _email} = attrs) do
+  def update_email(user, attrs) do
     user
     |> cast(attrs, [:email])
     |> validate_required([:email])
     |> validate_format(:email, ~r/@/)
   end
 
-  # Promotion / Demotion
-  def update_changeset(user, %{role: _role} = attrs) do
+  def update_role(user, attrs) do
     user
     |> cast(attrs, [:role])
     |> validate_required([:role])
   end
 
-  # Password reset token
-  def update_changeset(
-        user,
-        %{sign_in_token: _token, sign_in_token_created_at: _created_at} = attrs
-      ) do
+  def update_sign_in_token(user, attrs) do
     cast(user, attrs, [:sign_in_token, :sign_in_token_created_at])
   end
 
-  # XXX: Invalidate password reset when user is updated
-  def update_changeset(user, %{} = attrs) do
-    changeset(user, attrs)
-  end
-
-  def changeset(user, attrs) do
-    user
-    |> cast(attrs, [:email, :last_signed_in_method, :last_signed_in_at])
+  def update_last_signed_in(user, attrs) do
+    cast(user, attrs, [:last_signed_in_method, :last_signed_in_at])
   end
 
   defp verify_current_password(
          %Ecto.Changeset{
-           changes: %{current_password: _}
-         } = changeset,
-         user
+           data: %{password_hash: password_hash},
+           changes: %{current_password: current_password}
+         } = changeset
        ) do
-    case Argon2.check_pass(user, changeset.changes.current_password) do
-      {:ok, _user} -> changeset |> delete_change(:current_password)
-      {:error, error_msg} -> changeset |> add_error(:current_password, error_msg)
+    if Argon2.verify_pass(current_password, password_hash) do
+      delete_change(changeset, :current_password)
+    else
+      add_error(changeset, :current_password, "invalid password")
     end
   end
 
-  defp verify_current_password(changeset, _user), do: changeset
+  defp verify_current_password(changeset), do: changeset
 end

apps/fz_http/lib/fz_http_web/live/setting_live/account_form_component.ex

@@ -18,7 +18,7 @@ defmodule FzHttpWeb.SettingLive.AccountFormComponent do
   def handle_event("save", %{"user" => user_params}, socket) do
     user = socket.assigns.user
 
-    case Users.update_user(user, user_params) do
+    case Users.admin_update_self(user, user_params) do
       {:ok, _user} ->
         {:noreply,
          socket

apps/fz_http/lib/fz_http_web/live/setting_live/unprivileged/account_form_component.ex

@@ -6,11 +6,6 @@ defmodule FzHttpWeb.SettingLive.Unprivileged.AccountFormComponent do
 
   alias FzHttp.Users
 
-  @allowed_params [
-    "password",
-    "password_confirmation"
-  ]
-
   def update(assigns, socket) do
     changeset = Users.change_user(assigns.current_user)
 
@@ -21,9 +16,7 @@ defmodule FzHttpWeb.SettingLive.Unprivileged.AccountFormComponent do
   end
 
   def handle_event("save", %{"user" => user_params}, socket) do
-    allowed_params = Map.take(user_params, @allowed_params)
-
-    case Users.update_user(socket.assigns.current_user, allowed_params) do
+    case Users.unprivileged_update_self(socket.assigns.current_user, user_params) do
       {:ok, _user} ->
         {:noreply,
          socket

apps/fz_http/lib/fz_http_web/live/user_live/form_component.ex

@@ -47,7 +47,7 @@ defmodule FzHttpWeb.UserLive.FormComponent do
   def handle_event("save", %{"user" => user_params}, %{assigns: %{action: :edit}} = socket) do
     user = socket.assigns.user
 
-    case Users.update_user(user, user_params) do
+    case Users.admin_update_user(user, user_params) do
       {:ok, user} ->
         {:noreply,
          socket

apps/fz_http/lib/fz_http_web/live/user_live/show_live.ex

@@ -84,7 +84,7 @@ defmodule FzHttpWeb.UserLive.Show do
           "demote" -> :unprivileged
         end
 
-      case Users.update_user(user, %{role: role}) do
+      case Users.update_user_role(user, role) do
         {:ok, user} ->
           # Force reconnect with new role
           FzHttpWeb.Endpoint.broadcast("users_socket:#{user.id}", "disconnect", %{})

apps/fz_http/test/fz_http/users_test.exs

@@ -134,90 +134,143 @@ defmodule FzHttp.UsersTest do
     end
   end
 
-  describe "update_user/2" do
-    setup [:create_user]
+  @change_password_valid_params %{
+    password: "new_password",
+    password_confirmation: "new_password",
+    current_password: "password1234"
+  }
+  @change_password_invalid_params %{
+    "password" => "new_password",
+    "password_confirmation" => "new_password",
+    "current_password" => "invalid"
+  }
+  @password_params %{"password" => "new_password", "password_confirmation" => "new_password"}
+  @email_params %{"email" => "new_email@test", "current_password" => "password1234"}
+  @email_and_password_params %{
+    "password" => "new_password",
+    "password_confirmation" => "new_password",
+    "email" => "new_email@test",
+    "current_password" => "password1234"
+  }
+  @clear_hash_params %{"password_hash" => nil, "current_password" => "password1234"}
+  @empty_password_params %{
+    "password" => nil,
+    "password_confirmation" => nil,
+    "current_password" => "password1234"
+  }
+  @email_empty_password_params %{
+    "email" => "foobar@test",
+    "password" => "",
+    "password_confirmation" => "",
+    "current_password" => "password1234"
+  }
+
+  describe "admin_update_user/2" do
+    setup :create_user
+
+    test "changes password", %{user: user} do
+      {:ok, new_user} = Users.admin_update_user(user, @password_params)
+      assert new_user.password_hash != user.password_hash
+    end
+
+    test "prevents clearing the password", %{user: user} do
+      {:ok, new_user} = Users.admin_update_user(user, @clear_hash_params)
+      assert new_user.password_hash == user.password_hash
+    end
+
+    test "nil password params", %{user: user} do
+      {:ok, new_user} = Users.admin_update_user(user, @empty_password_params)
+      assert new_user.password_hash == user.password_hash
+    end
+
+    test "changes email", %{user: user} do
+      {:ok, new_user} = Users.admin_update_user(user, @email_params)
+      assert new_user.email == "new_email@test"
+    end
+
+    test "handles empty params", %{user: user} do
+      assert {:ok, _new_user} = Users.admin_update_user(user, %{})
+    end
+
+    test "handles nil password", %{user: user} do
+      assert {:ok, _new_user} = Users.admin_update_user(user, @email_empty_password_params)
+    end
+
+    test "changes email and password", %{user: user} do
+      {:ok, new_user} = Users.admin_update_user(user, @email_and_password_params)
+      assert new_user.email == "new_email@test"
+      assert new_user.password_hash != user.password_hash
+    end
+  end
+
+  describe "unprivileged_update_self/2" do
+    setup :create_user
+
+    test "changes password", %{user: user} do
+      {:ok, new_user} = Users.unprivileged_update_self(user, @password_params)
+      assert new_user.password_hash != user.password_hash
+    end
+
+    test "prevents clearing the password", %{user: user} do
+      assert {:error, _changeset} = Users.unprivileged_update_self(user, @clear_hash_params)
+    end
+
+    test "prevents changing email", %{user: user} do
+      {:ok, new_user} = Users.unprivileged_update_self(user, @email_and_password_params)
+      assert new_user.email == user.email
+    end
+  end
+
+  describe "admin_update_self/2" do
+    setup :create_user
+
+    test "does not change password when current_password invalid", %{user: user} do
+      {:error, changeset} = Users.admin_update_self(user, @change_password_invalid_params)
+      assert [current_password: _] = changeset.errors
+    end
+
+    test "changes password when current_password valid", %{user: user} do
+      {:ok, new_user} = Users.admin_update_self(user, @change_password_valid_params)
+      assert new_user.password_hash != user.password_hash
+    end
+  end
+
+  describe "update_*" do
+    setup :create_user
 
-    @change_password_valid_params %{
-      "password" => "new_password",
-      "password_confirmation" => "new_password",
-      "current_password" => "password1234"
-    }
-    @change_password_invalid_params %{
-      "password" => "new_password",
-      "password_confirmation" => "new_password",
-      "current_password" => "invalid"
-    }
-    @password_params %{"password" => "new_password", "password_confirmation" => "new_password"}
-    @email_params %{"email" => "new_email@test"}
-    @email_and_password_params %{
-      "password" => "new_password",
-      "password_confirmation" => "new_password",
-      "email" => "new_email@test"
-    }
-    @no_password_params %{"password_hash" => nil}
-    @empty_password_params %{
-      "password" => nil,
-      "password_confirmation" => nil,
-      "current_password" => nil
-    }
-    @email_empty_password_params %{
-      "email" => "foobar@test",
-      "password" => "",
-      "password_confirmation" => "",
-      "current_password" => ""
-    }
     @sign_in_token_params %{
       sign_in_token: "foobar",
       sign_in_token_created_at: DateTime.utc_now()
     }
 
-    test "changes password when only password is updated", %{user: user} do
-      {:ok, new_user} = Users.update_user(user, @password_params)
-      assert new_user.password_hash != user.password_hash
-    end
+    test "update sign_in_token", %{user: user} do
+      {:ok, new_user} = Users.update_user_sign_in_token(user, @sign_in_token_params)
 
-    test "changes password when current_password valid", %{user: user} do
-      {:ok, new_user} = Users.update_user(user, @change_password_valid_params)
-      assert new_user.password_hash != user.password_hash
-    end
-
-    test "does not change password when current_password invalid", %{user: user} do
-      {:error, changeset} = Users.update_user(user, @change_password_invalid_params)
-      assert [current_password: _] = changeset.errors
-    end
-
-    test "prevents clearing the password", %{user: user} do
-      {:ok, new_user} = Users.update_user(user, @no_password_params)
-      assert new_user.password_hash == user.password_hash
-    end
-
-    test "nil password params", %{user: user} do
-      {:ok, new_user} = Users.update_user(user, @empty_password_params)
-      assert new_user.password_hash == user.password_hash
-    end
-
-    test "adding a sign in token", %{user: user} do
-      {:ok, new_user} = Users.update_user(user, @sign_in_token_params)
       assert new_user.sign_in_token == @sign_in_token_params.sign_in_token
+
+      {:ok, new_user} =
+        Users.update_user_sign_in_token(new_user, %{
+          sign_in_token: nil,
+          sign_in_token_created_at: nil
+        })
+
+      assert is_nil(new_user.sign_in_token)
     end
 
-    test "changes email", %{user: user} do
-      {:ok, new_user} = Users.update_user(user, @email_params)
-      assert new_user.email == "new_email@test"
+    test "update role", %{user: user} do
+      {:ok, user} = Users.update_user_role(user, :admin)
+      assert user.role == :admin
+
+      {:ok, user} = Users.update_user_role(user, :unprivileged)
+      assert user.role == :unprivileged
     end
 
-    test "handles empty params", %{user: user} do
-      assert {:ok, _new_user} = Users.update_user(user, %{})
-    end
+    test "update last_signed_in_*", %{user: user} do
+      {:ok, user} = Users.update_last_signed_in(user, %{provider: :test})
+      assert user.last_signed_in_method == "test"
 
-    test "handles nil password", %{user: user} do
-      assert {:ok, _new_user} = Users.update_user(user, @email_empty_password_params)
-    end
-
-    test "changes email and password", %{user: user} do
-      {:ok, new_user} = Users.update_user(user, @email_and_password_params)
-      assert new_user.email == "new_email@test"
-      assert new_user.password_hash != user.password_hash
+      {:ok, user} = Users.update_last_signed_in(user, %{provider: :another_test})
+      assert user.last_signed_in_method == "another_test"
     end
   end
 

apps/fz_http/test/fz_http_web/live/setting_live/account_test.exs

@@ -25,7 +25,7 @@ defmodule FzHttpWeb.SettingLive.AccountTest do
   end
 
   describe "when live_action is edit" do
-    @valid_params %{"user" => %{"email" => "foobar@test"}}
+    @valid_params %{"user" => %{"email" => "foobar@test", "current_password" => "password1234"}}
     @invalid_params %{"user" => %{"email" => "foobar"}}
 
     test "loads the form" do

apps/fz_http/test/support/fixtures/users_fixtures.ex

@@ -15,14 +15,15 @@ defmodule FzHttp.UsersFixtures do
     case Repo.get_by(User, email: email) do
       nil ->
         {:ok, user} =
-          attrs
-          |> Enum.into(%{
-            email: email,
-            role: :admin,
-            password: "password1234",
-            password_confirmation: "password1234"
-          })
-          |> Users.create_user()
+          Users.create_user(
+            %{
+              email: email,
+              role: :admin,
+              password: "password1234",
+              password_confirmation: "password1234"
+            },
+            Enum.into(attrs, %{role: :admin})
+          )
 
         user
 

apps/fz_http/test/support/test_helpers.ex

@@ -193,13 +193,12 @@ defmodule FzHttp.TestHelpers do
   def create_user_with_expired_sign_in_token(_) do
     expired_at = DateTime.add(DateTime.utc_now(), -1 * 86_401)
 
-    {:ok, user} =
-      Users.update_user(UsersFixtures.user(), %{
-        sign_in_token: "EXPIRED_TOKEN",
-        sign_in_token_created_at: expired_at
-      })
-
-    {:ok, user: user}
+    {:ok,
+     user:
+       UsersFixtures.user(%{
+         sign_in_token: "EXPIRED_TOKEN",
+         sign_in_token_created_at: expired_at
+       })}
   end
 
   def create_users(opts) do

docker-compose.yml

@@ -36,7 +36,6 @@ services:
     environment:
       LOCAL_AUTH_ENABLED: 'true'
       FZ_WALL_CLI_MODULE: FzWall.CLI.Live
-      FZ_VPN_WGADAPTER_MODULE: FzVpn.Interface.WGAdapter.Live
     cap_add:
       - NET_ADMIN
       - SYS_MODULE