FIX: auth incorrectly handles duplicate usernames (#15197)

app/services/username_changer.rb

@@ -19,7 +19,7 @@ class UsernameChanger
       UsernameChanger.change(user, new_username, user)
       true
     elsif user.username != UserNameSuggester.fix_username(new_username)
-      suggested_username = UserNameSuggester.suggest(new_username)
+      suggested_username = UserNameSuggester.suggest(new_username, user.username)
       UsernameChanger.change(user, suggested_username, user)
       true
     else

lib/user_name_suggester.rb

@@ -4,9 +4,10 @@ module UserNameSuggester
   GENERIC_NAMES = ['i', 'me', 'info', 'support', 'admin', 'webmaster', 'hello', 'mail', 'office', 'contact', 'team']
   LAST_RESORT_USERNAME = "user"
 
-  def self.suggest(name_or_email)
+  def self.suggest(name_or_email,  current_username = nil)
     name = parse_name_from_email(name_or_email)
-    find_available_username_based_on(name)
+    name = fix_username(name)
+    find_available_username_based_on(name, current_username)
   end
 
   def self.parse_name_from_email(name_or_email)
@@ -20,13 +21,21 @@ module UserNameSuggester
     name
   end
 
-  def self.find_available_username_based_on(name)
-    name = fix_username(name)
+  def self.find_available_username_based_on(name, current_username = nil)
     offset = nil
     i = 1
 
     attempt = name
-    until User.username_available?(attempt) || i > 100
+    normalized_attempt = User.normalize_username(attempt)
+
+    original_allowed_username = current_username
+    current_username = User.normalize_username(current_username) if current_username
+
+    until (
+      normalized_attempt == current_username ||
+        User.username_available?(attempt) ||
+        i > 100
+    )
 
       if offset.nil?
         normalized = User.normalize_username(name)
@@ -42,7 +51,8 @@ module UserNameSuggester
 
           params = {
             count: count + 10,
-            name: normalized
+            name: normalized,
+            allowed_normalized: current_username || ''
           }
 
           # increasing the search space a bit to allow for some extra noise
@@ -50,7 +60,11 @@ module UserNameSuggester
             WITH numbers AS (SELECT generate_series(1, :count) AS n)
 
             SELECT n FROM numbers
-            LEFT JOIN users ON (username_lower = :name || n::varchar)
+            LEFT JOIN users ON (
+              username_lower = :name || n::varchar
+            ) AND (
+              username_lower <> :allowed_normalized
+            )
             WHERE users.id IS NULL
             ORDER by n ASC
             LIMIT 1
@@ -68,15 +82,21 @@ module UserNameSuggester
 
       max_length = User.username_length.end - suffix.length
       attempt = "#{truncate(name, max_length)}#{suffix}"
+      normalized_attempt = User.normalize_username(attempt)
       i += 1
     end
 
-    until User.username_available?(attempt) || i > 200
+    until normalized_attempt == current_username || User.username_available?(attempt) || i > 200
       attempt = SecureRandom.hex[1..SiteSetting.max_username_length]
+      normalized_attempt = User.normalize_username(attempt)
       i += 1
     end
 
-    attempt
+    if current_username == normalized_attempt
+      original_allowed_username
+    else
+      attempt
+    end
   end
 
   def self.fix_username(name)

spec/components/user_name_suggester_spec.rb

@@ -118,6 +118,21 @@ describe UserNameSuggester do
       expect(UserNameSuggester.suggest('uuuuuuu_u')).to eq('uuuuuuu1')
     end
 
+    it 'preserves current username' do
+      # if several users have username "bill" on the external site,
+      # they will have usernames bill, bill1, bill2 etc in Discourse:
+      Fabricate(:user, username: "bill")
+      Fabricate(:user, username: "bill1")
+      Fabricate(:user, username: "bill2")
+      Fabricate(:user, username: "bill3")
+      Fabricate(:user, username: "bill4")
+
+      # the number should be preserved, bill3 should remain bill3
+      suggestion = UserNameSuggester.suggest("bill", "bill3")
+
+      expect(suggestion).to eq "bill3"
+    end
+
     context "with Unicode usernames disabled" do
       before { SiteSetting.unicode_usernames = false }
 

spec/models/discourse_single_sign_on_spec.rb

@@ -366,6 +366,30 @@ describe DiscourseSingleSignOn do
     expect(user.username).to eq "testuser"
   end
 
+  it 'should preserve username when several users login with the same username' do
+    SiteSetting.auth_overrides_username = true
+
+    # if several users have username "bill" on the external site,
+    # they will have usernames bill, bill1, bill2 etc in Discourse:
+    Fabricate(:user, username: "bill")
+    Fabricate(:user, username: "bill1")
+    Fabricate(:user, username: "bill2")
+    Fabricate(:user, username: "bill4")
+
+    # the number should be preserved during subsequent logins
+    # bill3 should remain bill3
+    sso = new_discourse_sso
+    sso.username = "bill3"
+    sso.email = "test@test.com"
+    sso.external_id = "100"
+    sso.lookup_or_create_user(ip_address)
+
+    sso.username = "bill"
+    user = sso.lookup_or_create_user(ip_address)
+
+    expect(user.username).to eq "bill3"
+  end
+
   it "doesn't use email as a source for username suggestions by default" do
     sso = new_discourse_sso
     sso.external_id = "100"

spec/requests/omniauth_callbacks_controller_spec.rb

@@ -467,6 +467,30 @@ RSpec.describe Users::OmniauthCallbacksController do
         expect(user.name).to eq('Some name')
       end
 
+      it "should preserve username when several users login with the same username" do
+        SiteSetting.auth_overrides_username = true
+
+        # if several users have username "bill" on the external site,
+        # they will have usernames bill, bill1, bill2 etc in Discourse:
+        Fabricate(:user, username: "bill")
+        Fabricate(:user, username: "bill1")
+        Fabricate(:user, username: "bill2")
+        Fabricate(:user, username: "bill4")
+
+        # the number should be preserved during subsequent logins
+        # bill3 should remain bill3
+        user.update!(username: 'bill3')
+
+        uid = "12345"
+        UserAssociatedAccount.create!(provider_name: "google_oauth2", user_id: user.id, provider_uid: uid)
+        mock_auth(user.email, "bill", uid)
+
+        get "/auth/google_oauth2/callback.json"
+
+        user.reload
+        expect(user.username).to eq('bill3')
+      end
+
       it "will not update email if not verified" do
         SiteSetting.email_editable = false
         SiteSetting.auth_overrides_email = true
@@ -932,10 +956,10 @@ RSpec.describe Users::OmniauthCallbacksController do
       end
     end
 
-    def mock_auth(email, nickname)
+    def mock_auth(email, nickname, uid = '12345')
       OmniAuth.config.mock_auth[:google_oauth2] = OmniAuth::AuthHash.new(
         provider: 'google_oauth2',
-        uid: '123545',
+        uid: uid,
         info: OmniAuth::AuthHash::InfoHash.new(
           email: email,
           nickname: nickname