From 4b2c29016cb0a735aeeda535ab956507b2a7c546 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Wed, 12 Jun 2019 21:30:06 +0300 Subject: [PATCH] [#963] No redirect on OOB OAuth authorize request with existing authorization. OAuth-related refactoring. --- lib/pleroma/helpers/uri_helper.ex | 27 ++++ lib/pleroma/web/oauth/oauth_controller.ex | 132 ++++++++++-------- ...eex => oob_authorization_created.html.eex} | 0 .../o_auth/o_auth/oob_token_exists.html.eex | 2 + test/web/oauth/oauth_controller_test.exs | 33 ++++- 5 files changed, 133 insertions(+), 61 deletions(-) create mode 100644 lib/pleroma/helpers/uri_helper.ex rename lib/pleroma/web/templates/o_auth/o_auth/{results.html.eex => oob_authorization_created.html.eex} (100%) create mode 100644 lib/pleroma/web/templates/o_auth/o_auth/oob_token_exists.html.eex diff --git a/lib/pleroma/helpers/uri_helper.ex b/lib/pleroma/helpers/uri_helper.ex new file mode 100644 index 000000000..8a79b44c4 --- /dev/null +++ b/lib/pleroma/helpers/uri_helper.ex @@ -0,0 +1,27 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2019 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Helpers.UriHelper do + def append_uri_params(uri, appended_params) do + uri = URI.parse(uri) + appended_params = for {k, v} <- appended_params, into: %{}, do: {to_string(k), v} + existing_params = URI.query_decoder(uri.query || "") |> Enum.into(%{}) + updated_params_keys = Enum.uniq(Map.keys(existing_params) ++ Map.keys(appended_params)) + + updated_params = + for k <- updated_params_keys, do: {k, appended_params[k] || existing_params[k]} + + uri + |> Map.put(:query, URI.encode_query(updated_params)) + |> URI.to_string() + end + + def append_param_if_present(%{} = params, param_name, param_value) do + if param_value do + Map.put(params, param_name, param_value) + else + params + end + end +end diff --git a/lib/pleroma/web/oauth/oauth_controller.ex b/lib/pleroma/web/oauth/oauth_controller.ex index 79d803295..35a7c582e 100644 --- a/lib/pleroma/web/oauth/oauth_controller.ex +++ b/lib/pleroma/web/oauth/oauth_controller.ex @@ -5,6 +5,7 @@ defmodule Pleroma.Web.OAuth.OAuthController do use Pleroma.Web, :controller + alias Pleroma.Helpers.UriHelper alias Pleroma.Registration alias Pleroma.Repo alias Pleroma.User @@ -26,34 +27,25 @@ defmodule Pleroma.Web.OAuth.OAuthController do action_fallback(Pleroma.Web.OAuth.FallbackController) + @oob_token_redirect_uri "urn:ietf:wg:oauth:2.0:oob" + # Note: this definition is only called from error-handling methods with `conn.params` as 2nd arg - def authorize(conn, %{"authorization" => _} = params) do + def authorize(%Plug.Conn{} = conn, %{"authorization" => _} = params) do {auth_attrs, params} = Map.pop(params, "authorization") authorize(conn, Map.merge(params, auth_attrs)) end - def authorize(%{assigns: %{token: %Token{} = token}} = conn, params) do + def authorize(%Plug.Conn{assigns: %{token: %Token{}}} = conn, params) do if ControllerHelper.truthy_param?(params["force_login"]) do do_authorize(conn, params) else - redirect_uri = - if is_binary(params["redirect_uri"]) do - params["redirect_uri"] - else - app = Repo.preload(token, :app).app - - app.redirect_uris - |> String.split() - |> Enum.at(0) - end - - redirect(conn, external: redirect_uri(conn, redirect_uri)) + handle_existing_authorization(conn, params) end end - def authorize(conn, params), do: do_authorize(conn, params) + def authorize(%Plug.Conn{} = conn, params), do: do_authorize(conn, params) - defp do_authorize(conn, params) do + defp do_authorize(%Plug.Conn{} = conn, params) do app = Repo.get_by(App, client_id: params["client_id"]) available_scopes = (app && app.scopes) || [] scopes = Scopes.fetch_scopes(params, available_scopes) @@ -70,8 +62,33 @@ defp do_authorize(conn, params) do }) end + defp handle_existing_authorization( + %Plug.Conn{assigns: %{token: %Token{} = token}} = conn, + params + ) do + token = Repo.preload(token, :app) + + redirect_uri = + if is_binary(params["redirect_uri"]) do + params["redirect_uri"] + else + default_redirect_uri(token.app) + end + + redirect_uri = redirect_uri(conn, redirect_uri) + + if redirect_uri == @oob_token_redirect_uri do + render(conn, "oob_token_exists.html", %{token: token}) + else + url_params = %{access_token: token.token} + url_params = UriHelper.append_param_if_present(url_params, :state, params["state"]) + url = UriHelper.append_uri_params(redirect_uri, url_params) + redirect(conn, external: url) + end + end + def create_authorization( - conn, + %Plug.Conn{} = conn, %{"authorization" => _} = params, opts \\ [] ) do @@ -83,35 +100,23 @@ def create_authorization( end end - def after_create_authorization(conn, auth, %{ + def after_create_authorization(%Plug.Conn{} = conn, %Authorization{} = auth, %{ "authorization" => %{"redirect_uri" => redirect_uri} = auth_attrs }) do redirect_uri = redirect_uri(conn, redirect_uri) - if redirect_uri == "urn:ietf:wg:oauth:2.0:oob" do - render(conn, "results.html", %{ - auth: auth - }) + if redirect_uri == @oob_token_redirect_uri do + render(conn, "oob_authorization_created.html", %{auth: auth}) else - connector = if String.contains?(redirect_uri, "?"), do: "&", else: "?" - url = "#{redirect_uri}#{connector}" - url_params = %{:code => auth.token} - - url_params = - if auth_attrs["state"] do - Map.put(url_params, :state, auth_attrs["state"]) - else - url_params - end - - url = "#{url}#{Plug.Conn.Query.encode(url_params)}" - + url_params = %{code: auth.token} + url_params = UriHelper.append_param_if_present(url_params, :state, auth_attrs["state"]) + url = UriHelper.append_uri_params(redirect_uri, url_params) redirect(conn, external: url) end end defp handle_create_authorization_error( - conn, + %Plug.Conn{} = conn, {:error, scopes_issue}, %{"authorization" => _} = params ) @@ -125,7 +130,7 @@ defp handle_create_authorization_error( end defp handle_create_authorization_error( - conn, + %Plug.Conn{} = conn, {:auth_active, false}, %{"authorization" => _} = params ) do @@ -137,13 +142,13 @@ defp handle_create_authorization_error( |> authorize(params) end - defp handle_create_authorization_error(conn, error, %{"authorization" => _}) do + defp handle_create_authorization_error(%Plug.Conn{} = conn, error, %{"authorization" => _}) do Authenticator.handle_error(conn, error) end @doc "Renew access_token with refresh_token" def token_exchange( - conn, + %Plug.Conn{} = conn, %{"grant_type" => "refresh_token", "refresh_token" => token} = _params ) do with {:ok, app} <- Token.Utils.fetch_app(conn), @@ -159,7 +164,7 @@ def token_exchange( end end - def token_exchange(conn, %{"grant_type" => "authorization_code"} = params) do + def token_exchange(%Plug.Conn{} = conn, %{"grant_type" => "authorization_code"} = params) do with {:ok, app} <- Token.Utils.fetch_app(conn), fixed_token = Token.Utils.fix_padding(params["code"]), {:ok, auth} <- Authorization.get_by_token(app, fixed_token), @@ -176,7 +181,7 @@ def token_exchange(conn, %{"grant_type" => "authorization_code"} = params) do end def token_exchange( - conn, + %Plug.Conn{} = conn, %{"grant_type" => "password"} = params ) do with {:ok, %User{} = user} <- Authenticator.get_user(conn), @@ -207,7 +212,7 @@ def token_exchange( end def token_exchange( - conn, + %Plug.Conn{} = conn, %{"grant_type" => "password", "name" => name, "password" => _password} = params ) do params = @@ -218,7 +223,7 @@ def token_exchange( token_exchange(conn, params) end - def token_exchange(conn, %{"grant_type" => "client_credentials"} = _params) do + def token_exchange(%Plug.Conn{} = conn, %{"grant_type" => "client_credentials"} = _params) do with {:ok, app} <- Token.Utils.fetch_app(conn), {:ok, auth} <- Authorization.create_authorization(app, %User{}), {:ok, token} <- Token.exchange_token(app, auth) do @@ -231,9 +236,9 @@ def token_exchange(conn, %{"grant_type" => "client_credentials"} = _params) do end # Bad request - def token_exchange(conn, params), do: bad_request(conn, params) + def token_exchange(%Plug.Conn{} = conn, params), do: bad_request(conn, params) - def token_revoke(conn, %{"token" => _token} = params) 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 json(conn, %{}) @@ -244,17 +249,20 @@ def token_revoke(conn, %{"token" => _token} = params) do end end - def token_revoke(conn, params), do: bad_request(conn, params) + def token_revoke(%Plug.Conn{} = conn, params), do: bad_request(conn, params) # Response for bad request - defp bad_request(conn, _) do + defp bad_request(%Plug.Conn{} = conn, _) do conn |> put_status(500) |> json(%{error: "Bad request"}) end @doc "Prepares OAuth request to provider for Ueberauth" - def prepare_request(conn, %{"provider" => provider, "authorization" => auth_attrs}) do + def prepare_request(%Plug.Conn{} = conn, %{ + "provider" => provider, + "authorization" => auth_attrs + }) do scope = auth_attrs |> Scopes.fetch_scopes([]) @@ -275,7 +283,7 @@ def prepare_request(conn, %{"provider" => provider, "authorization" => auth_attr redirect(conn, to: o_auth_path(conn, :request, provider, params)) end - def request(conn, params) do + def request(%Plug.Conn{} = conn, params) do message = if params["provider"] do "Unsupported OAuth provider: #{params["provider"]}." @@ -288,7 +296,7 @@ def request(conn, params) do |> redirect(to: "/") end - def callback(%{assigns: %{ueberauth_failure: failure}} = conn, params) do + def callback(%Plug.Conn{assigns: %{ueberauth_failure: failure}} = conn, params) do params = callback_params(params) messages = for e <- Map.get(failure, :errors, []), do: e.message message = Enum.join(messages, "; ") @@ -298,7 +306,7 @@ def callback(%{assigns: %{ueberauth_failure: failure}} = conn, params) do |> redirect(external: redirect_uri(conn, params["redirect_uri"])) end - def callback(conn, params) do + def callback(%Plug.Conn{} = conn, params) do params = callback_params(params) with {:ok, registration} <- Authenticator.get_registration(conn) do @@ -333,7 +341,7 @@ defp callback_params(%{"state" => state} = params) do Map.merge(params, Jason.decode!(state)) end - def registration_details(conn, %{"authorization" => auth_attrs}) do + def registration_details(%Plug.Conn{} = conn, %{"authorization" => auth_attrs}) do render(conn, "register.html", %{ client_id: auth_attrs["client_id"], redirect_uri: auth_attrs["redirect_uri"], @@ -344,7 +352,7 @@ def registration_details(conn, %{"authorization" => auth_attrs}) do }) end - def register(conn, %{"authorization" => _, "op" => "connect"} = params) do + def register(%Plug.Conn{} = conn, %{"authorization" => _, "op" => "connect"} = params) do with registration_id when not is_nil(registration_id) <- get_session_registration_id(conn), %Registration{} = registration <- Repo.get(Registration, registration_id), {_, {:ok, auth}} <- @@ -363,7 +371,7 @@ def register(conn, %{"authorization" => _, "op" => "connect"} = params) do end end - def register(conn, %{"authorization" => _, "op" => "register"} = params) do + def register(%Plug.Conn{} = conn, %{"authorization" => _, "op" => "register"} = params) do with registration_id when not is_nil(registration_id) <- get_session_registration_id(conn), %Registration{} = registration <- Repo.get(Registration, registration_id), {:ok, user} <- Authenticator.create_from_registration(conn, registration) do @@ -399,7 +407,7 @@ def register(conn, %{"authorization" => _, "op" => "register"} = params) do end defp do_create_authorization( - conn, + %Plug.Conn{} = conn, %{ "authorization" => %{ @@ -420,13 +428,13 @@ defp do_create_authorization( end # Special case: Local MastodonFE - defp redirect_uri(conn, "."), do: mastodon_api_url(conn, :login) + defp redirect_uri(%Plug.Conn{} = conn, "."), do: mastodon_api_url(conn, :login) - defp redirect_uri(_conn, redirect_uri), do: redirect_uri + defp redirect_uri(%Plug.Conn{}, redirect_uri), do: redirect_uri - defp get_session_registration_id(conn), do: get_session(conn, :registration_id) + defp get_session_registration_id(%Plug.Conn{} = conn), do: get_session(conn, :registration_id) - defp put_session_registration_id(conn, registration_id), + defp put_session_registration_id(%Plug.Conn{} = conn, registration_id), do: put_session(conn, :registration_id, registration_id) @spec validate_scopes(App.t(), map()) :: @@ -436,4 +444,10 @@ defp validate_scopes(app, params) do |> Scopes.fetch_scopes(app.scopes) |> Scopes.validates(app.scopes) end + + defp default_redirect_uri(%App{} = app) do + app.redirect_uris + |> String.split() + |> Enum.at(0) + end end diff --git a/lib/pleroma/web/templates/o_auth/o_auth/results.html.eex b/lib/pleroma/web/templates/o_auth/o_auth/oob_authorization_created.html.eex similarity index 100% rename from lib/pleroma/web/templates/o_auth/o_auth/results.html.eex rename to lib/pleroma/web/templates/o_auth/o_auth/oob_authorization_created.html.eex diff --git a/lib/pleroma/web/templates/o_auth/o_auth/oob_token_exists.html.eex b/lib/pleroma/web/templates/o_auth/o_auth/oob_token_exists.html.eex new file mode 100644 index 000000000..961aad976 --- /dev/null +++ b/lib/pleroma/web/templates/o_auth/o_auth/oob_token_exists.html.eex @@ -0,0 +1,2 @@ +

Authorization exists

+

Access token is <%= @token.token %>

diff --git a/test/web/oauth/oauth_controller_test.exs b/test/web/oauth/oauth_controller_test.exs index 1c04ac9ad..242b7fdb3 100644 --- a/test/web/oauth/oauth_controller_test.exs +++ b/test/web/oauth/oauth_controller_test.exs @@ -408,7 +408,11 @@ test "renders authentication page if user is already authenticated but `force_lo assert html_response(conn, 200) =~ ~s(type="submit") end - test "redirects to app if user is already authenticated", %{app: app, conn: conn} do + test "with existing authentication and non-OOB `redirect_uri`, redirects to app with `token` and `state` params", + %{ + app: app, + conn: conn + } do token = insert(:oauth_token, app_id: app.id) conn = @@ -420,11 +424,36 @@ test "redirects to app if user is already authenticated", %{app: app, conn: conn "response_type" => "code", "client_id" => app.client_id, "redirect_uri" => app.redirect_uris, + "state" => "specific_client_state", "scope" => "read" } ) - assert redirected_to(conn) == "https://redirect.url" + assert URI.decode(redirected_to(conn)) == + "https://redirect.url?access_token=#{token.token}&state=specific_client_state" + end + + test "with existing authentication and OOB `redirect_uri`, redirects to app with `token` and `state` params", + %{ + app: app, + conn: conn + } do + token = insert(:oauth_token, app_id: app.id) + + conn = + conn + |> put_session(:oauth_token, token.token) + |> get( + "/oauth/authorize", + %{ + "response_type" => "code", + "client_id" => app.client_id, + "redirect_uri" => "urn:ietf:wg:oauth:2.0:oob", + "scope" => "read" + } + ) + + assert html_response(conn, 200) =~ "Authorization exists" end end