[#1682] Fixed Basic Auth permissions issue by disabling OAuth scopes checks when password is provided. Refactored plugs skipping functionality.

8 files changed, 100 insertions, 23 deletions

diff --git a/lib/pleroma/plugs/authentication_plug.ex b/lib/pleroma/plugs/authentication_plug.ex
index 089028d77..0061c69dc 100644
--- a/lib/pleroma/plugs/authentication_plug.ex
+++ b/lib/pleroma/plugs/authentication_plug.ex
@@ -4,8 +4,11 @@
 
 defmodule Pleroma.Plugs.AuthenticationPlug do
   alias Comeonin.Pbkdf2
-  import Plug.Conn
+  alias Pleroma.Plugs.OAuthScopesPlug
   alias Pleroma.User
+
+  import Plug.Conn
+
   require Logger
 
   def init(options), do: options
@@ -37,6 +40,7 @@ defmodule Pleroma.Plugs.AuthenticationPlug do
     if Pbkdf2.checkpw(password, password_hash) do
       conn
       |> assign(:user, auth_user)
+      |> OAuthScopesPlug.skip_plug()
     else
       conn
     end
diff --git a/lib/pleroma/plugs/legacy_authentication_plug.ex b/lib/pleroma/plugs/legacy_authentication_plug.ex
index 5c5c36c56..d346e01a6 100644
--- a/lib/pleroma/plugs/legacy_authentication_plug.ex
+++ b/lib/pleroma/plugs/legacy_authentication_plug.ex
@@ -4,6 +4,8 @@
 
 defmodule Pleroma.Plugs.LegacyAuthenticationPlug do
   import Plug.Conn
+
+  alias Pleroma.Plugs.OAuthScopesPlug
   alias Pleroma.User
 
   def init(options) do
@@ -27,6 +29,7 @@ defmodule Pleroma.Plugs.LegacyAuthenticationPlug do
       conn
       |> assign(:auth_user, user)
       |> assign(:user, user)
+      |> OAuthScopesPlug.skip_plug()
     else
       _ ->
         conn
diff --git a/lib/pleroma/plugs/plug_helper.ex b/lib/pleroma/plugs/plug_helper.ex
index 4f83e9414..9c67be8ef 100644
--- a/lib/pleroma/plugs/plug_helper.ex
+++ b/lib/pleroma/plugs/plug_helper.ex
@@ -5,30 +5,32 @@
 defmodule Pleroma.Plugs.PlugHelper do
   @moduledoc "Pleroma Plug helper"
 
-  def append_to_called_plugs(conn, plug_module) do
-    append_to_private_list(conn, :called_plugs, plug_module)
-  end
+  @called_plugs_list_id :called_plugs
+  def called_plugs_list_id, do: @called_plugs_list_id
 
-  def append_to_skipped_plugs(conn, plug_module) do
-    append_to_private_list(conn, :skipped_plugs, plug_module)
-  end
+  @skipped_plugs_list_id :skipped_plugs
+  def skipped_plugs_list_id, do: @skipped_plugs_list_id
 
+  @doc "Returns `true` if specified plug was called."
   def plug_called?(conn, plug_module) do
-    contained_in_private_list?(conn, :called_plugs, plug_module)
+    contained_in_private_list?(conn, @called_plugs_list_id, plug_module)
   end
 
+  @doc "Returns `true` if specified plug was explicitly marked as skipped."
   def plug_skipped?(conn, plug_module) do
-    contained_in_private_list?(conn, :skipped_plugs, plug_module)
+    contained_in_private_list?(conn, @skipped_plugs_list_id, plug_module)
   end
 
+  @doc "Returns `true` if specified plug was either called or explicitly marked as skipped."
   def plug_called_or_skipped?(conn, plug_module) do
     plug_called?(conn, plug_module) || plug_skipped?(conn, plug_module)
   end
 
-  defp append_to_private_list(conn, private_variable, value) do
-    list = conn.private[private_variable] || []
+  # Appends plug to known list (skipped, called). Intended to be used from within plug code only.
+  def append_to_private_list(conn, list_id, value) do
+    list = conn.private[list_id] || []
     modified_list = Enum.uniq(list ++ [value])
-    Plug.Conn.put_private(conn, private_variable, modified_list)
+    Plug.Conn.put_private(conn, list_id, modified_list)
   end
 
   defp contained_in_private_list?(conn, private_variable, value) do
diff --git a/lib/pleroma/web/web.ex b/lib/pleroma/web/web.ex
index ae7c94640..bf48ce26c 100644
--- a/lib/pleroma/web/web.ex
+++ b/lib/pleroma/web/web.ex
@@ -40,17 +40,22 @@ defmodule Pleroma.Web do
       # 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()
+          plug_module.skip_plug(conn)
         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)
