FIX: download SSO avatars in a background job to prevent hangs when avatars are huge

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

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