FEATURE: ban any SSO attempts with invalid external id

We now treat any external_id of blank string (" " or "     " or "", etc) or a
invalid word (none, nil, blank, null) - case insensitive - as invalid.

In this case the client will see "please contact admin" the logs will explain
the reason clearly.

app/models/discourse_single_sign_on.rb

@@ -4,6 +4,9 @@ require_dependency 'single_sign_on'
 
 class DiscourseSingleSignOn < SingleSignOn
 
+  class BlankExternalId < StandardError; end
+  class BannedExternalId < StandardError; end
+
   def self.sso_url
     SiteSetting.sso_url
   end
@@ -48,7 +51,21 @@ class DiscourseSingleSignOn < SingleSignOn
     "SSO_NONCE_#{nonce}"
   end
 
+  BANNED_EXTERNAL_IDS = %w{none nil blank null}
+
   def lookup_or_create_user(ip_address = nil)
+
+    # we don't want to ban 0 from being an external id
+    external_id = self.external_id.to_s
+
+    if external_id.blank?
+      raise BlankExternalId
+    end
+
+    if BANNED_EXTERNAL_IDS.include?(external_id.downcase)
+      raise BannedExternalId, external_id
+    end
+
     sso_record = SingleSignOnRecord.find_by(external_id: external_id)
 
     if sso_record && (user = sso_record.user)

spec/models/discourse_single_sign_on_spec.rb

@@ -71,6 +71,34 @@ describe DiscourseSingleSignOn do
 
   let(:ip_address) { "127.0.0.1" }
 
+  it "bans bad external id" do
+    sso = DiscourseSingleSignOn.new
+    sso.username = "test"
+    sso.name = ""
+    sso.email = "test@test.com"
+    sso.suppress_welcome_message = true
+
+    sso.external_id = "    "
+
+    expect do
+      sso.lookup_or_create_user(ip_address)
+    end.to raise_error(DiscourseSingleSignOn::BlankExternalId)
+
+    sso.external_id = nil
+
+    expect do
+      sso.lookup_or_create_user(ip_address)
+    end.to raise_error(DiscourseSingleSignOn::BlankExternalId)
+
+    # going for slight duplication here so our intent is crystal clear
+    %w{none nil Blank null}.each do |word|
+      sso.external_id = word
+      expect do
+        sso.lookup_or_create_user(ip_address)
+      end.to raise_error(DiscourseSingleSignOn::BannedExternalId)
+    end
+  end
+
   it "can lookup or create user when name is blank" do
     sso = DiscourseSingleSignOn.new
     sso.username = "test"

spec/requests/session_controller_spec.rb

@@ -320,6 +320,28 @@ RSpec.describe SessionController do
 
     end
 
+    it 'can handle invalid sso external ids due to blank' do
+      sso = get_sso("/")
+      sso.email = "test@test.com"
+      sso.external_id = '   '
+      sso.username = 'sam'
+
+      get "/session/sso_login", params: Rack::Utils.parse_query(sso.payload), headers: headers
+
+      expect(response.status).to eq(500)
+    end
+
+    it 'can handle invalid sso external ids due to banned word' do
+      sso = get_sso("/")
+      sso.email = "test@test.com"
+      sso.external_id = 'nil'
+      sso.username = 'sam'
+
+      get "/session/sso_login", params: Rack::Utils.parse_query(sso.payload), headers: headers
+
+      expect(response.status).to eq(500)
+    end
+
     it 'can take over an account' do
       sso = get_sso("/")
       user = Fabricate(:user)
@@ -348,7 +370,7 @@ RSpec.describe SessionController do
     it 'respects IP restrictions on create' do
       ScreenedIpAddress.all.destroy_all
       get "/"
-      screened_ip = Fabricate(:screened_ip_address, ip_address: request.remote_ip, action_type: ScreenedIpAddress.actions[:block])
+      _screened_ip = Fabricate(:screened_ip_address, ip_address: request.remote_ip, action_type: ScreenedIpAddress.actions[:block])
 
       sso = sso_for_ip_specs
       get "/session/sso_login", params: Rack::Utils.parse_query(sso.payload), headers: headers
@@ -364,7 +386,7 @@ RSpec.describe SessionController do
       DiscourseSingleSignOn.parse(sso.payload).lookup_or_create_user(request.remote_ip)
 
       sso = sso_for_ip_specs
-      screened_ip = Fabricate(:screened_ip_address, ip_address: request.remote_ip, action_type: ScreenedIpAddress.actions[:block])
+      _screened_ip = Fabricate(:screened_ip_address, ip_address: request.remote_ip, action_type: ScreenedIpAddress.actions[:block])
 
       get "/session/sso_login", params: Rack::Utils.parse_query(sso.payload), headers: headers
       logged_on_user = Discourse.current_user_provider.new(request.env).current_user
@@ -1052,7 +1074,7 @@ RSpec.describe SessionController do
         it "doesn't log in" do
           ScreenedIpAddress.all.destroy_all
           get "/"
-          screened_ip = Fabricate(:screened_ip_address, ip_address: request.remote_ip)
+          _screened_ip = Fabricate(:screened_ip_address, ip_address: request.remote_ip)
           post "/session.json", params: {
             login: "@" + user.username, password: 'myawesomepassword'
           }