diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index c0f24695246..0f1ac74f6bc 100644 --- a/app/controllers/admin/users_controller.rb +++ b/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 diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 18b239c31d3..75c45ca979c 100644 --- a/app/controllers/session_controller.rb +++ b/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 + return not_allowed_from_ip_address(user) end (user.active && user.email_confirmed?) ? login(user) : not_activated(user) diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb index 92af8952007..6c3597a1db0 100644 --- a/app/models/discourse_single_sign_on.rb +++ b/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,16 +80,17 @@ 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 try_username = username.blank? ? nil : username user_params = { - email: email, - name: User.suggest_name(try_name || try_username || email), - username: UserNameSuggester.suggest(try_username || try_name || email), + 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) diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 97434b3241d..60d492ccfe4 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/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" diff --git a/spec/controllers/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb index 2a6dccae2a0..a4389f4b9e3 100644 --- a/spec/controllers/session_controller_spec.rb +++ b/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 diff --git a/spec/models/discourse_single_sign_on_spec.rb b/spec/models/discourse_single_sign_on_spec.rb index f6aa259c308..e335d3d18eb 100644 --- a/spec/models/discourse_single_sign_on_spec.rb +++ b/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