Merge branch '54093-the-default_value_for-gem-doesn-t-handle-actioncontroller-parameters-correctly' into 'master'

Replace the `default_value_for` gem with the `gitlab-default_value_with` gem which handles `ActionController::Parameters` correctly

Closes #54093

See merge request gitlab-org/gitlab-ce!23114

Gemfile

@@ -4,10 +4,9 @@ def rails5?
 end
 
 gem_versions = {}
-gem_versions['activerecord_sane_schema_dumper'] = rails5? ? '1.0'      : '0.2'
-gem_versions['default_value_for']               = rails5? ? '~> 3.0.5' : '~> 3.0.0'
-gem_versions['rails']                           = rails5? ? '5.0.7'    : '4.2.10'
-gem_versions['rails-i18n']                      = rails5? ? '~> 5.1'   : '~> 4.0.9'
+gem_versions['activerecord_sane_schema_dumper'] = rails5? ? '1.0'    : '0.2'
+gem_versions['rails']                           = rails5? ? '5.0.7'  : '4.2.10'
+gem_versions['rails-i18n']                      = rails5? ? '~> 5.1' : '~> 4.0.9'
 # --- The end of special code for migrating to Rails 5.0 ---
 
 source 'https://rubygems.org'
@@ -24,7 +23,11 @@ gem 'responders', '~> 2.0'
 gem 'sprockets', '~> 3.7.0'
 
 # Default values for AR models
-gem 'default_value_for', gem_versions['default_value_for']
+if rails5?
+  gem 'gitlab-default_value_for', '~> 3.1.1', require: 'default_value_for'
+else
+  gem 'default_value_for', '~> 3.0.0'
+end
 
 # Supported DBs
 gem 'mysql2', '~> 0.4.10', group: :mysql

Gemfile.lock

@@ -146,8 +146,6 @@ GEM
       html-pipeline
     declarative (0.0.10)
     declarative-option (0.1.0)
-    default_value_for (3.0.5)
-      activerecord (>= 3.2.0, < 5.2)
     descendants_tracker (0.0.4)
       thread_safe (~> 0.3, >= 0.3.1)
     device_detector (1.0.0)
@@ -277,6 +275,8 @@ GEM
     gitaly-proto (0.123.0)
       grpc (~> 1.0)
     github-markup (1.7.0)
+    gitlab-default_value_for (3.1.1)
+      activerecord (>= 3.2.0, < 6.0)
     gitlab-markup (1.6.5)
     gitlab-sidekiq-fetcher (0.3.0)
       sidekiq (~> 5)
@@ -972,7 +972,6 @@ DEPENDENCIES
   creole (~> 0.5.0)
   database_cleaner (~> 1.5.0)
   deckar01-task_list (= 2.0.0)
-  default_value_for (~> 3.0.5)
   device_detector
   devise (~> 4.4)
   devise-two-factor (~> 3.0.0)
@@ -1008,6 +1007,7 @@ DEPENDENCIES
   gettext_i18n_rails_js (~> 1.3)
   gitaly-proto (~> 0.123.0)
   github-markup (~> 1.7.0)
+  gitlab-default_value_for (~> 3.1.1)
   gitlab-markup (~> 1.6.5)
   gitlab-sidekiq-fetcher
   gitlab-styles (~> 2.4)

app/services/users/build_service.rb

@@ -28,7 +28,7 @@ module Users
 
       identity_attrs = params.slice(:extern_uid, :provider)
 
-      if identity_attrs.any?
+      unless identity_attrs.empty?
         user.identities.build(identity_attrs)
       end
 

changelogs/unreleased/54093-the-default_value_for-gem-doesn-t-handle-actioncontroller-parameters-correctly.yml

@@ -0,0 +1,7 @@
+---
+title: Fixes an issue where default values from models would override values set in
+  the interface (e.g. users would be set to external even though their emails matches
+  the internal email address pattern)
+merge_request: 23114
+author:
+type: fixed

spec/features/admin/admin_users_spec.rb

@@ -130,7 +130,7 @@ describe "Admin::Users" do
       context 'with regex to match internal user email address set', :js do
         before do
           stub_application_setting(user_default_external: true)
-          stub_application_setting(user_default_internal_regex: '.internal@')
+          stub_application_setting(user_default_internal_regex: '\.internal@')
 
           visit new_admin_user_path
         end
@@ -169,6 +169,22 @@ describe "Admin::Users" do
 
           expects_warning_to_be_hidden
         end
+
+        it 'creates an internal user' do
+          user_name = 'tester1'
+          fill_in 'user_email', with: 'test.internal@domain.ch'
+          fill_in 'user_name', with: 'tester1 name'
+          fill_in 'user_username', with: user_name
+
+          expects_external_to_be_unchecked
+          expects_warning_to_be_shown
+
+          click_button 'Create user'
+
+          new_user = User.find_by(username: user_name)
+
+          expect(new_user.external).to be_falsy
+        end
       end
     end
   end

spec/services/users/build_service_spec.rb

@@ -8,7 +8,7 @@ describe Users::BuildService do
 
     context 'with an admin user' do
       let(:admin_user) { create(:admin) }
-      let(:service) { described_class.new(admin_user, params) }
+      let(:service) { described_class.new(admin_user, ActionController::Parameters.new(params).permit!) }
 
       it 'returns a valid user' do
         expect(service.execute).to be_valid