diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e4f2750..abc6dd46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,28 @@ This is a full rewrite of the library, and are several breaking changes. You're encouraged to test your app well if you upgrade from 0.4. +### Upgrading from 0.5 + +#### 1. DB + +Add the string fields `code_challenge` and `code_challenge_method` to the table `_access_grants`. +Example migration file: + +```elixir +# file: accounts/priv/repo/migrations/20210821193238_update_oauth_tables.exs +defmodule Accounts.Repo.Migrations.UpdateOauthTables do + use Ecto.Migration + + def change do + alter table(:oauth_access_grants) do + add :code_challenge, :string + add :code_challenge_method, :string + end + end +end + +``` + ### Upgrading from 0.4 #### 1. Schema modules diff --git a/README.md b/README.md index 83b491c4..00ea1079 100644 --- a/README.md +++ b/README.md @@ -91,7 +91,22 @@ Revocation will return `{:ok, %{}}` status even if the token is invalid. ### Authorization code flow in a Single Page Application -ExOauth2Provider doesn't support **implicit** grant flow. Instead you should set up an application with no client secret, and use the **Authorize code** grant flow. `client_secret` isn't required unless it has been set for the application. +ExOauth2Provider doesn't support **implicit** grant flow. Instead you should set up an application with no client secret, and use the **Authorize code** grant flow. `client_secret` isn't required unless it has been set for the application. +It's **strongly** encouraged to enable PKCE for these applications. + + +#### PKCE + +Enable PKCE in configuration `config/config.ex`: + +```elixir +config :my_app, ExOauth2Provider, + # ... + # this will enable PKCE for *all* applications + use_pkce: true +``` + +When making an authorization code flow you are now required to provide a `code_challenge` and `code_challenge_method` query fields for the authorization request and `code_verifier` field for the access token request, as per [RFC-7637](https://datatracker.ietf.org/doc/html/rfc7636). ### Other supported token grants diff --git a/lib/ex_oauth2_provider/access_grants/access_grant.ex b/lib/ex_oauth2_provider/access_grants/access_grant.ex index 6f26d59c..2255b450 100644 --- a/lib/ex_oauth2_provider/access_grants/access_grant.ex +++ b/lib/ex_oauth2_provider/access_grants/access_grant.ex @@ -27,7 +27,9 @@ defmodule ExOauth2Provider.AccessGrants.AccessGrant do {:expires_in, :integer, null: false}, {:redirect_uri, :string, null: false}, {:revoked_at, :utc_datetime}, - {:scopes, :string} + {:scopes, :string}, + {:code_challenge, :string}, + {:code_challenge_method, :string} ] end @@ -66,7 +68,7 @@ defmodule ExOauth2Provider.AccessGrants.AccessGrant do @spec changeset(Ecto.Schema.t(), map(), keyword()) :: Changeset.t() def changeset(grant, params, config) do grant - |> Changeset.cast(params, [:redirect_uri, :expires_in, :scopes]) + |> Changeset.cast(params, [:redirect_uri, :expires_in, :scopes, :code_challenge, :code_challenge_method]) |> Changeset.assoc_constraint(:application) |> Changeset.assoc_constraint(:resource_owner) |> put_token() diff --git a/lib/ex_oauth2_provider/config.ex b/lib/ex_oauth2_provider/config.ex index da8ca43e..dc885480 100644 --- a/lib/ex_oauth2_provider/config.ex +++ b/lib/ex_oauth2_provider/config.ex @@ -100,6 +100,11 @@ defmodule ExOauth2Provider.Config do def use_refresh_token?(config), do: get(config, :use_refresh_token, false) + # Use PKCE for 'authorization code' grant type (https://datatracker.ietf.org/doc/html/rfc7636) + @spec use_pkce?(keyword()) :: boolean() + def use_pkce?(config), + do: get(config, :use_pkce, false) + # Password auth method to use. Disabled by default. When set, it'll enable # password auth strategy. Set config as: # `password_auth: {MyModule, :my_auth_method}` diff --git a/lib/ex_oauth2_provider/oauth2/authorization/strategy/code.ex b/lib/ex_oauth2_provider/oauth2/authorization/strategy/code.ex index 40b3cb02..48352a27 100644 --- a/lib/ex_oauth2_provider/oauth2/authorization/strategy/code.ex +++ b/lib/ex_oauth2_provider/oauth2/authorization/strategy/code.ex @@ -63,7 +63,8 @@ defmodule ExOauth2Provider.Authorization.Code do Authorization.Utils.Response, RedirectURI, Scopes, - Utils.Error} + Utils.Error, + Utils.Validation} alias Ecto.Schema @doc """ @@ -140,9 +141,15 @@ defmodule ExOauth2Provider.Authorization.Code do defp issue_grant({:error, %{error: _error} = params}, _config), do: {:error, params} defp issue_grant({:ok, %{resource_owner: resource_owner, client: application, request: request} = params}, config) do - grant_params = - request - |> Map.take(["redirect_uri", "scope"]) + filtered_request = if Config.use_pkce?(config) do + Map.merge(%{"code_challenge_method" => "plain"}, request) + |> Map.take(["redirect_uri", "scope", "code_challenge", "code_challenge_method"]) + |> Map.update!("code_challenge", fn v -> String.replace(v, "=", "") end) + else + Map.take(request, ["redirect_uri", "scope"]) + end + + grant_params = filtered_request |> Map.new(fn {k, v} -> case k do "scope" -> {:scopes, v} @@ -188,6 +195,7 @@ defmodule ExOauth2Provider.Authorization.Code do |> validate_resource_owner() |> validate_redirect_uri(config) |> validate_scopes(config) + |> validate_pkce(Config.use_pkce?(config)) end defp validate_resource_owner({:ok, %{resource_owner: resource_owner} = params}) do @@ -225,4 +233,29 @@ defmodule ExOauth2Provider.Authorization.Code do end end defp validate_redirect_uri({:ok, params}, _config), do: Error.add_error({:ok, params}, Error.invalid_request()) + + defp validate_pkce({:error, params}, _use_pkce?), do: {:error, params} + defp validate_pkce({:ok, params}, false), do: {:ok, params} + defp validate_pkce({:ok, %{request: %{"code_challenge" => code_challenge} = request} = params}, true) do + code_challenge_method = Map.get(request, "code_challenge_method", "plain") + + if valid_code_challenge_format?(code_challenge, code_challenge_method) do + {:ok, params} + else + Error.add_error({:ok, params}, Error.invalid_request()) + end + end + defp validate_pkce({:ok, params}, true), do: Error.add_error({:ok, params}, Error.invalid_request()) # missing code_challenge + + @sha256_byte_size 256/8 + + defp valid_code_challenge_format?(nil, _code_challenge_method), do: false + defp valid_code_challenge_format?(code_challenge, "plain"), do: Validation.valid_code_verifier_format?(code_challenge) + defp valid_code_challenge_format?(code_challenge, "S256") do + case Base.url_decode64(code_challenge, padding: false) do # padding '=' deliberately accepted + {:ok, bin} -> byte_size(bin) == @sha256_byte_size + :error -> false + end + end + defp valid_code_challenge_format?(_code_challenge, _code_challenge_method), do: false end diff --git a/lib/ex_oauth2_provider/oauth2/token/strategy/authorization_code.ex b/lib/ex_oauth2_provider/oauth2/token/strategy/authorization_code.ex index d1c1abe7..f595213f 100644 --- a/lib/ex_oauth2_provider/oauth2/token/strategy/authorization_code.ex +++ b/lib/ex_oauth2_provider/oauth2/token/strategy/authorization_code.ex @@ -8,7 +8,8 @@ defmodule ExOauth2Provider.Token.AuthorizationCode do Config, Token.Utils, Token.Utils.Response, - Utils.Error} + Utils.Error, + Utils.Validation} @doc """ Will grant access token by client credentials. @@ -32,6 +33,7 @@ defmodule ExOauth2Provider.Token.AuthorizationCode do |> Utils.load_client(config) |> load_active_access_grant(config) |> validate_redirect_uri() + |> validate_pkce() |> issue_access_token_by_grant(config) |> Response.response(config) end @@ -89,4 +91,22 @@ defmodule ExOauth2Provider.Token.AuthorizationCode do end end defp validate_redirect_uri({:ok, params}), do: Error.add_error({:ok, params}, Error.invalid_grant()) + + defp validate_pkce({:error, params}), do: {:error, params} + defp validate_pkce({:ok, %{access_grant: %{code_challenge_method: nil}} = params}), do: {:ok, params} # pkce not enabled for this grant + defp validate_pkce({:ok, %{request: %{"code_verifier" => actual_code}, access_grant: %{code_challenge: expected_code, code_challenge_method: challenge_method}} = params}) do + if Validation.valid_code_verifier_format?(actual_code) && valid_pkce?(actual_code, expected_code, challenge_method) do + {:ok, params} + else + Error.add_error({:ok, params}, Error.invalid_grant()) + end + end + defp validate_pkce({:ok, params}), do: Error.add_error({:ok, params}, Error.invalid_request()) + + defp valid_pkce?(actual_code, expected_code, "plain"), do: Plug.Crypto.secure_compare(actual_code, expected_code) + defp valid_pkce?(actual_code, expected_code, "S256") do + :crypto.hash(:sha256, actual_code) + |> Base.url_encode64(padding: false) + |> Plug.Crypto.secure_compare(expected_code) + end end diff --git a/lib/ex_oauth2_provider/oauth2/utils/validation.ex b/lib/ex_oauth2_provider/oauth2/utils/validation.ex new file mode 100644 index 00000000..cb985aed --- /dev/null +++ b/lib/ex_oauth2_provider/oauth2/utils/validation.ex @@ -0,0 +1,10 @@ +defmodule ExOauth2Provider.Utils.Validation do + @moduledoc false + + # see https://datatracker.ietf.org/doc/html/rfc7636#section-4.1 + @code_verifier_regex ~r/^[[:alnum:]._~-]{43,128}$/ + + @spec valid_code_verifier_format?(String.t()) :: boolean + def valid_code_verifier_format?(nil), do: false + def valid_code_verifier_format?(code_verifier), do: String.match?(code_verifier, @code_verifier_regex) +end diff --git a/test/ex_oauth2_provider/oauth2/authorization/strategy/code_pkce_test.exs b/test/ex_oauth2_provider/oauth2/authorization/strategy/code_pkce_test.exs new file mode 100644 index 00000000..d3d15453 --- /dev/null +++ b/test/ex_oauth2_provider/oauth2/authorization/strategy/code_pkce_test.exs @@ -0,0 +1,135 @@ +defmodule ExOauth2Provider.Authorization.CodePkceTest do + use ExOauth2Provider.TestCase + + alias ExOauth2Provider.Authorization + alias ExOauth2Provider.Test.Fixtures + alias Dummy.{OauthAccessGrants.OauthAccessGrant, Repo} + + @code_challenge "1234567890abcdefrety1234567890abcdefrety.~-_" + @s256_code_challenge :crypto.hash(:sha256, @code_challenge) |> Base.url_encode64(padding: false) + @client_id "Jf5rM8hQBc" + @missing_code_challenge_request %{ + "client_id" => @client_id, + "response_type" => "code", + "scope" => "app:read app:write" + } + @valid_plain_request Map.merge(@missing_code_challenge_request, %{ + "code_challenge" => @code_challenge + }) + @valid_explicit_plain_request Map.merge(@missing_code_challenge_request, %{ + "code_challenge" => @code_challenge, + "code_challenge_method" => "plain" + }) + @valid_s256_request Map.merge(@missing_code_challenge_request, %{ + "code_challenge" => @s256_code_challenge, + "code_challenge_method" => "S256" + }) + @invalid_request %{ + error: :invalid_request, + error_description: + "The request is missing a required parameter, includes an unsupported parameter value, or is otherwise malformed." + } + + setup_all %{} do + new_conf = Application.get_env(:ex_oauth2_provider, ExOauth2Provider) ++ [use_pkce: true] + Application.put_env(:ex_oauth2_provider, ExOauth2Provider, new_conf) + + :ok + end + + setup do + resource_owner = Fixtures.resource_owner() + application = Fixtures.application(uid: @client_id, scopes: "app:read app:write") + {:ok, %{resource_owner: resource_owner, application: application}} + end + + test "#preauthorize/3 missing code_challenge", %{resource_owner: resource_owner} do + assert Authorization.preauthorize(resource_owner, @missing_code_challenge_request, + otp_app: :ex_oauth2_provider + ) == {:error, @invalid_request, :bad_request} + end + + test "#authorize/3 missing code_challenge", %{resource_owner: resource_owner} do + assert Authorization.authorize(resource_owner, @missing_code_challenge_request, + otp_app: :ex_oauth2_provider + ) == {:error, @invalid_request, :bad_request} + end + + test "#authorize/3 invalid code_challenge_method", %{resource_owner: resource_owner} do + request = Map.merge(@valid_plain_request, %{"code_challenge_method" => "invalid"}) + + assert Authorization.authorize(resource_owner, request, otp_app: :ex_oauth2_provider) == + {:error, @invalid_request, :bad_request} + end + + test "#authorize/3 invalid plain code_challenge", %{resource_owner: resource_owner} do + request = + Map.merge(@valid_plain_request, %{ + "code_challenge" => @code_challenge <> "<>" + }) + + assert Authorization.authorize(resource_owner, request, otp_app: :ex_oauth2_provider) == + {:error, @invalid_request, :bad_request} + end + + test "#authorize/3 invalid s256 code_challenge", %{resource_owner: resource_owner} do + request = Map.merge(@valid_s256_request, %{"code_challenge" => @s256_code_challenge <> "baddecode"}) + + assert Authorization.authorize(resource_owner, request, otp_app: :ex_oauth2_provider) == + {:error, @invalid_request, :bad_request} + end + + test "#authorize/3 implicit plain code_challenge_method generates grant", %{ + resource_owner: resource_owner + } do + assert {:native_redirect, %{code: code}} = + Authorization.authorize(resource_owner, @valid_plain_request, + otp_app: :ex_oauth2_provider + ) + + owner_id = resource_owner.id + + assert %{ + resource_owner_id: ^owner_id, + code_challenge: @code_challenge, + code_challenge_method: "plain", + scopes: "app:read app:write" + } = Repo.get_by(OauthAccessGrant, token: code) + end + + test "#authorize/3 explicit plain code_challenge_method generates grant", %{ + resource_owner: resource_owner + } do + assert {:native_redirect, %{code: code}} = + Authorization.authorize(resource_owner, @valid_explicit_plain_request, + otp_app: :ex_oauth2_provider + ) + + owner_id = resource_owner.id + + assert %{ + resource_owner_id: ^owner_id, + code_challenge: @code_challenge, + code_challenge_method: "plain", + scopes: "app:read app:write" + } = Repo.get_by(OauthAccessGrant, token: code) + end + + test "#authorize/3 S256 code_challenge_method generates grant", %{ + resource_owner: resource_owner + } do + assert {:native_redirect, %{code: code}} = + Authorization.authorize(resource_owner, @valid_s256_request, + otp_app: :ex_oauth2_provider + ) + + owner_id = resource_owner.id + + assert %{ + resource_owner_id: ^owner_id, + code_challenge: @s256_code_challenge, + code_challenge_method: "S256", + scopes: "app:read app:write" + } = Repo.get_by(OauthAccessGrant, token: code) + end +end diff --git a/test/ex_oauth2_provider/oauth2/authorization/strategy/code_test.exs b/test/ex_oauth2_provider/oauth2/authorization/strategy/code_test.exs index ca6d2d23..aa3f9503 100644 --- a/test/ex_oauth2_provider/oauth2/authorization/strategy/code_test.exs +++ b/test/ex_oauth2_provider/oauth2/authorization/strategy/code_test.exs @@ -154,6 +154,13 @@ defmodule ExOauth2Provider.Authorization.CodeTest do assert Authorization.authorize(resource_owner, request, otp_app: :ex_oauth2_provider) == {:error, @invalid_redirect_uri, :unprocessable_entity} end + test "#authorize/3 generates grant without persisting code_challenge, code_challenge_method on disabled pkce", %{resource_owner: resource_owner} do + request = Map.merge(@valid_request, %{"code_challenge" => "1234567890abcdefrety1234567890abcdefrety1234", "code_challenge_method" => "plain"}) + + assert {:native_redirect, %{code: code}} = Authorization.authorize(resource_owner, request, otp_app: :ex_oauth2_provider) + assert %{code_challenge: nil, code_challenge_method: nil} = Repo.get_by(OauthAccessGrant, token: code) + end + test "#authorize/3 generates grant", %{resource_owner: resource_owner} do assert {:native_redirect, %{code: code}} = Authorization.authorize(resource_owner, @valid_request, otp_app: :ex_oauth2_provider) access_grant = Repo.get_by(OauthAccessGrant, token: code) diff --git a/test/ex_oauth2_provider/oauth2/token/strategy/authorization_code_test.exs b/test/ex_oauth2_provider/oauth2/token/strategy/authorization_code_test.exs index 9b52b3df..8a3e0853 100644 --- a/test/ex_oauth2_provider/oauth2/token/strategy/authorization_code_test.exs +++ b/test/ex_oauth2_provider/oauth2/token/strategy/authorization_code_test.exs @@ -136,6 +136,72 @@ defmodule ExOauth2Provider.Token.Strategy.AuthorizationCodeTest do assert Token.grant(request_invalid_redirect_uri, otp_app: :ex_oauth2_provider) == {:error, @invalid_grant, :unprocessable_entity} end + describe "pkce enabled for grant" do + @code_plain "code_plain" + @code_s256 "code_s256" + + @code_challenge "1234567890abcdefrety1234567890abcdefrety.~-_" + @unmatching_code_challenge "1234567890abcdefrety1234567890abcdefrety.~-1" + @s256_code_challenge :crypto.hash(:sha256, @code_challenge) |> Base.url_encode64(padding: false) + + @valid_plain_request Map.merge(@valid_request, %{ + "code_verifier" => @code_challenge, + "code" => @code_plain + }) + @valid_s256_request Map.merge(@valid_request, %{ + "code_verifier" => @code_challenge, + "code" => @code_s256 + }) + + @invalid_request %{ + error: :invalid_request, + error_description: + "The request is missing a required parameter, includes an unsupported parameter value, or is otherwise malformed." + } + + setup %{resource_owner: resource_owner, application: application, access_grant: access_grant} do + plain_grant = Fixtures.access_grant( + application, resource_owner, @code_plain, @redirect_uri, code_challenge: @code_challenge, code_challenge_method: "plain" + ) + s256_grant = Fixtures.access_grant( + application, resource_owner, @code_s256, @redirect_uri, code_challenge: @s256_code_challenge, code_challenge_method: "S256" + ) + + {:ok, %{ + resource_owner: resource_owner, + application: application, + no_pkce_grant: access_grant, + plain_grant: plain_grant, + s256_grant: s256_grant + }} + end + + test "#grant/2 missing code_verifier" do + request = Map.merge(@valid_request, %{"code" => @code_plain}) + assert {:error, @invalid_request, :bad_request} == Token.grant(request, otp_app: :ex_oauth2_provider) + end + + test "#grant/2 plain challenge method with unmatching code verifier" do + request = Map.merge(@valid_plain_request, %{"code_verifier" => @unmatching_code_challenge}) + assert {:error, @invalid_grant, :unprocessable_entity} == Token.grant(request, otp_app: :ex_oauth2_provider) + end + + test "#grant/2 S256 challenge method with unmatching code verifier" do + request = Map.merge(@valid_s256_request, %{"code_verifier" => @unmatching_code_challenge}) + assert {:error, @invalid_grant, :unprocessable_entity} == Token.grant(request, otp_app: :ex_oauth2_provider) + end + + test "#grant/2 returns access token with plain challenge method" do + assert {:ok, %{access_token: actual_token}} = Token.grant(@valid_plain_request, otp_app: :ex_oauth2_provider) + assert %{token: ^actual_token} = QueryHelpers.get_latest_inserted(OauthAccessToken) + end + + test "#grant/2 returns access token with S256 challenge method" do + assert {:ok, %{access_token: actual_token}} = Token.grant(@valid_s256_request, otp_app: :ex_oauth2_provider) + assert %{token: ^actual_token} = QueryHelpers.get_latest_inserted(OauthAccessToken) + end + end + def access_token_response_body_handler(body, access_token) do Map.merge(body, %{custom_attr: access_token.inserted_at}) end diff --git a/test/support/fixtures.ex b/test/support/fixtures.ex index 988b1593..798501e7 100644 --- a/test/support/fixtures.ex +++ b/test/support/fixtures.ex @@ -52,7 +52,7 @@ defmodule ExOauth2Provider.Test.Fixtures do end - def access_grant(application, user, code, redirect_uri) do + def access_grant(application, user, code, redirect_uri, extra_attrs \\ []) do attrs = [ expires_in: 900, redirect_uri: "urn:ietf:wg:oauth:2.0:oob", @@ -61,7 +61,7 @@ defmodule ExOauth2Provider.Test.Fixtures do token: code, scopes: "read", redirect_uri: redirect_uri - ] + ] ++ extra_attrs %OauthAccessGrant{} |> Changeset.change(attrs)