diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb index 5b553f14bf6..305116758c9 100644 --- a/app/models/discourse_single_sign_on.rb +++ b/app/models/discourse_single_sign_on.rb @@ -239,20 +239,10 @@ class DiscourseSingleSignOn < SingleSignOn DistributedMutex.synchronize("discourse_single_sign_on_#{email}") do user = User.find_by_email(email) if !require_activation if !user - try_name = name.presence - try_username = username.presence - - name_suggester_input = try_username - username_suggester_input = try_username || try_name - if SiteSetting.use_email_for_username_and_name_suggestions - name_suggester_input = name_suggester_input || email - username_suggester_input = username_suggester_input || email - end - user_params = { primary_email: UserEmail.new(email: email, primary: true), - name: try_name || User.suggest_name(name_suggester_input), - username: UserNameSuggester.suggest(username_suggester_input), + name: resolve_name, + username: resolve_username, ip_address: ip_address } @@ -381,4 +371,22 @@ class DiscourseSingleSignOn < SingleSignOn sso_record.external_profile_background_url = profile_background_url sso_record.external_card_background_url = card_background_url end + + def resolve_username + username_suggester_input = username.presence || name.presence + if SiteSetting.use_email_for_username_and_name_suggestions + username_suggester_input = username_suggester_input || email + end + + UserNameSuggester.suggest(username_suggester_input) + end + + def resolve_name + name_suggester_input = username.presence + if SiteSetting.use_email_for_username_and_name_suggestions + name_suggester_input = name_suggester_input || email + end + + name.presence || User.suggest_name(name_suggester_input) + end end diff --git a/spec/models/discourse_single_sign_on_spec.rb b/spec/models/discourse_single_sign_on_spec.rb index 51fdaff791b..260186267bd 100644 --- a/spec/models/discourse_single_sign_on_spec.rb +++ b/spec/models/discourse_single_sign_on_spec.rb @@ -390,6 +390,54 @@ describe DiscourseSingleSignOn do expect(user.username).to eq "bill3" end + it "uses name if it's present in payload" do + sso = new_discourse_sso + sso.external_id = "100" + + name = "John" + sso.name = name + sso.email = "mail@mail.com" + user = sso.lookup_or_create_user(ip_address) + + expect(user.name).to eq name + end + + it "uses username for name suggestions if name isn't present in payload" do + sso = new_discourse_sso + sso.external_id = "100" + + sso.name = "" + sso.username = "user_john" + sso.email = "mail@mail.com" + user = sso.lookup_or_create_user(ip_address) + + expect(user.name).to eq "User John" + end + + it "uses username for username suggestions if it's present in payload" do + sso = new_discourse_sso + sso.external_id = "100" + + username = "user_john" + sso.username = username + sso.email = "mail@mail.com" + user = sso.lookup_or_create_user(ip_address) + + expect(user.username).to eq username + end + + it "uses name for username suggestions if username isn't present in payload" do + sso = new_discourse_sso + sso.external_id = "100" + + sso.username = "" + sso.name = "John Smith" + sso.email = "mail@mail.com" + user = sso.lookup_or_create_user(ip_address) + + expect(user.username).to eq "John_Smith" + end + it "doesn't use email as a source for username suggestions by default" do sso = new_discourse_sso sso.external_id = "100" @@ -1027,4 +1075,38 @@ describe DiscourseSingleSignOn do end end + context "when user is staged" do + it "uses username of the staged user if username is not present in payload" do + staged_username = "staged_user" + email = "staged@user.com" + Fabricate(:user, staged: true, username: staged_username, email: email) + + sso = new_discourse_sso + sso.email = email + sso.external_id = "B" + + user = sso.lookup_or_create_user(ip_address) + user.reload + + expect(user.username).to eq(staged_username) + end + + it "uses username of the staged user if username in payload is the same" do + # it's important to check this, because we had regressions + # where usernames were changed to the same username with "1" added at the end + staged_username = "staged_user" + email = "staged@user.com" + Fabricate(:user, staged: true, username: staged_username, email: email) + + sso = new_discourse_sso + sso.username = staged_username + sso.email = email + sso.external_id = "B" + + user = sso.lookup_or_create_user(ip_address) + user.reload + + expect(user.username).to eq(staged_username) + end + end end diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index 8d2a0c2a5ad..bebacda2330 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -916,6 +916,9 @@ RSpec.describe Users::OmniauthCallbacksController do end it "should use username of the staged user if username in payload is the same" do + # it's important to check this, because we had regressions + # when usernames were changed to the same username with "1" added at the end + mock_auth(staged_user.email, staged_user.username) get "/auth/google_oauth2/callback.json"