From 47dd7d0bbed879625f6ff19960ff390956e7c331 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 15 Nov 2018 13:32:17 +0100 Subject: [PATCH 1/2] Use gitlab-default_value_with Rails 5 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This forks live at https://github.com/gitlabhq/default_value_for/tree/69-fix-action_controller-parameters-handling and fixes an issue where default_value_for wouldn't handle `ActionController::Parameters` correctly with Rails 5. This fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/54093. Signed-off-by: Rémy Coutable --- Gemfile | 13 ++++++++----- Gemfile.lock | 6 +++--- app/services/users/build_service.rb | 2 +- ...handle-actioncontroller-parameters-correctly.yml | 7 +++++++ spec/services/users/build_service_spec.rb | 2 +- 5 files changed, 20 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/54093-the-default_value_for-gem-doesn-t-handle-actioncontroller-parameters-correctly.yml diff --git a/Gemfile b/Gemfile index 2a228b326ad..fa7c0d12eba 100644 --- a/Gemfile +++ b/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' @@ -21,7 +20,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 diff --git a/Gemfile.lock b/Gemfile.lock index e21a1b85457..defcfe8e3a8 100644 --- a/Gemfile.lock +++ b/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) @@ -971,7 +971,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) @@ -1007,6 +1006,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) diff --git a/app/services/users/build_service.rb b/app/services/users/build_service.rb index 24ac20fdd29..3f503f3da28 100644 --- a/app/services/users/build_service.rb +++ b/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 diff --git a/changelogs/unreleased/54093-the-default_value_for-gem-doesn-t-handle-actioncontroller-parameters-correctly.yml b/changelogs/unreleased/54093-the-default_value_for-gem-doesn-t-handle-actioncontroller-parameters-correctly.yml new file mode 100644 index 00000000000..3d6fd2d065a --- /dev/null +++ b/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 diff --git a/spec/services/users/build_service_spec.rb b/spec/services/users/build_service_spec.rb index 17bc880dec5..b7b9817efdb 100644 --- a/spec/services/users/build_service_spec.rb +++ b/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 From 94d518f5d68b9bcead7564b6f8d42047cdf5d23b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roger=20Ru=CC=88ttimann?= Date: Thu, 15 Nov 2018 10:56:21 +0100 Subject: [PATCH 2/2] Add a test for the creation of an internal user by an admin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- spec/features/admin/admin_users_spec.rb | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/spec/features/admin/admin_users_spec.rb b/spec/features/admin/admin_users_spec.rb index d32f33ca1e2..f7c7a257075 100644 --- a/spec/features/admin/admin_users_spec.rb +++ b/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