Core: [Github] authentication fails in multiple ways on discuss.flarum.org

[mlyle]

Initially I got an error-- blank white window with a message about invalid index after the redirect from github asking me whether to allow the app. Now after clicking "sign up" and selecting to use github, it fills in my user name and has a blank email. No matter what I fill in the email field it says "The email field is required."

[mlyle]

Initial problem report was on Safari on OSX. Reproduced the exact same scenario, including github "invalid state" on Chrome just now.

[tobyzerner]

This should all be fixed in dev-master, which discuss.flarum.org is running. Can you confirm? Let me know if it's still not working.

[mlyle]

Still not working.

[mlyle]

Confirmed still broken on Chrome, Safari on OSX.

[mlyle]

I played with this more.

1. I read the code of flarum-ext-auth-github and it seems to correctly request the user:email scope.
2. My Github account definitely has authorized the scope user:email for the discuss.flarum.org
3. It still didn't work.

Then, I made my email address public briefly. And then it worked. (!)

I looked at the github API docs about retrieving a user's resource. No bueno-- played with it. Even with user.email rights it's not good enough.

https://developer.github.com/v3/users/

It even states:

Note: The returned email is the user's publicly visible email address (or null if the user has not specified a public email address in their profile).

This is the correct API access to use instead:

https://developer.github.com/v3/users/emails/

[franzliedke]

Ah, nice, thanks for clarifying this!

[franzliedke]

@tobscure Shall I take care of this one?

[tobyzerner]

Yes please! I suppose you'll need to make an additional API request in GithubAuthController@getIdentification... the other option would be to add a github_id column to the users table and use that as the identification, then just suggest the email, forcing the user to set it manually if it's null. Similar to what we do for the Twitter auth extension.

[mlyle]

Pretty, pretty please.. If you guys fix this one I'm likely to deploy one for d-ronin/dRonin.

[franzliedke]

@mlyle I'll take a look tomorrow, or on the weekend. :)

[mlyle]

Much appreciated :)

[sijad]

sijad/flarum-ext-auth-github@a83041606c67b4720b31d7ce19a365d75a250244
this is only an example but it almost fix the issue (maybe it's not a bad idea remove AbstractOAuth2Controller due GitHubAuthController overrides handle?)
but as @tobscure recommended, maybe github_id is a better idea?

[tobyzerner]

@sijad with regard to AbstractOAuth2Controller, what if we pass $token and $provider into getIdentification so that the /user/emails API request can be made there, without overriding handle?

[sijad]

@tobscure yes, definitely

[sijad]

@tobscure if you're busy, can I do that?
how about this?

[tobyzerner]

@sijad looking good, just want @franzliedke to comment if he has any better ideas than passing all that stuff into getIdentification :)

[franzliedke]

I'll review it. :)

[franzliedke]

Thanks again, @sijad. I simplified your proposal a bit, so that I didn't have to overwrite the handle method.

@mlyle A little later than planned, sorry, but you can play with it now. :) Thanks for the thorough investigation, too!

[sijad]

there were an open PR https://github.com/flarum/core/pull/849, I forgot mention this issue there, should I close that?

[mlyle]

Thanks everyone! I'll pull it down in the next few days and play.

[franzliedke]

Hmm, one more thing: Technically, the recommended way to access low-level GitHub API endpoints is to use hypermedia. This would mean going to the API root first and then going to the URL provided in the "emails_url" field.

Right now, we construct the URL manually, which is frowned upon in REST land. Downside: one more request (although a cheap one, probably).

[kloknibor]

How do I update this fix to my flarum install :$? composer update doesn't work because it only sees the old beta...

[franzliedke]

@kloknibor You can apply the commits linked (further above) in this issue by hand, or wait for beta.5, which will come very soon (TM). :)

[kloknibor]

@franzliedke Damn that was a quick response! Thanks! And since I haven't found what you meant and only used "composer require flarum/flarum-ext-auth-github" by ssh, so it might be better for me to wait for the new beta haha! Thanks anyway