diff --git a/CHANGELOG.md b/CHANGELOG.md index 6abd662..87e7b4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,14 +4,24 @@ **This release consists of breaking changes.** +Userinfo is now cast to the correct type per https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.5.1. When upgrading you must ensure that you do not depend on a specific type in the returned userinfo for any of the strategies listed below. + ### Breaking changes * `Assent.Strategy.Auth0.authorize_url/2` no longer accepts `:domain` config, use `:base_url` instead +* `Assent.Strategy.Basecamp.callback/2` now encodes `sub` as a `binary()` instead of an `integer()` +* `Assent.Strategy.Github.callback/2` now encodes `sub` as a `binary()` instead of an `integer()` +* `Assent.Strategy.Google` now encodes `email_verified` as a `boolean()` instead of a `binary()` * `Assent.Strategy.Google` now return `hd` instead of `google_hd` +* `Assent.Strategy.Strava.callback/2` now encodes `sub` as a `binary()` instead of an `integer()` +* `Assent.Strategy.Telegram.callback/2` now encodes `sub` as a `binary()` instead of an `integer()` +* `Assent.Strategy.Twitter.callback/2` now encodes `sub` as a `binary()` instead of an `integer()` +* `Assent.Strategy.VK.callback/2` now encodes `sub` as a `binary()` 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 * `Assent.Strategy.request/5` removed, use `Assent.Strategy.http_request/5` instead +* `Assent.Strategy.prune/1` removed * `Assent.MissingParamError` no longer accepts `:expected_key`, use `:key` instead * `Assent.HTTPAdapter.Mint` removed * `Assent.Config` removed @@ -21,6 +31,7 @@ * `Assent.Strategy.Auth0` now uses OIDC instead of OAuth 2.0 base strategy * `Assent.Strategy.Gitlab` now uses OIDC instead of OAuth 2.0 base strategy * `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/strategies/auth0.ex b/lib/assent/strategies/auth0.ex index 8bbbca4..fbb00d0 100644 --- a/lib/assent/strategies/auth0.ex +++ b/lib/assent/strategies/auth0.ex @@ -26,4 +26,15 @@ defmodule Assent.Strategy.Auth0 do client_authentication_method: "client_secret_post" ] end + + @impl true + def normalize(_config, user) do + {:ok, updated_at, 0} = DateTime.from_iso8601(user["updated_at"]) + + {:ok, + %{ + user + | "updated_at" => DateTime.to_unix(updated_at) + }} + end end diff --git a/lib/assent/strategy.ex b/lib/assent/strategy.ex index 3de4607..0de48dd 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,36 +126,126 @@ 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 an `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) - ) + case cast_claims(@registered_claim_member_types, claims) do + {casted_claims, nil} -> + {:ok, deep_merge_claims(casted_claims, extra)} - {:ok, prune(Map.merge(extra, standard_claims))} + {_claims, invalid_claims} -> + {:error, + CastClaimsError.exception(claims: claims, invalid_types: Enum.into(invalid_claims, %{}))} + end end - @doc """ - Recursively prunes map for nil values. - """ - @spec prune(map()) :: map() - def prune(map) do - map - |> Enum.map(fn {k, v} -> if is_map(v), do: {k, prune(v)}, else: {k, v} end) - |> Enum.filter(fn {_, v} -> not is_nil(v) end) - |> Enum.into(%{}) + 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 + + defp deep_merge_claims(claims, extra) do + Enum.reduce(extra, claims, fn + {_key, nil}, claims -> claims + {key, value}, claims -> deep_merge_claim(claims, key, value, Map.get(claims, key)) + end) + end + + defp deep_merge_claim(claims, key, sub_extra, nil), do: Map.put(claims, key, sub_extra) + + defp deep_merge_claim(claims, key, %{} = sub_extra, %{} = sub_claims) do + Map.put(claims, key, deep_merge_claims(sub_claims, sub_extra)) + end + + defp deep_merge_claim(claims, _key, _sub_extra, _value), do: claims + @doc false def __normalize__({:ok, %{user: user} = results}, config, strategy) do config diff --git a/test/assent/strategies/auth0_test.exs b/test/assent/strategies/auth0_test.exs index c1caa5f..ba6c9a8 100644 --- a/test/assent/strategies/auth0_test.exs +++ b/test/assent/strategies/auth0_test.exs @@ -26,7 +26,7 @@ defmodule Assent.Strategy.Auth0Test do "name" => "John Doe", "nickname" => "john.doe", "picture" => "https://myawesomeavatar.com/avatar.png", - "updated_at" => "2017-03-30T15:13:40.474Z" + "updated_at" => 1_490_886_820 } test "authorize_url/2", %{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/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..faeb819 100644 --- a/test/assent/strategy_test.exs +++ b/test/assent/strategy_test.exs @@ -63,18 +63,114 @@ 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} - end - - test "prune/1" do - map = %{a: :ok, b: nil, c: "", d: %{a: :ok, b: nil}} - expected = %{a: :ok, c: "", d: %{a: :ok}} - - assert Strategy.prune(map) == 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, + "b" => nil, + "sub" => "other-sub", + "address" => %{ + "formatted" => "other-foramtted", + "a" => nil, + "b" => 2 + } + } + + expected_user = + @expected_user + |> Map.put("a", 1) + |> Map.put("address", Map.put(@expected_user["address"], "b", 2)) + + assert Strategy.normalize_userinfo(@valid_user, extra) == {:ok, expected_user} + end end end