Improve GitHub login for private sites

Contribute Feature
3

I have a very similar situation: we have been using a Discourse SSO for authentication, but upstream sources aren’t nearly as reliable as GitHub. I have a “GitHub with Email whitelist” plugin that bridges the gap between the current GithubAuthenticator and the current email whitelist. Now that it’s 95% complete, I’m rethinking the admin experience and wondering whether I should simply submit the changes as a PR for the core Discourse GithubAuthenticator.

Rationale:

  1. With the current existing builtin implementation, if you enable GitHub logins and also set an email whitelist, new logins/signups will often appear to work, only to be followed by failure and an inability to fix the problem. Immediately after you authenticate with GitHub (as a new Discourse user), you see:

    auth-1.png714×326 21.5 KB
    … and when you click “Create New Account”, you get:
    auth-2.png714×376 25.1 KB
    … but the “Email” field is not editable, and you’re stuck.

  2. Having multiple email addresses is “native” to GitHub, so there’s no reason not to automatically/always make use of these when the email whitelist is set.

  3. If “GitHub with email whitelist” is a separate plugin, the admin has to specifically disable the builtin GitHub login, or else you’ll end up with two “with GitHub” buttons on the login screen. True, in this case you’ve specifically configured Discourse to use the plugin, so you should know what you’re doing, but it still feels weird. (I’ve started looking into making the plugin simply alter the builtin auth’s handling, but I’m not savvy enough with Ruby to get this working yet.)

So… @erlend_sh (and team), what say you? Should I keep the email-whitelist-savvy GitHub logic isolated to a plugin, or is this something that makes sense in the builtin GitHub authenticator? (And if you’d prefer the former, I’d love it if someone could help me figure out the right incantation to make the “alter the behavior of the builtin authenticator when the plugin is enabled” thing work… I just haven’t figured out how to use add_class_method/define_singelton_method to hook/replace the builtin’s after_authenticate implementation.)

2 Likes