From 2ae7c47a3ccd0412db3eddb7f99d3a2e9e0e119e Mon Sep 17 00:00:00 2001 From: Jared Reisinger Date: Thu, 22 Sep 2016 11:31:10 -0700 Subject: [PATCH] Add support for email whitelist/blacklist to GitHub auth If a site is configured for GitHub logins, _**and**_ has an email domain whitelist, it's possible to get in a state where a new user is locked to a non-whitelist email (their GitHub primary) even though they have an alternate email that's on the whitelist. In all cases, the GitHub primary email is attempted first so that previously existing behavior will be the default. - Add whitelist/blacklist support to GithubAuthenticator (via EmailValidator) - Add multiple email support GithubAuthenticator - Add test specs for GithubAuthenticator - Add authenticator-agnostic "none of your email addresses are allowed" error message. --- config/locales/server.en.yml | 1 + lib/auth/github_authenticator.rb | 84 +++++++-- .../auth/github_authenticator_spec.rb | 172 ++++++++++++++++++ 3 files changed, 245 insertions(+), 12 deletions(-) create mode 100644 spec/components/auth/github_authenticator_spec.rb diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 527fffcbb..649f99fe9 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 256ad0959..e79414973 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 000000000..066fdecec --- /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