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

This commit is contained in:
Ivan Tashkinov 2020-04-17 21:21:10 +03:00 committed by rinpatch
parent da4923f2e5
commit 862d4886c9
8 changed files with 100 additions and 23 deletions

View File

@ -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 @@ def call(
if Pbkdf2.checkpw(password, password_hash) do
conn
|> assign(:user, auth_user)
|> OAuthScopesPlug.skip_plug()
else
conn
end

View File

@ -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 @@ def call(
conn
|> assign(:auth_user, user)
|> assign(:user, user)
|> OAuthScopesPlug.skip_plug()
else
_ ->
conn

View File

@ -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

View File

@ -40,17 +40,22 @@ defp set_put_layout(conn, _) 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 @@ defp action(conn, params) do
)
|> halt()
else
super(conn, params)
conn
end
end
end
@ -129,7 +134,16 @@ def plug 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 @@ def call(%Plug.Conn{} = conn, options) 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

View File

@ -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 @@ test "it does nothing if a user is assigned", %{conn: conn} 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

View File

@ -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 @@ test "it does nothing if a user is assigned", %{conn: conn, user: user} 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 @@ test "it authenticates the auth_user if present and password is correct and rese
conn = LegacyAuthenticationPlug.call(conn, %{})
assert conn.assigns.user.id == user.id
assert PlugHelper.plug_skipped?(conn, OAuthScopesPlug)
end
@tag :skip_on_mac

View File

@ -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 @@ 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.skip_plug()
|> OAuthScopesPlug.call(%{scopes: ["random_scope"]})
refute called(OAuthScopesPlug.perform(:_, :_))

View File

@ -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