From f3508065a3313735654def72872a08e825555bef Mon Sep 17 00:00:00 2001 From: Andrei Prigorshnev Date: Mon, 6 Dec 2021 17:49:04 +0100 Subject: [PATCH] FIX: auth incorrectly handles duplicate usernames (#15197) --- app/services/username_changer.rb | 2 +- lib/user_name_suggester.rb | 38 ++++++++++++++----- spec/components/user_name_suggester_spec.rb | 15 ++++++++ spec/models/discourse_single_sign_on_spec.rb | 24 ++++++++++++ .../omniauth_callbacks_controller_spec.rb | 28 +++++++++++++- 5 files changed, 95 insertions(+), 12 deletions(-) diff --git a/app/services/username_changer.rb b/app/services/username_changer.rb index fff781ab976..8cef5901b9a 100644 --- a/app/services/username_changer.rb +++ b/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 diff --git a/lib/user_name_suggester.rb b/lib/user_name_suggester.rb index dcb7877339e..ada89f05033 100644 --- a/lib/user_name_suggester.rb +++ b/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) diff --git a/spec/components/user_name_suggester_spec.rb b/spec/components/user_name_suggester_spec.rb index 21a44b2c101..7fb9a186bea 100644 --- a/spec/components/user_name_suggester_spec.rb +++ b/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 } diff --git a/spec/models/discourse_single_sign_on_spec.rb b/spec/models/discourse_single_sign_on_spec.rb index fdd34c579e6..51fdaff791b 100644 --- a/spec/models/discourse_single_sign_on_spec.rb +++ b/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" diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index 47754a28a80..eae07abf9ae 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/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