diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb index fc9b9ce8682..69bc0d1e837 100644 --- a/app/models/discourse_single_sign_on.rb +++ b/app/models/discourse_single_sign_on.rb @@ -119,13 +119,15 @@ class DiscourseSingleSignOn < SingleSignOn sso_record.last_payload = unsigned_payload sso_record.external_id = external_id else - UserAvatar.import_url_for_user(avatar_url, user) if avatar_url.present? - user.create_single_sign_on_record(last_payload: unsigned_payload, - external_id: external_id, - external_username: username, - external_email: email, - external_name: name, - external_avatar_url: avatar_url) + Jobs.enqueue(:download_avatar_from_url, url: avatar_url, user_id: user.id) if avatar_url.present? + user.create_single_sign_on_record( + last_payload: unsigned_payload, + external_id: external_id, + external_username: username, + external_email: email, + external_name: name, + external_avatar_url: avatar_url + ) end end @@ -148,12 +150,11 @@ class DiscourseSingleSignOn < SingleSignOn avatar_missing = user.uploaded_avatar_id.nil? || !Upload.exists?(user.uploaded_avatar_id) if (avatar_missing || avatar_force_update || SiteSetting.sso_overrides_avatar) && avatar_url.present? + avatar_changed = sso_record.external_avatar_url != avatar_url - avatar_changed = sso_record.external_avatar_url != avatar_url - - if avatar_force_update || avatar_changed || avatar_missing - UserAvatar.import_url_for_user(avatar_url, user) - end + if avatar_force_update || avatar_changed || avatar_missing + Jobs.enqueue(:download_avatar_from_url, url: avatar_url, user_id: user.id) + end end # change external attributes for sso record diff --git a/spec/models/discourse_single_sign_on_spec.rb b/spec/models/discourse_single_sign_on_spec.rb index 6c01e380e30..10a49a09d94 100644 --- a/spec/models/discourse_single_sign_on_spec.rb +++ b/spec/models/discourse_single_sign_on_spec.rb @@ -1,4 +1,5 @@ require "rails_helper" +require 'sidekiq/testing' describe DiscourseSingleSignOn do before do @@ -279,60 +280,67 @@ describe DiscourseSingleSignOn do it "correctly handles provided avatar_urls" do + Sidekiq::Testing.inline! do + sso = DiscourseSingleSignOn.new + sso.external_id = 666 + sso.email = "sam@sam.com" + sso.name = "sam" + sso.username = "sam" + sso.avatar_url = "http://awesome.com/image.png" - sso = DiscourseSingleSignOn.new - sso.external_id = 666 - sso.email = "sam@sam.com" - sso.name = "sam" - sso.username = "sam" - sso.avatar_url = "http://awesome.com/image.png" + FileHelper.stubs(:download).returns(file_from_fixtures("logo.png")) + user = sso.lookup_or_create_user(ip_address) + user.reload + avatar_id = user.uploaded_avatar_id - FileHelper.stubs(:download).returns(file_from_fixtures("logo.png")) - user = sso.lookup_or_create_user(ip_address) - avatar_id = user.uploaded_avatar_id + # initial creation ... + expect(avatar_id).to_not eq(nil) - # initial creation ... - expect(avatar_id).to_not eq(nil) + # junk avatar id should be updated + old_id = user.uploaded_avatar_id + Upload.destroy(old_id) - # junk avatar id should be updated - old_id = user.uploaded_avatar_id - Upload.destroy(old_id) + user = sso.lookup_or_create_user(ip_address) + user.reload + avatar_id = user.uploaded_avatar_id - user = sso.lookup_or_create_user(ip_address) - avatar_id = user.uploaded_avatar_id + expect(avatar_id).to_not eq(nil) + expect(old_id).to_not eq(avatar_id) - expect(avatar_id).to_not eq(nil) - expect(old_id).to_not eq(avatar_id) + FileHelper.stubs(:download) { raise "should not be called" } + sso.avatar_url = "https://some.new/avatar.png" + user = sso.lookup_or_create_user(ip_address) + user.reload - FileHelper.stubs(:download) { raise "should not be called" } - sso.avatar_url = "https://some.new/avatar.png" - user = sso.lookup_or_create_user(ip_address) + # avatar updated but no override specified ... + expect(user.uploaded_avatar_id).to eq(avatar_id) - # avatar updated but no override specified ... - expect(user.uploaded_avatar_id).to eq(avatar_id) + sso.avatar_force_update = true + FileHelper.stubs(:download).returns(file_from_fixtures("logo-dev.png")) + user = sso.lookup_or_create_user(ip_address) + user.reload - sso.avatar_force_update = true - FileHelper.stubs(:download).returns(file_from_fixtures("logo-dev.png")) - user = sso.lookup_or_create_user(ip_address) + # we better have a new avatar + expect(user.uploaded_avatar_id).not_to eq(avatar_id) + expect(user.uploaded_avatar_id).not_to eq(nil) - # we better have a new avatar - expect(user.uploaded_avatar_id).not_to eq(avatar_id) - expect(user.uploaded_avatar_id).not_to eq(nil) + avatar_id = user.uploaded_avatar_id - avatar_id = user.uploaded_avatar_id + sso.avatar_force_update = true + FileHelper.stubs(:download) { raise "not found" } + user = sso.lookup_or_create_user(ip_address) + user.reload - sso.avatar_force_update = true - FileHelper.stubs(:download) { raise "not found" } - user = sso.lookup_or_create_user(ip_address) - - # we better have the same avatar - expect(user.uploaded_avatar_id).to eq(avatar_id) + # we better have the same avatar + expect(user.uploaded_avatar_id).to eq(avatar_id) + end end end context 'when sso_overrides_avatar is enabled' do let!(:sso_record) { Fabricate(:single_sign_on_record, external_avatar_url: "http://example.com/an_image.png") } + let!(:sso) { sso = DiscourseSingleSignOn.new sso.username = "test" @@ -341,6 +349,7 @@ describe DiscourseSingleSignOn do sso.external_id = sso_record.external_id sso } + let(:logo) { file_from_fixtures("logo.png") } before do @@ -348,27 +357,32 @@ describe DiscourseSingleSignOn do end it "deal with no avatar url passed for an existing user with an avatar" do - # Deliberately not setting avatar_url so it should not update + Sidekiq::Testing.inline! do + # Deliberately not setting avatar_url so it should not update + sso_record.user.update_columns(uploaded_avatar_id: -1) + user = sso.lookup_or_create_user(ip_address) + user.reload - sso_record.user.update_columns(uploaded_avatar_id: -1) - user = sso.lookup_or_create_user(ip_address) - - expect(user).to_not be_nil - expect(user.uploaded_avatar_id).to eq(-1) + expect(user).to_not be_nil + expect(user.uploaded_avatar_id).to eq(-1) + end end it "deal with no avatar_force_update passed as a boolean" do - FileHelper.stubs(:download).returns(logo) + Sidekiq::Testing.inline! do + FileHelper.stubs(:download).returns(logo) - sso_record.user.update_columns(uploaded_avatar_id: -1) + sso_record.user.update_columns(uploaded_avatar_id: -1) - sso.avatar_url = "http://example.com/a_different_image.png" - sso.avatar_force_update = false + sso.avatar_url = "http://example.com/a_different_image.png" + sso.avatar_force_update = false - user = sso.lookup_or_create_user(ip_address) + user = sso.lookup_or_create_user(ip_address) + user.reload - expect(user).to_not be_nil - expect(user.uploaded_avatar_id).to_not eq(-1) + expect(user).to_not be_nil + expect(user.uploaded_avatar_id).to_not eq(-1) + end end end end