+      # Executed just before actual controller action, invokes before-action hooks (callbacks)
       defp action(conn, params) do
+        with %Plug.Conn{halted: false} <- maybe_halt_on_missing_oauth_scopes_check(conn) do
+          super(conn, params)
+        end
+      end
+
+      # Halts if authenticated API action neither performs nor explicitly skips OAuth scopes check
+      defp maybe_halt_on_missing_oauth_scopes_check(conn) do
         if Pleroma.Plugs.AuthExpectedPlug.auth_expected?(conn) &&
              not PlugHelper.plug_called_or_skipped?(conn, Pleroma.Plugs.OAuthScopesPlug) do
           conn
@@ -60,7 +65,7 @@ defmodule Pleroma.Web do
           )
           |> halt()
         else
-          super(conn, params)
+          conn
         end
       end
     end
@@ -129,7 +134,16 @@ defmodule Pleroma.Web do
     quote do
       alias Pleroma.Plugs.PlugHelper
 
-      def ensure_skippable, do: :noop
+      @doc """
+      Marks a plug intentionally skipped and blocks its execution if it's present in plugs chain.
+      """
+      def skip_plug(conn) do
+        PlugHelper.append_to_private_list(
+          conn,
+          PlugHelper.skipped_plugs_list_id(),
+          __MODULE__
+        )
+      end
 
       @impl Plug
       @doc "If marked as skipped, returns `conn`, and calls `perform/2` otherwise."
@@ -138,7 +152,7 @@ defmodule Pleroma.Web do
           conn
         else
           conn
-          |> PlugHelper.append_to_called_plugs(__MODULE__)
+          |> PlugHelper.append_to_private_list(PlugHelper.called_plugs_list_id(), __MODULE__)
           |> perform(options)
         end
       end
diff --git a/test/plugs/authentication_plug_test.exs b/test/plugs/authentication_plug_test.exs
index ae2f3f8ec..646bda9d3 100644
--- a/test/plugs/authentication_plug_test.exs
+++ b/test/plugs/authentication_plug_test.exs
@@ -6,6 +6,8 @@ defmodule Pleroma.Plugs.AuthenticationPlugTest do
   use Pleroma.Web.ConnCase, async: true
 
   alias Pleroma.Plugs.AuthenticationPlug
+  alias Pleroma.Plugs.OAuthScopesPlug
+  alias Pleroma.Plugs.PlugHelper
   alias Pleroma.User
 
   import ExUnit.CaptureLog
@@ -36,13 +38,16 @@ defmodule Pleroma.Plugs.AuthenticationPlugTest do
     assert ret_conn == conn
   end
 
-  test "with a correct password in the credentials, it assigns the auth_user", %{conn: conn} do
+  test "with a correct password in the credentials, " <>
+         "it assigns the auth_user and marks OAuthScopesPlug as skipped",
+       %{conn: conn} do
     conn =
       conn
       |> assign(:auth_credentials, %{password: "guy"})
       |> AuthenticationPlug.call(%{})
 
     assert conn.assigns.user == conn.assigns.auth_user
+    assert PlugHelper.plug_skipped?(conn, OAuthScopesPlug)
   end
 
   test "with a wrong password in the credentials, it does nothing", %{conn: conn} do
diff --git a/test/plugs/legacy_authentication_plug_test.exs b/test/plugs/legacy_authentication_plug_test.exs
index 7559de7d3..3b8c07627 100644
--- a/test/plugs/legacy_authentication_plug_test.exs
+++ b/test/plugs/legacy_authentication_plug_test.exs
@@ -8,6 +8,8 @@ defmodule Pleroma.Plugs.LegacyAuthenticationPlugTest do
   import Pleroma.Factory
 
   alias Pleroma.Plugs.LegacyAuthenticationPlug
