From: Ivan Tashkinov <ivantashkinov@gmail.com>
Date: Sat, 21 Nov 2020 16:47:25 +0000 (+0300)
Subject: Session-based OAuth auth fixes (token expiration check), refactoring, tweaks.
X-Git-Url: https://git.squeep.com/?a=commitdiff_plain;h=ccc2cf0e87f47618163da588ead76846c64cba7a;p=akkoma

Session-based OAuth auth fixes (token expiration check), refactoring, tweaks.
---

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 @@ defmodule Pleroma.Web.OAuth.OAuthController 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 @@ defmodule Pleroma.Web.Plugs.OAuthPlug do
 
   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 @@ defmodule Pleroma.Web.Plugs.OAuthPlug 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 @@ defmodule Pleroma.Web.Plugs.OAuthPlug 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 @@ defmodule Pleroma.Web.Plugs.OAuthPlug 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 <https://pleroma.social/>
-# 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 @@ defmodule Pleroma.Web.Plugs.UserEnabledPlug 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 "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), it assigns the user", %{conn: conn} = opts do
+  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 @@ defmodule Pleroma.Web.Plugs.OAuthPlugTest do
     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 @@ defmodule Pleroma.Web.Plugs.OAuthPlugTest 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(%{})
-
-    assert conn.assigns[:user] == opts[:user]
+  describe "with :oauth_token in session, " do
+    setup %{token: oauth_token, conn: conn} do
+      session_opts = [
+        store: :cookie,
+        key: "_test",
+        signing_salt: "cooldude"
+      ]
+
+      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 <https://pleroma.social/>
-# 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