Merge pull request #167 from pow-auth/cast-user-claims-values

Cast user claim values

Author: danschultzer

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
 

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.
 

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

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

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

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

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

test/assent/strategies/google_test.exs

@@ -19,7 +19,7 @@ defmodule Assent.Strategy.GoogleTest do
   }
   @user %{
     "email" => "[email protected]",
-    "email_verified" => "true",
+    "email_verified" => true,
     "hd" => "example.com",
     "sub" => "10769150350006150715113082367"
   }

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",

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",

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"
   }
 

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" => "[email protected]"
   }