diff --git a/app/assets/stylesheets/common/base/login.scss b/app/assets/stylesheets/common/base/login.scss
index 6f75cf30dca..494dcfd9968 100644
--- a/app/assets/stylesheets/common/base/login.scss
+++ b/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;
diff --git a/app/assets/stylesheets/desktop/login.scss b/app/assets/stylesheets/desktop/login.scss
index 188f6eff1a6..f2ec4bd06c8 100644
--- a/app/assets/stylesheets/desktop/login.scss
+++ b/app/assets/stylesheets/desktop/login.scss
@@ -97,7 +97,7 @@
.has-alt-auth {
.tip,
td label {
- max-width: 250px;
+ max-width: 100%;
}
}
diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb
index 4bcd8c8f5fd..fff323ae0c5 100644
--- a/app/controllers/users/omniauth_callbacks_controller.rb
+++ b/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
diff --git a/app/services/user_authenticator.rb b/app/services/user_authenticator.rb
index 1cc37613e67..d5fdb91dcdd 100644
--- a/app/services/user_authenticator.rb
+++ b/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
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 03d621d3de9..1b039512fa0 100644
--- a/config/locales/server.en.yml
+++ b/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."
diff --git a/lib/auth/result.rb b/lib/auth/result.rb
index 3596017e298..18dff598aa6 100644
--- a/lib/auth/result.rb
+++ b/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
diff --git a/lib/guardian/user_guardian.rb b/lib/guardian/user_guardian.rb
index 517c924b4a3..85cbffb547d 100644
--- a/lib/guardian/user_guardian.rb
+++ b/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)
diff --git a/lib/validators/sso_overrides_email_validator.rb b/lib/validators/sso_overrides_email_validator.rb
index c9636a0e99d..fd53f4fa2da 100644
--- a/lib/validators/sso_overrides_email_validator.rb
+++ b/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
diff --git a/spec/components/validators/sso_overrides_email_validator_spec.rb b/spec/components/validators/sso_overrides_email_validator_spec.rb
index fd38caf3f47..c4b0da34e16 100644
--- a/spec/components/validators/sso_overrides_email_validator_spec.rb
+++ b/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
diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb
index 78a82e51f5e..ed449baa3fd 100644
--- a/spec/requests/omniauth_callbacks_controller_spec.rb
+++ b/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)
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index b5a13429be8..e1e2e508e26 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/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