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

@@ -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

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

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)

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"