From ccc2cf0e87f47618163da588ead76846c64cba7a Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Sat, 21 Nov 2020 19:47:25 +0300 Subject: [PATCH] Session-based OAuth auth fixes (token expiration check), refactoring, tweaks. --- lib/pleroma/helpers/auth_helper.ex | 10 +- lib/pleroma/web/o_auth/o_auth_controller.ex | 10 +- lib/pleroma/web/o_auth/token.ex | 8 ++ lib/pleroma/web/plugs/o_auth_plug.ex | 92 ++++++++---------- .../web/plugs/session_authentication_plug.ex | 31 ------ .../web/plugs/set_user_session_id_plug.ex | 7 +- lib/pleroma/web/plugs/user_enabled_plug.ex | 9 +- lib/pleroma/web/router.ex | 12 ++- test/pleroma/web/plugs/o_auth_plug_test.exs | 97 ++++++++++++++----- .../session_authentication_plug_test.exs | 65 ------------- .../plugs/set_user_session_id_plug_test.exs | 19 ++-- 11 files changed, 164 insertions(+), 196 deletions(-) delete mode 100644 lib/pleroma/web/plugs/session_authentication_plug.ex delete mode 100644 test/pleroma/web/plugs/session_authentication_plug_test.exs diff --git a/lib/pleroma/helpers/auth_helper.ex b/lib/pleroma/helpers/auth_helper.ex index 6e29c006a..878fec346 100644 --- a/lib/pleroma/helpers/auth_helper.ex +++ b/lib/pleroma/helpers/auth_helper.ex @@ -5,13 +5,21 @@ defmodule Pleroma.Helpers.AuthHelper do alias Pleroma.Web.Plugs.OAuthScopesPlug + import Plug.Conn + @doc """ Skips OAuth permissions (scopes) checks, assigns nil `:token`. Intended to be used with explicit authentication and only when OAuth token cannot be determined. """ def skip_oauth(conn) do conn - |> Plug.Conn.assign(:token, nil) + |> assign(:token, nil) |> OAuthScopesPlug.skip_plug() end + + def drop_auth_info(conn) do + conn + |> assign(:user, nil) + |> assign(:token, nil) + end end diff --git a/lib/pleroma/web/o_auth/o_auth_controller.ex b/lib/pleroma/web/o_auth/o_auth_controller.ex index d2f9d1ceb..83a25907d 100644 --- a/lib/pleroma/web/o_auth/o_auth_controller.ex +++ b/lib/pleroma/web/o_auth/o_auth_controller.ex @@ -363,7 +363,15 @@ defp handle_token_exchange_error(%Plug.Conn{} = conn, _error) do def token_revoke(%Plug.Conn{} = conn, %{"token" => _token} = params) do with {:ok, app} <- Token.Utils.fetch_app(conn), - {:ok, _token} <- RevokeToken.revoke(app, params) do + {:ok, %Token{} = oauth_token} <- RevokeToken.revoke(app, params) do + conn = + with session_token = get_session(conn, :oauth_token), + %Token{token: ^session_token} <- oauth_token do + delete_session(conn, :oauth_token) + else + _ -> conn + end + json(conn, %{}) else _error -> diff --git a/lib/pleroma/web/o_auth/token.ex b/lib/pleroma/web/o_auth/token.ex index de37998f2..9170a7ec7 100644 --- a/lib/pleroma/web/o_auth/token.ex +++ b/lib/pleroma/web/o_auth/token.ex @@ -27,6 +27,14 @@ defmodule Pleroma.Web.OAuth.Token do timestamps() end + @doc "Gets token by unique access token" + @spec get_by_token(String.t()) :: {:ok, t()} | {:error, :not_found} + def get_by_token(token) do + token + |> Query.get_by_token() + |> Repo.find_resource() + end + @doc "Gets token for app by access token" @spec get_by_token(App.t(), String.t()) :: {:ok, t()} | {:error, :not_found} def get_by_token(%App{id: app_id} = _app, token) do diff --git a/lib/pleroma/web/plugs/o_auth_plug.ex b/lib/pleroma/web/plugs/o_auth_plug.ex index c7b58d90f..a3b7d42f7 100644 --- a/lib/pleroma/web/plugs/o_auth_plug.ex +++ b/lib/pleroma/web/plugs/o_auth_plug.ex @@ -3,6 +3,8 @@ # SPDX-License-Identifier: AGPL-3.0-only defmodule Pleroma.Web.Plugs.OAuthPlug do + @moduledoc "Performs OAuth authentication by token from params / headers / cookies." + import Plug.Conn import Ecto.Query @@ -17,45 +19,26 @@ def init(options), do: options def call(%{assigns: %{user: %User{}}} = conn, _), do: conn - def call(%{params: %{"access_token" => access_token}} = conn, _) do - with {:ok, user, token_record} <- fetch_user_and_token(access_token) do - conn - |> assign(:token, token_record) - |> assign(:user, user) - else - _ -> - # token found, but maybe only with app - with {:ok, app, token_record} <- fetch_app_and_token(access_token) do - conn - |> assign(:token, token_record) - |> assign(:app, app) - else - _ -> conn - end - end - end - def call(conn, _) do - case fetch_token_str(conn) do - {:ok, token} -> - with {:ok, user, token_record} <- fetch_user_and_token(token) do - conn - |> assign(:token, token_record) - |> assign(:user, user) - else - _ -> - # token found, but maybe only with app - with {:ok, app, token_record} <- fetch_app_and_token(token) do - conn - |> assign(:token, token_record) - |> assign(:app, app) - else - _ -> conn - end - end - - _ -> + with {:ok, token_str} <- fetch_token_str(conn) do + with {:ok, user, user_token} <- fetch_user_and_token(token_str), + false <- Token.is_expired?(user_token) do conn + |> assign(:token, user_token) + |> assign(:user, user) + else + _ -> + with {:ok, app, app_token} <- fetch_app_and_token(token_str), + false <- Token.is_expired?(app_token) do + conn + |> assign(:token, app_token) + |> assign(:app, app) + else + _ -> conn + end + end + else + _ -> conn end end @@ -70,7 +53,6 @@ defp fetch_user_and_token(token) do preload: [user: user] ) - # credo:disable-for-next-line Credo.Check.Readability.MaxLineLength with %Token{user: user} = token_record <- Repo.one(query) do {:ok, user, token_record} end @@ -86,29 +68,23 @@ defp fetch_app_and_token(token) do end end - # Gets token from session by :oauth_token key + # Gets token string from conn (in params / headers / session) # - @spec fetch_token_from_session(Plug.Conn.t()) :: :no_token_found | {:ok, String.t()} - defp fetch_token_from_session(conn) do - case get_session(conn, :oauth_token) do - nil -> :no_token_found - token -> {:ok, token} - end + @spec fetch_token_str(Plug.Conn.t() | list(String.t())) :: :no_token_found | {:ok, String.t()} + defp fetch_token_str(%Plug.Conn{params: %{"access_token" => access_token}} = _conn) do + {:ok, access_token} end - # Gets token from headers - # - @spec fetch_token_str(Plug.Conn.t()) :: :no_token_found | {:ok, String.t()} defp fetch_token_str(%Plug.Conn{} = conn) do headers = get_req_header(conn, "authorization") - with :no_token_found <- fetch_token_str(headers), - do: fetch_token_from_session(conn) + with {:ok, token} <- fetch_token_str(headers) do + {:ok, token} + else + _ -> fetch_token_from_session(conn) + end end - @spec fetch_token_str(Keyword.t()) :: :no_token_found | {:ok, String.t()} - defp fetch_token_str([]), do: :no_token_found - defp fetch_token_str([token | tail]) do trimmed_token = String.trim(token) @@ -117,4 +93,14 @@ defp fetch_token_str([token | tail]) do _ -> fetch_token_str(tail) end end + + defp fetch_token_str([]), do: :no_token_found + + @spec fetch_token_from_session(Plug.Conn.t()) :: :no_token_found | {:ok, String.t()} + defp fetch_token_from_session(conn) do + case get_session(conn, :oauth_token) do + nil -> :no_token_found + token -> {:ok, token} + end + end end diff --git a/lib/pleroma/web/plugs/session_authentication_plug.ex b/lib/pleroma/web/plugs/session_authentication_plug.ex deleted file mode 100644 index 51704e273..000000000 --- a/lib/pleroma/web/plugs/session_authentication_plug.ex +++ /dev/null @@ -1,31 +0,0 @@ -# Pleroma: A lightweight social networking server -# Copyright © 2017-2020 Pleroma Authors -# SPDX-License-Identifier: AGPL-3.0-only - -defmodule Pleroma.Web.Plugs.SessionAuthenticationPlug do - @moduledoc """ - Authenticates user by session-stored `:user_id` and request-contained username. - Username can be provided via HTTP Basic Auth (the password is not checked and can be anything). - """ - - import Plug.Conn - - alias Pleroma.Helpers.AuthHelper - - def init(options) do - options - end - - def call(%{assigns: %{user: %Pleroma.User{}}} = conn, _), do: conn - - def call(conn, _) do - with saved_user_id <- get_session(conn, :user_id), - %{auth_user: %{id: ^saved_user_id}} <- conn.assigns do - conn - |> assign(:user, conn.assigns.auth_user) - |> AuthHelper.skip_oauth() - else - _ -> conn - end - end -end diff --git a/lib/pleroma/web/plugs/set_user_session_id_plug.ex b/lib/pleroma/web/plugs/set_user_session_id_plug.ex index 6ddb6b5e5..d2338c03f 100644 --- a/lib/pleroma/web/plugs/set_user_session_id_plug.ex +++ b/lib/pleroma/web/plugs/set_user_session_id_plug.ex @@ -4,14 +4,15 @@ defmodule Pleroma.Web.Plugs.SetUserSessionIdPlug do import Plug.Conn - alias Pleroma.User + + alias Pleroma.Web.OAuth.Token def init(opts) do opts end - def call(%{assigns: %{user: %User{id: id}}} = conn, _) do - put_session(conn, :user_id, id) + def call(%{assigns: %{token: %Token{} = oauth_token}} = conn, _) do + put_session(conn, :oauth_token, oauth_token.token) end def call(conn, _), do: conn diff --git a/lib/pleroma/web/plugs/user_enabled_plug.ex b/lib/pleroma/web/plugs/user_enabled_plug.ex index fa28ee48b..291d1f568 100644 --- a/lib/pleroma/web/plugs/user_enabled_plug.ex +++ b/lib/pleroma/web/plugs/user_enabled_plug.ex @@ -3,7 +3,7 @@ # SPDX-License-Identifier: AGPL-3.0-only defmodule Pleroma.Web.Plugs.UserEnabledPlug do - import Plug.Conn + alias Pleroma.Helpers.AuthHelper alias Pleroma.User def init(options) do @@ -12,8 +12,11 @@ def init(options) do def call(%{assigns: %{user: %User{} = user}} = conn, _) do case User.account_status(user) do - :active -> conn - _ -> assign(conn, :user, nil) + :active -> + conn + + _ -> + AuthHelper.drop_auth_info(conn) end end diff --git a/lib/pleroma/web/router.ex b/lib/pleroma/web/router.ex index c075fc7d3..2b8b3e95c 100644 --- a/lib/pleroma/web/router.ex +++ b/lib/pleroma/web/router.ex @@ -34,6 +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) end pipeline :expect_authentication do @@ -48,7 +49,6 @@ defmodule Pleroma.Web.Router do plug(Pleroma.Web.Plugs.OAuthPlug) plug(Pleroma.Web.Plugs.BasicAuthDecoderPlug) plug(Pleroma.Web.Plugs.UserFetcherPlug) - plug(Pleroma.Web.Plugs.SessionAuthenticationPlug) plug(Pleroma.Web.Plugs.AuthenticationPlug) end @@ -319,18 +319,24 @@ defmodule Pleroma.Web.Router do scope "/oauth", Pleroma.Web.OAuth do scope [] do pipe_through(:oauth) + get("/authorize", OAuthController, :authorize) + post("/authorize", OAuthController, :create_authorization) end - post("/authorize", OAuthController, :create_authorization) post("/token", OAuthController, :token_exchange) - post("/revoke", OAuthController, :token_revoke) get("/registration_details", OAuthController, :registration_details) post("/mfa/challenge", MFAController, :challenge) post("/mfa/verify", MFAController, :verify, as: :mfa_verify) get("/mfa", MFAController, :show) + scope [] do + pipe_through(:fetch_session) + + post("/revoke", OAuthController, :token_revoke) + end + scope [] do pipe_through(:browser) diff --git a/test/pleroma/web/plugs/o_auth_plug_test.exs b/test/pleroma/web/plugs/o_auth_plug_test.exs index b9d722f76..ad2aa5d1b 100644 --- a/test/pleroma/web/plugs/o_auth_plug_test.exs +++ b/test/pleroma/web/plugs/o_auth_plug_test.exs @@ -5,43 +5,48 @@ defmodule Pleroma.Web.Plugs.OAuthPlugTest do use Pleroma.Web.ConnCase, async: true + alias Pleroma.Web.OAuth.Token + alias Pleroma.Web.OAuth.Token.Strategy.Revoke alias Pleroma.Web.Plugs.OAuthPlug - import Pleroma.Factory + alias Plug.Session - @session_opts [ - store: :cookie, - key: "_test", - signing_salt: "cooldude" - ] + import Pleroma.Factory setup %{conn: conn} do user = insert(:user) - {:ok, %{token: token}} = Pleroma.Web.OAuth.Token.create(insert(:oauth_app), user) - %{user: user, token: token, conn: conn} + {:ok, oauth_token} = Token.create(insert(:oauth_app), user) + %{user: user, token: oauth_token, conn: conn} end - test "with valid token(uppercase), it assigns the user", %{conn: conn} = opts do + test "it does nothing if a user is assigned", %{conn: conn} do + conn = assign(conn, :user, %Pleroma.User{}) + ret_conn = OAuthPlug.call(conn, %{}) + + assert ret_conn == conn + end + + test "with valid token (uppercase) in auth header, it assigns the user", %{conn: conn} = opts do conn = conn - |> put_req_header("authorization", "BEARER #{opts[:token]}") + |> put_req_header("authorization", "BEARER #{opts[:token].token}") |> OAuthPlug.call(%{}) assert conn.assigns[:user] == opts[:user] end - test "with valid token(downcase), it assigns the user", %{conn: conn} = opts do + test "with valid token (downcase) in auth header, it assigns the user", %{conn: conn} = opts do conn = conn - |> put_req_header("authorization", "bearer #{opts[:token]}") + |> put_req_header("authorization", "bearer #{opts[:token].token}") |> OAuthPlug.call(%{}) assert conn.assigns[:user] == opts[:user] end - test "with valid token(downcase) in url parameters, it assigns the user", opts do + test "with valid token (downcase) in url parameters, it assigns the user", opts do conn = :get - |> build_conn("/?access_token=#{opts[:token]}") + |> build_conn("/?access_token=#{opts[:token].token}") |> put_req_header("content-type", "application/json") |> fetch_query_params() |> OAuthPlug.call(%{}) @@ -49,16 +54,16 @@ test "with valid token(downcase) in url parameters, it assigns the user", opts d assert conn.assigns[:user] == opts[:user] end - test "with valid token(downcase) in body parameters, it assigns the user", opts do + test "with valid token (downcase) in body parameters, it assigns the user", opts do conn = :post - |> build_conn("/api/v1/statuses", access_token: opts[:token], status: "test") + |> build_conn("/api/v1/statuses", access_token: opts[:token].token, status: "test") |> OAuthPlug.call(%{}) assert conn.assigns[:user] == opts[:user] end - test "with invalid token, it not assigns the user", %{conn: conn} do + test "with invalid token, it does not assign the user", %{conn: conn} do conn = conn |> put_req_header("authorization", "bearer TTTTT") @@ -67,14 +72,56 @@ test "with invalid token, it not assigns the user", %{conn: conn} do refute conn.assigns[:user] end - test "when token is missed but token in session, it assigns the user", %{conn: conn} = opts do - conn = - conn - |> Plug.Session.call(Plug.Session.init(@session_opts)) - |> fetch_session() - |> put_session(:oauth_token, opts[:token]) - |> OAuthPlug.call(%{}) + describe "with :oauth_token in session, " do + setup %{token: oauth_token, conn: conn} do + session_opts = [ + store: :cookie, + key: "_test", + signing_salt: "cooldude" + ] - assert conn.assigns[:user] == opts[:user] + conn = + conn + |> Session.call(Session.init(session_opts)) + |> fetch_session() + |> put_session(:oauth_token, oauth_token.token) + + %{conn: conn} + end + + test "if session-stored token matches a valid OAuth token, assigns :user and :token", %{ + conn: conn, + user: user, + token: oauth_token + } do + conn = OAuthPlug.call(conn, %{}) + + assert conn.assigns.user && conn.assigns.user.id == user.id + assert conn.assigns.token && conn.assigns.token.id == oauth_token.id + end + + test "if session-stored token matches an expired OAuth token, does nothing", %{ + conn: conn, + token: oauth_token + } do + expired_valid_until = NaiveDateTime.add(NaiveDateTime.utc_now(), -3600 * 24, :second) + + oauth_token + |> Ecto.Changeset.change(valid_until: expired_valid_until) + |> Pleroma.Repo.update() + + ret_conn = OAuthPlug.call(conn, %{}) + assert ret_conn == conn + end + + test "if session-stored token matches a revoked OAuth token, does nothing", %{ + conn: conn, + token: oauth_token + } do + Revoke.revoke(oauth_token) + + ret_conn = OAuthPlug.call(conn, %{}) + assert ret_conn == conn + end end end diff --git a/test/pleroma/web/plugs/session_authentication_plug_test.exs b/test/pleroma/web/plugs/session_authentication_plug_test.exs deleted file mode 100644 index d027331a9..000000000 --- a/test/pleroma/web/plugs/session_authentication_plug_test.exs +++ /dev/null @@ -1,65 +0,0 @@ -# Pleroma: A lightweight social networking server -# Copyright © 2017-2020 Pleroma Authors -# SPDX-License-Identifier: AGPL-3.0-only - -defmodule Pleroma.Web.Plugs.SessionAuthenticationPlugTest do - use Pleroma.Web.ConnCase, async: true - - alias Pleroma.User - alias Pleroma.Web.Plugs.OAuthScopesPlug - alias Pleroma.Web.Plugs.PlugHelper - alias Pleroma.Web.Plugs.SessionAuthenticationPlug - - setup %{conn: conn} do - session_opts = [ - store: :cookie, - key: "_test", - signing_salt: "cooldude" - ] - - conn = - conn - |> Plug.Session.call(Plug.Session.init(session_opts)) - |> fetch_session() - |> assign(:auth_user, %User{id: 1}) - - %{conn: conn} - end - - test "it does nothing if a user is assigned", %{conn: conn} do - conn = assign(conn, :user, %User{}) - ret_conn = SessionAuthenticationPlug.call(conn, %{}) - - assert ret_conn == conn - end - - # Scenario: requester has the cookie and knows the username (not necessarily knows the password) - test "if the auth_user has the same id as the user_id in the session, it assigns the user", %{ - conn: conn - } do - conn = - conn - |> put_session(:user_id, conn.assigns.auth_user.id) - |> SessionAuthenticationPlug.call(%{}) - - assert conn.assigns.user == conn.assigns.auth_user - assert conn.assigns.token == nil - assert PlugHelper.plug_skipped?(conn, OAuthScopesPlug) - end - - # Scenario: requester has the cookie but doesn't know the username - test "if the auth_user has a different id as the user_id in the session, it does nothing", %{ - conn: conn - } do - conn = put_session(conn, :user_id, -1) - ret_conn = SessionAuthenticationPlug.call(conn, %{}) - - assert ret_conn == conn - end - - test "if the session does not contain user_id, it does nothing", %{ - conn: conn - } do - assert conn == SessionAuthenticationPlug.call(conn, %{}) - end -end diff --git a/test/pleroma/web/plugs/set_user_session_id_plug_test.exs b/test/pleroma/web/plugs/set_user_session_id_plug_test.exs index a89b5628f..a50e80107 100644 --- a/test/pleroma/web/plugs/set_user_session_id_plug_test.exs +++ b/test/pleroma/web/plugs/set_user_session_id_plug_test.exs @@ -5,7 +5,6 @@ defmodule Pleroma.Web.Plugs.SetUserSessionIdPlugTest do use Pleroma.Web.ConnCase, async: true - alias Pleroma.User alias Pleroma.Web.Plugs.SetUserSessionIdPlug setup %{conn: conn} do @@ -18,28 +17,26 @@ defmodule Pleroma.Web.Plugs.SetUserSessionIdPlugTest do conn = conn |> Plug.Session.call(Plug.Session.init(session_opts)) - |> fetch_session + |> fetch_session() %{conn: conn} end test "doesn't do anything if the user isn't set", %{conn: conn} do - ret_conn = - conn - |> SetUserSessionIdPlug.call(%{}) + ret_conn = SetUserSessionIdPlug.call(conn, %{}) assert ret_conn == conn end - test "sets the user_id in the session to the user id of the user assign", %{conn: conn} do - Code.ensure_compiled(Pleroma.User) + test "sets :oauth_token in session to :token assign", %{conn: conn} do + %{user: user, token: oauth_token} = oauth_access(["read"]) - conn = + ret_conn = conn - |> assign(:user, %User{id: 1}) + |> assign(:user, user) + |> assign(:token, oauth_token) |> SetUserSessionIdPlug.call(%{}) - id = get_session(conn, :user_id) - assert id == 1 + assert get_session(ret_conn, :oauth_token) == oauth_token.token end end