+  alias Pleroma.Plugs.OAuthScopesPlug
+  alias Pleroma.Plugs.PlugHelper
   alias Pleroma.User
 
   setup do
@@ -36,7 +38,8 @@ defmodule Pleroma.Plugs.LegacyAuthenticationPlugTest do
   end
 
   @tag :skip_on_mac
-  test "it authenticates the auth_user if present and password is correct and resets the password",
+  test "if `auth_user` is present and password is correct, " <>
+         "it authenticates the user, resets the password, marks OAuthScopesPlug as skipped",
        %{
          conn: conn,
          user: user
@@ -49,6 +52,7 @@ defmodule Pleroma.Plugs.LegacyAuthenticationPlugTest do
     conn = LegacyAuthenticationPlug.call(conn, %{})
 
     assert conn.assigns.user.id == user.id
+    assert PlugHelper.plug_skipped?(conn, OAuthScopesPlug)
   end
 
   @tag :skip_on_mac
diff --git a/test/plugs/oauth_scopes_plug_test.exs b/test/plugs/oauth_scopes_plug_test.exs
index 85105f968..d855d4f54 100644
--- a/test/plugs/oauth_scopes_plug_test.exs
+++ b/test/plugs/oauth_scopes_plug_test.exs
@@ -7,7 +7,6 @@ defmodule Pleroma.Plugs.OAuthScopesPlugTest do
 
   alias Pleroma.Plugs.EnsurePublicOrAuthenticatedPlug
   alias Pleroma.Plugs.OAuthScopesPlug
-  alias Pleroma.Plugs.PlugHelper
   alias Pleroma.Repo
 
   import Mock
@@ -21,7 +20,7 @@ defmodule Pleroma.Plugs.OAuthScopesPlugTest do
     with_mock OAuthScopesPlug, [:passthrough], perform: &passthrough([&1, &2]) do
       conn =
         conn
-        |> PlugHelper.append_to_skipped_plugs(OAuthScopesPlug)
+        |> OAuthScopesPlug.skip_plug()
         |> OAuthScopesPlug.call(%{scopes: ["random_scope"]})
 
       refute called(OAuthScopesPlug.perform(:_, :_))
diff --git a/test/web/auth/basic_auth_test.exs b/test/web/auth/basic_auth_test.exs
new file mode 100644
index 000000000..64f8a6863
--- /dev/null
+++ b/test/web/auth/basic_auth_test.exs
@@ -0,0 +1,46 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
+# SPDX-License-Identifier: AGPL-3.0-only
+
+defmodule Pleroma.Web.Auth.BasicAuthTest do
+  use Pleroma.Web.ConnCase
+
+  import Pleroma.Factory
+
+  test "with HTTP Basic Auth used, grants access to OAuth scope-restricted endpoints", %{
+    conn: conn
+  } do
+    user = insert(:user)
+    assert Comeonin.Pbkdf2.checkpw("test", user.password_hash)
+
+    basic_auth_contents =
+      (URI.encode_www_form(user.nickname) <> ":" <> URI.encode_www_form("test"))
+      |> Base.encode64()
+
+    # Succeeds with HTTP Basic Auth
+    response =
+      conn
+      |> put_req_header("authorization", "Basic " <> basic_auth_contents)
+      |> get("/api/v1/accounts/verify_credentials")
+      |> json_response(200)
+
+    user_nickname = user.nickname
+    assert %{"username" => ^user_nickname} = response
+
+    # Succeeds with a properly scoped OAuth token
+    valid_token = insert(:oauth_token, scopes: ["read:accounts"])
+
+    conn
+    |> put_req_header("authorization", "Bearer #{valid_token.token}")
+    |> get("/api/v1/accounts/verify_credentials")
+    |> json_response(200)
+
+    # Fails with a wrong-scoped OAuth token (proof of restriction)
+    invalid_token = insert(:oauth_token, scopes: ["read:something"])
+
+    conn
+    |> put_req_header("authorization", "Bearer #{invalid_token.token}")
+    |> get("/api/v1/accounts/verify_credentials")
+    |> json_response(403)
+  end
+end