From 1a2c17d764d09329ceb17d32a73cad809e0329f6 Mon Sep 17 00:00:00 2001 From: Andrew Dryga Date: Thu, 11 May 2023 14:21:35 -0600 Subject: [PATCH 1/3] Move redirect_uri from configs to a function argument and support other grant types for token endpoint --- lib/openid_connect.ex | 30 ++++----- test/openid_connect_test.exs | 121 ++++++++++++++++++++++++++--------- 2 files changed, 106 insertions(+), 45 deletions(-) diff --git a/lib/openid_connect.ex b/lib/openid_connect.ex index 3b0185c..554729e 100644 --- a/lib/openid_connect.ex +++ b/lib/openid_connect.ex @@ -50,7 +50,7 @@ defmodule OpenIDConnect do required(:discovery_document_uri) => discovery_document_uri(), required(:client_id) => client_id(), required(:client_secret) => client_secret(), - required(:redirect_uri) => redirect_uri(), + optional(:redirect_uri) => redirect_uri(), required(:response_type) => response_type(), required(:scope) => scope(), optional(:leeway) => non_neg_integer() @@ -76,7 +76,7 @@ defmodule OpenIDConnect do """ @spec authorization_uri(config(), params :: %{optional(atom) => term()}) :: {:ok, uri :: String.t()} | {:error, term()} - def authorization_uri(config, params \\ %{}) do + def authorization_uri(config, redirect_uri, params \\ %{}) do discovery_document_uri = config.discovery_document_uri with {:ok, document} <- Document.fetch_document(discovery_document_uri), @@ -86,7 +86,7 @@ defmodule OpenIDConnect do Map.merge( %{ client_id: config.client_id, - redirect_uri: config.redirect_uri, + redirect_uri: redirect_uri, response_type: response_type, scope: scope }, @@ -165,11 +165,16 @@ defmodule OpenIDConnect do end @doc """ - Fetches the authentication tokens from the provider + Fetches the authentication tokens from the provider using the token endpoint retrieved from a discovery document. - The `params` option should at least include the key/value pairs of the `response_type` that - was requested during authorization. `params` may also include any one-off overrides for token - fetching. + The `params` option depends on the `grant_type`: + + * for "authorization_code" grant type, `params` should at least include the `redirect_uri` and `code` params; + * for "refresh_token" grant type, `params` should at least include the `refresh_token` param; + * for other grant types and more details see the + [OpenID Connect spec](https://openid.net/specs/openid-connect-core-1_0.html#TokenEndpoint). + + `params` may also include any one-off overrides for token fetching. """ @spec fetch_tokens(config(), params :: %{optional(atom) => term()}) :: {:ok, response :: map()} | {:error, term()} @@ -177,15 +182,8 @@ defmodule OpenIDConnect do discovery_document_uri = config.discovery_document_uri form_body = - Map.merge( - %{ - client_id: config.client_id, - client_secret: config.client_secret, - redirect_uri: config.redirect_uri, - grant_type: "authorization_code" - }, - params - ) + %{client_id: config.client_id, client_secret: config.client_secret} + |> Map.merge(params) |> URI.encode_query(:www_form) headers = [{"Content-Type", "application/x-www-form-urlencoded"}] diff --git a/test/openid_connect_test.exs b/test/openid_connect_test.exs index 277d48c..e8f506d 100644 --- a/test/openid_connect_test.exs +++ b/test/openid_connect_test.exs @@ -3,25 +3,26 @@ defmodule OpenIDConnectTest do import OpenIDConnect.Fixtures import OpenIDConnect + @redirect_uri "https://localhost/redirect_uri" + @config %{ discovery_document_uri: nil, client_id: "CLIENT_ID", client_secret: "CLIENT_SECRET", - redirect_uri: "https://localhost/redirect_uri", response_type: "code id_token token", scope: "openid email profile" } - describe "authorization_uri/2" do + describe "authorization_uri/3" do test "generates authorization url with scope and response_type as binaries" do {_bypass, uri} = start_fixture("google") config = %{@config | discovery_document_uri: uri} - assert authorization_uri(config) == + assert authorization_uri(config, @redirect_uri) == {:ok, "https://accounts.google.com/o/oauth2/v2/auth?" <> "client_id=CLIENT_ID" <> - "&redirect_uri=https%3A%2F%2Flocalhost%2Fredirect_uri" <> + "&redirect_uri=#{URI.encode_www_form(@redirect_uri)}" <> "&response_type=code+id_token+token" <> "&scope=openid+email+profile"} end @@ -30,11 +31,11 @@ defmodule OpenIDConnectTest do {_bypass, uri} = start_fixture("google") config = %{@config | discovery_document_uri: uri, scope: ["openid", "email", "profile"]} - assert authorization_uri(config) == + assert authorization_uri(config, @redirect_uri) == {:ok, "https://accounts.google.com/o/oauth2/v2/auth?" <> "client_id=CLIENT_ID" <> - "&redirect_uri=https%3A%2F%2Flocalhost%2Fredirect_uri" <> + "&redirect_uri=#{URI.encode_www_form(@redirect_uri)}" <> "&response_type=code+id_token+token" <> "&scope=openid+email+profile"} end @@ -48,11 +49,11 @@ defmodule OpenIDConnectTest do response_type: ["code", "id_token", "token"] } - assert authorization_uri(config) == + assert authorization_uri(config, @redirect_uri) == {:ok, "https://accounts.google.com/o/oauth2/v2/auth?" <> "client_id=CLIENT_ID" <> - "&redirect_uri=https%3A%2F%2Flocalhost%2Fredirect_uri" <> + "&redirect_uri=#{URI.encode_www_form(@redirect_uri)}" <> "&response_type=code+id_token+token" <> "&scope=openid+email+profile"} end @@ -61,37 +62,37 @@ defmodule OpenIDConnectTest do {_bypass, uri} = start_fixture("google") config = %{@config | discovery_document_uri: uri, scope: nil} - assert authorization_uri(config) == {:error, :invalid_scope} + assert authorization_uri(config, @redirect_uri) == {:error, :invalid_scope} config = %{@config | discovery_document_uri: uri, scope: ""} - assert authorization_uri(config) == {:error, :invalid_scope} + assert authorization_uri(config, @redirect_uri) == {:error, :invalid_scope} config = %{@config | discovery_document_uri: uri, scope: []} - assert authorization_uri(config) == {:error, :invalid_scope} + assert authorization_uri(config, @redirect_uri) == {:error, :invalid_scope} end test "returns error on empty response_type" do {_bypass, uri} = start_fixture("google") config = %{@config | discovery_document_uri: uri, response_type: nil} - assert authorization_uri(config) == {:error, :invalid_response_type} + assert authorization_uri(config, @redirect_uri) == {:error, :invalid_response_type} config = %{@config | discovery_document_uri: uri, response_type: ""} - assert authorization_uri(config) == {:error, :invalid_response_type} + assert authorization_uri(config, @redirect_uri) == {:error, :invalid_response_type} config = %{@config | discovery_document_uri: uri, response_type: []} - assert authorization_uri(config) == {:error, :invalid_response_type} + assert authorization_uri(config, @redirect_uri) == {:error, :invalid_response_type} end test "adds optional params" do {_bypass, uri} = start_fixture("google") config = %{@config | discovery_document_uri: uri} - assert authorization_uri(config, %{"state" => "foo"}) == + assert authorization_uri(config, @redirect_uri, %{"state" => "foo"}) == {:ok, "https://accounts.google.com/o/oauth2/v2/auth?" <> "client_id=CLIENT_ID" <> - "&redirect_uri=https%3A%2F%2Flocalhost%2Fredirect_uri" <> + "&redirect_uri=#{URI.encode_www_form(@redirect_uri)}" <> "&response_type=code+id_token+token" <> "&scope=openid+email+profile" <> "&state=foo"} @@ -101,11 +102,11 @@ defmodule OpenIDConnectTest do {_bypass, uri} = start_fixture("google") config = %{@config | discovery_document_uri: uri} - assert authorization_uri(config, %{client_id: "foo"}) == + assert authorization_uri(config, @redirect_uri, %{client_id: "foo"}) == {:ok, "https://accounts.google.com/o/oauth2/v2/auth?" <> "client_id=foo" <> - "&redirect_uri=https%3A%2F%2Flocalhost%2Fredirect_uri" <> + "&redirect_uri=#{URI.encode_www_form(@redirect_uri)}" <> "&response_type=code+id_token+token" <> "&scope=openid+email+profile"} end @@ -117,7 +118,7 @@ defmodule OpenIDConnectTest do config = %{@config | discovery_document_uri: uri} - assert authorization_uri(config, %{client_id: "foo"}) == + assert authorization_uri(config, @redirect_uri, %{client_id: "foo"}) == {:error, %Mint.TransportError{reason: :econnrefused}} end end @@ -187,8 +188,14 @@ defmodule OpenIDConnectTest do {_bypass, uri} = start_fixture("google", %{token_endpoint: token_endpoint}) config = %{@config | discovery_document_uri: uri} - assert fetch_tokens(config, %{code: "1234", id_token: "abcd"}) == - {:ok, token_response_attrs} + params = %{ + grant_type: "authorization_code", + redirect_uri: @redirect_uri, + code: "1234", + id_token: "abcd" + } + + assert fetch_tokens(config, params) == {:ok, token_response_attrs} assert_receive {:req, body} @@ -198,7 +205,7 @@ defmodule OpenIDConnectTest do "&code=1234" <> "&grant_type=authorization_code" <> "&id_token=abcd" <> - "&redirect_uri=https%3A%2F%2Flocalhost%2Fredirect_uri" + "&redirect_uri=#{URI.encode_www_form(@redirect_uri)}" end test "allows to override the default params" do @@ -221,7 +228,11 @@ defmodule OpenIDConnectTest do {_bypass, uri} = start_fixture("google", %{token_endpoint: token_endpoint}) config = %{@config | discovery_document_uri: uri} - fetch_tokens(config, %{client_id: "foo"}) + fetch_tokens(config, %{ + client_id: "foo", + grant_type: "authorization_code", + redirect_uri: @redirect_uri + }) assert_receive {:req, body} @@ -229,7 +240,38 @@ defmodule OpenIDConnectTest do "client_id=foo" <> "&client_secret=CLIENT_SECRET" <> "&grant_type=authorization_code" <> - "&redirect_uri=https%3A%2F%2Flocalhost%2Fredirect_uri" + "&redirect_uri=#{URI.encode_www_form(@redirect_uri)}" + end + + test "allows to use refresh_token grant type" do + bypass = Bypass.open() + test_pid = self() + + token_response_attrs = %{ + "access_token" => "ACCESS_TOKEN", + "id_token" => "ID_TOKEN", + "refresh_token" => "REFRESH_TOKEN" + } + + Bypass.expect_once(bypass, "POST", "/token", fn conn -> + {:ok, body, conn} = Plug.Conn.read_body(conn) + send(test_pid, {:req, body}) + Plug.Conn.resp(conn, 200, Jason.encode!(token_response_attrs)) + end) + + token_endpoint = "http://localhost:#{bypass.port}/token" + {_bypass, uri} = start_fixture("google", %{token_endpoint: token_endpoint}) + config = %{@config | discovery_document_uri: uri} + + fetch_tokens(config, %{grant_type: "refresh_token", refresh_token: "foo"}) + + assert_receive {:req, body} + + assert body == + "client_id=CLIENT_ID" <> + "&client_secret=CLIENT_SECRET" <> + "&grant_type=refresh_token" <> + "&refresh_token=foo" end test "returns error when token endpoint is not available" do @@ -238,8 +280,9 @@ defmodule OpenIDConnectTest do token_endpoint = "http://localhost:#{bypass.port}/token" {_bypass, uri} = start_fixture("google", %{token_endpoint: token_endpoint}) config = %{@config | discovery_document_uri: uri} + params = %{grant_type: "authorization_code", redirect_uri: @redirect_uri} - assert fetch_tokens(config, %{client_id: "foo"}) == + assert fetch_tokens(config, params) == {:error, %Mint.TransportError{reason: :econnrefused}} end @@ -254,14 +297,21 @@ defmodule OpenIDConnectTest do {_bypass, uri} = start_fixture("google", %{token_endpoint: token_endpoint}) config = %{@config | discovery_document_uri: uri} - assert fetch_tokens(config, %{client_id: "foo"}) == + assert fetch_tokens(config, %{}) == {:error, {401, "{\"error\":\"unauthorized\"}"}} end test "returns error when real provider token endpoint is responded with invalid code" do {_bypass, uri} = start_fixture("google") config = %{@config | discovery_document_uri: uri} - assert {:error, {401, resp}} = fetch_tokens(config, %{code: "foo"}) + + assert {:error, {401, resp}} = + fetch_tokens(config, %{ + grant_type: "authorization_code", + redirect_uri: @redirect_uri, + code: "foo" + }) + resp_json = Jason.decode!(resp) assert resp_json == %{ @@ -272,7 +322,14 @@ defmodule OpenIDConnectTest do for provider <- ["auth0", "okta", "onelogin"] do {_bypass, uri} = start_fixture(provider) config = %{@config | discovery_document_uri: uri} - assert {:error, {status, _resp}} = fetch_tokens(config, %{code: "foo"}) + + assert {:error, {status, _resp}} = + fetch_tokens(config, %{ + grant_type: "authorization_code", + redirect_uri: @redirect_uri, + code: "foo" + }) + assert status in 400..499 end end @@ -284,7 +341,13 @@ defmodule OpenIDConnectTest do config = %{@config | discovery_document_uri: uri} - assert fetch_tokens(config, %{code: "foo"}) == + params = %{ + grant_type: "authorization_code", + redirect_uri: @redirect_uri, + code: "foo" + } + + assert fetch_tokens(config, params) == {:error, %Mint.TransportError{reason: :econnrefused}} end end From 69ee6b169ffbfe73c195eb94ddd5cefd5adf447f Mon Sep 17 00:00:00 2001 From: Andrew Dryga Date: Thu, 11 May 2023 14:35:43 -0600 Subject: [PATCH 2/3] Fix child specs --- lib/openid_connect.ex | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/openid_connect.ex b/lib/openid_connect.ex index 554729e..91df80c 100644 --- a/lib/openid_connect.ex +++ b/lib/openid_connect.ex @@ -50,7 +50,6 @@ defmodule OpenIDConnect do required(:discovery_document_uri) => discovery_document_uri(), required(:client_id) => client_id(), required(:client_secret) => client_secret(), - optional(:redirect_uri) => redirect_uri(), required(:response_type) => response_type(), required(:scope) => scope(), optional(:leeway) => non_neg_integer() @@ -74,8 +73,10 @@ defmodule OpenIDConnect do > It is *highly suggested* that you add the `state` param for security reasons. Your > OpenID Connect provider should have more information on this topic. """ - @spec authorization_uri(config(), params :: %{optional(atom) => term()}) :: - {:ok, uri :: String.t()} | {:error, term()} + @spec authorization_uri( + config(), + redirect_uri :: redirect_uri(), + params :: %{optional(atom) => term()} def authorization_uri(config, redirect_uri, params \\ %{}) do discovery_document_uri = config.discovery_document_uri From 13320ed8b0d347330d07e1375a9661f3089b9c03 Mon Sep 17 00:00:00 2001 From: Andrew Dryga Date: Thu, 11 May 2023 14:43:07 -0600 Subject: [PATCH 3/3] Fix type spec once more (forgot to select line in commit code range) --- lib/openid_connect.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/openid_connect.ex b/lib/openid_connect.ex index 91df80c..b8b5529 100644 --- a/lib/openid_connect.ex +++ b/lib/openid_connect.ex @@ -77,6 +77,7 @@ defmodule OpenIDConnect do config(), redirect_uri :: redirect_uri(), params :: %{optional(atom) => term()} + ) :: {:ok, uri :: String.t()} | {:error, term()} def authorization_uri(config, redirect_uri, params \\ %{}) do discovery_document_uri = config.discovery_document_uri