Ignore unknown OAuth sources in ApplicationSetting46783-removed-omniauth-provider-causing-invalid-application-setting

Signed-off-by: Rémy Coutable <remy@rymai.me>

4 files changed, 60 insertions, 17 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 3d58a14882f..bddeb8b0352 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -212,14 +212,6 @@ class ApplicationSetting < ActiveRecord::Base
     end
   end
 
-  validates_each :disabled_oauth_sign_in_sources do |record, attr, value|
-    value&.each do |source|
-      unless Devise.omniauth_providers.include?(source.to_sym)
-        record.errors.add(attr, "'#{source}' is not an OAuth sign-in source")
-      end
-    end
-  end
-
   validate :terms_exist, if: :enforce_terms?
 
   before_validation :ensure_uuid!
@@ -330,6 +322,11 @@ class ApplicationSetting < ActiveRecord::Base
     ::Gitlab::Database.cached_column_exists?(:application_settings, :sidekiq_throttling_enabled)
   end
 
+  def disabled_oauth_sign_in_sources=(sources)
+    sources = (sources || []).map(&:to_s) & Devise.omniauth_providers.map(&:to_s)
+    super(sources)
+  end
+
   def domain_whitelist_raw
     self.domain_whitelist&.join("\n")
   end
diff --git a/changelogs/unreleased/46783-removed-omniauth-provider-causing-invalid-application-setting.yml b/changelogs/unreleased/46783-removed-omniauth-provider-causing-invalid-application-setting.yml
new file mode 100644
index 00000000000..d5ecf5163d4
--- /dev/null
+++ b/changelogs/unreleased/46783-removed-omniauth-provider-causing-invalid-application-setting.yml
@@ -0,0 +1,5 @@
+---
+title: Ignore unknown OAuth sources in ApplicationSetting
+merge_request: 20129
+author:
+type: fixed
diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb
index e7aca94db66..f3ab4ff771a 100644
--- a/spec/features/admin/admin_settings_spec.rb
+++ b/spec/features/admin/admin_settings_spec.rb
@@ -124,6 +124,29 @@ feature 'Admin updates settings' do
     expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).not_to include('google_oauth2')
   end
 
+  scenario 'Oauth providers do not raise validation errors when saving unrelated changes' 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')
+
+    # Remove google_oauth2 from the Omniauth strategies
+    allow(Devise).to receive(:omniauth_providers).and_return([])
+
+    # Save an unrelated setting
+    page.within('.as-ci-cd') do
+      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')
+  end
+
   scenario 'Change Help page' do
     page.within('.as-help-page') do
       fill_in 'Help page text', with: 'Example text'
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 3e6656e0f12..02f74e2ea54 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -25,15 +25,6 @@ describe ApplicationSetting do
     it { is_expected.to allow_value(https).for(:after_sign_out_path) }
     it { is_expected.not_to allow_value(ftp).for(:after_sign_out_path) }
 
-    describe 'disabled_oauth_sign_in_sources validations' do
-      before do
-        allow(Devise).to receive(:omniauth_providers).and_return([:github])
-      end
-
-      it { is_expected.to allow_value(['github']).for(:disabled_oauth_sign_in_sources) }
-      it { is_expected.not_to allow_value(['test']).for(:disabled_oauth_sign_in_sources) }
-    end
-
     describe 'default_artifacts_expire_in' do
       it 'sets an error if it cannot parse' do
         setting.update(default_artifacts_expire_in: 'a')
@@ -314,6 +305,33 @@ describe ApplicationSetting do
     end
   end
 
+  describe '#disabled_oauth_sign_in_sources=' do
+    before do
+      allow(Devise).to receive(:omniauth_providers).and_return([:github])
+    end
+
+    it 'removes unknown sources (as strings) from the array' do
+      subject.disabled_oauth_sign_in_sources = %w[github test]
+
+      expect(subject).to be_valid
+      expect(subject.disabled_oauth_sign_in_sources).to eq ['github']
+    end
+
+    it 'removes unknown sources (as symbols) from the array' do
+      subject.disabled_oauth_sign_in_sources = %i[github test]
+
+      expect(subject).to be_valid
+      expect(subject.disabled_oauth_sign_in_sources).to eq ['github']
+    end
+
+    it 'ignores nil' do
+      subject.disabled_oauth_sign_in_sources = nil
+
+      expect(subject).to be_valid
+      expect(subject.disabled_oauth_sign_in_sources).to be_empty
+    end
+  end
+
   context 'restricted signup domains' do
     it 'sets single domain' do
       setting.domain_whitelist_raw = 'example.com'