From 3d2c3bd478474b6f179bd6c8052c503f9b800821 Mon Sep 17 00:00:00 2001
From: Sam Saffron <sam.saffron@gmail.com>
Date: Tue, 28 May 2019 16:48:30 +1000
Subject: [PATCH] FIX: username suggester incorrectly skipping over whitelisted
 username

SSO uses a special param to username suggester that whitelists a username
due to previous work we amended our lookup logic and started ignoring this
whitelist.

The fix ensures we always respect it, and also improves on the original
implementation that forgot to normalize the username.
---
 lib/user_name_suggester.rb                  | 41 ++++++++++++++++++---
 spec/components/user_name_suggester_spec.rb |  8 ++++
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/lib/user_name_suggester.rb b/lib/user_name_suggester.rb
index bea9de61b7c..38ee8be5ffd 100644
--- a/lib/user_name_suggester.rb
+++ b/lib/user_name_suggester.rb
@@ -25,9 +25,18 @@ module UserNameSuggester
     name = fix_username(name)
     offset = nil
     i = 1
-    attempt = name
 
-    until attempt == allowed_username || User.username_available?(attempt) || i > 100
+    attempt = name
+    normalized_attempt = User.normalize_username(attempt)
+
+    original_allowed_username = allowed_username
+    allowed_username = User.normalize_username(allowed_username) if allowed_username
+
+    until (
+      normalized_attempt == allowed_username ||
+      User.username_available?(attempt) ||
+      i > 100
+    )
 
       if offset.nil?
         normalized = User.normalize_username(name)
@@ -40,12 +49,23 @@ module UserNameSuggester
         SQL
 
         if count > 0
+
+          params = {
+            count: count + 10,
+            name: normalized,
+            allowed_normalized: allowed_username || ''
+          }
+
           # increasing the search space a bit to allow for some extra noise
-          available = DB.query_single(<<~SQL, count: count + 10, name: normalized).first
+          available = DB.query_single(<<~SQL, params).first
             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
@@ -60,16 +80,25 @@ module UserNameSuggester
       end
 
       suffix = (i + offset).to_s
+
       max_length = User.username_length.end - suffix.length
       attempt = "#{truncate(name, max_length)}#{suffix}"
+      normalized_attempt = User.normalize_username(attempt)
       i += 1
     end
 
-    until attempt == allowed_username || User.username_available?(attempt) || i > 200
+    until normalized_attempt == allowed_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 allowed_username == normalized_attempt
+      original_allowed_username
+    else
+      attempt
+    end
+
   end
 
   def self.fix_username(name)
diff --git a/spec/components/user_name_suggester_spec.rb b/spec/components/user_name_suggester_spec.rb
index ae9c88b760c..d319f78c3e6 100644
--- a/spec/components/user_name_suggester_spec.rb
+++ b/spec/components/user_name_suggester_spec.rb
@@ -149,6 +149,14 @@ describe UserNameSuggester do
         expect(UserNameSuggester.suggest('য়া')).to eq('য়া11')
       end
 
+      it "does not skip ove allowed names" do
+        Fabricate(:user, username: 'sam')
+        Fabricate(:user, username: 'saM1')
+        Fabricate(:user, username: 'sam2')
+
+        expect(UserNameSuggester.suggest('SaM', 'Sam1')).to eq('Sam1')
+      end
+
       it "normalizes usernames" do
         actual = 'Löwe'    # NFD, "Lo\u0308we"
         expected = 'Löwe'  # NFC, "L\u00F6we"