FIX: always respect avatar_force_update

app/models/discourse_single_sign_on.rb

@@ -49,7 +49,7 @@ class DiscourseSingleSignOn < SingleSignOn
   def lookup_or_create_user(ip_address=nil)
     sso_record = SingleSignOnRecord.find_by(external_id: external_id)
 
-    if sso_record && user = sso_record.user
+    if sso_record && (user = sso_record.user)
       sso_record.last_payload = unsigned_payload
     else
       user = match_email_or_create_user(ip_address)
@@ -145,9 +145,8 @@ class DiscourseSingleSignOn < SingleSignOn
       user.name = name || User.suggest_name(username.blank? ? email : username)
     end
 
-    if SiteSetting.sso_overrides_avatar && avatar_url.present? && (
+    if (SiteSetting.sso_overrides_avatar && avatar_url.present? && (
-      avatar_force_update ||
+      sso_record.external_avatar_url != avatar_url)) || avatar_force_update
-      sso_record.external_avatar_url != avatar_url)
 
       UserAvatar.import_url_for_user(avatar_url, user)
     end

spec/models/discourse_single_sign_on_spec.rb

@@ -275,6 +275,52 @@ describe DiscourseSingleSignOn do
 
   end
 
+  context 'when sso_overrides_avatar is not enabled' do
+
+
+    it "correctly handles provided avatar_urls" 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"
+
+      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)
+
+      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)
+
+      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)
+
+      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)
+
+      # we better have the same avatar
+      expect(user.uploaded_avatar_id).to eq(avatar_id)
+    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) {
@@ -292,20 +338,27 @@ describe DiscourseSingleSignOn do
     end
 
     it "deal with no avatar url passed for an existing user with an avatar" do
-      # Deliberately not setting avatar_url.
+      # 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)
+
       expect(user).to_not be_nil
+      expect(user.uploaded_avatar_id).to eq(-1)
     end
 
     it "deal with no avatar_force_update passed as a boolean" do
       FileHelper.stubs(:download).returns(logo)
 
+      sso_record.user.update_columns(uploaded_avatar_id: -1)
+
       sso.avatar_url = "http://example.com/a_different_image.png"
-      sso.avatar_force_update = true
+      sso.avatar_force_update = false
 
       user = sso.lookup_or_create_user(ip_address)
+
       expect(user).to_not be_nil
+      expect(user.uploaded_avatar_id).to_not eq(-1)
     end
   end
 end