FEATURE: sso_overrides_(email|username|name) for all auth methods

These settings previously applied only to discourse-sso. Now they work for all external authentication methods.

app/assets/javascripts/discourse/app/controllers/create-account.js

@@ -2,7 +2,7 @@ import getURL from "discourse-common/lib/get-url";
 import I18n from "I18n";
 import { A } from "@ember/array";
 import { isEmpty } from "@ember/utils";
-import { notEmpty, or, not } from "@ember/object/computed";
+import { notEmpty, and } from "@ember/object/computed";
 import Controller, { inject as controller } from "@ember/controller";
 import { ajax } from "discourse/lib/ajax";
 import ModalFunctionality from "discourse/mixins/modal-functionality";
@@ -69,7 +69,8 @@ export default Controller.extend(
       return false;
     },
 
-    usernameRequired: not("authOptions.omit_username"),
+    usernameDisabled: and("authOptions", "!authOptions.can_edit_username"),
+    nameDisabled: and("authOptions", "!authOptions.can_edit_name"),
 
     @discourseComputed
     fullnameRequired() {

app/assets/javascripts/discourse/app/controllers/login.js

@@ -389,6 +389,6 @@ export default Controller.extend(ModalFunctionality, {
       authOptions: EmberObject.create(options)
     });
 
-    showModal("createAccount");
+    showModal("createAccount", { modalClass: "create-account" });
   }
 });

app/assets/javascripts/discourse/app/templates/modal/create-account.hbs

@@ -15,7 +15,11 @@
                 <tr class="input create-account-email">
                   <td class="label"><label for="new-account-email">{{i18n "user.email.title"}}</label></td>
                   <td>
