From e9859b68fcb9c38b2ec27a45ffe0921e8d78b5e1 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Sun, 6 Dec 2020 13:59:10 +0300 Subject: [PATCH] [#3112] Ensured presence and consistency of :user and :token assigns (EnsureUserTokenAssignsPlug). Refactored auth info dropping functions. --- lib/pleroma/helpers/auth_helper.ex | 6 ++ lib/pleroma/web.ex | 3 +- .../web/plugs/admin_secret_authentication_plug.ex | 18 ++--- lib/pleroma/web/plugs/ensure_user_key_plug.ex | 19 ------ .../web/plugs/ensure_user_token_assigns_plug.ex | 36 ++++++++++ .../web/plugs/mapped_signature_to_identity_plug.ex | 79 ++++++++++++---------- lib/pleroma/web/plugs/o_auth_scopes_plug.ex | 12 +--- lib/pleroma/web/plugs/user_enabled_plug.ex | 10 ++- lib/pleroma/web/router.ex | 7 +- .../controllers/admin_api_controller_test.exs | 4 -- .../controllers/config_controller_test.exs | 12 +--- .../controllers/chat_controller_test.exs | 5 +- .../web/plugs/ensure_user_key_plug_test.exs | 29 -------- .../plugs/ensure_user_token_assigns_plug_test.exs | 69 +++++++++++++++++++ 14 files changed, 178 insertions(+), 131 deletions(-) delete mode 100644 lib/pleroma/web/plugs/ensure_user_key_plug.ex create mode 100644 lib/pleroma/web/plugs/ensure_user_token_assigns_plug.ex delete mode 100644 test/pleroma/web/plugs/ensure_user_key_plug_test.exs create mode 100644 test/pleroma/web/plugs/ensure_user_token_assigns_plug_test.exs diff --git a/lib/pleroma/helpers/auth_helper.ex b/lib/pleroma/helpers/auth_helper.ex index 392fa7d5d..8f87b38be 100644 --- a/lib/pleroma/helpers/auth_helper.ex +++ b/lib/pleroma/helpers/auth_helper.ex @@ -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 diff --git a/lib/pleroma/web.ex b/lib/pleroma/web.ex index 6ed19d3dd..3ca20455d 100644 --- a/lib/pleroma/web.ex +++ b/lib/pleroma/web.ex @@ -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 diff --git a/lib/pleroma/web/plugs/admin_secret_authentication_plug.ex b/lib/pleroma/web/plugs/admin_secret_authentication_plug.ex index ff49801f4..ff851a874 100644 --- a/lib/pleroma/web/plugs/admin_secret_authentication_plug.ex +++ b/lib/pleroma/web/plugs/admin_secret_authentication_plug.ex @@ -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 index 31608dbbf..000000000 --- a/lib/pleroma/web/plugs/ensure_user_key_plug.ex +++ /dev/null @@ -1,19 +0,0 @@ -# Pleroma: A lightweight social networking server -# Copyright © 2017-2020 Pleroma Authors -# 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 index 000000000..4253458b2 --- /dev/null +++ b/lib/pleroma/web/plugs/ensure_user_token_assigns_plug.ex @@ -0,0 +1,36 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2020 Pleroma Authors +# 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 diff --git a/lib/pleroma/web/plugs/mapped_signature_to_identity_plug.ex b/lib/pleroma/web/plugs/mapped_signature_to_identity_plug.ex index f44d4dee5..a0a0c5a9b 100644 --- a/lib/pleroma/web/plugs/mapped_signature_to_identity_plug.ex +++ b/lib/pleroma/web/plugs/mapped_signature_to_identity_plug.ex @@ -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,6 +13,47 @@ defmodule Pleroma.Web.Plugs.MappedSignatureToIdentityPlug do def init(options), do: options + 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 + conn + |> assign(:user, user) + |> AuthHelper.skip_oauth() + else + {:user_match, false} -> + Logger.debug("Failed to map identity from signature (payload actor mismatch)") + Logger.debug("key_id=#{inspect(key_id_from_conn(conn))}, actor=#{inspect(actor)}") + assign(conn, :valid_signature, false) + + # remove me once testsuite uses mapped capabilities instead of what we do now + {:user, nil} -> + Logger.debug("Failed to map identity from signature (lookup failure)") + Logger.debug("key_id=#{inspect(key_id_from_conn(conn))}, actor=#{actor}") + conn + end + end + + # no payload, probably a signed fetch + def call(%{assigns: %{valid_signature: true}} = conn, _opts) do + with %User{} = user <- user_from_key_id(conn) do + conn + |> assign(:user, user) + |> AuthHelper.skip_oauth() + else + _ -> + Logger.debug("Failed to map identity from signature (no payload actor mismatch)") + Logger.debug("key_id=#{inspect(key_id_from_conn(conn))}") + assign(conn, :valid_signature, false) + end + end + + # 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 @@ -31,41 +73,4 @@ defmodule Pleroma.Web.Plugs.MappedSignatureToIdentityPlug do nil end end - - def call(%{assigns: %{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) - else - {:user_match, false} -> - Logger.debug("Failed to map identity from signature (payload actor mismatch)") - Logger.debug("key_id=#{inspect(key_id_from_conn(conn))}, actor=#{inspect(actor)}") - assign(conn, :valid_signature, false) - - # remove me once testsuite uses mapped capabilities instead of what we do now - {:user, nil} -> - Logger.debug("Failed to map identity from signature (lookup failure)") - Logger.debug("key_id=#{inspect(key_id_from_conn(conn))}, actor=#{actor}") - conn - end - end - - # 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) - else - _ -> - Logger.debug("Failed to map identity from signature (no payload actor mismatch)") - Logger.debug("key_id=#{inspect(key_id_from_conn(conn))}") - assign(conn, :valid_signature, false) - end - end - - # no signature at all - def call(conn, _opts), do: conn end diff --git a/lib/pleroma/web/plugs/o_auth_scopes_plug.ex b/lib/pleroma/web/plugs/o_auth_scopes_plug.ex index cfc30837c..e6d398b14 100644 --- a/lib/pleroma/web/plugs/o_auth_scopes_plug.ex +++ b/lib/pleroma/web/plugs/o_auth_scopes_plug.ex @@ -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( diff --git a/lib/pleroma/web/plugs/user_enabled_plug.ex b/lib/pleroma/web/plugs/user_enabled_plug.ex index 291d1f568..4f1b163bd 100644 --- a/lib/pleroma/web/plugs/user_enabled_plug.ex +++ b/lib/pleroma/web/plugs/user_enabled_plug.ex @@ -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 diff --git a/lib/pleroma/web/router.ex b/lib/pleroma/web/router.ex index b3462ba00..aefc9f0be 100644 --- a/lib/pleroma/web/router.ex +++ b/lib/pleroma/web/router.ex @@ -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 diff --git a/test/pleroma/web/admin_api/controllers/admin_api_controller_test.exs b/test/pleroma/web/admin_api/controllers/admin_api_controller_test.exs index c06ae55ca..e50d1425b 100644 --- a/test/pleroma/web/admin_api/controllers/admin_api_controller_test.exs +++ b/test/pleroma/web/admin_api/controllers/admin_api_controller_test.exs @@ -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) diff --git a/test/pleroma/web/admin_api/controllers/config_controller_test.exs b/test/pleroma/web/admin_api/controllers/config_controller_test.exs index 4e897455f..765a5a4b7 100644 --- a/test/pleroma/web/admin_api/controllers/config_controller_test.exs +++ b/test/pleroma/web/admin_api/controllers/config_controller_test.exs @@ -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) diff --git a/test/pleroma/web/pleroma_api/controllers/chat_controller_test.exs b/test/pleroma/web/pleroma_api/controllers/chat_controller_test.exs index c1e6a8cc5..a6c9d0c1b 100644 --- a/test/pleroma/web/pleroma_api/controllers/chat_controller_test.exs +++ b/test/pleroma/web/pleroma_api/controllers/chat_controller_test.exs @@ -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 index f912ef755..000000000 --- a/test/pleroma/web/plugs/ensure_user_key_plug_test.exs +++ /dev/null @@ -1,29 +0,0 @@ -# Pleroma: A lightweight social networking server -# Copyright © 2017-2020 Pleroma Authors -# 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 index 000000000..9592820c7 --- /dev/null +++ b/test/pleroma/web/plugs/ensure_user_token_assigns_plug_test.exs @@ -0,0 +1,69 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2020 Pleroma Authors +# 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