Don't reset application settings oauth providers by mistake

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

app/controllers/admin/application_settings_controller.rb

@@ -58,11 +58,14 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
   def application_setting_params
     params[:application_setting] ||= {}
 
-    enabled_oauth_sign_in_sources = params[:application_setting].delete(:enabled_oauth_sign_in_sources)
+    if params[:application_setting].key?(:enabled_oauth_sign_in_sources)
+      enabled_oauth_sign_in_sources = params[:application_setting].delete(:enabled_oauth_sign_in_sources)
+      enabled_oauth_sign_in_sources&.delete("")
 
-    params[:application_setting][:disabled_oauth_sign_in_sources] =
-      AuthHelper.button_based_providers.map(&:to_s) -
-      Array(enabled_oauth_sign_in_sources)
+      params[:application_setting][:disabled_oauth_sign_in_sources] =
+        AuthHelper.button_based_providers.map(&:to_s) -
+        Array(enabled_oauth_sign_in_sources)
+    end
 
     params[:application_setting][:import_sources]&.delete("")
     params[:application_setting][:restricted_visibility_levels]&.delete("")

app/helpers/application_settings_helper.rb

@@ -74,10 +74,12 @@ module ApplicationSettingsHelper
       css_class = 'btn'
       css_class << ' active' unless disabled
       checkbox_name = 'application_setting[enabled_oauth_sign_in_sources][]'
+      name = Gitlab::Auth::OAuth::Provider.label_for(source)
 
       label_tag(checkbox_name, class: css_class) do
         check_box_tag(checkbox_name, source, !disabled,
-                      autocomplete: 'off') + Gitlab::Auth::OAuth::Provider.label_for(source)
+                      autocomplete: 'off',
+                      id: name.tr(' ', '_')) + name
       end
     end
   end

app/views/admin/application_settings/_signin.html.haml

@@ -24,6 +24,7 @@
     - if omniauth_enabled? && button_based_providers.any?
       .form-group
         = f.label :enabled_oauth_sign_in_sources, 'Enabled OAuth sign-in sources', class: 'control-label col-sm-2'
+        = hidden_field_tag 'application_setting[enabled_oauth_sign_in_sources][]'
         .col-sm-10
           .btn-group{ data: { toggle: 'buttons' } }
             - oauth_providers_checkboxes.each do |source|

spec/features/admin/admin_settings_spec.rb

@@ -85,6 +85,26 @@ feature 'Admin updates settings' do
     expect(page).to have_content "Application settings saved successfully"
   end
 
+  scenario 'Modify oauth providers' do
+    expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).to be_empty
+
+    page.within('.as-signin') do
+      uncheck 'Google'
+      click_button 'Save changes'
+    end
+
+    expect(page).to have_content "Application settings saved successfully"
+    expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).to include('google_oauth2')
+
+    page.within('.as-signin') do
+      check "Google"
+      click_button 'Save changes'
+    end
+
+    expect(page).to have_content "Application settings saved successfully"
+    expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).not_to include('google_oauth2')
+  end
+
   scenario 'Change Help page' do
     page.within('.as-help-page') do
       fill_in 'Help page text', with: 'Example text'
@@ -234,16 +254,6 @@ feature 'Admin updates settings' do
     expect(find('#service_push_channel').value).to eq '#test_channel'
   end
 
-  context 'sign-in restrictions', :js do
-    it 'de-activates oauth sign-in source' do
-      page.within('.as-signin') do
-        find('input#application_setting_enabled_oauth_sign_in_sources_[value=gitlab]').send_keys(:return)
-
-        expect(find('.btn', text: 'GitLab.com')).not_to have_css('.active')
-      end
-    end
-  end
-
   scenario 'Change Keys settings' do
     page.within('.as-visibility-access') do
       select 'Are forbidden', from: 'RSA SSH keys'