Merge pull request #4457 from JaredReisinger/github-auth-with-email-whitelist

Add support for email whitelist/blacklist to GitHub auth

config/locales/server.en.yml

@@ -1538,6 +1538,7 @@ en:
     something_already_taken: "Something went wrong, perhaps the username or email is already registered. Try the forgot password link."
     omniauth_error: "Sorry, there was an error authorizing your account. Perhaps you did not approve authorization?"
     omniauth_error_unknown: "Something went wrong processing your log in, please try again."
+    authenticator_error_no_valid_email: "No email addresses associated with %{account} are allowed. You may need to configure your account with a different email address."
     new_registrations_disabled: "New account registrations are not allowed at this time."
     password_too_long: "Passwords are limited to 200 characters."
     email_too_long: "The email you provided is too long. Mailbox names must be no more than 254 characters, and domain names must be no more than 253 characters."

lib/auth/github_authenticator.rb

@@ -1,18 +1,34 @@
+require_dependency 'has_errors'
+
 class Auth::GithubAuthenticator < Auth::Authenticator
 
   def name
     "github"
   end
 
+  class GithubEmailChecker
+    include ::HasErrors
+
+    def initialize(validator, email)
+      @validator = validator
+      @email = Email.downcase(email)
+    end
+
+    def valid?()
+      @validator.validate_each(self, :email, @email)
+      return errors.blank?
+    end
+
+  end
+
   def after_authenticate(auth_token)
     result = Auth::Result.new
 
     data = auth_token[:info]
+    result.username = screen_name = data[:nickname]
+    result.name = data[:name]
 
-    result.username = screen_name = data["nickname"]
-    result.email = email = data["email"]
-
-    github_user_id = auth_token["uid"]
+    github_user_id = auth_token[:uid]
 
     result.extra_data = {
       github_user_id: github_user_id,
@@ -20,20 +36,64 @@ class Auth::GithubAuthenticator < Auth::Authenticator
     }
 
     user_info = GithubUserInfo.find_by(github_user_id: github_user_id)
-    result.email_valid = !!data["email_verified"]
 
     if user_info
+      # If there's existing user info with the given GitHub ID, that's all we
+      # need to know.
       user = user_info.user
-    elsif result.email_valid && (user = User.find_by_email(email))
-      user_info = GithubUserInfo.create(
-          user_id: user.id,
-          screen_name: screen_name,
-          github_user_id: github_user_id
-      )
+      result.email = data[:email],
+      result.email_valid = !!data[:email_verified]
+    else
+      # Potentially use *any* of the emails from GitHub to find a match or
+      # register a new user, with preference given to the primary email.
+      all_emails = Array.new(auth_token[:extra][:all_emails])
+      all_emails.unshift({
+          :email => data[:email],
+          :verified => !!data[:email_verified]
+      })
+
+      # Only consider verified emails to match an existing user.  We don't want
+      # someone to be able to create a GitHub account with an unverified email
+      # in order to access someone else's Discourse account!
+      all_emails.each do |candidate|
+        if !!candidate[:verified] && (user = User.find_by_email(candidate[:email]))
+          result.email = candidate[:email]
+          result.email_valid = !!candidate[:verified]
+          GithubUserInfo.create(
+              user_id: user.id,
+              screen_name: screen_name,
+              github_user_id: github_user_id
+          )
+          break
+        end
+      end
+
+      # If we *still* don't have a user, check to see if there's an email that
+      # passes validation (this includes whitelist/blacklist filtering if any is
+      # configured).  When no whitelist/blacklist is in play, this will simply
+      # choose the primary email since it's at the front of the list.
+      if !user
+        validator = EmailValidator.new(attributes: :email)
+        found_email = false
+        all_emails.each do |candidate|
+          checker = GithubEmailChecker.new(validator, candidate[:email])
+          if checker.valid?
+            result.email = candidate[:email]
+            result.email_valid = !!candidate[:verified]
+            found_email = true
+            break
+          end
+        end
+
+        if !found_email
+          result.failed = true
+          escaped = Rack::Utils.escape_html(screen_name)
+          result.failed_reason = I18n.t("login.authenticator_error_no_valid_email", account: escaped)
+        end
+      end
     end
 
     result.user = user
-
     result
   end
 

spec/components/auth/github_authenticator_spec.rb

