From 9365c4b364f8d79dd62d6693d20310d640fcbb5e Mon Sep 17 00:00:00 2001 From: Andrei Prigorshnev Date: Thu, 16 Dec 2021 16:44:07 +0100 Subject: [PATCH] DEV: make sure we handle staged users correctly in DiscourseConnect (#15320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some time ago, we made this fix to external authentication – https://github.com/discourse/discourse/pull/13706. We didn't address Discourse Connect (https://meta.discourse.org/t/discourseconnect-official-single-sign-on-for-discourse-sso/13045) at that moment, so I wanted to fix it for Discourse Connect as well. Turned out though that Discourse Connect doesn't contain this problem and already handles staged users correctly. This PR adds tests that confirm it. Also, I've extracted two functions in Discourse Connect implementation along the way and decided to merge this refactoring too (the refactoring is supported with tests). --- app/models/discourse_single_sign_on.rb | 32 +++++--- spec/models/discourse_single_sign_on_spec.rb | 82 +++++++++++++++++++ .../omniauth_callbacks_controller_spec.rb | 3 + 3 files changed, 105 insertions(+), 12 deletions(-) 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"