diff --git a/lib/auth/managed_authenticator.rb b/lib/auth/managed_authenticator.rb index a7753050b8c..f42fe720128 100644 --- a/lib/auth/managed_authenticator.rb +++ b/lib/auth/managed_authenticator.rb @@ -77,17 +77,6 @@ class Auth::ManagedAuthenticator < Auth::Authenticator # Save to the DB. Do this even if we don't have a user - it might be linked up later in after_create_account association.save! - # Update the user's email address from the auth payload - if association.user && - (always_update_user_email? || association.user.email.end_with?(".invalid")) && - primary_email_verified?(auth_token) && - (email = auth_token.dig(:info, :email)) && - (email != association.user.email) && - !User.find_by_email(email) - - association.user.update!(email: email) - end - # Update avatar/profile retrieve_avatar(association.user, association.info["image"]) retrieve_profile(association.user, association.info) @@ -104,6 +93,7 @@ class Auth::ManagedAuthenticator < Auth::Authenticator end result.username = info[:nickname] result.email_valid = primary_email_verified?(auth_token) if result.email.present? + result.overrides_email = always_update_user_email? result.extra_data = { provider: auth_token[:provider], uid: auth_token[:uid] diff --git a/lib/auth/result.rb b/lib/auth/result.rb index 74dc509e57f..1a3efcf3031 100644 --- a/lib/auth/result.rb +++ b/lib/auth/result.rb @@ -15,14 +15,16 @@ class Auth::Result :requires_invite, :not_allowed_from_ip_address, :admin_not_allowed_from_ip_address, - :omit_username, # Used by plugins to prevent username edits :skip_email_validation, :destination_url, :omniauth_disallow_totp, :failed, :failed_reason, :failed_code, - :associated_groups + :associated_groups, + :overrides_email, + :overrides_username, + :overrides_name, ] attr_accessor *ATTRIBUTES @@ -33,12 +35,14 @@ class Auth::Result :email, :username, :email_valid, - :omit_username, :name, :authenticator_name, :extra_data, :skip_email_validation, - :associated_groups + :associated_groups, + :overrides_email, + :overrides_username, + :overrides_name, ] def [](key) @@ -79,16 +83,19 @@ class Auth::Result def apply_user_attributes! change_made = false - if SiteSetting.auth_overrides_username? && username.present? + if (SiteSetting.auth_overrides_username? || overrides_username) && username.present? change_made = UsernameChanger.override(user, username) end - if SiteSetting.auth_overrides_email && email_valid && email.present? && user.email != Email.downcase(email) + if (SiteSetting.auth_overrides_email || overrides_email || user&.email&.ends_with?(".invalid")) && + email_valid && + email.present? && + user.email != Email.downcase(email) user.email = email change_made = true end - if SiteSetting.auth_overrides_name && name.present? && user.name != name + if (SiteSetting.auth_overrides_name || overrides_name) && name.present? && user.name != name user.name = name change_made = true end @@ -120,11 +127,11 @@ class Auth::Result end def can_edit_name - !SiteSetting.auth_overrides_name + !(SiteSetting.auth_overrides_name || overrides_name) end def can_edit_username - !(SiteSetting.auth_overrides_username || omit_username) + !(SiteSetting.auth_overrides_username || overrides_username) end def to_client_hash diff --git a/spec/components/auth/managed_authenticator_spec.rb b/spec/components/auth/managed_authenticator_spec.rb index 2f6e371a0ec..1c04b910f21 100644 --- a/spec/components/auth/managed_authenticator_spec.rb +++ b/spec/components/auth/managed_authenticator_spec.rb @@ -222,35 +222,6 @@ describe Auth::ManagedAuthenticator do end end - describe "email update" do - fab!(:user) { Fabricate(:user) } - let!(:associated) { UserAssociatedAccount.create!(user: user, provider_name: 'myauth', provider_uid: "1234") } - - it "updates the user's email if currently invalid" do - user.update!(email: "someemail@discourse.org") - # Existing email is valid, do not change - expect { result = authenticator.after_authenticate(hash) } - .not_to change { user.reload.email } - - user.update!(email: "someemail@discourse.invalid") - # Existing email is invalid, expect change - expect { result = authenticator.after_authenticate(hash) } - .to change { user.reload.email } - - expect(user.email).to eq("awesome@example.com") - end - - it "doesn't raise error if email is taken" do - other_user = Fabricate(:user, email: "awesome@example.com") - user.update!(email: "someemail@discourse.invalid") - - expect { result = authenticator.after_authenticate(hash) } - .not_to change { user.reload.email } - - expect(user.email).to eq("someemail@discourse.invalid") - end - end - describe "avatar on create" do fab!(:user) { Fabricate(:user) } let!(:association) { UserAssociatedAccount.create!(provider_name: 'myauth', provider_uid: "1234") } diff --git a/spec/lib/auth/result_spec.rb b/spec/lib/auth/result_spec.rb new file mode 100644 index 00000000000..3c39cf7a944 --- /dev/null +++ b/spec/lib/auth/result_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true +require 'rails_helper' + +describe Auth::Result do + fab!(:initial_email) { "initialemail@example.org" } + fab!(:initial_username) { "initialusername" } + fab!(:initial_name) { "Initial Name" } + fab!(:user) { Fabricate(:user, email: initial_email, username: initial_username, name: initial_name) } + + let(:new_email) { "newemail@example.org" } + let(:new_username) { "newusername" } + let(:new_name) { "New Name" } + + let(:result) do + result = Auth::Result.new + result.email = new_email + result.username = new_username + result.name = new_name + result.user = user + result.email_valid = true + result + end + + it "doesn't override user attributes by default" do + result.apply_user_attributes! + expect(user.email).to eq(initial_email) + expect(user.username).to eq(initial_username) + expect(user.name).to eq(initial_name) + end + + it "overrides user attributes when site settings enabled" do + SiteSetting.email_editable = false + SiteSetting.auth_overrides_email = true + SiteSetting.auth_overrides_name = true + SiteSetting.auth_overrides_username = true + + result.apply_user_attributes! + + expect(user.email).to eq(new_email) + expect(user.username).to eq(new_username) + expect(user.name).to eq(new_name) + end + + it "overrides user attributes when result attributes set" do + result.overrides_email = true + result.overrides_name = true + result.overrides_username = true + + result.apply_user_attributes! + + expect(user.email).to eq(new_email) + expect(user.username).to eq(new_username) + expect(user.name).to eq(new_name) + end + + it "updates the user's email if currently invalid" do + user.update!(email: "someemail@discourse.org") + expect { result.apply_user_attributes! }.not_to change { user.email } + + user.update!(email: "someemail@discourse.invalid") + expect { result.apply_user_attributes! }.to change { user.email } + + expect(user.email).to eq(new_email) + end +end diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index bebacda2330..f03ec5b49b0 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -411,7 +411,7 @@ RSpec.describe Users::OmniauthCallbacksController do expect(user.confirm_password?("securepassword")).to eq(false) end - it "should update name/username/email when sso_overrides is enabled" do + it "should update name/username/email when SiteSetting.auth_overrides_* are enabled" do SiteSetting.email_editable = false SiteSetting.auth_overrides_email = true SiteSetting.auth_overrides_name = true