diff --git a/apps/fz_http/lib/fz_http/release.ex b/apps/fz_http/lib/fz_http/release.ex index ef6947056..099188160 100644 --- a/apps/fz_http/lib/fz_http/release.ex +++ b/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 diff --git a/apps/fz_http/lib/fz_http/users.ex b/apps/fz_http/lib/fz_http/users.ex index 511052ac1..821257ba9 100644 --- a/apps/fz_http/lib/fz_http/users.ex +++ b/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} diff --git a/apps/fz_http/lib/fz_http/users/password_helpers.ex b/apps/fz_http/lib/fz_http/users/password_helpers.ex index b9aba0370..4d1362fd2 100644 --- a/apps/fz_http/lib/fz_http/users/password_helpers.ex +++ b/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 diff --git a/apps/fz_http/lib/fz_http/users/user.ex b/apps/fz_http/lib/fz_http/users/user.ex index 55c03041c..16eca2605 100644 --- a/apps/fz_http/lib/fz_http/users/user.ex +++ b/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 diff --git a/apps/fz_http/lib/fz_http_web/live/setting_live/account_form_component.ex b/apps/fz_http/lib/fz_http_web/live/setting_live/account_form_component.ex index 8b3f1a33a..03461fb78 100644 --- a/apps/fz_http/lib/fz_http_web/live/setting_live/account_form_component.ex +++ b/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 diff --git a/apps/fz_http/lib/fz_http_web/live/setting_live/unprivileged/account_form_component.ex b/apps/fz_http/lib/fz_http_web/live/setting_live/unprivileged/account_form_component.ex index 6fd9e28d4..3dd53e4a1 100644 --- a/apps/fz_http/lib/fz_http_web/live/setting_live/unprivileged/account_form_component.ex +++ b/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 diff --git a/apps/fz_http/lib/fz_http_web/live/user_live/form_component.ex b/apps/fz_http/lib/fz_http_web/live/user_live/form_component.ex index ef90c4d13..d92282b41 100644 --- a/apps/fz_http/lib/fz_http_web/live/user_live/form_component.ex +++ b/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 diff --git a/apps/fz_http/lib/fz_http_web/live/user_live/show_live.ex b/apps/fz_http/lib/fz_http_web/live/user_live/show_live.ex index 6c2885662..cb947abc6 100644 --- a/apps/fz_http/lib/fz_http_web/live/user_live/show_live.ex +++ b/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", %{}) diff --git a/apps/fz_http/test/fz_http/users_test.exs b/apps/fz_http/test/fz_http/users_test.exs index b34b3fdb4..d572fede9 100644 --- a/apps/fz_http/test/fz_http/users_test.exs +++ b/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 diff --git a/apps/fz_http/test/fz_http_web/live/setting_live/account_test.exs b/apps/fz_http/test/fz_http_web/live/setting_live/account_test.exs index e78cced7c..aa88dd4a7 100644 --- a/apps/fz_http/test/fz_http_web/live/setting_live/account_test.exs +++ b/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 diff --git a/apps/fz_http/test/support/fixtures/users_fixtures.ex b/apps/fz_http/test/support/fixtures/users_fixtures.ex index 69d9086ea..20b0c7b15 100644 --- a/apps/fz_http/test/support/fixtures/users_fixtures.ex +++ b/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 diff --git a/apps/fz_http/test/support/test_helpers.ex b/apps/fz_http/test/support/test_helpers.ex index c63001ca2..a13c970ed 100644 --- a/apps/fz_http/test/support/test_helpers.ex +++ b/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 diff --git a/docker-compose.yml b/docker-compose.yml index 2339e304e..fe43a3507 100644 --- a/docker-compose.yml +++ b/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