Fixes #10946 - Make user email optional if email is disabled

Currently, users without email are asked to input one even if they do
not have email enabled on the Email preferrences tab. Also, the user
cannot submit a blank email as this too leads to an error message.

To verify this, you can create a new user without an  email and then
impersonate him or log in as the user. The behavior is the same for users
with internal and external authentication.

I saw there were attemps before to make the email optional (#9279), but it
seems that the behavior changed since this PR was merged. I am building up
on this previous effort.

Also resolves another Redmine issue 34666.

Since the mail is no longer required for user with external
authentication, newly created user will not be redirected to the user
edit page.

app/controllers/application_controller.rb

@@ -81,13 +81,14 @@ def set_gettext_locale_db
   end
 
   def require_mail
-    if User.current && !User.current.hidden? && User.current.mail.blank?
+    user = User.current
+    if user && !user.hidden? && user.mail_enabled && user.mail.blank?
       msg = _("An email address is required, please update your account details")
       respond_to do |format|
         format.html do
           error msg
           flash.keep # keep any warnings added by the user login process, they may explain why this occurred
-          redirect_to main_app.edit_user_path(:id => User.current)
+          redirect_to main_app.edit_user_path(:id => user)
         end
         format.text do
           render :plain => msg, :status => :unprocessable_entity, :content_type => Mime[:text]

app/models/user.rb

@@ -83,7 +83,7 @@ class User < ApplicationRecord
 
   validates :mail, :email => true, :allow_blank => true
   validates :mail, :presence => true, :on => :update,
-            :if => proc { |u| !AuthSourceHidden.where(:id => u.auth_source_id).any? && (u.mail_enabled || (User.current == u && !User.current.hidden?)) }
+            :if => proc { |u| !AuthSourceHidden.where(:id => u.auth_source_id).any? && u.mail_enabled }
 
   validates :locale, :format => { :with => /\A\w{2}([_-]\w{2})?\Z/ }, :allow_blank => true, :if => proc { |user| user.respond_to?(:locale) }
   before_validation :normalize_locale

app/services/sso/apache.rb

@@ -30,6 +30,7 @@ def support_fallback?
     def authenticated?
       return false unless (self.user = request.env[CAS_USERNAME])
       attrs = { :login => user }.merge(additional_attributes)
+      attrs[:mail] = nil if attrs[:mail] == "(null)"
       if request.env.has_key?('HTTP_REMOTE_USER_GROUPS')
         attrs[:groups] = []
         groups = request.env['HTTP_REMOTE_USER_GROUPS'].split(':')

test/controllers/application_controller_subclass_test.rb

@@ -62,9 +62,9 @@ class TestableResourcesControllerTest < ActionController::TestCase
       assert_equal '/realms', session[:original_uri]
     end
 
-    it "requires an account with mail" do
+    it "requires email to be set if the account has mail_enabled set to true" do
       as_admin do
-        @user = FactoryBot.create(:user)
+        @user = FactoryBot.create(:user, :mail_enabled => true)
       end
       get :index, session: set_session_user.merge(:user => @user.id)
       assert_response :redirect

test/controllers/unattended_controller_test.rb

@@ -201,10 +201,10 @@ class UnattendedControllerTest < ActionController::TestCase
       assert_response :redirect
     end
 
-    test "should not render a template to user w/o email" do
+    test "should not render a template to user w/o email if they have email enabled" do
       @rh_host.update(build: false)
 
-      user = FactoryBot.create(:user)
+      user = FactoryBot.create(:user, :mail_enabled => true)
       get :host_template, params: { :kind => 'PXELinux', :spoof => @rh_host.ip, :format => 'text' }, session: set_session_user(user)
       assert_response :unprocessable_entity
     end

test/controllers/users_controller_test.rb

@@ -271,8 +271,9 @@ class UsersControllerTest < ActionController::TestCase
     Setting['authorize_login_delegation_auth_source_user_autocreate'] = 'apache_mod'
     @request.session.clear
     @request.env['HTTP_REMOTE_USER'] = 'ares'
+    @request.env['HTTP_REMOTE_USER_EMAIL'] = '(null)'
     get :extlogin
-    assert_redirected_to edit_user_path(User.unscoped.find_by_login('ares'))
+    assert_redirected_to ApplicationHelper.current_hosts_path
   end
 
   test "should use intercept if available" do
@@ -393,7 +394,7 @@ class UsersControllerTest < ActionController::TestCase
   end
 
   test "#login shows a warning for any user model errors" do
-    attrs = {:firstname => "foo", :mail => "foo#bar", :login => "ldap-user", :auth_source_id => auth_sources(:one).id}
+    attrs = {:firstname => "foo", :mail => "foo#bar", :login => "ldap-user", :auth_source_id => auth_sources(:one).id, :mail_enabled => true}
     AuthSourceLdap.any_instance.stubs(:authenticate).returns(attrs)
     AuthSourceLdap.any_instance.stubs(:update_usergroups).returns(true)
     AuthSourceLdap.any_instance.stubs(:organizations).returns([taxonomies(:organization1)])

test/models/user_test.rb

@@ -157,18 +157,9 @@ def setup
     refute u.valid?
   end
 
-  test "mail is required for own user" do
-    user = FactoryBot.create(:user)
-    user.password = nil
-    # refute_valid user can check only one field and due to we need to set password to nil after adding current_password field to verify password change
-    as_user user do
-      refute_valid user, :mail
-    end
-  end
-
   test "hidden users don't need mail when updating" do
     u = User.anonymous_admin
-    u.firstname = 'Bob'
+    u.mail_enabled = true
     assert_valid u
   end