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

Retrieve github user email #22

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kefahi
Copy link

@kefahi kefahi commented Sep 27, 2020

One additional api call is required to retrieve the github user's email.
The returned json i format is

[ {"email":"email ... address", ...}]

For simplicity it was done as a one-liner.

One additional api call is required to retrieve the github user's email.
The returned json i format is 
```json
[ {"email":"email ... address", ...}]
```
For simplicity it was done as a one-liner.
@@ -58,6 +58,8 @@ class MultiAuth::Provider::Github < MultiAuth::Provider
gh_user = GhUser.from_json(raw_json)
gh_user.access_token = access_token
gh_user.raw_json = raw_json
raw_email_json = api.get("/user/emails").body
gh_user.email = JSON.parse(raw_email_json)[0]["email"].to_s
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code will raise an exception if no emails a returned (e.g. authorize_uri scope does not have email). Please make code work when no email in the response.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msa7
Done. Kindly confirm.

@stephendolan
Copy link
Contributor

@kefahi Do you have plans to apply the requested changes? This would save me an extra API call I'm manually making, as well 😄

Absorb exceptions if the requested user email is not accessible.
@kefahi
Copy link
Author

kefahi commented May 9, 2021

@stephendolan
The suggested code is now surrounded within begin/rescue/end block. This should protect in case email is not returned.
I hope this is an acceptable solution. Let me know what you think.

@msa7
Copy link
Owner

msa7 commented May 20, 2021

@stephendolan @kefahi I just realized that get-the-authenticated-user already return the email. Why needs additional requests?

@stephendolan
Copy link
Contributor

stephendolan commented May 21, 2021

@msa7 It had something to do with this user setting:

CleanShot 2021-05-21 at 11 27 00

When that box was checked, I'd get no email in the default response and had to make a follow-up request, which did return the email, even if it was private. Here's the full method I wrote that jogged my memory:

  # If a user's email is set to private, we have to make a subsequent request
  # and fetch the primary email.
  private def fetch_email_from_github(user : GitHubOAuthUser)
    headers = HTTP::Headers{
      "Authorization" => "token #{user.access_token}",
      "Accept"        => "application/vnd.github.v3+json",
    }
    api_response = HTTP::Client.get "https://api.github.com/user/emails", headers: headers

    if api_response.status == HTTP::Status::OK
      github_emails = Array(GitHubEmail).from_json(api_response.body)
      primary_github_email = github_emails.find(&.primary)

      if primary_github_email
        primary_github_email.email
      else
        raise "User retrieved from GitHub API had no primary email."
      end
    else
      raise "Failed to retrieve emails for user from GitHub API."
    end
  end

And the little helper class for the response:

  class GitHubEmail
    include JSON::Serializable

    property email : String
    property primary : Bool
  end

@stephendolan
Copy link
Contributor

In reviewing the code I'd previously written, @kefahi, I do wonder if it'd be better to search for the primary email explicitly rather than just pulling the first one from the response.

I'm not sure that the primary email is always guaranteed to be first in the response.

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 this pull request may close these issues.

None yet

3 participants