[#3112] Ensured presence and consistency of :user and :token assigns (EnsureUserToken...
authorIvan Tashkinov <ivantashkinov@gmail.com>
Sun, 6 Dec 2020 10:59:10 +0000 (13:59 +0300)
committerIvan Tashkinov <ivantashkinov@gmail.com>
Sun, 6 Dec 2020 10:59:10 +0000 (13:59 +0300)
14 files changed:
lib/pleroma/helpers/auth_helper.ex
lib/pleroma/web.ex
lib/pleroma/web/plugs/admin_secret_authentication_plug.ex
lib/pleroma/web/plugs/ensure_user_key_plug.ex [deleted file]
lib/pleroma/web/plugs/ensure_user_token_assigns_plug.ex [new file with mode: 0644]
lib/pleroma/web/plugs/mapped_signature_to_identity_plug.ex
lib/pleroma/web/plugs/o_auth_scopes_plug.ex
lib/pleroma/web/plugs/user_enabled_plug.ex
lib/pleroma/web/router.ex
test/pleroma/web/admin_api/controllers/admin_api_controller_test.exs
test/pleroma/web/admin_api/controllers/config_controller_test.exs
test/pleroma/web/pleroma_api/controllers/chat_controller_test.exs
test/pleroma/web/plugs/ensure_user_key_plug_test.exs [deleted file]
test/pleroma/web/plugs/ensure_user_token_assigns_plug_test.exs [new file with mode: 0644]

index 392fa7d5dc9f9ca827355f43ab3eae0b18ecefbc..8f87b38be99bdb99a3b95287801026f96e07c8c6 100644 (file)
@@ -20,20 +20,26 @@ defmodule Pleroma.Helpers.AuthHelper do
     |> OAuthScopesPlug.skip_plug()
   end
 
+  @doc "Drops authentication info from connection"
   def drop_auth_info(conn) do
+    # To simplify debugging, setting a private variable on `conn` if auth info is dropped
     conn
     |> assign(:user, nil)
     |> assign(:token, nil)
+    |> put_private(:authentication_ignored, true)
   end
 
+  @doc "Gets OAuth token string from session"
   def get_session_token(%Conn{} = conn) do
     get_session(conn, @oauth_token_session_key)
   end
 
+  @doc "Updates OAuth token string in session"
   def put_session_token(%Conn{} = conn, token) when is_binary(token) do
     put_session(conn, @oauth_token_session_key, token)
   end
 
+  @doc "Deletes OAuth token string from session"
   def delete_session_token(%Conn{} = conn) do
     delete_session(conn, @oauth_token_session_key)
   end
index 6ed19d3dd9dfdf5c345564a680c0073992c6bd91..3ca20455d930fd4600c574b467efa64f02ccc8b4 100644 (file)
@@ -20,6 +20,7 @@ defmodule Pleroma.Web do
   below.
   """
 
+  alias Pleroma.Helpers.AuthHelper
   alias Pleroma.Web.Plugs.EnsureAuthenticatedPlug
   alias Pleroma.Web.Plugs.EnsurePublicOrAuthenticatedPlug
   alias Pleroma.Web.Plugs.ExpectAuthenticatedCheckPlug
@@ -75,7 +76,7 @@ defmodule Pleroma.Web do
       defp maybe_drop_authentication_if_oauth_check_ignored(conn) do
         if PlugHelper.plug_called?(conn, ExpectPublicOrAuthenticatedCheckPlug) and
              not PlugHelper.plug_called_or_skipped?(conn, OAuthScopesPlug) do
-          OAuthScopesPlug.drop_auth_info(conn)
+          AuthHelper.drop_auth_info(conn)
         else
           conn
         end
index ff49801f4cca8692160b90e15e835fe32a658e30..ff851a8748e543f21a7bab41ff8e2f855265a2ff 100644 (file)
@@ -13,13 +13,6 @@ defmodule Pleroma.Web.Plugs.AdminSecretAuthenticationPlug do
     options
   end
 
-  def secret_token do
-    case Pleroma.Config.get(:admin_token) do
-      blank when blank in [nil, ""] -> nil
-      token -> token
-    end
-  end
-
   def call(%{assigns: %{user: %User{}}} = conn, _), do: conn
 
   def call(conn, _) do
@@ -30,7 +23,7 @@ defmodule Pleroma.Web.Plugs.AdminSecretAuthenticationPlug do
     end
   end
 
-  def authenticate(%{params: %{"admin_token" => admin_token}} = conn) do
+  defp authenticate(%{params: %{"admin_token" => admin_token}} = conn) do
     if admin_token == secret_token() do
       assign_admin_user(conn)
     else
@@ -38,7 +31,7 @@ defmodule Pleroma.Web.Plugs.AdminSecretAuthenticationPlug do
     end
   end
 
-  def authenticate(conn) do
+  defp authenticate(conn) do
     token = secret_token()
 
     case get_req_header(conn, "x-admin-token") do
@@ -48,6 +41,13 @@ defmodule Pleroma.Web.Plugs.AdminSecretAuthenticationPlug do
     end
   end
 
+  defp secret_token do
+    case Pleroma.Config.get(:admin_token) do
+      blank when blank in [nil, ""] -> nil
+      token -> token
+    end
+  end
+
   defp assign_admin_user(conn) do
     conn
     |> assign(:user, %User{is_admin: true})
diff --git a/lib/pleroma/web/plugs/ensure_user_key_plug.ex b/lib/pleroma/web/plugs/ensure_user_key_plug.ex
deleted file mode 100644 (file)
index 31608db..0000000
+++ /dev/null
@@ -1,19 +0,0 @@
-# Pleroma: A lightweight social networking server
-# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
-# SPDX-License-Identifier: AGPL-3.0-only
-
-defmodule Pleroma.Web.Plugs.EnsureUserKeyPlug do
-  import Plug.Conn
-
-  @moduledoc "Ensures `conn.assigns.user` is initialized."
-
-  def init(opts) do
-    opts
-  end
-
-  def call(%{assigns: %{user: _}} = conn, _), do: conn
-
-  def call(conn, _) do
-    assign(conn, :user, nil)
-  end
-end
diff --git a/lib/pleroma/web/plugs/ensure_user_token_assigns_plug.ex b/lib/pleroma/web/plugs/ensure_user_token_assigns_plug.ex
new file mode 100644 (file)
index 0000000..4253458
--- /dev/null
@@ -0,0 +1,36 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
+# SPDX-License-Identifier: AGPL-3.0-only
+
+defmodule Pleroma.Web.Plugs.EnsureUserTokenAssignsPlug do
+  import Plug.Conn
+
+  alias Pleroma.Helpers.AuthHelper
+  alias Pleroma.User
+  alias Pleroma.Web.OAuth.Token
+
+  @moduledoc "Ensures presence and consistency of :user and :token assigns."
+
+  def init(opts) do
+    opts
+  end
+
+  def call(%{assigns: %{user: %User{id: user_id}} = assigns} = conn, _) do
+    with %Token{user_id: ^user_id} <- assigns[:token] do
+      conn
+    else
+      %Token{} ->
+        # A safety net for abnormal (unexpected) scenario: :token belongs to another user
+        AuthHelper.drop_auth_info(conn)
+
+      _ ->
+        assign(conn, :token, nil)
+    end
+  end
+
+  def call(conn, _) do
+    conn
+    |> assign(:user, nil)
+    |> assign(:token, nil)
+  end
+end
index f44d4dee533b0bb7b2cd978b8b0662167fde8286..a0a0c5a9be0aa15287fe47fd0f953ac599a3bdd3 100644 (file)
@@ -3,6 +3,7 @@
 # SPDX-License-Identifier: AGPL-3.0-only
 
 defmodule Pleroma.Web.Plugs.MappedSignatureToIdentityPlug do
+  alias Pleroma.Helpers.AuthHelper
   alias Pleroma.Signature
   alias Pleroma.User
   alias Pleroma.Web.ActivityPub.Utils
@@ -12,34 +13,16 @@ defmodule Pleroma.Web.Plugs.MappedSignatureToIdentityPlug do
 
   def init(options), do: options
 
-  defp key_id_from_conn(conn) do
-    with %{"keyId" => key_id} <- HTTPSignatures.signature_for_conn(conn),
-         {:ok, ap_id} <- Signature.key_id_to_actor_id(key_id) do
-      ap_id
-    else
-      _ ->
-        nil
-    end
-  end
-
-  defp user_from_key_id(conn) do
-    with key_actor_id when is_binary(key_actor_id) <- key_id_from_conn(conn),
-         {:ok, %User{} = user} <- User.get_or_fetch_by_ap_id(key_actor_id) do
-      user
-    else
-      _ ->
-        nil
-    end
-  end
-
-  def call(%{assigns: %{user: _}} = conn, _opts), do: conn
+  def call(%{assigns: %{user: %User{}}} = conn, _opts), do: conn
 
   # if this has payload make sure it is signed by the same actor that made it
   def call(%{assigns: %{valid_signature: true}, params: %{"actor" => actor}} = conn, _opts) do
     with actor_id <- Utils.get_ap_id(actor),
          {:user, %User{} = user} <- {:user, user_from_key_id(conn)},
          {:user_match, true} <- {:user_match, user.ap_id == actor_id} do
-      assign(conn, :user, user)
+      conn
+      |> assign(:user, user)
+      |> AuthHelper.skip_oauth()
     else
       {:user_match, false} ->
         Logger.debug("Failed to map identity from signature (payload actor mismatch)")
@@ -57,7 +40,9 @@ defmodule Pleroma.Web.Plugs.MappedSignatureToIdentityPlug do
   # no payload, probably a signed fetch
   def call(%{assigns: %{valid_signature: true}} = conn, _opts) do
     with %User{} = user <- user_from_key_id(conn) do
-      assign(conn, :user, user)
+      conn
+      |> assign(:user, user)
+      |> AuthHelper.skip_oauth()
     else
       _ ->
         Logger.debug("Failed to map identity from signature (no payload actor mismatch)")
@@ -68,4 +53,24 @@ defmodule Pleroma.Web.Plugs.MappedSignatureToIdentityPlug do
 
   # no signature at all
   def call(conn, _opts), do: conn
+
+  defp key_id_from_conn(conn) do
+    with %{"keyId" => key_id} <- HTTPSignatures.signature_for_conn(conn),
+         {:ok, ap_id} <- Signature.key_id_to_actor_id(key_id) do
+      ap_id
+    else
+      _ ->
+        nil
+    end
+  end
+
+  defp user_from_key_id(conn) do
+    with key_actor_id when is_binary(key_actor_id) <- key_id_from_conn(conn),
+         {:ok, %User{} = user} <- User.get_or_fetch_by_ap_id(key_actor_id) do
+      user
+    else
+      _ ->
+        nil
+    end
+  end
 end
index cfc30837c0244bc579dd7adafde57915f7950686..e6d398b149e81aa4554eb942a24adb27e8c697a0 100644 (file)
@@ -7,6 +7,7 @@ defmodule Pleroma.Web.Plugs.OAuthScopesPlug do
   import Pleroma.Web.Gettext
 
   alias Pleroma.Config
+  alias Pleroma.Helpers.AuthHelper
 
   use Pleroma.Web, :plug
 
@@ -28,7 +29,7 @@ defmodule Pleroma.Web.Plugs.OAuthScopesPlug do
         conn
 
       options[:fallback] == :proceed_unauthenticated ->
-        drop_auth_info(conn)
+        AuthHelper.drop_auth_info(conn)
 
       true ->
         missing_scopes = scopes -- matched_scopes
@@ -44,15 +45,6 @@ defmodule Pleroma.Web.Plugs.OAuthScopesPlug do
     end
   end
 
-  @doc "Drops authentication info from connection"
-  def drop_auth_info(conn) do
-    # To simplify debugging, setting a private variable on `conn` if auth info is dropped
-    conn
-    |> put_private(:authentication_ignored, true)
-    |> assign(:user, nil)
-    |> assign(:token, nil)
-  end
-
   @doc "Keeps those of `scopes` which are descendants of `supported_scopes`"
   def filter_descendants(scopes, supported_scopes) do
     Enum.filter(
index 291d1f568009753751cd6fd04d14827d2f4301e1..4f1b163bd836980ef52011bad88a4e406d901f6b 100644 (file)
@@ -11,12 +11,10 @@ defmodule Pleroma.Web.Plugs.UserEnabledPlug do
   end
 
   def call(%{assigns: %{user: %User{} = user}} = conn, _) do
-    case User.account_status(user) do
-      :active ->
-        conn
-
-      _ ->
-        AuthHelper.drop_auth_info(conn)
+    if User.account_status(user) == :active do
+      conn
+    else
+      AuthHelper.drop_auth_info(conn)
     end
   end
 
index b3462ba000d66b6098b632abaf02e9936bfd5ac8..aefc9f0be47598df4bcf0acbff48c5347277e5ce 100644 (file)
@@ -34,7 +34,7 @@ defmodule Pleroma.Web.Router do
     plug(:fetch_session)
     plug(Pleroma.Web.Plugs.OAuthPlug)
     plug(Pleroma.Web.Plugs.UserEnabledPlug)
-    plug(Pleroma.Web.Plugs.EnsureUserKeyPlug)
+    plug(Pleroma.Web.Plugs.EnsureUserTokenAssignsPlug)
   end
 
   pipeline :expect_authentication do
@@ -55,7 +55,7 @@ defmodule Pleroma.Web.Router do
   pipeline :after_auth do
     plug(Pleroma.Web.Plugs.UserEnabledPlug)
     plug(Pleroma.Web.Plugs.SetUserSessionIdPlug)
-    plug(Pleroma.Web.Plugs.EnsureUserKeyPlug)
+    plug(Pleroma.Web.Plugs.EnsureUserTokenAssignsPlug)
   end
 
   pipeline :base_api do
@@ -99,7 +99,7 @@ defmodule Pleroma.Web.Router do
   pipeline :pleroma_html do
     plug(:browser)
     plug(:authenticate)
-    plug(Pleroma.Web.Plugs.EnsureUserKeyPlug)
+    plug(Pleroma.Web.Plugs.EnsureUserTokenAssignsPlug)
   end
 
   pipeline :well_known do
@@ -291,7 +291,6 @@ defmodule Pleroma.Web.Router do
 
     post("/main/ostatus", UtilController, :remote_subscribe)
     get("/ostatus_subscribe", RemoteFollowController, :follow)
-
     post("/ostatus_subscribe", RemoteFollowController, :do_follow)
   end
 
index c06ae55cae2c7580c3850ab5cd8f12060b52bf57..e50d1425b78c068851f434f03f1bf7222bf57d99 100644 (file)
@@ -941,7 +941,6 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIControllerTest do
 
   describe "/api/pleroma/admin/stats" do
     test "status visibility count", %{conn: conn} do
-      admin = insert(:user, is_admin: true)
       user = insert(:user)
       CommonAPI.post(user, %{visibility: "public", status: "hey"})
       CommonAPI.post(user, %{visibility: "unlisted", status: "hey"})
@@ -949,7 +948,6 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIControllerTest do
 
       response =
         conn
-        |> assign(:user, admin)
         |> get("/api/pleroma/admin/stats")
         |> json_response(200)
 
@@ -958,7 +956,6 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIControllerTest do
     end
 
     test "by instance", %{conn: conn} do
-      admin = insert(:user, is_admin: true)
       user1 = insert(:user)
       instance2 = "instance2.tld"
       user2 = insert(:user, %{ap_id: "https://#{instance2}/@actor"})
@@ -969,7 +966,6 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIControllerTest do
 
       response =
         conn
-        |> assign(:user, admin)
         |> get("/api/pleroma/admin/stats", instance: instance2)
         |> json_response(200)
 
index 4e897455f03bbc0b241fa04052898def8556639e..765a5a4b7fc35e29105a53837d14615f930affb2 100644 (file)
@@ -1415,11 +1415,7 @@ defmodule Pleroma.Web.AdminAPI.ConfigControllerTest do
 
   describe "GET /api/pleroma/admin/config/descriptions" do
     test "structure", %{conn: conn} do
-      admin = insert(:user, is_admin: true)
-
-      conn =
-        assign(conn, :user, admin)
-        |> get("/api/pleroma/admin/config/descriptions")
+      conn = get(conn, "/api/pleroma/admin/config/descriptions")
 
       assert [child | _others] = json_response_and_validate_schema(conn, 200)
 
@@ -1437,11 +1433,7 @@ defmodule Pleroma.Web.AdminAPI.ConfigControllerTest do
         {:esshd}
       ])
 
-      admin = insert(:user, is_admin: true)
-
-      conn =
-        assign(conn, :user, admin)
-        |> get("/api/pleroma/admin/config/descriptions")
+      conn = get(conn, "/api/pleroma/admin/config/descriptions")
 
       children = json_response_and_validate_schema(conn, 200)
 
index c1e6a8cc5e18bddd931b3be6ca33535dd433fd88..a6c9d0c1b56bb26a247ff3963f5c59df72bb5ca4 100644 (file)
@@ -264,9 +264,10 @@ defmodule Pleroma.Web.PleromaAPI.ChatControllerTest do
       assert length(result) == 3
 
       # Trying to get the chat of a different user
+      other_user_chat = Chat.get(other_user.id, user.ap_id)
+
       conn
-      |> assign(:user, other_user)
-      |> get("/api/v1/pleroma/chats/#{chat.id}/messages")
+      |> get("/api/v1/pleroma/chats/#{other_user_chat.id}/messages")
       |> json_response_and_validate_schema(404)
     end
   end
diff --git a/test/pleroma/web/plugs/ensure_user_key_plug_test.exs b/test/pleroma/web/plugs/ensure_user_key_plug_test.exs
deleted file mode 100644 (file)
index f912ef7..0000000
+++ /dev/null
@@ -1,29 +0,0 @@
-# Pleroma: A lightweight social networking server
-# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
-# SPDX-License-Identifier: AGPL-3.0-only
-
-defmodule Pleroma.Web.Plugs.EnsureUserKeyPlugTest do
-  use Pleroma.Web.ConnCase, async: true
-
-  alias Pleroma.Web.Plugs.EnsureUserKeyPlug
-
-  test "if the conn has a user key set, it does nothing", %{conn: conn} do
-    conn =
-      conn
-      |> assign(:user, 1)
-
-    ret_conn =
-      conn
-      |> EnsureUserKeyPlug.call(%{})
-
-    assert conn == ret_conn
-  end
-
-  test "if the conn has no key set, it sets it to nil", %{conn: conn} do
-    conn =
-      conn
-      |> EnsureUserKeyPlug.call(%{})
-
-    assert Map.has_key?(conn.assigns, :user)
-  end
-end
diff --git a/test/pleroma/web/plugs/ensure_user_token_assigns_plug_test.exs b/test/pleroma/web/plugs/ensure_user_token_assigns_plug_test.exs
new file mode 100644 (file)
index 0000000..9592820
--- /dev/null
@@ -0,0 +1,69 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
+# SPDX-License-Identifier: AGPL-3.0-only
+
+defmodule Pleroma.Web.Plugs.EnsureUserTokenAssignsPlugTest do
+  use Pleroma.Web.ConnCase, async: true
+
+  import Pleroma.Factory
+
+  alias Pleroma.Web.Plugs.EnsureUserTokenAssignsPlug
+
+  test "with :user assign set to a User record " <>
+         "and :token assign set to a Token belonging to this user, " <>
+         "it does nothing" do
+    %{conn: conn} = oauth_access(["read"])
+
+    ret_conn = EnsureUserTokenAssignsPlug.call(conn, %{})
+
+    assert conn == ret_conn
+  end
+
+  test "with :user assign set to a User record " <>
+         "but :token assign not set or not a Token, " <>
+         "it assigns :token to `nil`",
+       %{conn: conn} do
+    user = insert(:user)
+    conn = assign(conn, :user, user)
+
+    ret_conn = EnsureUserTokenAssignsPlug.call(conn, %{})
+
+    assert %{token: nil} = ret_conn.assigns
+
+    ret_conn2 =
+      conn
+      |> assign(:token, 1)
+      |> EnsureUserTokenAssignsPlug.call(%{})
+
+    assert %{token: nil} = ret_conn2.assigns
+  end
+
+  # Abnormal (unexpected) scenario
+  test "with :user assign set to a User record " <>
+         "but :token assign set to a Token NOT belonging to :user, " <>
+         "it drops auth info" do
+    %{conn: conn} = oauth_access(["read"])
+    other_user = insert(:user)
+
+    conn = assign(conn, :user, other_user)
+
+    ret_conn = EnsureUserTokenAssignsPlug.call(conn, %{})
+
+    assert %{user: nil, token: nil} = ret_conn.assigns
+  end
+
+  test "if :user assign is not set to a User record, it sets :user and :token to nil", %{
+    conn: conn
+  } do
+    ret_conn = EnsureUserTokenAssignsPlug.call(conn, %{})
+
+    assert %{user: nil, token: nil} = ret_conn.assigns
+
+    ret_conn2 =
+      conn
+      |> assign(:user, 1)
+      |> EnsureUserTokenAssignsPlug.call(%{})
+
+    assert %{user: nil, token: nil} = ret_conn2.assigns
+  end
+end