From 19d95c64af9d38c7e34173f96745ae040ef0190e Mon Sep 17 00:00:00 2001 From: Andrei Prigorshnev Date: Wed, 27 Oct 2021 14:41:24 +0400 Subject: [PATCH] DEV: simplify username suggester (#14531) This PR doesn't change any behavior, but just removes code that wasn't in use. This is a pretty dangerous place to change, since it gets called during user's registration. At the same time the refactoring is very straightforward, it's clear that this code wasn't doing any work (it still needs to be double-checked during review though). Also, the test coverage of UserNameSuggester is good. --- app/models/discourse_single_sign_on.rb | 4 +- lib/auth/result.rb | 4 +- lib/user_name_suggester.rb | 37 +++--------- spec/components/user_name_suggester_spec.rb | 8 --- spec/models/discourse_single_sign_on_spec.rb | 61 +++++++++++++------- 5 files changed, 52 insertions(+), 62 deletions(-) diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb index e268d98b678..04812f13f4e 100644 --- a/app/models/discourse_single_sign_on.rb +++ b/app/models/discourse_single_sign_on.rb @@ -323,8 +323,8 @@ class DiscourseSingleSignOn < SingleSignOn 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 != username - user.username = UserNameSuggester.suggest(username || name || email, user.username) + elsif user.username != UserNameSuggester.fix_username(username) + user.username = UserNameSuggester.suggest(username) end end diff --git a/lib/auth/result.rb b/lib/auth/result.rb index 08a12203d2a..5f68606d2a1 100644 --- a/lib/auth/result.rb +++ b/lib/auth/result.rb @@ -77,8 +77,8 @@ class Auth::Result def apply_user_attributes! change_made = false - if SiteSetting.auth_overrides_username? && username.present? && username != user.username - user.username = UserNameSuggester.suggest(username_suggester_attributes, user.username) + if SiteSetting.auth_overrides_username? && username.present? && UserNameSuggester.fix_username(username) != user.username + user.username = UserNameSuggester.suggest(username) change_made = true end diff --git a/lib/user_name_suggester.rb b/lib/user_name_suggester.rb index 3a294c0c36e..dcb7877339e 100644 --- a/lib/user_name_suggester.rb +++ b/lib/user_name_suggester.rb @@ -4,9 +4,9 @@ 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, allowed_username = nil) + def self.suggest(name_or_email) name = parse_name_from_email(name_or_email) - find_available_username_based_on(name, allowed_username) + find_available_username_based_on(name) end def self.parse_name_from_email(name_or_email) @@ -20,22 +20,13 @@ module UserNameSuggester name end - def self.find_available_username_based_on(name, allowed_username = nil) + def self.find_available_username_based_on(name) 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 ( - normalized_attempt == allowed_username || - User.username_available?(attempt) || - i > 100 - ) + until User.username_available?(attempt) || i > 100 if offset.nil? normalized = User.normalize_username(name) @@ -51,8 +42,7 @@ module UserNameSuggester params = { count: count + 10, - name: normalized, - allowed_normalized: allowed_username || '' + name: normalized } # increasing the search space a bit to allow for some extra noise @@ -60,11 +50,7 @@ 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 - ) AND ( - username_lower <> :allowed_normalized - ) + LEFT JOIN users ON (username_lower = :name || n::varchar) WHERE users.id IS NULL ORDER by n ASC LIMIT 1 @@ -82,22 +68,15 @@ 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 normalized_attempt == allowed_username || User.username_available?(attempt) || i > 200 + until User.username_available?(attempt) || i > 200 attempt = SecureRandom.hex[1..SiteSetting.max_username_length] - normalized_attempt = User.normalize_username(attempt) i += 1 end - if allowed_username == normalized_attempt - original_allowed_username - else - attempt - end - + attempt 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 3adc48a6068..21a44b2c101 100644 --- a/spec/components/user_name_suggester_spec.rb +++ b/spec/components/user_name_suggester_spec.rb @@ -160,14 +160,6 @@ 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" diff --git a/spec/models/discourse_single_sign_on_spec.rb b/spec/models/discourse_single_sign_on_spec.rb index b9fdba6fb4c..ebc2d0861c7 100644 --- a/spec/models/discourse_single_sign_on_spec.rb +++ b/spec/models/discourse_single_sign_on_spec.rb @@ -265,27 +265,6 @@ describe DiscourseSingleSignOn do expect(add_group4.usernames).to eq(user.username) end - it 'can override username properly when only the case changes' do - SiteSetting.auth_overrides_username = true - - sso = new_discourse_sso - sso.username = "testuser" - sso.name = "test user" - sso.email = "test@test.com" - sso.external_id = "100" - sso.bio = "This **is** the bio" - sso.suppress_welcome_message = true - - # create the original user - user = sso.lookup_or_create_user(ip_address) - expect(user.username).to eq "testuser" - - # change the username case - sso.username = "TestUser" - user = sso.lookup_or_create_user(ip_address) - expect(user.username).to eq "TestUser" - end - it 'behaves properly when auth_overrides_username is set but username is missing or blank' do SiteSetting.auth_overrides_username = true @@ -347,6 +326,46 @@ describe DiscourseSingleSignOn do expect(admin.name).to eq "Louis C.K." end + it 'can override username properly when only the case changes' do + SiteSetting.auth_overrides_username = true + + sso = new_discourse_sso + sso.username = "testuser" + sso.name = "test user" + sso.email = "test@test.com" + sso.external_id = "100" + sso.bio = "This **is** the bio" + sso.suppress_welcome_message = true + + # create the original user + user = sso.lookup_or_create_user(ip_address) + expect(user.username).to eq "testuser" + + # change the username case + sso.username = "TestUser" + user = sso.lookup_or_create_user(ip_address) + expect(user.username).to eq "TestUser" + end + + it 'do not override username when a new username after fixing is the same' do + SiteSetting.auth_overrides_username = true + + sso = new_discourse_sso + sso.username = "testuser" + sso.name = "test user" + sso.email = "test@test.com" + sso.external_id = "100" + + # create the original user + user = sso.lookup_or_create_user(ip_address) + expect(user.username).to eq "testuser" + + # change the username case + sso.username = "testuserგამარჯობა" + user = sso.lookup_or_create_user(ip_address) + expect(user.username).to eq "testuser" + end + it "doesn't use email as a source for username suggestions by default" do sso = new_discourse_sso sso.external_id = "100"