@@ -0,0 +1,172 @@
+require 'rails_helper'
+
+# In the ghetto ... getting the spec to run in autospec
+#  thing is we need to load up all auth really early pre-fork
+#  it means that the require is not going to get a new copy
+Auth.send(:remove_const, :GithubAuthenticator)
+load 'auth/github_authenticator.rb'
+
+describe Auth::GithubAuthenticator do
+
+  context 'after_authenticate' do
+
+    it 'can authenticate and create a user record for already existing users' do
+      user = Fabricate(:user)
+
+      hash = {
+        :extra => {
+          :all_emails => [{
+            :email => user.email,
+            :primary => true,
+            :verified => true,
+          }]
+        },
+        :info => {
+          :email => user.email,
+          :email_verified => true,
+          :nickname => user.username,
+          :name => user.name,
+        },
+        :uid => "100"
+      }
+
+      authenticator = Auth::GithubAuthenticator.new
+      result = authenticator.after_authenticate(hash)
+
+      expect(result.user.id).to eq(user.id)
+      expect(result.username).to eq(user.username)
+      expect(result.name).to eq(user.name)
+      expect(result.email).to eq(user.email)
+      expect(result.email_valid).to eq(true)
+    end
+
+    it 'will not authenticate for already existing users with an unverified email' do
+      user = Fabricate(:user)
+
+      hash = {
+        :extra => {
+          :all_emails => [{
+            :email => user.email,
+            :primary => true,
+            :verified => false,
+          }]
+        },
+        :info => {
+          :email => user.email,
+          :email_verified => false,
+          :nickname => user.username,
+          :name => user.name,
+        },
+        :uid => "100"
+      }
+
+      authenticator = Auth::GithubAuthenticator.new
+      result = authenticator.after_authenticate(hash)
+
+      expect(result.user).to eq(nil)
+      expect(result.username).to eq(user.username)
+      expect(result.name).to eq(user.name)
+      expect(result.email).to eq(user.email)
+      expect(result.email_valid).to eq(false)
+    end
+
+    it 'can create a proper result for non existing users' do
+      hash = {
+        :extra => {
+          :all_emails => [{
+            :email => "person@example.com",
+            :primary => true,
+            :verified => true,
+          }]
+        },
+        :info => {
+          :email => "person@example.com",
+          :email_verified => true,
+          :nickname => "person",
+          :name => "Person Lastname",
+        },
+        :uid => "100"
+      }
+
+      authenticator = Auth::GithubAuthenticator.new
+      result = authenticator.after_authenticate(hash)
+
+      expect(result.user).to eq(nil)
+      expect(result.username).to eq(hash[:info][:nickname])
+      expect(result.name).to eq(hash[:info][:name])
+      expect(result.email).to eq(hash[:info][:email])
+      expect(result.email_valid).to eq(hash[:info][:email_verified])
+    end
+
+    it 'will skip blacklisted domains for non existing users' do
+      hash = {
+        :extra => {
+          :all_emails => [{
+            :email => "not_allowed@blacklist.com",
+            :primary => true,
+            :verified => true,
+          },{
+            :email => "allowed@whitelist.com",
+            :primary => false,
+            :verified => true,
+          }]
+        },
+        :info => {
+          :email => "not_allowed@blacklist.com",
+          :email_verified => true,
+          :nickname => "person",
+          :name => "Person Lastname",
+        },
+        :uid => "100"
+      }
+
+      authenticator = Auth::GithubAuthenticator.new
+      SiteSetting.email_domains_blacklist = "blacklist.com"
+      result = authenticator.after_authenticate(hash)
+
+      expect(result.user).to eq(nil)
+      expect(result.username).to eq(hash[:info][:nickname])
+      expect(result.name).to eq(hash[:info][:name])
+      expect(result.email).to eq("allowed@whitelist.com")
+      expect(result.email_valid).to eq(true)
+    end
+
+    it 'will find whitelisted domains for non existing users' do
+      hash = {
+        :extra => {
+          :all_emails => [{
+            :email => "person@example.com",
+            :primary => true,
+            :verified => true,
+          },{
+            :email => "not_allowed@blacklist.com",
+            :primary => true,
+            :verified => true,
+          },{
+            :email => "allowed@whitelist.com",
+            :primary => false,
+            :verified => true,
+          }]
+        },
+        :info => {
+          :email => "person@example.com",
+          :email_verified => true,
+          :nickname => "person",
+          :name => "Person Lastname",
+        },
+        :uid => "100"
+      }
+
+      authenticator = Auth::GithubAuthenticator.new
+      SiteSetting.email_domains_whitelist = "whitelist.com"
+      result = authenticator.after_authenticate(hash)
+
+      expect(result.user).to eq(nil)
+      expect(result.username).to eq(hash[:info][:nickname])
+      expect(result.name).to eq(hash[:info][:name])
+      expect(result.email).to eq("allowed@whitelist.com")
+      expect(result.email_valid).to eq(true)
+    end
+
+  end
+end