-                    {{input type="email" value=accountEmail id="new-account-email" disabled=emailValidated name="email" autofocus="autofocus"}}
+                    {{#if emailValidated}}
+                      <span class="value">{{accountEmail}}</span>
+                    {{else}}
+                      {{input type="email" value=accountEmail id="new-account-email" name="email" autofocus="autofocus"}}
+                    {{/if}}
                   </td>
                 </tr>
 
@@ -25,19 +29,21 @@
                   <td><label>{{i18n "user.email.instructions"}}</label></td>
                 </tr>
 
-                {{#if usernameRequired}}
-                  <tr class="input">
-                    <td class="label"><label for="new-account-username">{{i18n "user.username.title"}}</label></td>
-                    <td>
+                <tr class="input">
+                  <td class="label"><label for="new-account-username">{{i18n "user.username.title"}}</label></td>
+                  <td>
+                    {{#if usernameDisabled}}
+                      <span class="value">{{accountUsername}}</span>
+                    {{else}}
                       {{input value=accountUsername id="new-account-username" name="username" maxlength=maxUsernameLength autocomplete="discourse"}}
-                    </td>
-                  </tr>
-                  <tr class="instructions">
-                    <td></td>
-                    {{input-tip validation=usernameValidation id="username-validation"}}
-                    <td><label>{{i18n "user.username.instructions"}}</label></td>
-                  </tr>
-                {{/if}}
+                    {{/if}}
+                  </td>
+                </tr>
+                <tr class="instructions">
+                  <td></td>
+                  {{input-tip validation=usernameValidation id="username-validation"}}
+                  <td><label>{{i18n "user.username.instructions"}}</label></td>
+                </tr>
 
                 {{#if fullnameRequired}}
                   <tr class="input">
@@ -45,7 +51,11 @@
                       <label for="new-account-name">{{i18n "user.name.title"}}</label>
                     </td>
                     <td>
-                      {{text-field value=accountName id="new-account-name"}}
+                      {{#if nameDisabled}}
+                        <span class="value">{{accountName}}</span>
+                      {{else}}
+                        {{text-field value=accountName id="new-account-name"}}
+                      {{/if}}
                     </td>
                   </tr>
                   <tr class="instructions">

app/assets/stylesheets/common/base/login.scss

@@ -69,6 +69,10 @@
     margin-top: 0.5em;
   }
 
+  tr.input span.value {
+    margin-left: 10px;
+  }
+
   .user-field {
     > label {
       margin-top: 0.75em;

app/assets/stylesheets/desktop/login.scss

@@ -97,7 +97,7 @@
 .has-alt-auth {
   .tip,
   td label {
-    max-width: 250px;
+    max-width: 100%;
   }
 }
 

app/controllers/users/omniauth_callbacks_controller.rb

@@ -137,6 +137,7 @@ class Users::OmniauthCallbacksController < ApplicationController
     elsif ScreenedIpAddress.block_admin_login?(user, request.remote_ip)
       @auth_result.admin_not_allowed_from_ip_address = true
     elsif Guardian.new(user).can_access_forum? && user.active # log on any account that is active with forum access
+      user.save! if @auth_result.apply_user_attributes!
       log_on_user(user)
       Invite.invalidate_for_email(user.email) # invite link can't be used to log in anymore
       session[:authentication] = nil # don't carry around old auth info, perhaps move elsewhere

app/services/user_authenticator.rb

@@ -6,7 +6,7 @@ class UserAuthenticator
     @user = user
     @session = session
     if session[:authentication] && session[:authentication].is_a?(Hash)
-      @auth_result = Auth::Result.from_session_data(session[:authentication])
+      @auth_result = Auth::Result.from_session_data(session[:authentication], user: user)
     end
     @authenticator_finder = authenticator_finder
   end
@@ -14,6 +14,7 @@ class UserAuthenticator
   def start
     if authenticated?
       @user.active = true
+      @auth_result.apply_user_attributes!
     else
       @user.password_required!
     end
@@ -38,7 +39,10 @@ class UserAuthenticator
   end
 
   def authenticated?
-    @auth_result && @auth_result.email.downcase == @user.email.downcase && @auth_result.email_valid.to_s == "true"
+    return false if !@auth_result
+    return false if @auth_result.email.downcase != @user.email.downcase
+    return false if @auth_result.email_valid != true # strong check for truth, in case we have another object type
+    true
   end
 
   private

config/locales/server.en.yml

@@ -1632,9 +1632,9 @@ en:
     sso_provider_secrets: "A list of domain-secret pairs that are using Discourse as an SSO provider. Make sure SSO secret is 10 characters or longer. Wildcard symbol * can be used to match any domain or only a part of it (e.g. *.example.com)."
     sso_overrides_bio: "Overrides user bio in user profile and prevents user from changing it"
     sso_overrides_groups: "Synchronize all manual group membership with groups specified in the groups sso attribute (WARNING: if you do not specify groups all manual group membership will be cleared for user)"
-    sso_overrides_email: "Overrides local email with external site email from SSO payload on every login, and prevent local changes. (WARNING: discrepancies can occur due to normalization of local emails)"
-    sso_overrides_username: "Overrides local username with external site username from SSO payload on every login, and prevent local changes. (WARNING: discrepancies can occur due to differences in username length/requirements)"
-    sso_overrides_name: "Overrides local full name with external site full name from SSO payload on every login, and prevent local changes."
+    sso_overrides_email: "Overrides local email with external site email from SSO payload on every login, and prevent local changes. Applies to all authentication providers. (WARNING: discrepancies can occur due to normalization of local emails)"
+    sso_overrides_username: "Overrides local username with external site username from SSO payload on every login, and prevent local changes.  Applies to all authentication providers. (WARNING: discrepancies can occur due to differences in username length/requirements)"
+    sso_overrides_name: "Overrides local full name with external site full name from SSO payload on every login, and prevent local changes.  Applies to all authentication providers."
     sso_overrides_avatar: "Overrides user avatar with external site avatar from SSO payload. If enabled, users will not be allowed to upload avatars on Discourse."
     sso_overrides_location: "Overrides user location with external location from SSO payload and prevent local changes."
     sso_overrides_website: "Overrides user website with external location from SSO payload and prevent local changes."
@@ -2244,7 +2244,6 @@ en:
       user_locale_not_enabled: "You must first enable 'allow user locale' before enabling this setting."
       invalid_regex: "Regex is invalid or not allowed."
       email_editable_enabled: "You must disable 'email editable' before enabling this setting."
-      enable_sso_disabled: "You must first enable 'enable sso' before enabling this setting."
       staged_users_disabled: "You must first enable 'staged users' before enabling this setting."
       reply_by_email_disabled: "You must first enable 'reply by email' before enabling this setting."
       sso_url_is_empty: "You must set a 'sso url' before enabling this setting."

lib/auth/result.rb

@@ -15,7 +15,7 @@ class Auth::Result
     :requires_invite,
     :not_allowed_from_ip_address,
     :admin_not_allowed_from_ip_address,
-    :omit_username,
+    :omit_username, # Used by plugins to prevent username edits
     :skip_email_validation,
     :destination_url,
     :omniauth_disallow_totp,
@@ -60,13 +60,42 @@ class Auth::Result
     SESSION_ATTRIBUTES.map { |att| [att, public_send(att)] }.to_h
   end
 
-  def self.from_session_data(data)
+  def self.from_session_data(data, user:)
     result = new
     data = data.symbolize_keys
     SESSION_ATTRIBUTES.each { |att| result.public_send("#{att}=", data[att]) }
+    result.user = user
     result
   end
 
+  def apply_user_attributes!
+    change_made = false
+    if SiteSetting.sso_overrides_username? && username.present? && username != user.username
+      user.username = UserNameSuggester.suggest(username || name || email, user.username)
+      change_made = true
+    end
+
+    if SiteSetting.sso_overrides_email && email_valid && email.present? && user.email != Email.downcase(email)
+      user.email = email
+      change_made = true
+    end
+
+    if SiteSetting.sso_overrides_name && name.present? && user.name != name
+      user.name = name
+      change_made = true
+    end
+
+    change_made
+  end
+
+  def can_edit_name
+    !SiteSetting.sso_overrides_name
+  end
+
+  def can_edit_username
+    !(SiteSetting.sso_overrides_username || omit_username)
+  end
+
   def to_client_hash
     if requires_invite
       return { requires_invite: true }
@@ -106,13 +135,15 @@ class Auth::Result
       username: UserNameSuggester.suggest(username || name || email),
       auth_provider: authenticator_name,
       email_valid: !!email_valid,
-      omit_username: !!omit_username
+      can_edit_username: can_edit_username,
+      can_edit_name: can_edit_name
     }
 
     result[:destination_url] = destination_url if destination_url.present?
 
     if SiteSetting.enable_names?
-      result[:name] = name.presence || User.suggest_name(username || email)
+      result[:name] = name.presence
+      result[:name] ||= User.suggest_name(username || email) if can_edit_name
     end
 
     result

lib/guardian/user_guardian.rb

@@ -23,7 +23,7 @@ module UserGuardian
   end
 
   def can_edit_username?(user)
-    return false if SiteSetting.sso_overrides_username? && SiteSetting.enable_sso?
+    return false if SiteSetting.sso_overrides_username?
     return true if is_staff?
     return false if SiteSetting.username_change_period <= 0
     return false if is_anonymous?
@@ -31,7 +31,7 @@ module UserGuardian
   end
 
   def can_edit_email?(user)
-    return false if SiteSetting.sso_overrides_email? && SiteSetting.enable_sso?
+    return false if SiteSetting.sso_overrides_email?
     return false unless SiteSetting.email_editable?
     return true if is_staff?
     return false if is_anonymous?
@@ -40,7 +40,7 @@ module UserGuardian
 
   def can_edit_name?(user)
     return false unless SiteSetting.enable_names?
-    return false if SiteSetting.sso_overrides_name? && SiteSetting.enable_sso?
+    return false if SiteSetting.sso_overrides_name?
     return true if is_staff?
     return false if is_anonymous?
     can_edit?(user)

lib/validators/sso_overrides_email_validator.rb

@@ -7,15 +7,12 @@ class SsoOverridesEmailValidator
 
   def valid_value?(val)
     return true if val == 'f'
-    return false if !SiteSetting.enable_sso?
     return false if SiteSetting.email_editable?
     true
   end
 
   def error_message
-    if !SiteSetting.enable_sso?
-      I18n.t('site_settings.errors.enable_sso_disabled')
-    elsif SiteSetting.email_editable?
+    if SiteSetting.email_editable?
       I18n.t('site_settings.errors.email_editable_enabled')
     end
   end

spec/components/validators/sso_overrides_email_validator_spec.rb

@@ -49,27 +49,5 @@ RSpec.describe SsoOverridesEmailValidator do
         end
       end
     end
-
-    describe "when 'enable sso' is false" do
-      before do
-        SiteSetting.enable_sso = false
-      end
-
-      describe 'when value is false' do
-        it 'should be valid' do
-          expect(subject.valid_value?('f')).to eq(true)
-        end
-      end
-
-      describe 'when value is true' do
-        it 'should not be valid' do
-          expect(subject.valid_value?('t')).to eq(false)
-
-          expect(subject.error_message).to eq(I18n.t(
-            'site_settings.errors.enable_sso_disabled'
-          ))
-        end
-      end
-    end
   end
 end

spec/requests/omniauth_callbacks_controller_spec.rb

@@ -206,7 +206,7 @@ RSpec.describe Users::OmniauthCallbacksController do
         expect(data["username"]).to eq("Some_Name")
         expect(data["auth_provider"]).to eq("google_oauth2")
         expect(data["email_valid"]).to eq(true)
-        expect(data["omit_username"]).to eq(false)
+        expect(data["can_edit_username"]).to eq(true)
         expect(data["name"]).to eq("Some Name")
         expect(data["destination_url"]).to eq(destination_url)
       end
@@ -229,7 +229,8 @@ RSpec.describe Users::OmniauthCallbacksController do
           uid: '123545',
           info: OmniAuth::AuthHash::InfoHash.new(
             email: user.email,
-            name: 'Some name'
+            name: 'Some name',
+            nickname: 'Somenickname'
           ),
           extra: {
             raw_info: OmniAuth::AuthHash.new(
@@ -347,6 +348,44 @@ 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
+        SiteSetting.email_editable = false
+        SiteSetting.sso_overrides_email = true
+        SiteSetting.sso_overrides_name = true
+        SiteSetting.sso_overrides_username = true
+
+        UserAssociatedAccount.create!(provider_name: "google_oauth2", user_id: user.id, provider_uid: '123545')
+
+        old_email = user.email
+        user.update!(name: 'somename', username: 'somusername', email: 'email@example.com')
+
+        get "/auth/google_oauth2/callback.json"
+        expect(response.status).to eq(302)
+
+        user.reload
+        expect(user.email).to eq(old_email)
+        expect(user.username).to eq('Somenickname')
+        expect(user.name).to eq('Some name')
+      end
+
+      it "will not update email if not verified" do
+        SiteSetting.email_editable = false
+        SiteSetting.sso_overrides_email = true
+
+        OmniAuth.config.mock_auth[:google_oauth2][:extra][:raw_info][:email_verified] = false
+
+        UserAssociatedAccount.create!(provider_name: "google_oauth2", user_id: user.id, provider_uid: '123545')
+
+        old_email = user.email
+        user.update!(email: 'email@example.com')
+
+        get "/auth/google_oauth2/callback.json"
+        expect(response.status).to eq(302)
+
+        user.reload
+        expect(user.email).to eq('email@example.com')
+      end
+
       context 'when user has TOTP enabled' do
         before do
           user.create_totp(enabled: true)

spec/requests/users_controller_spec.rb

@@ -984,7 +984,8 @@ describe UsersController do
             uid: '123545',
             info: OmniAuth::AuthHash::InfoHash.new(
               email: "osama@mail.com",
-              nickname: "testosama"
+              nickname: "testosama",
+              name: "Osama Test"
             )
           )
 
@@ -1036,6 +1037,24 @@ describe UsersController do
           json = response.parsed_body
           expect(json['success']).to eq(true)
         end
+
+        it "doesn't use provided username/name if sso_overrides is enabled" do
+          SiteSetting.sso_overrides_username = true
+          SiteSetting.sso_overrides_name = true
+          post "/u.json", params: {
+            username: "attemptednewname",
+            name: "Attempt At New Name",
+            password: "strongpassword",
+            email: "osama@mail.com"
+          }
+
+          expect(response.status).to eq(200)
+          json = response.parsed_body
+          expect(json['success']).to eq(true)
+          expect(User.last.username).to eq('testosama')
+          expect(User.last.name).to eq('Osama Test')
+        end
+
       end
     end