From 20c0da8516070d4bf6d70508b9b0ebd77a331775 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 23 Nov 2020 11:06:08 +0000 Subject: [PATCH] FIX: Improve email validation error handling for external logins (#11307) - Display reason for validation error when logging in via an authenticator - Fix email validation handling for 'Discourse SSO', and add a spec Previously, validation errors (e.g. blocked or already-taken emails) would raise a generic error with no useful information. --- app/controllers/session_controller.rb | 2 +- .../users/omniauth_callbacks_controller.rb | 40 ++++++++++++------- .../omniauth_callbacks_controller_spec.rb | 19 +++++++++ spec/requests/session_controller_spec.rb | 16 ++++++++ 4 files changed, 62 insertions(+), 15 deletions(-) diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 7bb72f9787f..1f0e573fdb3 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -244,7 +244,7 @@ class SessionController < ApplicationController text = nil # If there's a problem with the email we can explain that - if (e.record.is_a?(User) && e.record.errors[:email].present?) + if (e.record.is_a?(User) && e.record.errors[:primary_email].present?) if e.record.email.blank? text = I18n.t("sso.no_email") else diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index bbbd839f428..c4b99bb4414 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -67,20 +67,20 @@ class Users::OmniauthCallbacksController < ApplicationController end @auth_result.destination_url = @origin + @auth_result.authenticator_name = authenticator.name - if @auth_result.failed? - flash[:error] = @auth_result.failed_reason.html_safe - render('failure') - else - @auth_result.authenticator_name = authenticator.name - complete_response_data - cookies['_bypass_cache'] = true - cookies[:authentication_data] = { - value: @auth_result.to_client_hash.to_json, - path: Discourse.base_path("/") - } - redirect_to @origin - end + return render_auth_result_failure if @auth_result.failed? + + complete_response_data + + return render_auth_result_failure if @auth_result.failed? + + cookies['_bypass_cache'] = true + cookies[:authentication_data] = { + value: @auth_result.to_client_hash.to_json, + path: Discourse.base_path("/") + } + redirect_to @origin end def failure @@ -98,6 +98,11 @@ class Users::OmniauthCallbacksController < ApplicationController protected + def render_auth_result_failure + flash[:error] = @auth_result.failed_reason.html_safe + render 'failure' + end + def complete_response_data if @auth_result.user user_found(@auth_result.user) @@ -138,7 +143,14 @@ class Users::OmniauthCallbacksController < ApplicationController elsif ScreenedIpAddress.block_admin_login?(user, request.remote_ip) @auth_result.admin_not_allowed_from_ip_address = true elsif Guardian.new(user).can_access_forum? && user.active # log on any account that is active with forum access - user.save! if @auth_result.apply_user_attributes! + begin + user.save! if @auth_result.apply_user_attributes! + rescue ActiveRecord::RecordInvalid => e + @auth_result.failed = true + @auth_result.failed_reason = e.record.errors.full_messages.join(", ") + return + end + log_on_user(user) Invite.invalidate_for_email(user.email) # invite link can't be used to log in anymore session[:authentication] = nil # don't carry around old auth info, perhaps move elsewhere diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index e34271c263e..041179d838f 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -387,6 +387,25 @@ RSpec.describe Users::OmniauthCallbacksController do expect(user.email).to eq('email@example.com') end + it "shows error when sso_overrides_email causes a validation error" do + SiteSetting.email_editable = false + SiteSetting.sso_overrides_email = true + + UserAssociatedAccount.create!(provider_name: "google_oauth2", user_id: user.id, provider_uid: '123545') + + google_email = user.email + user.update!(email: 'anotheremail@example.com') + Fabricate(:user, email: google_email) # Another user has the google account email + + get "/auth/google_oauth2/callback" + expect(response.status).to eq(200) + expect(response.body).to include(I18n.t("errors.messages.taken")) + expect(session[:current_user_id]).to eq(nil) + + user.reload + expect(user.email).to eq('anotheremail@example.com') + end + context 'when user has TOTP enabled' do before do user.create_totp(enabled: true) diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 710874b57fb..d858d2ac5b8 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -580,6 +580,22 @@ RSpec.describe SessionController do expect(response.body).to include(I18n.t('sso.blank_id_error')) end + it 'can handle invalid sso email validation errors' do + SiteSetting.blocked_email_domains = "test.com" + sso = get_sso("/") + sso.email = "test@test.com" + sso.external_id = '123' + sso.username = 'sam' + + messages = track_log_messages(level: Logger::WARN) do + get "/session/sso_login", params: Rack::Utils.parse_query(sso.payload), headers: headers + end + + expect(messages.length).to eq(0) + expect(response.status).to eq(500) + expect(response.body).to include(I18n.t("sso.email_error", email: ERB::Util.html_escape("test@test.com"))) + end + it 'can handle invalid sso external ids due to banned word' do sso = get_sso("/") sso.email = "test@test.com"