GroupSaml strategy falls back to default options

ee/app/models/saml_provider.rb

@@ -8,14 +8,21 @@ class SamlProvider < ActiveRecord::Base
 
   after_initialize :set_defaults, if: :new_record?
 
-  delegate :assertion_consumer_service_url, :issuer, :name_identifier_format, to: :settings
+  delegate :assertion_consumer_service_url, :issuer, :name_identifier_format, to: :defaults
 
   def certificate_fingerprint=(value)
     super(strip_left_to_right_chars(value))
   end
 
   def settings
-    ConfiguredOptions.new(self).to_h
+    defaults.to_h.merge(
+      idp_cert_fingerprint: certificate_fingerprint,
+      idp_sso_target_url: sso_url
+    )
+  end
+
+  def defaults
+    @defaults ||= DefaultOptions.new(group.full_path)
   end
 
   class DefaultOptions
@@ -45,7 +52,8 @@ class SamlProvider < ActiveRecord::Base
       {
         assertion_consumer_service_url: assertion_consumer_service_url,
         issuer: issuer,
-        name_identifier_format: name_identifier_format
+        name_identifier_format: name_identifier_format,
+        idp_sso_target_url_runtime_params: { redirect_to: :RelayState }
       }
     end
 
@@ -56,20 +64,6 @@ class SamlProvider < ActiveRecord::Base
     end
   end
 
-  class ConfiguredOptions < DefaultOptions
-    def initialize(saml_provider)
-      @group_path = saml_provider.group.full_path
-      @saml_provider = saml_provider
-    end
-
-    def to_h
-      super.merge(
-          idp_cert_fingerprint: @saml_provider.certificate_fingerprint,
-          idp_sso_target_url: @saml_provider.sso_url
-        )
-    end
-  end
-
   private
 
   def set_defaults

ee/lib/gitlab/auth/group_saml/dynamic_settings.rb

@@ -4,26 +4,26 @@ module Gitlab
       class DynamicSettings
         include Enumerable
 
-        delegate :each, :keys, :[], to: :settings
+        delegate :each, :keys, :[], to: :to_h
 
-        def initialize(saml_provider)
-          @saml_provider = saml_provider
+        def initialize(group)
+          @group = group
         end
 
-        def settings
-          @settings ||= configured_settings.merge(default_settings)
+        def to_h
+          return {} unless @group
+
+          configured_settings || default_settings
         end
 
         private
 
         def configured_settings
-          @saml_provider&.settings || {}
+          @configured_settings ||= @group.saml_provider&.settings
         end
 
         def default_settings
-          {
-            idp_sso_target_url_runtime_params: { redirect_to: :RelayState }
-          }
+          @default_settings ||= SamlProvider::DefaultOptions.new(@group.full_path).to_h
         end
       end
     end

ee/lib/omni_auth/strategies/group_saml.rb

@@ -14,9 +14,8 @@ module OmniAuth
           self.class.invalid_group!(group_lookup.path)
         end
 
-        saml_provider = group_lookup.saml_provider
-        dynamic_settings = Gitlab::Auth::GroupSaml::DynamicSettings.new(saml_provider)
-        env['omniauth.strategy'].options.merge!(dynamic_settings.settings)
+        settings = Gitlab::Auth::GroupSaml::DynamicSettings.new(group_lookup.group).to_h
+        env['omniauth.strategy'].options.merge!(settings)
 
         super
       end

ee/spec/lib/gitlab/auth/group_saml/dynamic_settings_spec.rb

@@ -2,13 +2,18 @@ require 'spec_helper'
 
 describe Gitlab::Auth::GroupSaml::DynamicSettings do
   let(:saml_provider) { create(:saml_provider) }
+  let(:group) { saml_provider.group }
 
-  subject { described_class.new(saml_provider) }
+  subject { described_class.new(group) }
 
-  it 'behaves like an enumerator for settings' do
+  it 'exposes a settings hash' do
     expect(subject.to_h).to be_a(Hash)
   end
 
+  it 'behaves like an enumerator for settings' do
+    expect(subject.to_a).to be_a(Array)
+  end
+
   it 'configures requests to transfrom redirect_to to RelayState' do
     expect(subject[:idp_sso_target_url_runtime_params]).to eq( redirect_to: :RelayState )
   end
@@ -34,4 +39,25 @@ describe Gitlab::Auth::GroupSaml::DynamicSettings do
       expect(subject.keys).to include(:name_identifier_format)
     end
   end
+
+  describe 'sets default settings without saml_provider' do
+    let(:group) { create(:group) }
+
+    specify 'assertion_consumer_service_url' do
+      expect(subject.keys).to include(:assertion_consumer_service_url)
+    end
+
+    specify 'issuer' do
+      expect(subject.keys).to include(:issuer)
+    end
+
+    specify 'name_identifier_format' do
+      expect(subject.keys).to include(:name_identifier_format)
+    end
+
+    it 'excludes configured keys' do
+      expect(subject.keys).not_to include(:idp_cert_fingerprint)
+      expect(subject.keys).not_to include(:idp_sso_target_url)
+    end
+  end
 end