Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Callback param "user" is not parsed when using Apple strategy with "name" scope #85

Closed
jtormey opened this issue Apr 8, 2021 · 2 comments · Fixed by #86
Closed

Callback param "user" is not parsed when using Apple strategy with "name" scope #85

jtormey opened this issue Apr 8, 2021 · 2 comments · Fixed by #86

Comments

@jtormey
Copy link

jtormey commented Apr 8, 2021

When using the "name" scope with sign-in with Apple, it doesn't seem like the "user" map that contains "firstName" and "lastName" is decoded, or added to the token map in OAuth2.callback/3. This means that when Strategy.Apple attempts to merge the name params into the other user params, nothing is there. In my application this results in "given_name" and "family_name" being absent when trying to insert a new user into the database.

I was able to fix this in my project by decoding the "user" map in OAuth2.callback/3, and assigning it to token before passing it off to fetch_user_with_strategy as follows:

  @impl true
  @spec callback(Config.t(), map(), atom()) :: {:ok, %{user: map(), token: map()}} | {:error, term()}
  def callback(config, params, strategy \\ __MODULE__) do
    with {:ok, session_params} <- Config.fetch(config, :session_params),
         :ok                   <- check_error_params(params),
         {:ok, code}           <- fetch_code_param(params),
         {:ok, redirect_uri}   <- Config.fetch(config, :redirect_uri),
         :ok                   <- maybe_check_state(session_params, params),
-        {:ok, token}          <- grant_access_token(config, "authorization_code", code: code, redirect_uri: redirect_uri) do
+        {:ok, token}          <- grant_access_token(config, "authorization_code", code: code, redirect_uri: redirect_uri),
+        token                 <- maybe_add_user_callback_params(config, token, params) do

      fetch_user_with_strategy(config, token, strategy)
    end
  end

+ defp maybe_add_user_callback_params(config, token, %{"provider" => "apple", "user" => user}) do
+   Map.put(token, "user", Config.json_library(config).decode!(user))
+ end
+ defp maybe_add_user_callback_params(_config, token, _params), do: token

Does this seem right? I've looked pretty closely at the code paths involved, and don't see any other place where this information seems to be handled. Happy to open a PR if this is a real issue, and if the fix seems appropriate!

@danschultzer
Copy link
Collaborator

Thanks @jtormey!

I've fixed it in #86, but haven't had the chance to test it out. Let me know if it works for you!

The reason I did it this way is that Apple for the most part adheres to the OIDC specs but not always. This is one such case. As I understand it, OIDC only permits code, state, and sometimes id_token to be returned in the auth code flow. OAuth 2.0 does permit additional params but they have to be explicitly recognized. And on top of that the value for the query param is JSON encoded.

Anyway, I hope this resolves your issue!

@danschultzer
Copy link
Collaborator

v0.1.25 has been released, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants