From 2c0fe2ea9e32d01caa1bc31093a7ddfdc2793659 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 13 Oct 2020 16:07:36 -0500 Subject: [PATCH] Remove toggle_confirmation; require explicit state change Also cosmetic changes to make the code clearer --- lib/mix/tasks/pleroma/user.ex | 10 +++--- lib/pleroma/user.ex | 32 +++++++++---------- lib/pleroma/web/auth/pleroma_authenticator.ex | 2 +- ...20201231185546_confirm_logged_in_users.exs | 4 +-- test/mix/tasks/pleroma/user_test.exs | 7 ---- .../confirm_logged_in_users_test.exs | 8 ++--- test/pleroma/user_test.exs | 2 +- .../controllers/admin_api_controller_test.exs | 7 ++-- .../web/o_auth/o_auth_controller_test.exs | 2 +- .../controllers/account_controller_test.exs | 2 +- .../web/twitter_api/controller_test.exs | 2 +- 11 files changed, 37 insertions(+), 41 deletions(-) diff --git a/lib/mix/tasks/pleroma/user.ex b/lib/mix/tasks/pleroma/user.ex index a397d1748..e87f1c271 100644 --- a/lib/mix/tasks/pleroma/user.ex +++ b/lib/mix/tasks/pleroma/user.ex @@ -213,7 +213,7 @@ def run(["set", nickname | rest]) do user = case Keyword.get(options, :confirmed) do nil -> user - value -> set_confirmed(user, value) + value -> set_confirmation(user, value) end user = @@ -373,7 +373,7 @@ def run(["confirm_all"]) do |> Pleroma.Repo.chunk_stream(500, :batches) |> Stream.each(fn users -> users - |> Enum.each(fn user -> User.need_confirmation(user, false) end) + |> Enum.each(fn user -> User.set_confirmation(user, true) end) end) |> Stream.run() end @@ -391,7 +391,7 @@ def run(["unconfirm_all"]) do |> Pleroma.Repo.chunk_stream(500, :batches) |> Stream.each(fn users -> users - |> Enum.each(fn user -> User.need_confirmation(user, true) end) + |> Enum.each(fn user -> User.set_confirmation(user, false) end) end) |> Stream.run() end @@ -454,8 +454,8 @@ defp set_locked(user, value) do user end - defp set_confirmed(user, value) do - {:ok, user} = User.need_confirmation(user, !value) + defp set_confirmation(user, value) do + {:ok, user} = User.set_confirmation(user, value) shell_info("Confirmation status of #{user.nickname}: #{user.is_confirmed}") user diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 04ce1768d..9efc27887 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -704,11 +704,11 @@ def register_changeset(struct, params \\ %{}, opts \\ []) do reason_limit = Config.get([:instance, :registration_reason_length], 500) params = Map.put_new(params, :accepts_chat_messages, true) - need_confirmation? = - if is_nil(opts[:need_confirmation]) do - Config.get([:instance, :account_activation_required]) + confirmed? = + if is_nil(opts[:confirmed]) do + !Config.get([:instance, :account_activation_required]) else - opts[:need_confirmation] + opts[:confirmed] end need_approval? = @@ -719,7 +719,7 @@ def register_changeset(struct, params \\ %{}, opts \\ []) do end struct - |> confirmation_changeset(need_confirmation: need_confirmation?) + |> confirmation_changeset(set_confirmation: confirmed?) |> approval_changeset(need_approval: need_approval?) |> cast(params, [ :bio, @@ -1643,7 +1643,7 @@ def confirm(users) when is_list(users) do end def confirm(%User{is_confirmed: false} = user) do - with chg <- confirmation_changeset(user, need_confirmation: false), + with chg <- confirmation_changeset(user, set_confirmation: true), {:ok, user} <- update_and_set_cache(chg) do post_register_action(user) {:ok, user} @@ -2138,10 +2138,10 @@ def touch_last_digest_emailed_at(user) do updated_user end - @spec need_confirmation(User.t(), boolean()) :: {:ok, User.t()} | {:error, Changeset.t()} - def need_confirmation(%User{} = user, bool) do + @spec set_confirmation(User.t(), boolean()) :: {:ok, User.t()} | {:error, Changeset.t()} + def set_confirmation(%User{} = user, bool) do user - |> confirmation_changeset(need_confirmation: bool) + |> confirmation_changeset(set_confirmation: bool) |> update_and_set_cache() end @@ -2309,18 +2309,18 @@ def mastodon_settings_update(user, settings) do end @spec confirmation_changeset(User.t(), keyword()) :: Changeset.t() - def confirmation_changeset(user, need_confirmation: need_confirmation?) do + def confirmation_changeset(user, set_confirmation: confirmed?) do params = - if need_confirmation? do - %{ - is_confirmed: false, - confirmation_token: :crypto.strong_rand_bytes(32) |> Base.url_encode64() - } - else + if confirmed? do %{ is_confirmed: true, confirmation_token: nil } + else + %{ + is_confirmed: false, + confirmation_token: :crypto.strong_rand_bytes(32) |> Base.url_encode64() + } end cast(user, params, [:is_confirmed, :confirmation_token]) diff --git a/lib/pleroma/web/auth/pleroma_authenticator.ex b/lib/pleroma/web/auth/pleroma_authenticator.ex index a2121e6a7..401f23c9f 100644 --- a/lib/pleroma/web/auth/pleroma_authenticator.ex +++ b/lib/pleroma/web/auth/pleroma_authenticator.ex @@ -84,7 +84,7 @@ def create_from_registration( password_confirmation: random_password }, external: true, - need_confirmation: false + confirmed: true ) |> Repo.insert(), {:ok, _} <- diff --git a/priv/repo/migrations/20201231185546_confirm_logged_in_users.exs b/priv/repo/migrations/20201231185546_confirm_logged_in_users.exs index 4372d093f..b9656c17b 100644 --- a/priv/repo/migrations/20201231185546_confirm_logged_in_users.exs +++ b/priv/repo/migrations/20201231185546_confirm_logged_in_users.exs @@ -11,9 +11,9 @@ defmodule Pleroma.Repo.Migrations.ConfirmLoggedInUsers do def up do User - |> where([u], u.confirmation_pending == true) + |> where([u], u.is_confirmed == false) |> join(:inner, [u], t in Token, on: t.user_id == u.id) - |> Repo.update_all(set: [confirmation_pending: false]) + |> Repo.update_all(set: [is_confirmed: true]) end def down do diff --git a/test/mix/tasks/pleroma/user_test.exs b/test/mix/tasks/pleroma/user_test.exs index a620e5960..2b5232283 100644 --- a/test/mix/tasks/pleroma/user_test.exs +++ b/test/mix/tasks/pleroma/user_test.exs @@ -436,13 +436,6 @@ test "invite is revoked" do assert_received {:mix_shell, :info, [message]} assert message =~ "Invite for token #{invite.token} was revoked." end - - test "it prints an error message when invite is not exist" do - Mix.Tasks.Pleroma.User.run(["revoke_invite", "foo"]) - - assert_received {:mix_shell, :error, [message]} - assert message =~ "No invite found" - end end describe "running delete_activities" do diff --git a/test/pleroma/repo/migrations/confirm_logged_in_users_test.exs b/test/pleroma/repo/migrations/confirm_logged_in_users_test.exs index b30faa257..99d17f62a 100644 --- a/test/pleroma/repo/migrations/confirm_logged_in_users_test.exs +++ b/test/pleroma/repo/migrations/confirm_logged_in_users_test.exs @@ -14,12 +14,12 @@ defmodule Pleroma.Repo.Migrations.ConfirmLoggedInUsersTest do test "up/0 confirms unconfirmed but previously-logged-in users", %{migration: migration} do insert_list(25, :oauth_token) - Repo.update_all(User, set: [confirmation_pending: true]) - insert_list(5, :user, confirmation_pending: true) + Repo.update_all(User, set: [is_confirmed: false]) + insert_list(5, :user, is_confirmed: false) count = User - |> where(confirmation_pending: true) + |> where(is_confirmed: false) |> Repo.aggregate(:count) assert count == 30 @@ -28,7 +28,7 @@ test "up/0 confirms unconfirmed but previously-logged-in users", %{migration: mi count = User - |> where(confirmation_pending: true) + |> where(is_confirmed: false) |> Repo.aggregate(:count) assert count == 5 diff --git a/test/pleroma/user_test.exs b/test/pleroma/user_test.exs index 7e50d53d3..ac9db9ffb 100644 --- a/test/pleroma/user_test.exs +++ b/test/pleroma/user_test.exs @@ -666,7 +666,7 @@ test "it creates unconfirmed user" do end test "it creates confirmed user if :confirmed option is given" do - changeset = User.register_changeset(%User{}, @full_user_data, need_confirmation: false) + changeset = User.register_changeset(%User{}, @full_user_data, confirmed: true) assert changeset.valid? {:ok, user} = Repo.insert(changeset) diff --git a/test/pleroma/web/admin_api/controllers/admin_api_controller_test.exs b/test/pleroma/web/admin_api/controllers/admin_api_controller_test.exs index 65f2a124f..23e4bc3af 100644 --- a/test/pleroma/web/admin_api/controllers/admin_api_controller_test.exs +++ b/test/pleroma/web/admin_api/controllers/admin_api_controller_test.exs @@ -906,8 +906,11 @@ test "it confirms emails of two users", %{conn: conn, admin: admin} do assert ret_conn.status == 200 - assert User.get_by_id(first_user.id).is_confirmed - assert User.get_by_id(second_user.id).is_confirmed + first_user = User.get_by_id(first_user.id) + second_user = User.get_by_id(second_user.id) + + assert first_user.is_confirmed + assert second_user.is_confirmed log_entry = Repo.one(ModerationLog) diff --git a/test/pleroma/web/o_auth/o_auth_controller_test.exs b/test/pleroma/web/o_auth/o_auth_controller_test.exs index 254d4e9a4..337d2650c 100644 --- a/test/pleroma/web/o_auth/o_auth_controller_test.exs +++ b/test/pleroma/web/o_auth/o_auth_controller_test.exs @@ -928,7 +928,7 @@ test "rejects token exchange for valid credentials belonging to unconfirmed user {:ok, user} = insert(:user, password_hash: Pleroma.Password.Pbkdf2.hash_pwd_salt(password)) - |> User.confirmation_changeset(need_confirmation: true) + |> User.confirmation_changeset(set_confirmation: false) |> User.update_and_set_cache() refute Pleroma.User.account_status(user) == :active diff --git a/test/pleroma/web/pleroma_api/controllers/account_controller_test.exs b/test/pleroma/web/pleroma_api/controllers/account_controller_test.exs index 668bb46fb..9f14c5577 100644 --- a/test/pleroma/web/pleroma_api/controllers/account_controller_test.exs +++ b/test/pleroma/web/pleroma_api/controllers/account_controller_test.exs @@ -17,7 +17,7 @@ defmodule Pleroma.Web.PleromaAPI.AccountControllerTest do setup do {:ok, user} = insert(:user) - |> User.confirmation_changeset(need_confirmation: true) + |> User.confirmation_changeset(set_confirmation: false) |> User.update_and_set_cache() refute user.is_confirmed diff --git a/test/pleroma/web/twitter_api/controller_test.exs b/test/pleroma/web/twitter_api/controller_test.exs index 8f29a4f63..583c904b2 100644 --- a/test/pleroma/web/twitter_api/controller_test.exs +++ b/test/pleroma/web/twitter_api/controller_test.exs @@ -64,7 +64,7 @@ test "with credentials, with params" do setup do {:ok, user} = insert(:user) - |> User.confirmation_changeset(need_confirmation: true) + |> User.confirmation_changeset(set_confirmation: false) |> Repo.update() refute user.is_confirmed