From e2ced0491770d6260fe51d5144b81200fd97f268 Mon Sep 17 00:00:00 2001 From: lain Date: Thu, 16 Apr 2020 15:21:47 +0200 Subject: [PATCH] ChatMessages: Better validation. --- .../web/activity_pub/object_validator.ex | 6 ++- .../chat_message_validator.ex | 26 ++++++++++ .../object_validators/common_validations.ex | 6 ++- .../create_chat_message_validator.ex | 5 ++ .../transmogrifier/chat_message_handling.ex | 3 ++ test/fixtures/create-chat-message.json | 2 +- .../activity_pub/object_validator_test.exs | 52 +++++++++++++++++++ .../transmogrifier/chat_message_test.exs | 34 +++++++++++- 8 files changed, 128 insertions(+), 6 deletions(-) diff --git a/lib/pleroma/web/activity_pub/object_validator.ex b/lib/pleroma/web/activity_pub/object_validator.ex index 49cc72561..259bbeb64 100644 --- a/lib/pleroma/web/activity_pub/object_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validator.ex @@ -31,7 +31,8 @@ def validate(%{"type" => "Like"} = object, meta) do def validate(%{"type" => "ChatMessage"} = object, meta) do with {:ok, object} <- object - |> ChatMessageValidator.cast_and_apply() do + |> ChatMessageValidator.cast_and_validate() + |> Ecto.Changeset.apply_action(:insert) do object = stringify_keys(object) {:ok, object, meta} end @@ -40,7 +41,8 @@ def validate(%{"type" => "ChatMessage"} = object, meta) do def validate(%{"type" => "Create"} = object, meta) do with {:ok, object} <- object - |> CreateChatMessageValidator.cast_and_apply() do + |> CreateChatMessageValidator.cast_and_validate() + |> Ecto.Changeset.apply_action(:insert) do object = stringify_keys(object) {:ok, object, meta} end diff --git a/lib/pleroma/web/activity_pub/object_validators/chat_message_validator.ex b/lib/pleroma/web/activity_pub/object_validators/chat_message_validator.ex index ab5be3596..a4e4460cd 100644 --- a/lib/pleroma/web/activity_pub/object_validators/chat_message_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validators/chat_message_validator.ex @@ -6,6 +6,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.ChatMessageValidator do use Ecto.Schema alias Pleroma.Web.ActivityPub.ObjectValidators.Types + alias Pleroma.User import Ecto.Changeset @@ -54,5 +55,30 @@ def validate_data(data_cng) do data_cng |> validate_inclusion(:type, ["ChatMessage"]) |> validate_required([:id, :actor, :to, :type, :content]) + |> validate_length(:to, is: 1) + |> validate_local_concern() + end + + @doc "Validates if at least one of the users in this ChatMessage is a local user, otherwise we don't want the message in our system. It also validates the presence of both users in our system." + def validate_local_concern(cng) do + with actor_ap <- get_field(cng, :actor), + {_, %User{} = actor} <- {:find_actor, User.get_cached_by_ap_id(actor_ap)}, + {_, %User{} = recipient} <- + {:find_recipient, User.get_cached_by_ap_id(get_field(cng, :to) |> hd())}, + {_, true} <- {:local?, Enum.any?([actor, recipient], & &1.local)} do + cng + else + {:local?, false} -> + cng + |> add_error(:actor, "actor and recipient are both remote") + + {:find_actor, _} -> + cng + |> add_error(:actor, "can't find user") + + {:find_recipient, _} -> + cng + |> add_error(:to, "can't find user") + end end end diff --git a/lib/pleroma/web/activity_pub/object_validators/common_validations.ex b/lib/pleroma/web/activity_pub/object_validators/common_validations.ex index b479c3918..02f3a6438 100644 --- a/lib/pleroma/web/activity_pub/object_validators/common_validations.ex +++ b/lib/pleroma/web/activity_pub/object_validators/common_validations.ex @@ -8,7 +8,11 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.CommonValidations do alias Pleroma.Object alias Pleroma.User - def validate_actor_presence(cng, field_name \\ :actor) do + def validate_actor_presence(cng) do + validate_user_presence(cng, :actor) + end + + def validate_user_presence(cng, field_name) do cng |> validate_change(field_name, fn field_name, actor -> if User.get_cached_by_ap_id(actor) do diff --git a/lib/pleroma/web/activity_pub/object_validators/create_chat_message_validator.ex b/lib/pleroma/web/activity_pub/object_validators/create_chat_message_validator.ex index 659311480..ce52d5623 100644 --- a/lib/pleroma/web/activity_pub/object_validators/create_chat_message_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validators/create_chat_message_validator.ex @@ -32,4 +32,9 @@ def cast_and_apply(data) do def cast_data(data) do cast(%__MODULE__{}, data, __schema__(:fields)) end + + # No validation yet + def cast_and_validate(data) do + cast_data(data) + end end diff --git a/lib/pleroma/web/activity_pub/transmogrifier/chat_message_handling.ex b/lib/pleroma/web/activity_pub/transmogrifier/chat_message_handling.ex index b5843736f..815b866c9 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier/chat_message_handling.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier/chat_message_handling.ex @@ -25,6 +25,9 @@ def handle_incoming( {_, {:ok, activity, _meta}} <- {:common_pipeline, Pipeline.common_pipeline(cast_data, local: false)} do {:ok, activity} + else + e -> + {:error, e} end end end diff --git a/test/fixtures/create-chat-message.json b/test/fixtures/create-chat-message.json index 4aa17f4a5..2e4608f43 100644 --- a/test/fixtures/create-chat-message.json +++ b/test/fixtures/create-chat-message.json @@ -3,7 +3,7 @@ "id": "http://2hu.gensokyo/objects/1", "object": { "attributedTo": "http://2hu.gensokyo/users/raymoo", - "content": "You expected a cute girl? Too bad.", + "content": "You expected a cute girl? Too bad. ", "id": "http://2hu.gensokyo/objects/2", "published": "2020-02-12T14:08:20Z", "to": [ diff --git a/test/web/activity_pub/object_validator_test.exs b/test/web/activity_pub/object_validator_test.exs index 3c5c3696e..bf0bfdfaf 100644 --- a/test/web/activity_pub/object_validator_test.exs +++ b/test/web/activity_pub/object_validator_test.exs @@ -5,9 +5,61 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidatorTest do alias Pleroma.Web.ActivityPub.ObjectValidators.LikeValidator alias Pleroma.Web.ActivityPub.Utils alias Pleroma.Web.CommonAPI + alias Pleroma.Web.ActivityPub.Builder import Pleroma.Factory + describe "chat messages" do + setup do + user = insert(:user) + recipient = insert(:user, local: false) + + {:ok, valid_chat_message, _} = Builder.chat_message(user, recipient.ap_id, "hey") + + %{user: user, recipient: recipient, valid_chat_message: valid_chat_message} + end + + test "validates for a basic object we build", %{valid_chat_message: valid_chat_message} do + assert {:ok, _object, _meta} = ObjectValidator.validate(valid_chat_message, []) + end + + test "does not validate if the actor or the recipient is not in our system", %{ + valid_chat_message: valid_chat_message + } do + chat_message = + valid_chat_message + |> Map.put("actor", "https://raymoo.com/raymoo") + + {:error, _} = ObjectValidator.validate(chat_message, []) + + chat_message = + valid_chat_message + |> Map.put("to", ["https://raymoo.com/raymoo"]) + + {:error, _} = ObjectValidator.validate(chat_message, []) + end + + test "does not validate for a message with multiple recipients", %{ + valid_chat_message: valid_chat_message, + user: user, + recipient: recipient + } do + chat_message = + valid_chat_message + |> Map.put("to", [user.ap_id, recipient.ap_id]) + + assert {:error, _} = ObjectValidator.validate(chat_message, []) + end + + test "does not validate if it doesn't concern local users" do + user = insert(:user, local: false) + recipient = insert(:user, local: false) + + {:ok, valid_chat_message, _} = Builder.chat_message(user, recipient.ap_id, "hey") + assert {:error, _} = ObjectValidator.validate(valid_chat_message, []) + end + end + describe "likes" do setup do user = insert(:user) diff --git a/test/web/activity_pub/transmogrifier/chat_message_test.exs b/test/web/activity_pub/transmogrifier/chat_message_test.exs index aed62c520..5b238f9c4 100644 --- a/test/web/activity_pub/transmogrifier/chat_message_test.exs +++ b/test/web/activity_pub/transmogrifier/chat_message_test.exs @@ -12,13 +12,43 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.ChatMessageTest do alias Pleroma.Web.ActivityPub.Transmogrifier describe "handle_incoming" do - test "it insert it" do + test "it rejects messages that don't contain content" do + data = + File.read!("test/fixtures/create-chat-message.json") + |> Poison.decode!() + + object = + data["object"] + |> Map.delete("content") + + data = + data + |> Map.put("object", object) + + _author = insert(:user, ap_id: data["actor"], local: false) + _recipient = insert(:user, ap_id: List.first(data["to"]), local: true) + + {:error, _} = Transmogrifier.handle_incoming(data) + end + + test "it rejects messages that don't concern local users" do + data = + File.read!("test/fixtures/create-chat-message.json") + |> Poison.decode!() + + _author = insert(:user, ap_id: data["actor"], local: false) + _recipient = insert(:user, ap_id: List.first(data["to"]), local: false) + + {:error, _} = Transmogrifier.handle_incoming(data) + end + + test "it inserts it and creates a chat" do data = File.read!("test/fixtures/create-chat-message.json") |> Poison.decode!() author = insert(:user, ap_id: data["actor"], local: false) - recipient = insert(:user, ap_id: List.first(data["to"]), local: false) + recipient = insert(:user, ap_id: List.first(data["to"]), local: true) {:ok, %Activity{} = activity} = Transmogrifier.handle_incoming(data)