From 73904e8f788bb462ce1090cbd36504f6ea49c245 Mon Sep 17 00:00:00 2001 From: D Anzorge Date: Fri, 1 Jun 2018 18:01:56 +0200 Subject: [PATCH 1/3] Make OAuth token endpoint work with HTTP Basic auth client_id/client_secret can now be supplied in an Authorization header --- lib/pleroma/web/oauth/oauth_controller.ex | 38 ++++++++++++++++------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/lib/pleroma/web/oauth/oauth_controller.ex b/lib/pleroma/web/oauth/oauth_controller.ex index 11dc1806f..bc6c365c9 100644 --- a/lib/pleroma/web/oauth/oauth_controller.ex +++ b/lib/pleroma/web/oauth/oauth_controller.ex @@ -56,12 +56,7 @@ def create_authorization(conn, %{ # TODO # - proper scope handling def token_exchange(conn, %{"grant_type" => "authorization_code"} = params) do - with %App{} = app <- - Repo.get_by( - App, - client_id: params["client_id"], - client_secret: params["client_secret"] - ), + with %App{} = app <- get_app_from_request(conn, params), fixed_token = fix_padding(params["code"]), %Authorization{} = auth <- Repo.get_by(Authorization, token: fixed_token, app_id: app.id), @@ -86,12 +81,7 @@ def token_exchange( conn, %{"grant_type" => "password", "name" => name, "password" => password} = params ) do - with %App{} = app <- - Repo.get_by( - App, - client_id: params["client_id"], - client_secret: params["client_secret"] - ), + with %App{} = app <- get_app_from_request(conn, params), %User{} = user <- User.get_cached_by_nickname(name), true <- Pbkdf2.checkpw(password, user.password_hash), {:ok, auth} <- Authorization.create_authorization(app, user), @@ -115,4 +105,28 @@ defp fix_padding(token) do |> Base.url_decode64!(padding: false) |> Base.url_encode64() end + + defp get_app_from_request(conn, params) do + # Per RFC 6749, HTTP Basic is preferred to body params + {client_id, client_secret} = + with ["Basic " <> encoded] <- get_req_header(conn, "authorization"), + {:ok, decoded} <- Base.decode64(encoded), + [id, secret] <- + String.split(decoded, ":") + |> Enum.map(fn s -> URI.decode_www_form(s) end) do + {id, secret} + else + _ -> {params["client_id"], params["client_secret"]} + end + + if client_id && client_secret do + Repo.get_by( + App, + client_id: client_id, + client_secret: client_secret + ) + else + nil + end + end end From 3607dc4558b2d30ca36d9a600b6bbf916b71f54a Mon Sep 17 00:00:00 2001 From: D Anzorge Date: Wed, 6 Jun 2018 03:14:50 +0200 Subject: [PATCH 2/3] Make token exchange return errors with 400 as status code --- lib/pleroma/web/oauth/oauth_controller.ex | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/pleroma/web/oauth/oauth_controller.ex b/lib/pleroma/web/oauth/oauth_controller.ex index bc6c365c9..3dd87d0ab 100644 --- a/lib/pleroma/web/oauth/oauth_controller.ex +++ b/lib/pleroma/web/oauth/oauth_controller.ex @@ -71,7 +71,9 @@ def token_exchange(conn, %{"grant_type" => "authorization_code"} = params) do json(conn, response) else - _error -> json(conn, %{error: "Invalid credentials"}) + _error -> + put_status(conn, 400) + |> json(%{error: "Invalid credentials"}) end end @@ -96,7 +98,9 @@ def token_exchange( json(conn, response) else - _error -> json(conn, %{error: "Invalid credentials"}) + _error -> + put_status(conn, 400) + |> json(%{error: "Invalid credentials"}) end end From 2cebaa7d3a6fcf0085888809c14c8b949b15257f Mon Sep 17 00:00:00 2001 From: D Anzorge Date: Wed, 6 Jun 2018 03:18:11 +0200 Subject: [PATCH 3/3] Add OAuth controller tests Tests for Pleroma.Web.OAuth.OAuthController --- test/support/factory.ex | 11 +++ test/web/oauth/oauth_controller_test.exs | 113 +++++++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 test/web/oauth/oauth_controller_test.exs diff --git a/test/support/factory.ex b/test/support/factory.ex index 5cf456e3c..6c48d390f 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -146,4 +146,15 @@ def websub_client_subscription_factory do subscribers: [] } end + + def oauth_app_factory do + %Pleroma.Web.OAuth.App{ + client_name: "Some client", + redirect_uris: "https://example.com/callback", + scopes: "read", + website: "https://example.com", + client_id: "aaabbb==", + client_secret: "aaa;/&bbb" + } + end end diff --git a/test/web/oauth/oauth_controller_test.exs b/test/web/oauth/oauth_controller_test.exs new file mode 100644 index 000000000..3a902f128 --- /dev/null +++ b/test/web/oauth/oauth_controller_test.exs @@ -0,0 +1,113 @@ +defmodule Pleroma.Web.OAuth.OAuthControllerTest do + use Pleroma.Web.ConnCase + import Pleroma.Factory + + alias Pleroma.Repo + alias Pleroma.Web.OAuth.{Authorization, Token} + + test "redirects with oauth authorization" do + user = insert(:user) + app = insert(:oauth_app) + + conn = + build_conn() + |> post("/oauth/authorize", %{ + "authorization" => %{ + "name" => user.nickname, + "password" => "test", + "client_id" => app.client_id, + "redirect_uri" => app.redirect_uris, + "state" => "statepassed" + } + }) + + target = redirected_to(conn) + assert target =~ app.redirect_uris + + query = URI.parse(target).query |> URI.query_decoder() |> Map.new() + + assert %{"state" => "statepassed", "code" => code} = query + assert Repo.get_by(Authorization, token: code) + end + + test "issues a token for an all-body request" do + user = insert(:user) + app = insert(:oauth_app) + + {:ok, auth} = Authorization.create_authorization(app, user) + + conn = + build_conn() + |> post("/oauth/token", %{ + "grant_type" => "authorization_code", + "code" => auth.token, + "redirect_uri" => app.redirect_uris, + "client_id" => app.client_id, + "client_secret" => app.client_secret + }) + + assert %{"access_token" => token} = json_response(conn, 200) + assert Repo.get_by(Token, token: token) + end + + test "issues a token for request with HTTP basic auth client credentials" do + user = insert(:user) + app = insert(:oauth_app) + + {:ok, auth} = Authorization.create_authorization(app, user) + + app_encoded = + (URI.encode_www_form(app.client_id) <> ":" <> URI.encode_www_form(app.client_secret)) + |> Base.encode64() + + conn = + build_conn() + |> put_req_header("authorization", "Basic " <> app_encoded) + |> post("/oauth/token", %{ + "grant_type" => "authorization_code", + "code" => auth.token, + "redirect_uri" => app.redirect_uris + }) + + assert %{"access_token" => token} = json_response(conn, 200) + assert Repo.get_by(Token, token: token) + end + + test "rejects token exchange with invalid client credentials" do + user = insert(:user) + app = insert(:oauth_app) + + {:ok, auth} = Authorization.create_authorization(app, user) + + conn = + build_conn() + |> put_req_header("authorization", "Basic JTIxOiVGMCU5RiVBNCVCNwo=") + |> post("/oauth/token", %{ + "grant_type" => "authorization_code", + "code" => auth.token, + "redirect_uri" => app.redirect_uris + }) + + assert resp = json_response(conn, 400) + assert %{"error" => _} = resp + refute Map.has_key?(resp, "access_token") + end + + test "rejects an invalid authorization code" do + app = insert(:oauth_app) + + conn = + build_conn() + |> post("/oauth/token", %{ + "grant_type" => "authorization_code", + "code" => "Imobviouslyinvalid", + "redirect_uri" => app.redirect_uris, + "client_id" => app.client_id, + "client_secret" => app.client_secret + }) + + assert resp = json_response(conn, 400) + assert %{"error" => _} = json_response(conn, 400) + refute Map.has_key?(resp, "access_token") + end +end