FIX: extract and fix overriding of usernames by external auth (#14637)

app/models/discourse_single_sign_on.rb

@@ -323,11 +323,7 @@ class DiscourseSingleSignOn < SingleSignOn
     end
 
     if SiteSetting.auth_overrides_username? && username.present?
-      if user.username.downcase == username.downcase
-        user.username = username # there may be a change of case
-      elsif user.username != UserNameSuggester.fix_username(username)
-        user.username = UserNameSuggester.suggest(username)
-      end
+      UsernameChanger.override(user, username)
     end
 
     if SiteSetting.auth_overrides_name && user.name != name && name.present?

app/models/user.rb

@@ -1455,6 +1455,10 @@ class User < ActiveRecord::Base
     seen_since?(30.days.ago)
   end
 
+  def username_equals_to?(another_username)
+    username_lower == User.normalize_username(another_username)
+  end
+
   protected
 
   def badge_grant

app/services/username_changer.rb

@@ -13,7 +13,23 @@ class UsernameChanger
     self.new(user, new_username, actor).change
   end
 
+  def self.override(user, new_username)
+    if user.username_equals_to?(new_username)
+      # override anyway since case could've been changed:
+      UsernameChanger.change(user, new_username, user)
+      true
+    elsif user.username != UserNameSuggester.fix_username(new_username)
+      suggested_username = UserNameSuggester.suggest(new_username)
+      UsernameChanger.change(user, suggested_username, user)
+      true
+    else
+      false
+    end
+  end
+
   def change(asynchronous: true, run_update_job: true)
+    return false if @user.username == @new_username
+
     @user.username = @new_username
 
     if @user.save

lib/auth/result.rb

@@ -77,9 +77,8 @@ class Auth::Result
 
   def apply_user_attributes!
     change_made = false
-    if SiteSetting.auth_overrides_username? && username.present? && UserNameSuggester.fix_username(username) != user.username
-      user.username = UserNameSuggester.suggest(username)
-      change_made = true
+    if SiteSetting.auth_overrides_username? && username.present?
+      change_made = UsernameChanger.override(user, username)
     end
 
     if SiteSetting.auth_overrides_email && email_valid && email.present? && user.email != Email.downcase(email)

spec/models/user_spec.rb

@@ -2601,4 +2601,30 @@ describe User do
       expect(user.invited_by).to eq(invite.invited_by)
     end
   end
+
+  describe "#username_equals_to?" do
+    [
+      ["returns true for equal usernames", "john", "john", true],
+      ["returns false for different usernames", "john", "bill", false],
+      ["considers usernames that are different only in case as equal", "john", "JoHN", true]
+    ].each do |testcase_name, current_username, another_username, is_equal|
+      it "#{testcase_name}" do
+        user = Fabricate(:user, username: current_username)
+        result = user.username_equals_to?(another_username)
+
+        expect(result).to be(is_equal)
+      end
+    end
+
+    it "considers usernames that are equal after unicode normalization as equal" do
+      SiteSetting.unicode_usernames = true
+
+      raw = "Lo\u0308we" # Löwe, u0308 stands for ¨, so o\u0308 adds up to ö
+      normalized = "l\u00F6we" # Löwe normilized, \u00F6 stands for ö
+      user = Fabricate(:user, username: normalized)
+      result = user.username_equals_to?(raw)
+
+      expect(result).to be(true)
+    end
+  end
 end

spec/services/username_changer_spec.rb

@@ -12,9 +12,10 @@ describe UsernameChanger do
 
     context 'success' do
       let!(:old_username) { user.username }
-      let(:new_username) { "#{user.username}1234" }
 
       it 'should change the username' do
+        new_username = "#{user.username}1234"
+
         events = DiscourseEvent.track_events {
           @result = UsernameChanger.change(user, new_username)
         }.last(2)
@@ -34,6 +35,17 @@ describe UsernameChanger do
         expect(user.username).to eq(new_username)
         expect(user.username_lower).to eq(new_username.downcase)
       end
+
+      it 'do nothing if the new username is the same' do
+        new_username = user.username
+
+        events = DiscourseEvent.track_events {
+          @result = UsernameChanger.change(user, new_username)
+        }
+
+        expect(@result).to eq(false)
+        expect(events.count).to be_zero
+      end
     end
 
     context 'failure' do
@@ -591,4 +603,70 @@ describe UsernameChanger do
     end
   end
 
+  describe '#override' do
+    common_test_cases = [
+      [
+        "overrides the username if a new name is different",
+        "john", "bill", "bill", false
+      ],
+      [
+        "does not change the username if a new name is the same",
+        "john", "john", "john", false
+      ],
+      [
+        "overrides the username if a new name has different case",
+        "john", "JoHN", "JoHN", false
+      ]
+    ]
+
+    context "unicode_usernames is off" do
+      before do
+        SiteSetting.unicode_usernames = false
+      end
+
+      [
+        *common_test_cases,
+        [
+          "does not change the username if a new name after unicode normalization is the same",
+          "john", "john¥¥", "john"
+        ],
+      ].each do |testcase_name, current, new, overrode|
+        it "#{testcase_name}" do
+          user = Fabricate(:user, username: current)
+          UsernameChanger.override(user, new)
+          expect(user.username).to eq(overrode)
+        end
+      end
+
+      it "overrides the username with username suggestions in case the username is already taken" do
+        user = Fabricate(:user, username: "bill")
+        Fabricate(:user, username: "john")
+
+        UsernameChanger.override(user, "john")
+
+        expect(user.username).to eq("john1")
+      end
+    end
+
+    context "unicode_usernames is on" do
+      before do
+        SiteSetting.unicode_usernames = true
+      end
+
+      [
+        *common_test_cases,
+        [
+          "overrides the username if a new name after unicode normalization is different only in case",
+          "lo\u0308we", "L\u00F6wee", "L\u00F6wee"
+        ],
+      ].each do |testcase_name, current, new, overrode|
+        it "#{testcase_name}" do
+          user = Fabricate(:user, username: current)
+          UsernameChanger.override(user, new)
+          expect(user.username).to eq(overrode)
+        end
+      end
+    end
+  end
+
 end