Merge branch 'jej/group-saml-metadata-common-changes' into 'master'

Extract SAML refactoring from metadata MR

See merge request gitlab-org/gitlab-ee!7997

ee/app/models/saml_provider.rb

@@ -8,48 +8,68 @@ class SamlProvider < ActiveRecord::Base
 
   after_initialize :set_defaults, if: :new_record?
 
-  NAME_IDENTIFIER_FORMAT = 'urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified'.freeze
-
-  def assertion_consumer_service_url
-    "#{full_group_path}/-/saml/callback"
-  end
-
-  def issuer
-    full_group_path
-  end
-
-  def name_identifier_format
-    NAME_IDENTIFIER_FORMAT
-  end
+  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
-    {
-      assertion_consumer_service_url: assertion_consumer_service_url,
-      issuer: issuer,
+    defaults.to_h.merge(
       idp_cert_fingerprint: certificate_fingerprint,
-      idp_sso_target_url: sso_url,
-      name_identifier_format: name_identifier_format
-    }
+      idp_sso_target_url: sso_url
+    )
   end
 
-  private
+  def defaults
+    @defaults ||= DefaultOptions.new(group.full_path)
+  end
+
+  class DefaultOptions
+    NAME_IDENTIFIER_FORMAT = 'urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified'.freeze
+
+    def initialize(group_path)
+      @group_path = group_path
+    end
 
-  def full_group_path
-    "#{host}/groups/#{group.full_path}"
+    def name_identifier_format
+      NAME_IDENTIFIER_FORMAT
+    end
+
+    def full_group_path
+      "#{host}/groups/#{@group_path}"
+    end
+
+    def issuer
+      full_group_path
+    end
+
+    def assertion_consumer_service_url
+      "#{full_group_path}/-/saml/callback"
+    end
+
+    def to_h
+      {
+        assertion_consumer_service_url: assertion_consumer_service_url,
+        issuer: issuer,
+        name_identifier_format: name_identifier_format,
+        idp_sso_target_url_runtime_params: { redirect_to: :RelayState }
+      }
+    end
+
+    private
+
+    def host
+      @host ||= Gitlab.config.gitlab.url
+    end
   end
 
+  private
+
   def set_defaults
     self.enabled = true
   end
 
-  def host
-    @host ||= Gitlab.config.gitlab.url
-  end
-
   def strip_left_to_right_chars(input)
     input&.gsub(/[\u200E]/, '')
   end

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/gitlab/auth/group_saml/group_lookup.rb

@@ -33,9 +33,11 @@ module Gitlab
           path.match(path_regex).try(:[], :group)
         end
 
-        def path_from_params
-          params = Rack::Request.new(env).params
+        def params
+          @params ||= Rack::Request.new(env).params
+        end
 
+        def path_from_params
           params['group_path']
         end
       end

ee/lib/omni_auth/strategies/group_saml.rb

@@ -5,18 +5,13 @@ module OmniAuth
       option :callback_path, ->(env) { callback?(env) }
 
       def setup_phase
+        require_saml_provider
+
         # Set devise scope for custom callback URL
         env["devise.mapping"] = Devise.mappings[:user]
 
-        group_lookup = Gitlab::Auth::GroupSaml::GroupLookup.new(env)
-
-        unless group_lookup.group_saml_enabled?
-          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
@@ -34,6 +29,18 @@ module OmniAuth
       def self.callback?(env)
         env['PATH_INFO'] =~ Gitlab::PathRegex.saml_callback_regex
       end
+
+      private
+
+      def group_lookup
+        @group_lookup ||= Gitlab::Auth::GroupSaml::GroupLookup.new(env)
+      end
+
+      def require_saml_provider
+        unless group_lookup.group_saml_enabled?
+          self.class.invalid_group!(group_lookup.path)
+        end
+      end
     end
   end
 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

ee/spec/lib/omni_auth/strategies/group_saml_spec.rb

@@ -50,6 +50,13 @@ describe OmniAuth::Strategies::GroupSaml, type: :strategy do
 
         expect(auth_hash[:info]['email']).to eq("user@example.com")
       end
+
+      it 'sets omniauth setings from configured settings' do
+        post "/groups/my-group/-/saml/callback", SAMLResponse: saml_response
+
+        options = last_request.env['omniauth.strategy'].options
+        expect(options['idp_cert_fingerprint']).to eq fingerprint
+      end
     end
 
     context 'with invalid SAMLResponse' do