From 4a2fe900cd7ad984580fa5fc36f93b2eac937bfb Mon Sep 17 00:00:00 2001
From: Thomas Citharel <tcit@tcit.fr>
Date: Wed, 24 Nov 2021 16:14:09 +0100
Subject: [PATCH] Refactor and test
 Mobilizon.Federation.ActivityPub.Utils.get_actor/1

Raise exception when object contains no actor. Friendica seems to send an Update activity with no actor

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
---
 lib/federation/activity_pub/utils.ex        | 46 +++++++++---
 test/federation/activity_pub/utils_test.exs | 79 +++++++++++++++++++++
 2 files changed, 116 insertions(+), 9 deletions(-)

diff --git a/lib/federation/activity_pub/utils.ex b/lib/federation/activity_pub/utils.ex
index c19f7688f..1d827acdb 100644
--- a/lib/federation/activity_pub/utils.ex
+++ b/lib/federation/activity_pub/utils.ex
@@ -21,6 +21,7 @@ defmodule Mobilizon.Federation.ActivityPub.Utils do
   require Logger
 
   @actor_types ["Group", "Person", "Application"]
+  @all_actor_types @actor_types ++ ["Organization", "Service"]
 
   # Wraps an object into an activity
   @spec create_activity(map(), boolean()) :: {:ok, Activity.t()}
@@ -286,24 +287,51 @@ defmodule Mobilizon.Federation.ActivityPub.Utils do
     actor
   end
 
-  def get_actor(%{"actor" => actor}) when is_list(actor) do
-    if is_binary(Enum.at(actor, 0)) do
-      Enum.at(actor, 0)
-    else
-      actor
-      |> Enum.find(fn %{"type" => type} -> type in ["Person", "Service", "Application"] end)
-      |> Map.get("id")
+  def get_actor(%{"actor" => [actor | tail] = actor_list} = object)
+      when is_list(actor_list) and length(actor_list) > 0 do
+    res =
+      try do
+        object
+        |> Map.put("actor", actor)
+        |> get_actor()
+      rescue
+        ArgumentError -> nil
+      end
+
+    case res do
+      id when is_binary(id) ->
+        id
+
+      _ ->
+        object
+        |> Map.put("actor", tail)
+        |> get_actor()
     end
   end
 
-  def get_actor(%{"actor" => %{"id" => id}}) when is_binary(id) do
+  def get_actor(%{"actor" => %{"id" => id, "type" => type}})
+      when is_binary(id) and type in @all_actor_types do
     id
   end
 
-  def get_actor(%{"actor" => nil, "attributedTo" => actor}) when not is_nil(actor) do
+  def get_actor(%{"actor" => _, "attributedTo" => actor}) when not is_nil(actor) do
     get_actor(%{"actor" => actor})
   end
 
+  def get_actor(%{"actor" => %{"id" => id, "type" => type}})
+      when is_binary(id) do
+    raise ArgumentError,
+      message: "Object contains an actor object with invalid type: #{inspect(type)}"
+  end
+
+  def get_actor(%{"actor" => nil, "attributedTo" => nil}) do
+    raise ArgumentError, message: "Object contains both actor and attributedTo fields being null"
+  end
+
+  def get_actor(%{"actor" => _}) do
+    raise ArgumentError, message: "Object contains not actor information"
+  end
+
   @doc """
   Checks that an incoming AP object's actor matches the domain it came from.
 
diff --git a/test/federation/activity_pub/utils_test.exs b/test/federation/activity_pub/utils_test.exs
index 9dd2ae8e0..28a271b76 100644
--- a/test/federation/activity_pub/utils_test.exs
+++ b/test/federation/activity_pub/utils_test.exs
@@ -59,4 +59,83 @@ defmodule Mobilizon.Federation.ActivityPub.UtilsTest do
              })
     end
   end
+
+  describe "get_actor/1" do
+    test "with a string" do
+      assert Utils.get_actor(%{"actor" => "https://somewhere.tld/@someone"}) ==
+               "https://somewhere.tld/@someone"
+    end
+
+    test "with an object" do
+      assert Utils.get_actor(%{
+               "actor" => %{"id" => "https://somewhere.tld/@someone", "type" => "Person"}
+             }) ==
+               "https://somewhere.tld/@someone"
+    end
+
+    test "with an invalid object" do
+      assert_raise ArgumentError,
+                   "Object contains an actor object with invalid type: \"Else\"",
+                   fn ->
+                     Utils.get_actor(%{
+                       "actor" => %{"id" => "https://somewhere.tld/@someone", "type" => "Else"}
+                     })
+                   end
+    end
+
+    test "with a list" do
+      assert Utils.get_actor(%{
+               "actor" => ["https://somewhere.tld/@someone", "https://somewhere.else/@other"]
+             }) ==
+               "https://somewhere.tld/@someone"
+    end
+
+    test "with a list of objects" do
+      assert Utils.get_actor(%{
+               "actor" => [
+                 %{"type" => "Person", "id" => "https://somewhere.tld/@someone"},
+                 "https://somewhere.else/@other"
+               ]
+             }) ==
+               "https://somewhere.tld/@someone"
+    end
+
+    test "with a list of objects containing an invalid one" do
+      assert Utils.get_actor(%{
+               "actor" => [
+                 %{"type" => "Else", "id" => "https://somewhere.tld/@someone"},
+                 "https://somewhere.else/@other"
+               ]
+             }) ==
+               "https://somewhere.else/@other"
+    end
+
+    test "with an empty list" do
+      assert_raise ArgumentError,
+                   "Object contains not actor information",
+                   fn ->
+                     Utils.get_actor(%{
+                       "actor" => []
+                     })
+                   end
+    end
+
+    test "fallbacks to attributed_to" do
+      assert Utils.get_actor(%{
+               "actor" => nil,
+               "attributedTo" => "https://somewhere.tld/@someone"
+             }) == "https://somewhere.tld/@someone"
+    end
+
+    test "with no actor information" do
+      assert_raise ArgumentError,
+                   "Object contains both actor and attributedTo fields being null",
+                   fn ->
+                     Utils.get_actor(%{
+                       "actor" => nil,
+                       "attributedTo" => nil
+                     })
+                   end
+    end
+  end
 end