Merge branch 'fix-user-count' into 'master'

Fixes for user deletion

See merge request framasoft/mobilizon!606

config/test.exs

@@ -14,6 +14,7 @@ config :mobilizon, Mobilizon.Web.Endpoint,
     host: "mobilizon.test",
     scheme: "http"
   ],
+  debug_errors: true,
   secret_key_base: "some secret",
   server: false
 

docs/administration/CLI tasks/manage_users.md

@@ -53,4 +53,5 @@ mix mobilizon.users.delete <email>
 
 ### Options
 
-* `--assume_yes`/`-y` Don't ask for confirmation
+* `--assume_yes`/`-y` Don't ask for confirmation
+* `--keep_email`/`-k` Keep user entry with just email information (to prevent future registrations with same email)

js/src/views/Settings/AccountSettings.vue

@@ -288,7 +288,8 @@ export default class AccountSettings extends Vue {
     return !this.loggedUser.provider;
   }
 
-  static providerName(id: string): string {
+  // eslint-disable-next-line class-methods-use-this
+  providerName(id: string): string {
     if (SELECTED_PROVIDERS[id]) {
       return SELECTED_PROVIDERS[id];
     }

lib/graphql/resolvers/user.ex

@@ -410,7 +410,7 @@ defmodule Mobilizon.GraphQL.Resolvers.User do
            {:moderator_actor, Users.get_actor_for_user(moderator_user)},
          %User{disabled: false} = user <- Users.get_user(user_id),
          {:ok, %User{}} <-
-           do_delete_account(%User{} = user, Relay.get_actor()) do
+           do_delete_account(%User{} = user, actor_performing: Relay.get_actor()) do
       Admin.log_action(moderator_actor, "delete", user)
     else
       {:moderator_actor, nil} ->
@@ -429,11 +429,11 @@ defmodule Mobilizon.GraphQL.Resolvers.User do
            {:confirmation_password, Map.get(args, :password)},
          {:current_password, {:ok, _}} <-
            {:current_password, Authenticator.authenticate(email, password)} do
-      do_delete_account(user)
+      do_delete_account(user, reserve_email: false)
     else
       # If the user hasn't got any password (3rd-party auth)
       {:user_has_password, false} ->
-        do_delete_account(user)
+        do_delete_account(user, reserve_email: false)
 
       {:confirmation_password, nil} ->
         {:error, dgettext("errors", "The password provided is invalid")}
@@ -447,23 +447,21 @@ defmodule Mobilizon.GraphQL.Resolvers.User do
     {:error, dgettext("errors", "You need to be logged-in to delete your account")}
   end
 
-  defp do_delete_account(%User{} = user, actor_performing \\ nil) do
+  @spec do_delete_account(User.t(), Keyword.t()) :: {:ok, User.t()}
+  defp do_delete_account(%User{} = user, options) do
     with actors <- Users.get_actors_for_user(user),
          activated <- not is_nil(user.confirmed_at),
          # Detach actors from user
-         :ok <-
+         :ok <- Enum.each(actors, fn actor -> Actors.update_actor(actor, %{user_id: nil}) end),
-           if(activated,
-             do: :ok,
-             else: Enum.each(actors, fn actor -> Actors.update_actor(actor, %{user_id: nil}) end)
-           ),
          # Launch a background job to delete actors
          :ok <-
            Enum.each(actors, fn actor ->
-             actor_performing = actor_performing || actor
+             actor_performing = Keyword.get(options, :actor_performing, actor)
              ActivityPub.delete(actor, actor_performing, true)
            end),
          # Delete user
-         {:ok, user} <- Users.delete_user(user, reserve_email: activated) do
+         {:ok, user} <-
+           Users.delete_user(user, reserve_email: Keyword.get(options, :reserve_email, activated)) do
       {:ok, user}
     end
   end

lib/mix/tasks/mobilizon/create_bot.ex

@@ -11,7 +11,7 @@ defmodule Mix.Tasks.Mobilizon.CreateBot do
 
   require Logger
 
-  @shortdoc "Register user"
+  @shortdoc "Create bot"
   def run([email, name, summary, type, url]) do
     Mix.Task.run("app.start")
 

lib/mix/tasks/mobilizon/users/delete.ex

@@ -15,23 +15,23 @@ defmodule Mix.Tasks.Mobilizon.Users.Delete do
         rest,
         strict: [
           assume_yes: :boolean,
-          force: :boolean
+          keep_email: :boolean
         ],
         aliases: [
           y: :assume_yes,
-          f: :force
+          k: :keep_email
         ]
       )
 
     assume_yes? = Keyword.get(options, :assume_yes, false)
-    force? = Keyword.get(options, :force, false)
+    keep_email? = Keyword.get(options, :keep_email, false)
 
     Mix.Task.run("app.start")
 
     with {:ok, %User{} = user} <- Users.get_user_by_email(email),
          true <- assume_yes? or Mix.shell().yes?("Continue with deleting user #{user.email}?"),
          {:ok, %User{} = user} <-
-           Users.delete_user(user, reserve_email: !force?) do
+           Users.delete_user(user, reserve_email: keep_email?) do
       Mix.shell().info("""
       The user #{user.email} has been deleted
       """)

lib/mix/tasks/mobilizon/users/show.ex

@@ -20,6 +20,7 @@ defmodule Mix.Tasks.Mobilizon.Users.Show do
       Mix.shell().info("""
       Informations for the user #{user.email}:
         - Activated: #{user.confirmed_at}
+        - Disabled: #{user.disabled}
         - Role: #{user.role}
         #{display_actors(actors)}
       """)

lib/mobilizon/users/users.ex

@@ -280,7 +280,7 @@ defmodule Mobilizon.Users do
   Counts users.
   """
   @spec count_users :: integer
-  def count_users, do: Repo.one(from(u in User, select: count(u.id)))
+  def count_users, do: Repo.one(from(u in User, select: count(u.id), where: u.disabled == false))
 
   @doc """
   Gets a settings for an user.
@@ -383,7 +383,7 @@ defmodule Mobilizon.Users do
   defp user_by_email_query(email, true) do
     from(
       u in User,
-      where: u.email == ^email and not is_nil(u.confirmed_at),
+      where: u.email == ^email and not is_nil(u.confirmed_at) and not u.disabled,
       preload: :default_actor
     )
   end

test/graphql/resolvers/user_test.exs

@@ -757,6 +757,23 @@ defmodule Mobilizon.GraphQL.Resolvers.UserTest do
       assert hd(res["errors"])["message"] ==
                "This user can't reset their password"
     end
+
+    test "test send_reset_password/3 for a deactivated user doesn't send email", %{conn: conn} do
+      {:ok, %User{email: email} = user} =
+        Users.register(%{email: "toto@tata.tld", password: "p4ssw0rd"})
+
+      Users.update_user(user, %{confirmed_at: DateTime.utc_now(), disabled: true})
+
+      res =
+        conn
+        |> AbsintheHelpers.graphql_query(
+          query: @send_reset_password_mutation,
+          variables: %{email: email}
+        )
+
+      assert hd(res["errors"])["message"] ==
+               "No user with this email was found"
+    end
   end
 
   describe "Resolver: Reset user's password" do
@@ -1379,7 +1396,7 @@ defmodule Mobilizon.GraphQL.Resolvers.UserTest do
 
       assert MapSet.new([actor1.id, actor2.id]) == MapSet.new([actor1_id, actor2_id])
 
-      assert Users.get_user(user.id).disabled == true
+      assert is_nil(Users.get_user(user.id))
 
       assert %{success: 2, failure: 0} == Oban.drain_queue(queue: :background)
 

test/tasks/users_test.exs

@@ -52,13 +52,13 @@ defmodule Mix.Tasks.Mobilizon.UsersTest do
     test "delete existing user" do
       insert(:user, email: @email)
       Delete.run([@email, "-y"])
-      assert {:ok, %User{disabled: true}} = Users.get_user_by_email(@email)
+      assert {:error, :user_not_found} = Users.get_user_by_email(@email)
     end
 
     test "full delete existing user" do
       insert(:user, email: @email)
-      Delete.run([@email, "-y", "-f"])
+      Delete.run([@email, "-y", "-k"])
-      assert {:error, :user_not_found} == Users.get_user_by_email(@email)
+      assert {:ok, %User{disabled: true}} = Users.get_user_by_email(@email)
     end
 
     test "delete non-existing user" do
@@ -68,14 +68,18 @@ defmodule Mix.Tasks.Mobilizon.UsersTest do
 
   describe "show user" do
     test "show existing user" do
-      %User{confirmed_at: confirmed_at, role: role} = user = insert(:user, email: @email)
+      %User{confirmed_at: confirmed_at, role: role, disabled: disabled} =
+        user = insert(:user, email: @email)
+
       actor1 = insert(:actor, user: user)
       actor2 = insert(:actor, user: user)
 
       output =
-        "Informations for the user #{@email}:\n  - Activated: #{confirmed_at}\n  - Role: #{role}\n  Identities (2):\n    - @#{
+        "Informations for the user #{@email}:\n  - Activated: #{confirmed_at}\n  - Disabled: #{
-          actor1.preferred_username
+          disabled
-        } / \n    - @#{actor2.preferred_username} / \n\n\n"
+        }\n  - Role: #{role}\n  Identities (2):\n    - @#{actor1.preferred_username} / \n    - @#{
+          actor2.preferred_username
+        } / \n\n\n"
 
       Show.run([@email])
       assert_received {:mix_shell, :info, [output_received]}