From bde1189c349dc114aca2e9310dda840a1007825f Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Wed, 15 Apr 2020 21:19:16 +0300 Subject: [PATCH] [#2349] Made :skip_plug/2 prevent plug from being executed even if explicitly called. Refactoring. Tests. --- lib/pleroma/plugs/auth_expected_plug.ex | 4 ++ lib/pleroma/plugs/oauth_scopes_plug.ex | 6 +-- lib/pleroma/tests/oauth_test_controller.ex | 31 +++++++++++++ lib/pleroma/web/router.ex | 11 +++++ lib/pleroma/web/web.ex | 32 +++++++++++-- test/plugs/oauth_scopes_plug_test.exs | 13 ++++++ test/web/auth/oauth_test_controller_test.exs | 49 ++++++++++++++++++++ 7 files changed, 140 insertions(+), 6 deletions(-) create mode 100644 lib/pleroma/tests/oauth_test_controller.ex create mode 100644 test/web/auth/oauth_test_controller_test.exs diff --git a/lib/pleroma/plugs/auth_expected_plug.ex b/lib/pleroma/plugs/auth_expected_plug.ex index 9e4a4bec8..f79597dc3 100644 --- a/lib/pleroma/plugs/auth_expected_plug.ex +++ b/lib/pleroma/plugs/auth_expected_plug.ex @@ -10,4 +10,8 @@ defmodule Pleroma.Plugs.AuthExpectedPlug do def call(conn, _) do put_private(conn, :auth_expected, true) end + + def auth_expected?(conn) do + conn.private[:auth_expected] + end end diff --git a/lib/pleroma/plugs/oauth_scopes_plug.ex b/lib/pleroma/plugs/oauth_scopes_plug.ex index b09e1bb4d..66f48c28c 100644 --- a/lib/pleroma/plugs/oauth_scopes_plug.ex +++ b/lib/pleroma/plugs/oauth_scopes_plug.ex @@ -10,13 +10,13 @@ defmodule Pleroma.Plugs.OAuthScopesPlug do alias Pleroma.Plugs.EnsurePublicOrAuthenticatedPlug alias Pleroma.Plugs.PlugHelper + use Pleroma.Web, :plug + @behaviour Plug def init(%{scopes: _} = options), do: options - def call(%Plug.Conn{assigns: assigns} = conn, %{scopes: scopes} = options) do - conn = PlugHelper.append_to_called_plugs(conn, __MODULE__) - + def perform(%Plug.Conn{assigns: assigns} = conn, %{scopes: scopes} = options) do op = options[:op] || :| token = assigns[:token] diff --git a/lib/pleroma/tests/oauth_test_controller.ex b/lib/pleroma/tests/oauth_test_controller.ex new file mode 100644 index 000000000..58d517f78 --- /dev/null +++ b/lib/pleroma/tests/oauth_test_controller.ex @@ -0,0 +1,31 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2020 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +# A test controller reachable only in :test env. +# Serves to test OAuth scopes check skipping / enforcement. +defmodule Pleroma.Tests.OAuthTestController do + @moduledoc false + + use Pleroma.Web, :controller + + alias Pleroma.Plugs.OAuthScopesPlug + + plug(:skip_plug, OAuthScopesPlug when action == :skipped_oauth) + + plug(OAuthScopesPlug, %{scopes: ["read"]} when action != :missed_oauth) + + def skipped_oauth(conn, _params) do + noop(conn) + end + + def performed_oauth(conn, _params) do + noop(conn) + end + + def missed_oauth(conn, _params) do + noop(conn) + end + + defp noop(conn), do: json(conn, %{}) +end diff --git a/lib/pleroma/web/router.ex b/lib/pleroma/web/router.ex index 8d13cd6c9..c85ad9f8b 100644 --- a/lib/pleroma/web/router.ex +++ b/lib/pleroma/web/router.ex @@ -672,6 +672,17 @@ defmodule Pleroma.Web.Router do end end + # Test-only routes needed to test action dispatching and plug chain execution + if Pleroma.Config.get(:env) == :test do + scope "/test/authenticated_api", Pleroma.Tests do + pipe_through(:authenticated_api) + + for action <- [:skipped_oauth, :performed_oauth, :missed_oauth] do + get("/#{action}", OAuthTestController, action) + end + end + end + scope "/", Pleroma.Web.MongooseIM do get("/user_exists", MongooseIMController, :user_exists) get("/check_password", MongooseIMController, :check_password) diff --git a/lib/pleroma/web/web.ex b/lib/pleroma/web/web.ex index 1af29ce78..ae7c94640 100644 --- a/lib/pleroma/web/web.ex +++ b/lib/pleroma/web/web.ex @@ -37,15 +37,21 @@ defmodule Pleroma.Web do put_layout(conn, Pleroma.Config.get(:app_layout, "app.html")) end - # Marks a plug as intentionally skipped - # (states that the plug is not called for a good reason, not by a mistake) + # Marks a plug intentionally skipped and blocks its execution if it's present in plugs chain defp skip_plug(conn, plug_module) do + try do + plug_module.ensure_skippable() + rescue + UndefinedFunctionError -> + raise "#{plug_module} is not skippable. Append `use Pleroma.Web, :plug` to its code." + end + PlugHelper.append_to_skipped_plugs(conn, plug_module) end # Here we can apply before-action hooks (e.g. verify whether auth checks were preformed) defp action(conn, params) do - if conn.private[:auth_expected] && + if Pleroma.Plugs.AuthExpectedPlug.auth_expected?(conn) && not PlugHelper.plug_called_or_skipped?(conn, Pleroma.Plugs.OAuthScopesPlug) do conn |> render_error( @@ -119,6 +125,26 @@ defmodule Pleroma.Web do end end + def plug do + quote do + alias Pleroma.Plugs.PlugHelper + + def ensure_skippable, do: :noop + + @impl Plug + @doc "If marked as skipped, returns `conn`, and calls `perform/2` otherwise." + def call(%Plug.Conn{} = conn, options) do + if PlugHelper.plug_skipped?(conn, __MODULE__) do + conn + else + conn + |> PlugHelper.append_to_called_plugs(__MODULE__) + |> perform(options) + end + end + end + end + @doc """ When used, dispatch to the appropriate controller/view/etc. """ diff --git a/test/plugs/oauth_scopes_plug_test.exs b/test/plugs/oauth_scopes_plug_test.exs index e79ecf263..abab7abb0 100644 --- a/test/plugs/oauth_scopes_plug_test.exs +++ b/test/plugs/oauth_scopes_plug_test.exs @@ -7,6 +7,7 @@ defmodule Pleroma.Plugs.OAuthScopesPlugTest do alias Pleroma.Plugs.EnsurePublicOrAuthenticatedPlug alias Pleroma.Plugs.OAuthScopesPlug + alias Pleroma.Plugs.PlugHelper alias Pleroma.Repo import Mock @@ -16,6 +17,18 @@ defmodule Pleroma.Plugs.OAuthScopesPlugTest do :ok end + test "is not performed if marked as skipped", %{conn: conn} do + with_mock OAuthScopesPlug, [:passthrough], perform: &passthrough([&1, &2]) do + conn = + conn + |> PlugHelper.append_to_skipped_plugs(OAuthScopesPlug) + |> OAuthScopesPlug.call(%{scopes: ["random_scope"]}) + + refute called(OAuthScopesPlug.perform(:_, :_)) + refute conn.halted + end + end + test "if `token.scopes` fulfills specified 'any of' conditions, " <> "proceeds with no op", %{conn: conn} do diff --git a/test/web/auth/oauth_test_controller_test.exs b/test/web/auth/oauth_test_controller_test.exs new file mode 100644 index 000000000..a2f6009ac --- /dev/null +++ b/test/web/auth/oauth_test_controller_test.exs @@ -0,0 +1,49 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2020 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Tests.OAuthTestControllerTest do + use Pleroma.Web.ConnCase + + import Pleroma.Factory + + setup %{conn: conn} do + user = insert(:user) + conn = assign(conn, :user, user) + %{conn: conn, user: user} + end + + test "missed_oauth", %{conn: conn} do + res = + conn + |> get("/test/authenticated_api/missed_oauth") + |> json_response(403) + + assert res == + %{ + "error" => + "Security violation: OAuth scopes check was neither handled nor explicitly skipped." + } + end + + test "skipped_oauth", %{conn: conn} do + conn + |> assign(:token, nil) + |> get("/test/authenticated_api/skipped_oauth") + |> json_response(200) + end + + test "performed_oauth", %{user: user} do + %{conn: good_token_conn} = oauth_access(["read"], user: user) + + good_token_conn + |> get("/test/authenticated_api/performed_oauth") + |> json_response(200) + + %{conn: bad_token_conn} = oauth_access(["follow"], user: user) + + bad_token_conn + |> get("/test/authenticated_api/performed_oauth") + |> json_response(403) + end +end