FIX: username suggester incorrectly skipping over whitelisted username

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.

diff --git a/lib/user_name_suggester.rb b/lib/user_name_suggester.rb
index bea9de6..38ee8be 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
+    normalized_attempt = User.normalize_username(attempt)
+
+    original_allowed_username = allowed_username
+    allowed_username = User.normalize_username(allowed_username) if allowed_username
 
-    until attempt == allowed_username || User.username_available?(attempt) || i > 100
+    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 ae9c88b..d319f78 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"

GitHub sha: 3d2c3bd4

1 Like