From 568819c08afee68636a4871e78838db1ac1f590c Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 11 Jun 2024 17:58:02 -0400 Subject: [PATCH 1/5] WebPush refactoring: separate build and deliver steps --- lib/pleroma/web/push/impl.ex | 95 ++++++++++++------------ lib/pleroma/workers/web_pusher_worker.ex | 4 +- test/pleroma/web/push/impl_test.exs | 77 +++++++++++-------- 3 files changed, 97 insertions(+), 79 deletions(-) diff --git a/lib/pleroma/web/push/impl.ex b/lib/pleroma/web/push/impl.ex index 13c054e05..c5ba7ca65 100644 --- a/lib/pleroma/web/push/impl.ex +++ b/lib/pleroma/web/push/impl.ex @@ -19,69 +19,72 @@ defmodule Pleroma.Web.Push.Impl do @body_chars 140 @types ["Create", "Follow", "Announce", "Like", "Move", "EmojiReact", "Update"] - @doc "Performs sending notifications for user subscriptions" - @spec perform(Notification.t()) :: list(any) | :error | {:error, :unknown_type} - def perform( + @doc "Builds webpush notification payloads for the subscriptions enabled by the receiving user" + @spec build(Notification.t()) :: + list(%{content: map(), subscription: Subscription.t()}) + | :error + | {:error, :unknown_type} + def build( %{ activity: %{data: %{"type" => activity_type}} = activity, - user: %User{id: user_id} + user: user } = notification ) when activity_type in @types do - user = User.get_cached_by_ap_id(notification.activity.data["actor"]) + notification_actor = User.get_cached_by_ap_id(notification.activity.data["actor"]) + avatar_url = User.avatar_url(notification_actor) - gcm_api_key = Application.get_env(:web_push_encryption, :gcm_api_key) - avatar_url = User.avatar_url(user) object = Object.normalize(activity, fetch: false) - user = User.get_cached_by_id(user_id) direct_conversation_id = Activity.direct_conversation_id(activity, user) - for subscription <- fetch_subscriptions(user_id), - Subscription.enabled?(subscription, notification.type) do - %{ - access_token: subscription.token.token, - notification_id: notification.id, - notification_type: notification.type, - icon: avatar_url, - preferred_locale: "en", - pleroma: %{ - activity_id: notification.activity.id, - direct_conversation_id: direct_conversation_id + subscriptions = fetch_subscriptions(user.id) + + subscriptions + |> Enum.filter(&Subscription.enabled?(&1, notification.type)) + |> Enum.map(fn subscription -> + payload = + %{ + access_token: subscription.token.token, + notification_id: notification.id, + notification_type: notification.type, + icon: avatar_url, + preferred_locale: "en", + pleroma: %{ + activity_id: notification.activity.id, + direct_conversation_id: direct_conversation_id + } } - } - |> Map.merge(build_content(notification, user, object)) - |> Jason.encode!() - |> push_message(build_sub(subscription), gcm_api_key, subscription) - end - |> (&{:ok, &1}).() + |> Map.merge(build_content(notification, notification_actor, object)) + |> Jason.encode!() + + %{payload: payload, subscription: subscription} + end) end - def perform(_) do + def build(_) do Logger.warning("Unknown notification type") {:error, :unknown_type} end - @doc "Push message to web" - def push_message(body, sub, api_key, subscription) do - try do - case WebPushEncryption.send_web_push(body, sub, api_key) do - {:ok, %{status: code}} when code in 400..499 -> - Logger.debug("Removing subscription record") - Repo.delete!(subscription) - :ok + @doc "Deliver push notification to the provided webpush subscription" + @spec deliver(%{payload: String.t(), subscription: Subscription.t()}) :: :ok | :error + def deliver(%{payload: payload, subscription: subscription}) do + gcm_api_key = Application.get_env(:web_push_encryption, :gcm_api_key) + formatted_subscription = build_sub(subscription) - {:ok, %{status: code}} when code in 200..299 -> - :ok + case WebPushEncryption.send_web_push(payload, formatted_subscription, gcm_api_key) do + {:ok, %{status: code}} when code in 200..299 -> + :ok - {:ok, %{status: code}} -> - Logger.error("Web Push Notification failed with code: #{code}") - :error + {:ok, %{status: code}} when code in 400..499 -> + Logger.debug("Removing subscription record") + Repo.delete!(subscription) + :ok + + {:ok, %{status: code}} -> + Logger.error("Web Push Notification failed with code: #{code}") + :error - error -> - Logger.error("Web Push Notification failed with #{inspect(error)}") - :error - end - rescue error -> Logger.error("Web Push Notification failed with #{inspect(error)}") :error @@ -140,9 +143,7 @@ defmodule Pleroma.Web.Push.Impl do content_text = content <> "\n" - options_text = - Enum.map(options, fn x -> "○ #{x["name"]}" end) - |> Enum.join("\n") + options_text = Enum.map_join(options, "\n", fn x -> "○ #{x["name"]}" end) [content_text, options_text] |> Enum.join("\n") diff --git a/lib/pleroma/workers/web_pusher_worker.ex b/lib/pleroma/workers/web_pusher_worker.ex index 67e84b0c9..c549d3cd6 100644 --- a/lib/pleroma/workers/web_pusher_worker.ex +++ b/lib/pleroma/workers/web_pusher_worker.ex @@ -5,6 +5,7 @@ defmodule Pleroma.Workers.WebPusherWorker do alias Pleroma.Notification alias Pleroma.Repo + alias Pleroma.Web.Push.Impl use Pleroma.Workers.WorkerHelper, queue: "web_push" @@ -15,7 +16,8 @@ defmodule Pleroma.Workers.WebPusherWorker do |> Repo.get(notification_id) |> Repo.preload([:activity, :user]) - Pleroma.Web.Push.Impl.perform(notification) + Impl.build(notification) + |> Enum.each(&Impl.deliver(&1)) end @impl Oban.Worker diff --git a/test/pleroma/web/push/impl_test.exs b/test/pleroma/web/push/impl_test.exs index c263a1280..7f8dc2e6e 100644 --- a/test/pleroma/web/push/impl_test.exs +++ b/test/pleroma/web/push/impl_test.exs @@ -32,17 +32,6 @@ defmodule Pleroma.Web.Push.ImplTest do :ok end - @sub %{ - endpoint: "https://example.com/example/1234", - keys: %{ - auth: "8eDyX_uCN0XRhSbY5hs7Hg==", - p256dh: - "BCIWgsnyXDv1VkhqL2P7YRBvdeuDnlwAPT2guNhdIoW3IP7GmHh1SMKPLxRf7x8vJy6ZFK3ol2ohgn_-0yP7QQA=" - } - } - @api_key "BASgACIHpN1GYgzSRp" - @message "@Bob: Lorem ipsum dolor sit amet, consectetur adipiscing elit. Fusce sagittis finibus turpis." - test "performs sending notifications" do user = insert(:user) user2 = insert(:user) @@ -68,39 +57,65 @@ defmodule Pleroma.Web.Push.ImplTest do type: "mention" ) - assert Impl.perform(notif) == {:ok, [:ok, :ok]} + Impl.build(notif) + |> Enum.each(fn push -> assert match?(:ok, Impl.deliver(push)) end) end @tag capture_log: true test "returns error if notif does not match " do - assert Impl.perform(%{}) == {:error, :unknown_type} - end - - test "successful message sending" do - assert Impl.push_message(@message, @sub, @api_key, %Subscription{}) == :ok + assert Impl.build(%{}) == {:error, :unknown_type} end @tag capture_log: true test "fail message sending" do - assert Impl.push_message( - @message, - Map.merge(@sub, %{endpoint: "https://example.com/example/bad"}), - @api_key, - %Subscription{} - ) == :error + user = insert(:user) + + insert(:push_subscription, + user: user, + endpoint: "https://example.com/example/bad", + data: %{alerts: %{"follow" => true}} + ) + + other_user = insert(:user) + {:ok, _, _, activity} = CommonAPI.follow(user, other_user) + + notif = + insert(:notification, + user: user, + activity: activity, + type: "follow" + ) + + [push] = Impl.build(notif) + + assert Impl.deliver(push) == :error end test "delete subscription if result send message between 400..500" do - subscription = insert(:push_subscription) + user = insert(:user) - assert Impl.push_message( - @message, - Map.merge(@sub, %{endpoint: "https://example.com/example/not_found"}), - @api_key, - subscription - ) == :ok + bad_subscription = + insert(:push_subscription, + user: user, + endpoint: "https://example.com/example/not_found", + data: %{alerts: %{"follow" => true}} + ) - refute Pleroma.Repo.get(Subscription, subscription.id) + other_user = insert(:user) + {:ok, _, _, activity} = CommonAPI.follow(user, other_user) + + notif = + insert(:notification, + user: user, + activity: activity, + type: "follow" + ) + + [push] = Impl.build(notif) + + assert Impl.deliver(push) == :ok + + refute Pleroma.Repo.get(Subscription, bad_subscription.id) end test "deletes subscription when token has been deleted" do From 603a57576638c9732fe873db3c2b8d56d0a413a5 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 11 Jun 2024 18:14:07 -0400 Subject: [PATCH 2/5] The user is not always preloaded into the notification --- lib/pleroma/web/push/impl.ex | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/pleroma/web/push/impl.ex b/lib/pleroma/web/push/impl.ex index c5ba7ca65..65801922d 100644 --- a/lib/pleroma/web/push/impl.ex +++ b/lib/pleroma/web/push/impl.ex @@ -27,7 +27,7 @@ defmodule Pleroma.Web.Push.Impl do def build( %{ activity: %{data: %{"type" => activity_type}} = activity, - user: user + user_id: user_id } = notification ) when activity_type in @types do @@ -35,9 +35,10 @@ defmodule Pleroma.Web.Push.Impl do avatar_url = User.avatar_url(notification_actor) object = Object.normalize(activity, fetch: false) + user = User.get_cached_by_id(user_id) direct_conversation_id = Activity.direct_conversation_id(activity, user) - subscriptions = fetch_subscriptions(user.id) + subscriptions = fetch_subscriptions(user_id) subscriptions |> Enum.filter(&Subscription.enabled?(&1, notification.type)) From a291a6b8c0c6e27ba6f9a03d9a1183184fc2ef1d Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 11 Jun 2024 18:14:48 -0400 Subject: [PATCH 3/5] Ensure the webpush notification for e.g., mentions start with the nickname of the actor it originates from --- changelog.d/web_push_actor_regression.skip | 0 test/pleroma/web/push/impl_test.exs | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 changelog.d/web_push_actor_regression.skip diff --git a/changelog.d/web_push_actor_regression.skip b/changelog.d/web_push_actor_regression.skip new file mode 100644 index 000000000..e69de29bb diff --git a/test/pleroma/web/push/impl_test.exs b/test/pleroma/web/push/impl_test.exs index 7f8dc2e6e..0eead956f 100644 --- a/test/pleroma/web/push/impl_test.exs +++ b/test/pleroma/web/push/impl_test.exs @@ -417,4 +417,23 @@ defmodule Pleroma.Web.Push.ImplTest do } end end + + test "build/1 notification payload body starts with nickname of actor the notification originated from" do + user = insert(:user, nickname: "Bob") + user2 = insert(:user, nickname: "Tom") + insert(:push_subscription, user: user2, data: %{alerts: %{"mention" => true}}) + + {:ok, activity} = + CommonAPI.post(user, %{ + status: "@Tom Hey are you okay?" + }) + + {:ok, [notification]} = Notification.create_notifications(activity) + + [push] = Impl.build(notification) + + {:ok, payload} = Jason.decode(push.payload) + + assert String.starts_with?(payload["body"], "@Bob:") + end end From 6a9d9da26feefe9b19e8072fd378f01a977b9d1a Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 11 Jun 2024 18:38:41 -0400 Subject: [PATCH 4/5] Cyclical complexity --- lib/pleroma/web/push/impl.ex | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/lib/pleroma/web/push/impl.ex b/lib/pleroma/web/push/impl.ex index 65801922d..2bcfa97f5 100644 --- a/lib/pleroma/web/push/impl.ex +++ b/lib/pleroma/web/push/impl.ex @@ -201,19 +201,15 @@ defmodule Pleroma.Web.Push.Impl do "New Direct Message" end - def format_title(%{type: type}) do - case type do - "mention" -> "New Mention" - "status" -> "New Status" - "follow" -> "New Follower" - "follow_request" -> "New Follow Request" - "reblog" -> "New Repeat" - "favourite" -> "New Favorite" - "update" -> "New Update" - "pleroma:chat_mention" -> "New Chat Message" - "pleroma:emoji_reaction" -> "New Reaction" - "poll" -> "Poll Results" - type -> "New #{String.capitalize(type || "event")}" - end - end + def format_title(%{type: "mention"}), do: "New Mention" + def format_title(%{type: "status"}), do: "New Status" + def format_title(%{type: "follow"}), do: "New Follower" + def format_title(%{type: "follow_request"}), do: "New Follow Request" + def format_title(%{type: "reblog"}), do: "New Repeat" + def format_title(%{type: "favourite"}), do: "New Favorite" + def format_title(%{type: "update"}), do: "New Update" + def format_title(%{type: "pleroma:chat_mention"}), do: "New Chat Message" + def format_title(%{type: "pleroma:emoji_reaction"}), do: "New Reaction" + def format_title(%{type: "poll"}), do: "Poll Results" + def format_title(%{type: type}), do: "New #{String.capitalize(type || "event")}" end From 5c8afbe646c874eea32d7063aa499c97191f3a6e Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 11 Jun 2024 18:54:22 -0400 Subject: [PATCH 5/5] Fix tests --- lib/pleroma/web/push/impl.ex | 10 ++++------ test/pleroma/web/push/impl_test.exs | 7 +++++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/pleroma/web/push/impl.ex b/lib/pleroma/web/push/impl.ex index 2bcfa97f5..d71e134cb 100644 --- a/lib/pleroma/web/push/impl.ex +++ b/lib/pleroma/web/push/impl.ex @@ -21,9 +21,7 @@ defmodule Pleroma.Web.Push.Impl do @doc "Builds webpush notification payloads for the subscriptions enabled by the receiving user" @spec build(Notification.t()) :: - list(%{content: map(), subscription: Subscription.t()}) - | :error - | {:error, :unknown_type} + list(%{content: map(), subscription: Subscription.t()}) | [] def build( %{ activity: %{data: %{"type" => activity_type}} = activity, @@ -62,9 +60,9 @@ defmodule Pleroma.Web.Push.Impl do end) end - def build(_) do - Logger.warning("Unknown notification type") - {:error, :unknown_type} + def build(notif) do + Logger.warning("WebPush: unknown activity type: #{inspect(notif)}") + [] end @doc "Deliver push notification to the provided webpush subscription" diff --git a/test/pleroma/web/push/impl_test.exs b/test/pleroma/web/push/impl_test.exs index 0eead956f..169c380c7 100644 --- a/test/pleroma/web/push/impl_test.exs +++ b/test/pleroma/web/push/impl_test.exs @@ -5,6 +5,7 @@ defmodule Pleroma.Web.Push.ImplTest do use Pleroma.DataCase, async: true + import ExUnit.CaptureLog import Mox import Pleroma.Factory @@ -62,8 +63,10 @@ defmodule Pleroma.Web.Push.ImplTest do end @tag capture_log: true - test "returns error if notif does not match " do - assert Impl.build(%{}) == {:error, :unknown_type} + test "returns error if notification activity type does not match" do + assert capture_log(fn -> + assert Impl.build(%{}) == [] + end) =~ "WebPush: unknown activity type" end @tag capture_log: true