From a5af7f99ff46f35862bfe6a734a8ac1457d6612a Mon Sep 17 00:00:00 2001 From: Chocobozzz 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")