Revert "oauth2 fixes (#177)"

This reverts commit 429e2ac832.
2022-08-21 16:22:15 +01:00 · 2022-08-21 16:22:15 +01:00 · b0130bfa7b
commit b0130bfa7b
parent d72f9e39d9
12 changed files with 243 additions and 69 deletions
--- a/lib/pleroma/helpers/auth_helper.ex
+++ b/lib/pleroma/helpers/auth_helper.ex
@ -4,9 +4,12 @@

 defmodule Pleroma.Helpers.AuthHelper do
  alias Pleroma.Web.Plugs.OAuthScopesPlug
+  alias Plug.Conn

  import Plug.Conn

+  @oauth_token_session_key :oauth_token
+
  @doc """
  Skips OAuth permissions (scopes) checks, assigns nil `:token`.
  Intended to be used with explicit authentication and only when OAuth token cannot be determined.
@ -25,4 +28,19 @@ def drop_auth_info(conn) do
    |> assign(:token, nil)
    |> put_private(:authentication_ignored, true)
  end
+
+  @doc "Gets OAuth token string from session"
+  def get_session_token(%Conn{} = conn) do
+    get_session(conn, @oauth_token_session_key)
+  end
+
+  @doc "Updates OAuth token string in session"
+  def put_session_token(%Conn{} = conn, token) when is_binary(token) do
+    put_session(conn, @oauth_token_session_key, token)
+  end
+
+  @doc "Deletes OAuth token string from session"
+  def delete_session_token(%Conn{} = conn) do
+    delete_session(conn, @oauth_token_session_key)
+  end
 end
--- a/lib/pleroma/web/mastodon_api/controllers/auth_controller.ex
+++ b/lib/pleroma/web/mastodon_api/controllers/auth_controller.ex
@ -7,6 +7,7 @@ defmodule Pleroma.Web.MastodonAPI.AuthController do

  import Pleroma.Web.ControllerHelper, only: [json_response: 3]

+  alias Pleroma.Helpers.AuthHelper
  alias Pleroma.Helpers.UriHelper
  alias Pleroma.User
  alias Pleroma.Web.OAuth.App
@ -33,6 +34,7 @@ def login(conn, %{"code" => auth_token} = params) do
        |> UriHelper.modify_uri_params(%{"access_token" => oauth_token.token})

      conn
+      |> AuthHelper.put_session_token(oauth_token.token)
      |> redirect(to: redirect_to)
    else
      _ -> redirect_to_oauth_form(conn, params)
@ -40,9 +42,9 @@ def login(conn, %{"code" => auth_token} = params) do
  end

  def login(conn, params) do
-    with %{assigns: %{user: %User{}, token: %Token{app_id: app_id, token: token}}} <- conn,
+    with %{assigns: %{user: %User{}, token: %Token{app_id: app_id}}} <- conn,
         {:ok, %{id: ^app_id}} <- local_mastofe_app() do
-      redirect(conn, to: local_mastodon_post_login_path(conn) <> "?access_token=#{token}")
+      redirect(conn, to: local_mastodon_post_login_path(conn))
    else
      _ -> redirect_to_oauth_form(conn, params)
    end
@ -66,8 +68,9 @@ defp redirect_to_oauth_form(conn, _params) do
  def logout(conn, _) do
    conn =
      with %{assigns: %{token: %Token{} = oauth_token}} <- conn,
-           {:ok, %Token{token: _session_token}} <- RevokeToken.revoke(oauth_token) do
-        conn
+           session_token = AuthHelper.get_session_token(conn),
+           {:ok, %Token{token: ^session_token}} <- RevokeToken.revoke(oauth_token) do
+        AuthHelper.delete_session_token(conn)
      else
        _ -> conn
      end
--- a/lib/pleroma/web/o_auth/o_auth_controller.ex
+++ b/lib/pleroma/web/o_auth/o_auth_controller.ex
@ -5,6 +5,7 @@
 defmodule Pleroma.Web.OAuth.OAuthController do
  use Pleroma.Web, :controller

+  alias Pleroma.Helpers.AuthHelper
  alias Pleroma.Helpers.UriHelper
  alias Pleroma.Maps
  alias Pleroma.MFA
@ -76,46 +77,33 @@ defp do_authorize(%Plug.Conn{} = conn, params) do
    available_scopes = (app && app.scopes) || []
    scopes = Scopes.fetch_scopes(params, available_scopes)

-    # if we already have a token for this specific setup, we can use that
-    with false <- Params.truthy_param?(params["force_login"]),
-         %App{} <- app,
-         {:ok, _} <- Scopes.validate(scopes, app.scopes),
-         {:ok, %Token{} = token} <- Token.get_by_app(app) do
-      token = Repo.preload(token, :app)
+    user =
+      with %{assigns: %{user: %User{} = user}} <- conn do
+        user
+      else
+        _ -> nil
+      end

-      conn
-      |> assign(:token, token)
-      |> handle_existing_authorization(params)
-    else
-      _ ->
-        user =
-          with %{assigns: %{user: %User{} = user}} <- conn do
-            user
-          else
-            _ -> nil
-          end
+    scopes =
+      if scopes == [] do
+        available_scopes
+      else
+        scopes
+      end

-        scopes =
-          if scopes == [] do
-            available_scopes
-          else
-            scopes
-          end
-
-        # Note: `params` might differ from `conn.params`; use `@params` not `@conn.params` in template
-        render(conn, Authenticator.auth_template(), %{
-          user: user,
-          app: app && Map.delete(app, :client_secret),
-          response_type: params["response_type"],
-          client_id: params["client_id"],
-          available_scopes: available_scopes,
-          scopes: scopes,
-          redirect_uri: params["redirect_uri"],
-          state: params["state"],
-          params: params,
-          view_module: OAuthView
-        })
-    end
+    # Note: `params` might differ from `conn.params`; use `@params` not `@conn.params` in template
+    render(conn, Authenticator.auth_template(), %{
+      user: user,
+      app: app && Map.delete(app, :client_secret),
+      response_type: params["response_type"],
+      client_id: params["client_id"],
+      available_scopes: available_scopes,
+      scopes: scopes,
+      redirect_uri: params["redirect_uri"],
+      state: params["state"],
+      params: params,
+      view_module: OAuthView
+    })
  end

  defp handle_existing_authorization(
@ -330,8 +318,9 @@ def token_exchange(%Plug.Conn{} = conn, %{"grant_type" => "client_credentials"}
  # Bad request
  def token_exchange(%Plug.Conn{} = conn, params), do: bad_request(conn, params)

-  def after_token_exchange(%Plug.Conn{} = conn, %{token: _token} = view_params) do
+  def after_token_exchange(%Plug.Conn{} = conn, %{token: token} = view_params) do
    conn
+    |> AuthHelper.put_session_token(token.token)
    |> json(OAuthView.render("token.json", view_params))
  end

@ -390,7 +379,15 @@ defp handle_token_exchange_error(%Plug.Conn{} = conn, _error) do

  def token_revoke(%Plug.Conn{} = conn, %{"token" => token}) do
    with {:ok, %Token{} = oauth_token} <- Token.get_by_token(token),
-         {:ok, _oauth_token} <- RevokeToken.revoke(oauth_token) do
+         {:ok, oauth_token} <- RevokeToken.revoke(oauth_token) do
+      conn =
+        with session_token = AuthHelper.get_session_token(conn),
+             %Token{token: ^session_token} <- oauth_token do
+          AuthHelper.delete_session_token(conn)
+        else
+          _ -> conn
+        end
+
      json(conn, %{})
    else
      _error ->
--- a/lib/pleroma/web/o_auth/token.ex
+++ b/lib/pleroma/web/o_auth/token.ex
@ -39,12 +39,6 @@ def get_by_token(token) do
    |> Repo.find_resource()
  end

-  def get_by_app(%App{} = app) do
-    app.id
-    |> Query.get_unexpired_by_app()
-    |> 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
--- a/lib/pleroma/web/o_auth/token/query.ex
+++ b/lib/pleroma/web/o_auth/token/query.ex
@ -23,15 +23,9 @@ def get_by_token(query \\ Token, token) do
    from(q in query, where: q.token == ^token)
  end

-  @spec get_unexpired_by_app(query, String.t()) :: query
-  def get_unexpired_by_app(query \\ Token, app_id) do
-    time = NaiveDateTime.utc_now()
-    from(q in query, where: q.app_id == ^app_id and q.valid_until > ^time, limit: 1)
-  end
-
  @spec get_by_app(query, String.t()) :: query
  def get_by_app(query \\ Token, app_id) do
-    from(q in query, where: q.app_id == ^app_id, limit: 1)
+    from(q in query, where: q.app_id == ^app_id)
  end

  @spec get_by_id(query, String.t()) :: query
--- a/lib/pleroma/web/plugs/o_auth_plug.ex
+++ b/lib/pleroma/web/plugs/o_auth_plug.ex
@ -8,6 +8,7 @@ defmodule Pleroma.Web.Plugs.OAuthPlug do
  import Plug.Conn
  import Ecto.Query

+  alias Pleroma.Helpers.AuthHelper
  alias Pleroma.Repo
  alias Pleroma.User
  alias Pleroma.Web.OAuth.App
@ -17,6 +18,8 @@ defmodule Pleroma.Web.Plugs.OAuthPlug do

  def init(options), do: options

+  def call(%{assigns: %{user: %User{}}} = conn, _), do: conn
+
  def call(conn, _) do
    with {:ok, token_str} <- fetch_token_str(conn) do
      with {:ok, user, user_token} <- fetch_user_and_token(token_str),
@ -79,7 +82,7 @@ defp fetch_token_str(%Plug.Conn{} = conn) do
    with {:ok, token} <- fetch_token_str(headers) do
      {:ok, token}
    else
-      _ -> :no_token_found
+      _ -> fetch_token_from_session(conn)
    end
  end

@ -93,4 +96,12 @@ defp fetch_token_str([token | tail]) do
  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 AuthHelper.get_session_token(conn) do
+      nil -> :no_token_found
+      token -> {:ok, token}
+    end
+  end
 end
--- a/lib/pleroma/web/plugs/set_user_session_id_plug.ex
+++ b/lib/pleroma/web/plugs/set_user_session_id_plug.ex
@ -0,0 +1,18 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2021 Pleroma Authors <https://pleroma.social/>
+# SPDX-License-Identifier: AGPL-3.0-only
+
+defmodule Pleroma.Web.Plugs.SetUserSessionIdPlug do
+  alias Pleroma.Helpers.AuthHelper
+  alias Pleroma.Web.OAuth.Token
+
+  def init(opts) do
+    opts
+  end
+
+  def call(%{assigns: %{token: %Token{} = oauth_token}} = conn, _) do
+    AuthHelper.put_session_token(conn, oauth_token.token)
+  end
+
+  def call(conn, _), do: conn
+end
--- a/lib/pleroma/web/router.ex
+++ b/lib/pleroma/web/router.ex
@ -57,6 +57,7 @@ defmodule Pleroma.Web.Router do

  pipeline :after_auth do
    plug(Pleroma.Web.Plugs.UserEnabledPlug)
+    plug(Pleroma.Web.Plugs.SetUserSessionIdPlug)
    plug(Pleroma.Web.Plugs.EnsureUserTokenAssignsPlug)
    plug(Pleroma.Web.Plugs.UserTrackingPlug)
  end
@ -792,9 +793,11 @@ defmodule Pleroma.Web.Router do

    get("/web/login", MastodonAPI.AuthController, :login)
    delete("/auth/sign_out", MastodonAPI.AuthController, :logout)
-    get("/web/*path", MastoFEController, :index)
+
    post("/auth/password", MastodonAPI.AuthController, :password_reset)

+    get("/web/*path", MastoFEController, :index)
+
    get("/embed/:id", EmbedController, :show)
  end

--- a/mix.exs
+++ b/mix.exs
@ -131,7 +131,7 @@ defp deps do
      {:trailing_format_plug, "~> 0.0.7"},
      {:fast_sanitize, "~> 0.2.3"},
      {:html_entities, "~> 0.5", override: true},
-      {:phoenix_html, "~> 3.0", override: true},
+      {:phoenix_html, "~> 3.1", override: true},
      {:calendar, "~> 1.0"},
      {:cachex, "~> 3.4"},
      {:poison, "~> 3.0", override: true},
@ -152,7 +152,7 @@ defp deps do
       ref: "f75cd55325e33cbea198fb41fe41871392f8fb76"},
      {:cors_plug, "~> 2.0"},
      {:web_push_encryption, "~> 0.3.1"},
-      {:swoosh, "~> 1.3"},
+      {:swoosh, "~> 1.0"},
      {:phoenix_swoosh, "~> 0.3"},
      {:gen_smtp, "~> 0.13"},
      {:ex_syslogger, "~> 1.4"},
--- a/test/pleroma/web/o_auth/o_auth_controller_test.exs
+++ b/test/pleroma/web/o_auth/o_auth_controller_test.exs
@ -7,6 +7,7 @@ defmodule Pleroma.Web.OAuth.OAuthControllerTest do

  import Pleroma.Factory

+  alias Pleroma.Helpers.AuthHelper
  alias Pleroma.MFA
  alias Pleroma.MFA.TOTP
  alias Pleroma.Repo
@ -455,7 +456,7 @@ test "renders authentication page if user is already authenticated but `force_lo

      conn =
        conn
-        |> put_req_header("authorization", "Bearer #{token.token}")
+        |> AuthHelper.put_session_token(token.token)
        |> get(
          "/oauth/authorize",
          %{
@ -479,7 +480,7 @@ test "renders authentication page if user is already authenticated but user requ

      conn =
        conn
-        |> put_req_header("authorization", "Bearer #{token.token}")
+        |> AuthHelper.put_session_token(token.token)
        |> get(
          "/oauth/authorize",
          %{
@ -502,7 +503,7 @@ test "with existing authentication and non-OOB `redirect_uri`, redirects to app

      conn =
        conn
-        |> put_req_header("authorization", "Bearer #{token.token}")
+        |> AuthHelper.put_session_token(token.token)
        |> get(
          "/oauth/authorize",
          %{
@ -528,7 +529,7 @@ test "with existing authentication and unlisted non-OOB `redirect_uri`, redirect

      conn =
        conn
-        |> put_req_header("authorization", "Bearer #{token.token}")
+        |> AuthHelper.put_session_token(token.token)
        |> get(
          "/oauth/authorize",
          %{
@ -552,7 +553,7 @@ test "with existing authentication and OOB `redirect_uri`, redirects to app with

      conn =
        conn
-        |> put_req_header("authorization", "Bearer #{token.token}")
+        |> AuthHelper.put_session_token(token.token)
        |> get(
          "/oauth/authorize",
          %{
@ -610,6 +611,41 @@ test "redirects with oauth authorization, " <>
      end
    end

+    test "authorize from cookie" do
+      user = insert(:user)
+      app = insert(:oauth_app)
+      oauth_token = insert(:oauth_token, user: user, app: app)
+      redirect_uri = OAuthController.default_redirect_uri(app)
+
+      conn =
+        build_conn()
+        |> Plug.Session.call(Plug.Session.init(@session_opts))
+        |> fetch_session()
+        |> AuthHelper.put_session_token(oauth_token.token)
+        |> post(
+          "/oauth/authorize",
+          %{
+            "authorization" => %{
+              "name" => user.nickname,
+              "client_id" => app.client_id,
+              "redirect_uri" => redirect_uri,
+              "scope" => app.scopes,
+              "state" => "statepassed"
+            }
+          }
+        )
+
+      target = redirected_to(conn)
+      assert target =~ redirect_uri
+
+      query = URI.parse(target).query |> URI.query_decoder() |> Map.new()
+
+      assert %{"state" => "statepassed", "code" => code} = query
+      auth = Repo.get_by(Authorization, token: code)
+      assert auth
+      assert auth.scopes == app.scopes
+    end
+
    test "redirect to on two-factor auth page" do
      otp_secret = TOTP.generate_secret()

@ -1182,7 +1218,6 @@ test "issues a new token if token expired" do

      response =
        build_conn()
-        |> put_req_header("authorization", "Bearer #{access_token.token}")
        |> post("/oauth/token", %{
          "grant_type" => "refresh_token",
          "refresh_token" => access_token.refresh_token,
@ -1232,11 +1267,12 @@ test "when authenticated with request token, revokes it and clears it from sessi
        build_conn()
        |> Plug.Session.call(Plug.Session.init(@session_opts))
        |> fetch_session()
-        |> put_req_header("authorization", "Bearer #{oauth_token.token}")
+        |> AuthHelper.put_session_token(oauth_token.token)
        |> post("/oauth/revoke", %{"token" => oauth_token.token})

      assert json_response(conn, 200)

+      refute AuthHelper.get_session_token(conn)
      assert Token.get_by_token(oauth_token.token) == {:error, :not_found}
    end

@ -1250,11 +1286,12 @@ test "if request is authenticated with a different token, " <>
        build_conn()
        |> Plug.Session.call(Plug.Session.init(@session_opts))
        |> fetch_session()
-        |> put_req_header("authorization", "Bearer #{oauth_token.token}")
+        |> AuthHelper.put_session_token(oauth_token.token)
        |> post("/oauth/revoke", %{"token" => other_app_oauth_token.token})

      assert json_response(conn, 200)

+      assert AuthHelper.get_session_token(conn) == oauth_token.token
      assert Token.get_by_token(other_app_oauth_token.token) == {:error, :not_found}
    end

--- a/test/pleroma/web/plugs/o_auth_plug_test.exs
+++ b/test/pleroma/web/plugs/o_auth_plug_test.exs
@ -5,8 +5,11 @@
 defmodule Pleroma.Web.Plugs.OAuthPlugTest do
  use Pleroma.Web.ConnCase, async: true

+  alias Pleroma.Helpers.AuthHelper
  alias Pleroma.Web.OAuth.Token
+  alias Pleroma.Web.OAuth.Token.Strategy.Revoke
  alias Pleroma.Web.Plugs.OAuthPlug
+  alias Plug.Session

  import Pleroma.Factory

@ -69,4 +72,57 @@ test "with invalid token, it does not assign the user", %{conn: conn} do

    refute conn.assigns[:user]
  end
+
+  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()
+        |> AuthHelper.put_session_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
--- a/test/pleroma/web/plugs/set_user_session_id_plug_test.exs
+++ b/test/pleroma/web/plugs/set_user_session_id_plug_test.exs
@ -0,0 +1,43 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2021 Pleroma Authors <https://pleroma.social/>
+# SPDX-License-Identifier: AGPL-3.0-only
+
+defmodule Pleroma.Web.Plugs.SetUserSessionIdPlugTest do
+  use Pleroma.Web.ConnCase, async: true
+
+  alias Pleroma.Helpers.AuthHelper
+  alias Pleroma.Web.Plugs.SetUserSessionIdPlug
+
+  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()
+
+    %{conn: conn}
+  end
+
+  test "doesn't do anything if the user isn't set", %{conn: conn} do
+    ret_conn = SetUserSessionIdPlug.call(conn, %{})
+
+    assert ret_conn == conn
+  end
+
+  test "sets session token basing on :token assign", %{conn: conn} do
+    %{user: user, token: oauth_token} = oauth_access(["read"])
+
+    ret_conn =
+      conn
+      |> assign(:user, user)
+      |> assign(:token, oauth_token)
+      |> SetUserSessionIdPlug.call(%{})
+
+    assert AuthHelper.get_session_token(ret_conn) == oauth_token.token
+  end
+end