DEV: reduce logging when no external id is specified

DEV: reduce logging when no external id is specified

Previously we were returning an unknown sso error and logging a message when external id was blank. This noise is not needed.

diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb
index c8696e0..5f8b1c3 100644
--- a/app/controllers/session_controller.rb
+++ b/app/controllers/session_controller.rb
@@ -254,6 +254,10 @@ class SessionController < ApplicationController
 
       render_sso_error(text: text || I18n.t("sso.unknown_error"), status: 500)
 
+    rescue DiscourseSingleSignOn::BlankExternalId
+
+      render_sso_error(text: I18n.t("sso.blank_id_error"), status: 500)
+
     rescue => e
       message = +"Failed to create or lookup user: #{e}."
       message << "  "
diff --git a/spec/components/scheduler/defer_spec.rb b/spec/components/scheduler/defer_spec.rb
index 533e2ff..ab1aa06 100644
--- a/spec/components/scheduler/defer_spec.rb
+++ b/spec/components/scheduler/defer_spec.rb
@@ -15,26 +15,6 @@ describe Scheduler::Defer do
     end
   end
 
-  class TrackingLogger < ::Logger
-    attr_reader :messages
-    def initialize
-      super(nil)
-      @messages = []
-    end
-    def add(*args, &block)
-      @messages << args
-    end
-  end
-
-  def track_log_messages
-    old_logger = Rails.logger
-    logger = Rails.logger = TrackingLogger.new
-    yield logger.messages
-    logger.messages
-  ensure
-    Rails.logger = old_logger
-  end
-
   before do
     @defer = DeferInstance.new
     @defer.async = true
diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb
index dc649a9..011dc03 100644
--- a/spec/rails_helper.rb
+++ b/spec/rails_helper.rb
@@ -385,3 +385,26 @@ def silence_stdout
 ensure
   STDOUT.unstub(:write)
 end
+
+class TrackingLogger < ::Logger
+  attr_reader :messages
+  def initialize(level: nil)
+    super(nil)
+    @messages = []
+    @level = level
+  end
+  def add(*args, &block)
+    if !level || args[0].to_i >= level
+      @messages << args
+    end
+  end
+end
+
+def track_log_messages(level: nil)
+  old_logger = Rails.logger
+  logger = Rails.logger = TrackingLogger.new(level: level)
+  yield logger.messages
+  logger.messages
+ensure
+  Rails.logger = old_logger
+end
diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb
index 068230b..316bc71 100644
--- a/spec/requests/session_controller_spec.rb
+++ b/spec/requests/session_controller_spec.rb
@@ -563,9 +563,13 @@ RSpec.describe SessionController do
       sso.external_id = '   '
       sso.username = 'sam'
 
-      get "/session/sso_login", params: Rack::Utils.parse_query(sso.payload), headers: headers
+      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.blank_id_error'))
     end
 
     it 'can handle invalid sso external ids due to banned word' do
@@ -1165,12 +1169,9 @@ RSpec.describe SessionController do
         SiteSetting.enable_local_logins_via_email = false
       end
       it 'doesnt matter, logs in correctly' do
-        events = DiscourseEvent.track_events do
-          post "/session.json", params: {
-            login: user.username, password: 'myawesomepassword'
-          }
-        end
-
+        post "/session.json", params: {
+          login: user.username, password: 'myawesomepassword'
+        }
         expect(response.status).to eq(200)
       end
     end

GitHub sha: 0375a5ac

2 Likes