FIX: SSO code should respect IP address filters

app/controllers/admin/users_controller.rb

@@ -273,7 +273,7 @@ class Admin::UsersController < Admin::AdminController
     return render nothing: true, status: 404 unless SiteSetting.enable_sso
 
     sso = DiscourseSingleSignOn.parse("sso=#{params[:sso]}&sig=#{params[:sig]}")
-    user = sso.lookup_or_create_user
+    user = sso.lookup_or_create_user(request.remote_ip)
 
     render_serialized(user, AdminDetailedUserSerializer, root: false)
   end

app/controllers/session_controller.rb

@@ -66,7 +66,8 @@ class SessionController < ApplicationController
     sso.expire_nonce!
 
     begin
-      if user = sso.lookup_or_create_user
+      if user = sso.lookup_or_create_user(request.remote_ip)
+
         if SiteSetting.must_approve_users? && !user.approved?
           render text: I18n.t("sso.account_not_approved"), status: 403
         else
@@ -145,8 +146,7 @@ class SessionController < ApplicationController
     end
 
     if ScreenedIpAddress.block_login?(user, request.remote_ip)
-      not_allowed_from_ip_address(user)
+      return not_allowed_from_ip_address(user)
-      return
     end
 
     (user.active && user.email_confirmed?) ? login(user) : not_activated(user)

app/models/discourse_single_sign_on.rb

@@ -42,13 +42,13 @@ class DiscourseSingleSignOn < SingleSignOn
     "SSO_NONCE_#{nonce}"
   end
 
-  def lookup_or_create_user
+  def lookup_or_create_user(ip_address)
     sso_record = SingleSignOnRecord.find_by(external_id: external_id)
 
     if sso_record && user = sso_record.user
       sso_record.last_payload = unsigned_payload
     else
-      user = match_email_or_create_user
+      user = match_email_or_create_user(ip_address)
       sso_record = user.single_sign_on_record
     end
 
@@ -67,6 +67,7 @@ class DiscourseSingleSignOn < SingleSignOn
       user.custom_fields[k] = v
     end
 
+    user.ip_address = ip_address
     user.admin = admin unless admin.nil?
     user.moderator = moderator unless moderator.nil?
 
@@ -79,7 +80,7 @@ class DiscourseSingleSignOn < SingleSignOn
 
   private
 
-  def match_email_or_create_user
+  def match_email_or_create_user(ip_address)
     user = User.find_by_email(email)
 
     try_name = name.blank? ? nil : name
@@ -89,6 +90,7 @@ class DiscourseSingleSignOn < SingleSignOn
       email: email,
       name:  User.suggest_name(try_name || try_username || email),
       username: UserNameSuggester.suggest(try_username || try_name || email),
+      ip_address: ip_address
     }
 
     if user || user = User.create!(user_params)

spec/controllers/admin/users_controller_spec.rb

@@ -499,7 +499,7 @@ describe Admin::UsersController do
     sso.external_id = "1"
 
     user = DiscourseSingleSignOn.parse(sso.payload)
-                                .lookup_or_create_user
+                                .lookup_or_create_user('127.0.0.1')
 
 
     sso.name = "Bill"

spec/controllers/session_controller_spec.rb

@@ -67,6 +67,21 @@ describe SessionController do
       expect(logged_on_user.single_sign_on_record.external_username).to eq('sam')
     end
 
+    it 'respects IP restrictions' do
+      sso = get_sso('/a/')
+      sso.external_id = '666' # the number of the beast
+      sso.email = 'bob@bob.com'
+      sso.name = 'Sam Saffron'
+      sso.username = 'sam'
+
+      screened_ip = Fabricate(:screened_ip_address)
+      ActionDispatch::Request.any_instance.stubs(:remote_ip).returns(screened_ip.ip_address)
+      get :sso_login, Rack::Utils.parse_query(sso.payload)
+
+      logged_on_user = Discourse.current_user_provider.new(request.env).current_user
+      expect(logged_on_user).to eq(nil)
+    end
+
     it 'allows you to create an admin account' do
       sso = get_sso('/a/')
       sso.external_id = '666' # the number of the beast
@@ -409,7 +424,6 @@ describe SessionController do
 
       context 'when admins are restricted by ip address' do
         let(:permitted_ip_address) { '111.234.23.11' }
-
         before do
           Fabricate(:screened_ip_address, ip_address: permitted_ip_address, action_type: ScreenedIpAddress.actions[:allow_admin])
         end

spec/models/discourse_single_sign_on_spec.rb

@@ -48,6 +48,8 @@ describe DiscourseSingleSignOn do
     expect(sso.email).to eq "sam@sam.com"
   end
 
+  let(:ip_address) { "127.0.0.1" }
+
   it "can lookup or create user when name is blank" do
     # so we can create system messages
     Fabricate(:admin)
@@ -56,7 +58,7 @@ describe DiscourseSingleSignOn do
     sso.name = ""
     sso.email = "test@test.com"
     sso.external_id = "A"
-    user = sso.lookup_or_create_user
+    user = sso.lookup_or_create_user(ip_address)
     expect(user).to_not be_nil
   end
 
@@ -123,7 +125,7 @@ describe DiscourseSingleSignOn do
     it "deal with no avatar url passed for an existing user with an avatar" do
       # Deliberately not setting avatar_url.
 
-      user = sso.lookup_or_create_user
+      user = sso.lookup_or_create_user(ip_address)
       expect(user).to_not be_nil
     end
 
@@ -133,7 +135,7 @@ describe DiscourseSingleSignOn do
       sso.avatar_url = "http://example.com/a_different_image.png"
       sso.avatar_force_update = true
 
-      user = sso.lookup_or_create_user
+      user = sso.lookup_or_create_user(ip_address)
       expect(user).to_not be_nil
     end
   end