commit: e9859b68fcb9c38b2ec27a45ffe0921e8d78b5e1 parent d50a3345ae7873f8a8744eba8a3eb755e2b8dfdc Author: Ivan Tashkinov <ivantashkinov@gmail.com> Date: Sun, 6 Dec 2020 13:59:10 +0300 [#3112] Ensured presence and consistency of :user and :token assigns (EnsureUserTokenAssignsPlug). Refactored auth info dropping functions.Diffstat:
14 files changed, 164 insertions(+), 117 deletions(-)
diff --git 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 @@ -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 @@ -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 @@ -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 @@ -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 diff --git 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,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 diff --git 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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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