DEV: Introduce Auth::Result API for overrides_* (#15378)

DEV: Introduce Auth::Result API for overrides_* (#15378)

This allows authenticators to instruct the Auth::Result to override attributes without using the general site settings. This provides an easy migration path for auth plugins which offer their own “overrides email”, “overrides username” or “overrides name” settings. With this new api, they can set overrides_* on the result object, and the attribute will be overriden regardless of the general site setting.

ManagedAuthenticator is updated to use this new API. Plugins which consume ManagedAuthenticator will instantly take advantage of this change.

diff --git a/lib/auth/managed_authenticator.rb b/lib/auth/managed_authenticator.rb
index a775305..f42fe72 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 74dc509..1a3efcf 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 2f6e371..1c04b91 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 0000000..3c39cf7
--- /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 bebacda..f03ec5b 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
 

[... diff too long, it was truncated ...]

GitHub sha: cdf4d7156ee6ec5469702ad1719677d03cc8b9b0

This commit appears in #15378 which was merged by davidtaylorhq.

@davidtaylorhq Just wondering what the thinking is in this approach with the ability of the user to edit their username? I’m updating an auth plugin to this scheme, and I previously prevented this manually. See

Currently in core this is dependent on the site setting