From f21c79cf6a9bc798a972c4df2c0c385e22e6d978 Mon Sep 17 00:00:00 2001
From: Chocobozzz <me@florianbigard.com>
Date: Mon, 26 Aug 2019 15:44:02 +0200
Subject: [PATCH] Don't delete the last admin of a group
---
lib/mobilizon/actors/member.ex | 27 +++++++++++++
lib/mobilizon_web/resolvers/group.ex | 6 +--
lib/mobilizon_web/resolvers/person.ex | 12 +++++-
.../resolvers/person_resolver_test.exs | 38 +++++++++++++++++--
4 files changed, 76 insertions(+), 7 deletions(-)
diff --git a/lib/mobilizon/actors/member.ex b/lib/mobilizon/actors/member.ex
index 6580a69df..c3b4767be 100644
--- a/lib/mobilizon/actors/member.ex
+++ b/lib/mobilizon/actors/member.ex
@@ -68,6 +68,33 @@ defmodule Mobilizon.Actors.Member do
)
end
+ @doc """
+ Get all group ids where the actor_id is the last administrator
+ """
+ def list_group_id_where_last_administrator(actor_id) do
+ in_query =
+ from(
+ m in Member,
+ where: m.actor_id == ^actor_id and (m.role == ^:creator or m.role == ^:administrator),
+ select: m.parent_id
+ )
+
+ Repo.all(
+ from(
+ m in Member,
+ where: m.role == ^:creator or m.role == ^:administrator,
+ join: m2 in subquery(in_query),
+ on: m.parent_id == m2.parent_id,
+ group_by: m.parent_id,
+ select: m.parent_id,
+ having: count(m.actor_id) == 1
+ )
+ )
+ end
+
+ @doc """
+ Returns true if the member is an administrator (admin or creator) of the group
+ """
def is_administrator(%Member{role: :administrator}), do: {:is_admin, true}
def is_administrator(%Member{role: :creator}), do: {:is_admin, true}
def is_administrator(%Member{}), do: {:is_admin, false}
diff --git a/lib/mobilizon_web/resolvers/group.ex b/lib/mobilizon_web/resolvers/group.ex
index 96b4214eb..bb25eaf0d 100644
--- a/lib/mobilizon_web/resolvers/group.ex
+++ b/lib/mobilizon_web/resolvers/group.ex
@@ -178,7 +178,7 @@ defmodule MobilizonWeb.Resolvers.Group do
with {:is_owned, true, actor} <- User.owns_actor(user, actor_id),
{:ok, %Member{} = member} <- Member.get_member(actor.id, group_id),
{:only_administrator, false} <-
- {:only_administrator, check_that_member_is_not_only_administrator(group_id, actor_id)},
+ {:only_administrator, check_that_member_is_not_last_administrator(group_id, actor_id)},
{:ok, _} <-
Mobilizon.Actors.delete_member(member) do
{
@@ -211,8 +211,8 @@ defmodule MobilizonWeb.Resolvers.Group do
# We check that the actor asking to leave the group is not it's only administrator
# We start by fetching the list of administrator or creators and if there's only one of them
# and that it's the actor requesting leaving the group we return true
- @spec check_that_member_is_not_only_administrator(integer(), integer()) :: boolean()
- defp check_that_member_is_not_only_administrator(group_id, actor_id) do
+ @spec check_that_member_is_not_last_administrator(integer(), integer()) :: boolean()
+ defp check_that_member_is_not_last_administrator(group_id, actor_id) do
case Member.list_administrator_members_for_group(group_id) do
[
%Member{
diff --git a/lib/mobilizon_web/resolvers/person.ex b/lib/mobilizon_web/resolvers/person.ex
index dd6fbb1d7..88092c65c 100644
--- a/lib/mobilizon_web/resolvers/person.ex
+++ b/lib/mobilizon_web/resolvers/person.ex
@@ -3,7 +3,7 @@ defmodule MobilizonWeb.Resolvers.Person do
Handles the person-related GraphQL calls
"""
alias Mobilizon.Actors
- alias Mobilizon.Actors.Actor
+ alias Mobilizon.Actors.{Actor, Member}
alias Mobilizon.Users.User
alias Mobilizon.Users
alias Mobilizon.Events
@@ -118,6 +118,7 @@ defmodule MobilizonWeb.Resolvers.Person do
{:find_actor, Actors.get_actor_by_name(preferred_username)},
{:is_owned, true, _} <- User.owns_actor(user, actor.id),
{:last_identity, false} <- {:last_identity, last_identity?(user)},
+ {:last_admin, false} <- {:last_admin, last_admin_of_a_group?(actor.id)},
{:ok, actor} <- Actors.delete_actor(actor) do
{:ok, actor}
else
@@ -127,6 +128,9 @@ defmodule MobilizonWeb.Resolvers.Person do
{:last_identity, true} ->
{:error, "Cannot remove the last identity of a user"}
+ {:last_admin, true} ->
+ {:error, "Cannot remove the last administrator of a group"}
+
{:is_owned, false} ->
{:error, "Actor is not owned by authenticated user"}
end
@@ -213,6 +217,12 @@ defmodule MobilizonWeb.Resolvers.Person do
|> proxify_banner
end
+ # We check that the actor is not the last administrator/creator of a group
+ @spec last_admin_of_a_group?(integer()) :: boolean()
+ defp last_admin_of_a_group?(actor_id) do
+ length(Member.list_group_id_where_last_administrator(actor_id)) > 0
+ end
+
@spec proxify_avatar(Actor.t()) :: Actor.t()
defp proxify_avatar(%Actor{avatar: %{url: avatar_url} = avatar} = actor) do
actor |> Map.put(:avatar, avatar |> Map.put(:url, MobilizonWeb.MediaProxy.url(avatar_url)))
diff --git a/test/mobilizon_web/resolvers/person_resolver_test.exs b/test/mobilizon_web/resolvers/person_resolver_test.exs
index 131834edb..e1ea66d19 100644
--- a/test/mobilizon_web/resolvers/person_resolver_test.exs
+++ b/test/mobilizon_web/resolvers/person_resolver_test.exs
@@ -354,7 +354,7 @@ defmodule MobilizonWeb.Resolvers.PersonResolverTest do
|> auth_conn(user1)
|> post("/api", AbsintheHelpers.mutation_skeleton(mutation))
- assert json_response(res, 200)["data"]["updatePerson"] == nil
+ assert json_response(res, 200)["data"]["deletePerson"] == nil
assert hd(json_response(res, 200)["errors"])["message"] ==
"Actor is not owned by authenticated user"
@@ -377,7 +377,7 @@ defmodule MobilizonWeb.Resolvers.PersonResolverTest do
|> auth_conn(user)
|> post("/api", AbsintheHelpers.mutation_skeleton(mutation))
- assert json_response(res, 200)["data"]["updatePerson"] == nil
+ assert json_response(res, 200)["data"]["deletePerson"] == nil
assert hd(json_response(res, 200)["errors"])["message"] ==
"Actor not found"
@@ -400,12 +400,44 @@ defmodule MobilizonWeb.Resolvers.PersonResolverTest do
|> auth_conn(user)
|> post("/api", AbsintheHelpers.mutation_skeleton(mutation))
- assert json_response(res, 200)["data"]["updatePerson"] == nil
+ assert json_response(res, 200)["data"]["deletePerson"] == nil
assert hd(json_response(res, 200)["errors"])["message"] ==
"Cannot remove the last identity of a user"
end
+ test "delete_person/3 should fail to delete an identity that is the last admin of a group",
+ context do
+ group = insert(:group)
+ classic_user = insert(:user)
+ classic_actor = insert(:actor, user: classic_user, preferred_username: "classic_user")
+
+ admin_user = insert(:user)
+ admin_actor = insert(:actor, user: admin_user, preferred_username: "last_admin")
+ insert(:actor, user: admin_user)
+
+ insert(:member, %{actor: admin_actor, role: :creator, parent: group})
+ insert(:member, %{actor: classic_actor, role: :member, parent: group})
+
+ mutation = """
+ mutation {
+ deletePerson(preferredUsername: "last_admin") {
+ id,
+ }
+ }
+ """
+
+ res =
+ context.conn
+ |> auth_conn(admin_user)
+ |> post("/api", AbsintheHelpers.mutation_skeleton(mutation))
+
+ assert json_response(res, 200)["data"]["deletePerson"] == nil
+
+ assert hd(json_response(res, 200)["errors"])["message"] ==
+ "Cannot remove the last administrator of a group"
+ end
+
test "delete_person/3 should delete a user identity", context do
user = insert(:user)
insert(:actor, user: user, preferred_username: "riri")