diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 527fffcbb38..649f99fe9af 100644 --- a/config/locales/server.en.yml +++ b/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." diff --git a/lib/auth/github_authenticator.rb b/lib/auth/github_authenticator.rb index 256ad095988..e79414973ac 100644 --- a/lib/auth/github_authenticator.rb +++ b/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 diff --git a/spec/components/auth/github_authenticator_spec.rb b/spec/components/auth/github_authenticator_spec.rb new file mode 100644 index 00000000000..066fdecec08 --- /dev/null +++ b/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