dwyl / elixir-auth-github

:octocat: Minimalist GitHub OAuth Authentication for Elixir Apps. Tested, Documented & Maintained. Setup in 5 mins. 🚀
GNU General Public License v2.0
38 stars 4 forks source link

GitHub profile.email: nil #46

Closed nelsonic closed 2 years ago

nelsonic commented 4 years ago

I've just discovered that it's possible to have a GitHub profile without an email address ... 🤦 image email is the only required field in a person. So the whole thing explodes. going to need to add a guard clause and figure out if this is viable for us. we might need to just disable GitHub Auth in our MVP and stick to Google or Email+Password. 💭 we will need to have a way of requesting the person's email address if their GitHub profile does not have one ... perhaps we can reuse the Email registration form. 🤔

Something to consider after MVP is launched. 👍

nelsonic commented 4 years ago

I can re-create this with my own account, just make email "Not visible": image

If the GitHub profile does not have an Email key:value we need to redirect to an email form. This is not strictly relevant to elixir-auth-github, it's more of "consumer" problem.

nelsonic commented 4 years ago

Different but related error: image

nelsonic commented 4 years ago

Going to need to address this pretty soon as we want to use GitHub Auth to validate accounts. ⏳

nelsonic commented 4 years ago

Thinking we should update our Model (Schema) in Auth to allow for an Alias when an Email Address is unavailable.

SimonLab commented 4 years ago

As noted in the following issue: https://github.com/dwyl/smart-home-auth-server/issues/14#issuecomment-678329528 I don't think using the Github username instead of the email is a good idea as the username could be used to access data of an other person

SimonLab commented 4 years ago

From https://github.community/t/oauth-and-private-email-address/13550 image

So we might still be able to access a private email with Oauth using the right scope, https://docs.github.com/en/developers/apps/scopes-for-oauth-apps#requested-scopes-and-granted-scopes

However it seems that auth is already using scopes:

https://github.com/dwyl/auth/blob/master/lib/auth_web/controllers/auth_controller.ex#L60

nelsonic commented 4 years ago

indeed, we are requesting the email address already in the auth app:

oauth_github_url = ElixirAuthGithub.login_url(%{scopes: ["user:email"], state: state})

If there is anything else we can do from a configuration perspective, please share / try it. 👍

SimonLab commented 4 years ago

I've added some logs to see if we could access the person's email when it is defined as private in the Github settings but it is indeed returned as nil even when using the user:email scope. At this stage I think the best solution is to redirect to an error page on the auth application which explain how to make an email public via Github. The person has still the possibility to authenticate with Gmail or email/password otherwise

nelsonic commented 4 years ago

@nelsonic our preference would be to redirect to a page requiring the person to "Add Email Address" which will add it to person.email and send them a verification mail.

SimonLab commented 4 years ago

My thought for having just an error page is to keep the authentication simple. With an email field page we will need to make sure that the email is updated if the person add a public email address on Github.

nelsonic commented 4 years ago

@nelsonic yeah, we already have code to make the update if they update their email address. https://github.com/dwyl/auth/blob/fafc5e059d65de420b4d99bf1d15ac45a3bcda12/lib/auth/person.ex#L104-L115 Basically each time the person logs in using GitHub (or Google) account we update all their data to the latest. 👍

nelsonic commented 4 years ago

However if we capture their email manually using a form and they then login with their GitHub account without email, we may need to continue asking them for their email address or we need a check in our upsert_person/1 function that we are not overwriting the data with nil. 💭 (but this is fairly easy to add).

SimonLab commented 4 years ago

Another issue when we ask the user to define the email manually (when the email is private on Github) is at each authentication we don't have a way to know if the person as already enter the email manually and the page asking for the email will always be displayed. This could also create user duplicates. We could check if a user exist in the database by looking for the username but as mentioned above https://github.com/dwyl/elixir-auth-github/issues/46#issuecomment-678330516 the username is not really unique

nelsonic commented 4 years ago

@SimonLab this isn't very complicated logic, we just need to write it out precisely, preferably with a diagram so that anyone else can understand+maintain it.

GitHub always returns the following profile info:

profile: %{
  public_gists: 32,
  name: "Nelson",
  type: "User",
  updated_at: "2020-08-23T22:30:55Z",
  events_url: "https://api.github.com/users/nelsonic/events{/privacy}",
  twitter_username: nil,
  site_admin: false,
  bio: "blah blah...",
  organizations_url: "https://api.github.com/users/nelsonic/orgs",
  repos_url: "https://api.github.com/users/nelsonic/repos",
  avatar_url: "https://avatars3.githubusercontent.com/u/194400?v=4",
  created_at: "2010-02-02T08:44:49Z",
  login: "nelsonic",
  email: nil,
  following_url: "https://api.github.com/users/nelsonic/following{/other_user}",
  url: "https://api.github.com/users/nelsonic",
  subscriptions_url: "https://api.github.com/users/nelsonic/subscriptions",
  public_repos: 298,
  gists_url: "https://api.github.com/users/nelsonic/gists{/gist_id}",
  company: "@dwyl",
  location: "London",
  blog: "https://dwyl.com",
  starred_url: "https://api.github.com/users/nelsonic/starred{/owner}{/repo}",
  access_token: "redacted",
  received_events_url: "https://api.github.com/users/nelsonic/received_events",
  id: 194400,
  followers_url: "https://api.github.com/users/nelsonic/followers",
  html_url: "https://github.com/nelsonic",
  followers: 2942
}

The important bit is that email: nil ... 🚫

We have the concept of a username in auth: https://github.com/dwyl/auth/blob/fafc5e059d65de420b4d99bf1d15ac45a3bcda12/lib/auth/person.ex#L18

So we can create a Person.changeset_github/2 specific to the GitHub "User" where the profile.login is used as their person.username. And then once we have created the person (without an email address), we can make a GET request to /user/emails using the user's profile.access_token:

GET https://api.github.com/user/emails
[
  {
    "email": "nelson@gmail.com",
    "primary": true,
    "verified": true
  }
]

See: https://stackoverflow.com/questions/35373995/github-user-email-is-null-despite-useremail-scope

This should avoid any additional UI or data entry from the GitHub User. Please try it. 👍
Thanks! 🌻

nelsonic commented 4 years ago

Docs: https://docs.github.com/en/rest/reference/users#list-email-addresses-for-the-authenticated-user

nelsonic commented 4 years ago

https://hex.pm/packages/elixir_auth_github/1.4.0 published. Thanks @SimonLab 👍

nelsonic commented 4 years ago

Works on localhost: image

Busy updating the demo app now: https://github.com/dwyl/elixir-auth-github-demo ⏳ 👨‍💻

nelsonic commented 4 years ago

@SimonLab v1.4.0 is working for Auth: ✅ image

Thanks. 👍

nelsonic commented 2 years ago

Fixed. Closing. ✅