From b0130bfa7b420550aa7acba6a88c71aa22c51246 Mon Sep 17 00:00:00 2001 From: FloatingGhost Date: Sun, 21 Aug 2022 16:22:15 +0100 Subject: [PATCH] Revert "oauth2 fixes (#177)" This reverts commit 429e2ac832a874ae8ba8a9c116da61a6273c8a87. --- lib/pleroma/helpers/auth_helper.ex | 18 +++++ .../controllers/auth_controller.ex | 11 ++- lib/pleroma/web/o_auth/o_auth_controller.ex | 77 +++++++++---------- lib/pleroma/web/o_auth/token.ex | 6 -- lib/pleroma/web/o_auth/token/query.ex | 8 +- lib/pleroma/web/plugs/o_auth_plug.ex | 13 +++- .../web/plugs/set_user_session_id_plug.ex | 18 +++++ lib/pleroma/web/router.ex | 5 +- mix.exs | 4 +- .../web/o_auth/o_auth_controller_test.exs | 53 +++++++++++-- test/pleroma/web/plugs/o_auth_plug_test.exs | 56 ++++++++++++++ .../plugs/set_user_session_id_plug_test.exs | 43 +++++++++++ 12 files changed, 243 insertions(+), 69 deletions(-) create mode 100644 lib/pleroma/web/plugs/set_user_session_id_plug.ex create mode 100644 test/pleroma/web/plugs/set_user_session_id_plug_test.exs diff --git a/lib/pleroma/helpers/auth_helper.ex b/lib/pleroma/helpers/auth_helper.ex index d56f6f461..13e4c8158 100644 --- 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 diff --git a/lib/pleroma/web/mastodon_api/controllers/auth_controller.ex b/lib/pleroma/web/mastodon_api/controllers/auth_controller.ex index f415e5931..4920d65da 100644 --- 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 diff --git a/lib/pleroma/web/o_auth/o_auth_controller.ex b/lib/pleroma/web/o_auth/o_auth_controller.ex index 43536f95d..358120fe6 100644 --- 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 -> diff --git a/lib/pleroma/web/o_auth/token.ex b/lib/pleroma/web/o_auth/token.ex index 08c8cd298..9d69e9db4 100644 --- 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 diff --git a/lib/pleroma/web/o_auth/token/query.ex b/lib/pleroma/web/o_auth/token/query.ex index 8edfcf5d7..d16a759d8 100644 --- 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 diff --git a/lib/pleroma/web/plugs/o_auth_plug.ex b/lib/pleroma/web/plugs/o_auth_plug.ex index 29b3316b3..5e06ac3f6 100644 --- 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 diff --git a/lib/pleroma/web/plugs/set_user_session_id_plug.ex b/lib/pleroma/web/plugs/set_user_session_id_plug.ex new file mode 100644 index 000000000..a1cfa0915 --- /dev/null +++ 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 +# 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 diff --git a/lib/pleroma/web/router.ex b/lib/pleroma/web/router.ex index f2d6b0aff..647d99278 100644 --- 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 diff --git a/mix.exs b/mix.exs index 170276b0a..e7f491997 100644 --- 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"}, diff --git a/test/pleroma/web/o_auth/o_auth_controller_test.exs b/test/pleroma/web/o_auth/o_auth_controller_test.exs index 4e197a485..0fdd5b8e9 100644 --- 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 diff --git a/test/pleroma/web/plugs/o_auth_plug_test.exs b/test/pleroma/web/plugs/o_auth_plug_test.exs index caabfc1cb..9e4be5559 100644 --- 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 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 new file mode 100644 index 000000000..9814c80d8 --- /dev/null +++ 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 +# 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