From 9a92172cd361e86c5e41ab6a353135a8af221a07 Mon Sep 17 00:00:00 2001 From: Dan Schultzer <1254724+danschultzer@users.noreply.github.com> Date: Fri, 27 Dec 2024 21:16:28 -0800 Subject: [PATCH] Cast user claim values --- CHANGELOG.md | 9 ++ lib/assent.ex | 29 ++++++ lib/assent/strategy.ex | 110 ++++++++++++++++++++--- test/assent/strategies/auth0_test.exs | 2 +- test/assent/strategies/basecamp_test.exs | 2 +- test/assent/strategies/github_test.exs | 2 +- test/assent/strategies/gitlab_test.exs | 2 +- test/assent/strategies/google_test.exs | 2 +- test/assent/strategies/strava_test.exs | 2 +- test/assent/strategies/telegram_test.exs | 2 +- test/assent/strategies/twitter_test.exs | 2 +- test/assent/strategies/vk_test.exs | 2 +- test/assent/strategy_test.exs | 101 +++++++++++++++++++-- 13 files changed, 241 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7935cb6..4385680 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,15 @@ ### Breaking changes +* `Assent.Strategy.Auth.callback/2` now encodes `updated_at` as an integer instead of string +* `Assent.Strategy.Basecamp.callback/2` now encodes `sub` as a string instead of an integer +* `Assent.Strategy.Github.callback/2` now encodes `sub` as a string instead of an integer +* `Assent.Strategy.Gitlab.callback/2` now encodes `sub` as a string instead of an integer * `Assent.Strategy.Google` now return `hd` instead of `google_hd` +* `Assent.Strategy.Strava.callback/2` now encodes `sub` as a string instead of an integer +* `Assent.Strategy.Telegram.callback/2` now encodes `sub` as a string instead of an integer +* `Assent.Strategy.Twitter.callback/2` now encodes `sub` as a string instead of an integer +* `Assent.Strategy.VK.callback/2` now encodes `sub` as a string instead of an integer * `:site` configuration option removed, use `:base_url` instead * `Assent.Strategy.OAuth2.authorize_url/2` no longer allows `:state` in `:authorization_params` * `Assent.Strategy.decode_response/2`removed, use `Assent.HTTPAdapter.decode_response/2` instead @@ -18,6 +26,7 @@ ### Changes * `Assent.Strategy.Google` now uses OIDC instead of OAuth 2.0 base strategy +* `Assent.Strategy.normalize_userinfo/2` now casts the user claims per OpenID specification ## v0.2 diff --git a/lib/assent.ex b/lib/assent.ex index 3cac8a3..0885d0d 100644 --- a/lib/assent.ex +++ b/lib/assent.ex @@ -132,6 +132,35 @@ defmodule Assent do end end + defmodule CastClaimsError do + defexception [:claims, :invalid_types] + + @type t :: %__MODULE__{ + claims: map(), + invalid_types: map() + } + + def message(exception) do + """ + The following claims couldn't be cast: + + #{exception.invalid_types |> to_lines() |> Enum.join("\n")} + """ + end + + defp to_lines(claim_types, prepend \\ "") do + claim_types + |> Enum.sort_by(&elem(&1, 0)) + |> Enum.reduce([], fn + {key, %{} = claim_types}, acc -> + acc ++ to_lines(claim_types, prepend <> "#{inspect(key)} -> ") + + {key, type}, acc -> + acc ++ ["- #{prepend}#{inspect(key)} to #{inspect(type)}"] + end) + end + end + @doc """ Fetches the key value from the configuration. diff --git a/lib/assent/strategy.ex b/lib/assent/strategy.ex index 3de4607..cf47578 100644 --- a/lib/assent/strategy.ex +++ b/lib/assent/strategy.ex @@ -26,6 +26,8 @@ defmodule Assent.Strategy do end end """ + alias Assent.CastClaimsError + @callback authorize_url(Keyword.t()) :: {:ok, %{:url => binary(), optional(atom()) => any()}} | {:error, term()} @callback callback(Keyword.t(), map()) :: @@ -124,23 +126,109 @@ defmodule Assent.Strategy do defp encode_value(value), do: URI.encode_www_form(Kernel.to_string(value)) + @registered_claim_member_types %{ + "sub" => :binary, + "name" => :binary, + "given_name" => :binary, + "family_name" => :binary, + "middle_name" => :binary, + "nickname" => :binary, + "preferred_username" => :binary, + "profile" => :binary, + "picture" => :binary, + "website" => :binary, + "email" => :binary, + "email_verified" => :boolean, + "gender" => :binary, + "birthdate" => :binary, + "zoneinfo" => :binary, + "locale" => :binary, + "phone_number" => :binary, + "phone_number_verified" => :boolean, + "address" => %{ + "formatted" => :binary, + "street_address" => :binary, + "locality" => :binary, + "region" => :binary, + "postal_code" => :binary, + "country" => :binary + }, + "updated_at" => :integer + } + @doc """ Normalize API user request response into standard claims. + The function will cast values to adhere to the following types: + + ``` + #{inspect(@registered_claim_member_types, pretty: true)} + ``` + + Returns a `Assent.CastClaimsError` if any of the above types can't be casted. + Based on https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.5.1 """ - @spec normalize_userinfo(map(), map()) :: {:ok, map()} + @spec normalize_userinfo(map(), map()) :: {:ok, map()} | {:error, term()} def normalize_userinfo(claims, extra \\ %{}) do - standard_claims = - Map.take( - claims, - ~w(sub name given_name family_name middle_name nickname - preferred_username profile picture website email email_verified - gender birthdate zoneinfo locale phone_number phone_number_verified - address updated_at) - ) - - {:ok, prune(Map.merge(extra, standard_claims))} + case cast_claims(@registered_claim_member_types, claims) do + {casted_claims, nil} -> + {:ok, extra |> prune() |> Map.merge(casted_claims)} + + {_claims, invalid_claims} -> + {:error, + CastClaimsError.exception(claims: claims, invalid_types: Enum.into(invalid_claims, %{}))} + end + end + + defp cast_claims(claim_types, claims) do + {casted_claims, invalid_claims} = + Enum.reduce(claim_types, {[], []}, fn {key, type}, acc -> + cast_claim(key, type, Map.get(claims, key), acc) + end) + + { + (casted_claims != [] && Enum.into(casted_claims, %{})) || nil, + (invalid_claims != [] && Enum.into(invalid_claims, %{})) || nil + } + end + + defp cast_claim(_key, _type, nil, acc), do: acc + + defp cast_claim(key, %{} = claim_types, %{} = claims, {casted_claims, invalid_claims}) do + {casted_sub_claims, invalid_sub_claims} = cast_claims(claim_types, claims) + + { + (casted_sub_claims && [{key, casted_sub_claims} | casted_claims]) || casted_claims, + (invalid_sub_claims && [{key, invalid_sub_claims} | invalid_claims]) || invalid_claims + } + end + + defp cast_claim(key, %{}, _value, {casted_claims, invalid_claims}) do + {casted_claims, [{key, :map} | invalid_claims]} + end + + defp cast_claim(key, type, value, {casted_claims, invalid_claims}) do + case cast_value(value, type) do + {:ok, value} -> {[{key, value} | casted_claims], invalid_claims} + :error -> {casted_claims, [{key, type} | invalid_claims]} + end + end + + defp cast_value(value, :binary) when is_binary(value), do: {:ok, value} + defp cast_value(value, :binary) when is_integer(value), do: {:ok, to_string(value)} + defp cast_value(value, :integer) when is_integer(value), do: {:ok, value} + defp cast_value(value, :integer) when is_binary(value), do: cast_integer(value) + defp cast_value(value, :boolean) when is_boolean(value), do: {:ok, value} + defp cast_value("true", :boolean), do: {:ok, true} + defp cast_value("false", :boolean), do: {:ok, false} + defp cast_value(_value, _type), do: :error + + defp cast_integer(value) do + case Integer.parse(value) do + {integer, ""} -> {:ok, integer} + _ -> :error + end end @doc """ diff --git a/test/assent/strategies/auth0_test.exs b/test/assent/strategies/auth0_test.exs index ed77ece..26fe86b 100644 --- a/test/assent/strategies/auth0_test.exs +++ b/test/assent/strategies/auth0_test.exs @@ -28,7 +28,7 @@ defmodule Assent.Strategy.Auth0Test do }, "updated_at" => "1556845729" } - @user @user_response + @user %{@user_response | "updated_at" => 1_556_845_729} describe "authorize_url/2" do test "requires domain or base_url configuration", %{config: config} do diff --git a/test/assent/strategies/basecamp_test.exs b/test/assent/strategies/basecamp_test.exs index 68b7425..305fc8f 100644 --- a/test/assent/strategies/basecamp_test.exs +++ b/test/assent/strategies/basecamp_test.exs @@ -42,7 +42,7 @@ defmodule Assent.Strategy.BasecampTest do "family_name" => "Fried", "given_name" => "Jason", "name" => "Jason Fried", - "sub" => 9_999_999 + "sub" => "9999999" } test "authorize_url/2", %{config: config} do diff --git a/test/assent/strategies/github_test.exs b/test/assent/strategies/github_test.exs index 342831d..118b2a8 100644 --- a/test/assent/strategies/github_test.exs +++ b/test/assent/strategies/github_test.exs @@ -65,7 +65,7 @@ defmodule Assent.Strategy.GithubTest do "picture" => "https://github.com/images/error/octocat_happy.gif", "preferred_username" => "octocat", "profile" => "https://github.com/octocat", - "sub" => 1 + "sub" => "1" } test "authorize_url/2", %{config: config} do diff --git a/test/assent/strategies/gitlab_test.exs b/test/assent/strategies/gitlab_test.exs index 8bd990a..cb845a8 100644 --- a/test/assent/strategies/gitlab_test.exs +++ b/test/assent/strategies/gitlab_test.exs @@ -45,7 +45,7 @@ defmodule Assent.Strategy.GitlabTest do "name" => "John Smith", "picture" => "http://localhost:3000/uploads/user/avatar/1/index.jpg", "preferred_username" => "john_smith", - "sub" => 1 + "sub" => "1" } test "authorize_url/2", %{config: config} do diff --git a/test/assent/strategies/google_test.exs b/test/assent/strategies/google_test.exs index ac3b18b..f8c5abb 100644 --- a/test/assent/strategies/google_test.exs +++ b/test/assent/strategies/google_test.exs @@ -19,7 +19,7 @@ defmodule Assent.Strategy.GoogleTest do } @user %{ "email" => "jsmith@example.com", - "email_verified" => "true", + "email_verified" => true, "hd" => "example.com", "sub" => "10769150350006150715113082367" } diff --git a/test/assent/strategies/strava_test.exs b/test/assent/strategies/strava_test.exs index b4ef4ab..b540418 100644 --- a/test/assent/strategies/strava_test.exs +++ b/test/assent/strategies/strava_test.exs @@ -53,7 +53,7 @@ defmodule Assent.Strategy.StravaTest do } @user %{ - "sub" => 1_234_567_890_987_654_321, + "sub" => "1234567890987654321", "given_name" => "Marianne", "family_name" => "Teutenberg", "preferred_username" => "marianne_t", diff --git a/test/assent/strategies/telegram_test.exs b/test/assent/strategies/telegram_test.exs index ff1335e..8d4204c 100644 --- a/test/assent/strategies/telegram_test.exs +++ b/test/assent/strategies/telegram_test.exs @@ -11,7 +11,7 @@ defmodule Assent.Strategy.TelegramTest do "username" => "duroff" } @user %{ - "sub" => 928_474_348, + "sub" => "928474348", "family_name" => "Duroff", "given_name" => "Paul", "preferred_username" => "duroff", diff --git a/test/assent/strategies/twitter_test.exs b/test/assent/strategies/twitter_test.exs index 1d4841e..f19ac37 100644 --- a/test/assent/strategies/twitter_test.exs +++ b/test/assent/strategies/twitter_test.exs @@ -124,7 +124,7 @@ defmodule Assent.Strategy.TwitterTest do "picture" => "https://pbs.twimg.com/profile_images/880136122604507136/xHrnqf1T_normal.jpg", "preferred_username" => "TwitterDev", "profile" => "https://twitter.com/TwitterDev", - "sub" => 2_244_994_945, + "sub" => "2244994945", "website" => "https://t.co/FGl7VOULyL" } diff --git a/test/assent/strategies/vk_test.exs b/test/assent/strategies/vk_test.exs index e6933d2..cc5fed0 100644 --- a/test/assent/strategies/vk_test.exs +++ b/test/assent/strategies/vk_test.exs @@ -19,7 +19,7 @@ defmodule Assent.Strategy.VKTest do @user %{ "given_name" => "Lindsay", "family_name" => "Stirling", - "sub" => 210_700_286, + "sub" => "210700286", "email" => "lindsay.stirling@example.com" } diff --git a/test/assent/strategy_test.exs b/test/assent/strategy_test.exs index 0bcc351..5dd9792 100644 --- a/test/assent/strategy_test.exs +++ b/test/assent/strategy_test.exs @@ -63,12 +63,101 @@ defmodule Assent.StrategyTest do "http://example.com/path?a=1&b[c]=2&b[d][e]=3&f[]=4&f[]=5" end - test "normalize_userinfo/2" do - user = %{"email" => "foo@example.com", "name" => nil, "nickname" => "foo"} - extra = %{"a" => "1"} - expected = %{"email" => "foo@example.com", "nickname" => "foo", "a" => "1"} - - assert Strategy.normalize_userinfo(user, extra) == {:ok, expected} + describe "normalize_userinfo/2" do + @valid_user %{ + "sub" => "123", + "email" => "foo@example.com", + "email_verified" => true, + "address" => %{ + "formatted" => "456" + }, + "updated_at" => 1_516_239_022 + } + + @invalid_user %{ + "sub" => true, + "address" => %{ + "formatted" => true + } + } + + @expected_user %{ + "sub" => "123", + "email" => "foo@example.com", + "email_verified" => true, + "address" => %{ + "formatted" => "456" + }, + "updated_at" => 1_516_239_022 + } + + test "with incorrect claim type" do + assert {:error, %Assent.CastClaimsError{} = error} = + Strategy.normalize_userinfo(@invalid_user, %{}) + + assert Exception.message(error) == """ + The following claims couldn't be cast: + + - "address" -> "formatted" to :binary + - "sub" to :binary + """ + end + + test "with atom value claim" do + user = %{"sub" => :invalid} + + assert {:error, %Assent.CastClaimsError{} = error} = Strategy.normalize_userinfo(user, %{}) + assert error.invalid_types == %{"sub" => :binary} + end + + test "with binary type claim with integer value" do + user = %{"sub" => 123} + + assert {:ok, %{"sub" => "123"}} = Strategy.normalize_userinfo(user, %{}) + end + + test "with integer type claim with invalid binary value" do + user = %{"updated_at" => "123a1"} + + assert {:error, %Assent.CastClaimsError{} = error} = Strategy.normalize_userinfo(user, %{}) + assert error.invalid_types == %{"updated_at" => :integer} + end + + test "with integer type claim with valid binary value" do + user = %{"updated_at" => "123"} + + assert {:ok, %{"updated_at" => 123}} = Strategy.normalize_userinfo(user, %{}) + end + + test "with boolean type claim with string binary value" do + user = %{"email_verified" => "true"} + + assert {:ok, %{"email_verified" => true}} = Strategy.normalize_userinfo(user, %{}) + + user = %{"email_verified" => "false"} + + assert {:ok, %{"email_verified" => false}} = Strategy.normalize_userinfo(user, %{}) + end + + test "casts" do + assert Strategy.normalize_userinfo(@valid_user) == {:ok, @expected_user} + end + + test "with unknown claims" do + user = + @valid_user + |> Map.put("foo", "bar") + |> Map.put("address", Map.put(@valid_user["address"], "foo", "bar")) + + assert Strategy.normalize_userinfo(user) == {:ok, @expected_user} + end + + test "with extra" do + extra = %{"a" => 1, "sub" => "different-sub"} + expected_user = Map.put(@expected_user, "a", 1) + + assert Strategy.normalize_userinfo(@valid_user, extra) == {:ok, expected_user} + end end test "prune/1" do