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'