Merge branch 'jej/group-saml-test-button-shows-response' into 'master'

"Verify SAML Configuration"  lets users see Response XML, NameID and validations

Closes #33714

See merge request gitlab-org/gitlab!18362

changelogs/unreleased/jej-group-saml-test-button-shows-response.yml

+---
+title: Users can verify SAML configuration and view SamlResponse XML
+merge_request: 18362
+author:
+type: added

ee/app/controllers/groups/saml_providers_controller.rb

@@ -10,6 +10,7 @@ class Groups::SamlProvidersController < Groups::ApplicationController
 
   def show
     @saml_provider = @group.saml_provider || @group.build_saml_provider
+    @saml_response_check = load_test_response if @saml_provider.persisted?
 
     scim_token = ScimOauthAccessToken.find_by_group_id(@group.id)
 
@@ -34,6 +35,13 @@ class Groups::SamlProvidersController < Groups::ApplicationController
 
   private
 
+  def load_test_response
+    test_response = Gitlab::Auth::GroupSaml::ResponseStore.new(session.id).get_raw
+    return if test_response.blank?
+
+    Gitlab::Auth::GroupSaml::ResponseCheck.for_group(group: @group, raw_response: test_response, user: current_user)
+  end
+
   def saml_provider_params
     allowed_params = %i[sso_url certificate_fingerprint enabled]
 

ee/app/views/groups/saml_providers/_response_debug.html.haml

+= bootstrap_form_for @saml_response_check.tap(&:valid?), url: '#', html: { class: 'gl-show-field-errors' } do |f|
+  - if f.object.valid?
+    .alert.alert-success
+      = s_('GroupSAML|Valid SAML Response')
+  = f.errors_on :xml_response, hide_attribute_name: true
+  = f.text_field :name_id, disabled: true, label: s_('GroupSAML|NameID'), label_class: 'label-bold', input_group_class: 'gl-field-error-anchor'
+  = f.text_field :name_id_format, disabled: true, label: s_('GroupSAML|NameID Format'), label_class: 'label-bold', input_group_class: 'gl-field-error-anchor'
+.file-holder
+  - indented_xml = Nokogiri.XML(@saml_response_check.xml).to_xml
+  .js-file-title.file-title
+    = s_("GroupSAML|SAML Response XML")
+    .file-actions
+      .btn-group
+        = clipboard_button(text: indented_xml, class: "btn btn-sm js-copy-blob-source-btn", title: s_('GroupSAML|Copy SAML Response XML'))
+  .file-content.code.js-syntax-highlight.qa-file-content
+    .blob-content
+      %pre.code.highlight
+        %code
+          = Gitlab::Highlight.highlight('response.xml', indented_xml, language: 'xml')

ee/app/views/groups/saml_providers/_test_button.html.haml

-= saml_link_for_provider _('Test SAML SSO'), saml_provider, redirect: request.url, html_class: "btn qa-saml-settings-test-button #{ 'd-none' unless saml_provider.persisted? }"
+= saml_link_for_provider _('Verify SAML Configuration'), saml_provider, redirect: ::OmniAuth::Strategies::GroupSaml::VERIFY_SAML_RESPONSE, html_class: "btn qa-saml-settings-test-button #{ 'd-none' unless saml_provider.persisted? }"

ee/app/views/groups/saml_providers/show.html.haml

@@ -16,6 +16,14 @@
       = s_('GroupSAML|Configuration')
   .col-lg-9
     = render 'form', group: @group, saml_provider: @saml_provider
+- if @saml_response_check
+  #response.pt-3
+    %section.row.border-top.mt-4
+      .col-lg-3.append-bottom-default
+        %h4.page-title
+          = s_('GroupSAML|SAML Response Output')
+      .col-lg-9
+        = render 'response_debug'
 %section.row.border-top.mt-4
   .col-lg-3.append-bottom-default
     %h4.page-title

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    module GroupSaml
+      class ResponseCheck
+        include ActiveModel::Model
+
+        attr_reader :xml_response, :identity
+        delegate :name_id, :name_id_format, :xml, to: :xml_response
+
+        validate :response_error_passthrough!
+        validates :name_id, presence: true
+        validate :name_id_matches_identity!
+        validate :name_id_format_persistent!
+        validate :name_id_randomly_generated!
+        validates :name_id_format, presence: true
+
+        def initialize(xml_response:, identity: nil)
+          @xml_response = xml_response
+          @identity = identity
+        end
+
+        def self.for_group(group:, raw_response:, user:)
+          identity = GroupSamlIdentityFinder.new(user: user).find_linked(group: group)
+          xml_response = XmlResponse.new(group: group, raw_response: raw_response)
+          self.new(xml_response: xml_response, identity: identity)
+        end
+
+        private
+
+        def response_error_passthrough!
+          return if xml_response.valid?
+
+          xml_response.errors.each do |message|
+            errors.add(:xml_response, message)
+          end
+        end
+
+        def name_id_matches_identity!
+          return unless name_id_changed?
+
+          message = s_('GroupSAML|must match stored NameID of "%{extern_uid}" as we use this to identify users. If the NameID changes users will be unable to sign in.') % { extern_uid: identity&.extern_uid }
+          errors.add(:name_id, message)
+        end
+
+        def name_id_format_persistent!
+          return if name_id_format.ends_with?(':persistent')
+          return if name_id_format.ends_with?(':emailAddress') && name_id_is_email?
+
+          errors.add(:name_id_format, s_('GroupSAML|should be "persistent"'))
+        end
+
+        def name_id_randomly_generated!
+          return unless name_id_is_new? && unreliable_name_id?
+
+          errors.add(:name_id, s_('GroupSAML|should be a random persistent ID, emails are discouraged'))
+        end
+
+        def unreliable_name_id?
+          name_id_is_email?
+        end
+
+        def name_id_is_email?
+          name_id.include?('@')
+        end
+
+        def name_id_is_new?
+          !name_id_from_identity || name_id_changed?
+        end
+
+        def name_id_changed?
+          name_id_from_identity && name_id != name_id_from_identity
+        end
+
+        def name_id_from_identity
+          identity&.extern_uid
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    module GroupSaml
+      class ResponseStore
+        STORAGE_KEY = 'last_saml_debug_response'.freeze
+        REDIS_EXPIRY_TIME = 5.minutes
+
+        attr_reader :session_id
+
+        def initialize(session_id)
+          @session_id = session_id
+        end
+
+        def set_raw(value)
+          Gitlab::Redis::SharedState.with { |redis| redis.set(redis_key, value, ex: REDIS_EXPIRY_TIME) }
+        end
+
+        def get_raw
+          Gitlab::Redis::SharedState.with do |redis|
+            response = redis.get(redis_key)
+            redis.del(redis_key)
+
+            response
+          end
+        end
+
+        private
+
+        def redis_key
+          "#{STORAGE_KEY}:#{session_id}"
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    module GroupSaml
+      class XmlResponse
+        attr_reader :saml_response
+
+        def initialize(group:, raw_response:)
+          settings = Gitlab::Auth::GroupSaml::DynamicSettings.new(group).to_h
+          @saml_response = OneLogin::RubySaml::Response.new(raw_response, settings: OneLogin::RubySaml::Settings.new(settings))
+        end
+
+        def errors
+          validate_all
+
+          saml_response.errors.to_set + (saml_response.decrypted_document&.errors || []) + (saml_response.document&.errors || [])
+        end
+
+        def valid?
+          validate_all
+        end
+
+        def name_id
+          saml_response.nameid
+        end
+
+        def name_id_format
+          saml_response.name_id_format
+        end
+
+        def xml
+          saml_response.response
+        end
+
+        private
+
+        def validate_all
+          # Pass true to detect multiple errors instead of
+          # raising an error on the first one
+          saml_response.is_valid?(true)
+        end
+      end
+    end
+  end
+end

ee/lib/omni_auth/strategies/group_saml.rb

@@ -5,6 +5,8 @@ module OmniAuth
     class GroupSaml < SAML
       extend ::Gitlab::Utils::Override
 
+      VERIFY_SAML_RESPONSE = 'VERIFY_SAML_RESPONSE'
+
       option :name, 'group_saml'
       option :callback_path, ->(env) { callback?(env) }
 
@@ -40,6 +42,26 @@ module OmniAuth
         end
       end
 
+      override :callback_phase
+      def callback_phase
+        return super unless bypass_signin_for_configuration_check?
+
+        store_saml_response
+        redirect("/groups/#{group_lookup.path}/-/saml#response")
+      end
+
+      def bypass_signin_for_configuration_check?
+        request.params['RelayState'] == VERIFY_SAML_RESPONSE
+      end
+
+      def store_saml_response
+        ::Gitlab::Auth::GroupSaml::ResponseStore.new(session_id).set_raw(request.params['SAMLResponse']) if session_id
+      end
+
+      def session_id
+        session.id
+      end
+
       def emulate_relay_state
         request.query_string.sub!('redirect_to', 'RelayState')
       end

ee/spec/features/groups/saml_providers_spec.rb

@@ -24,7 +24,7 @@ describe 'SAML provider settings' do
   end
 
   def test_sso
-    click_link('Test SAML SSO')
+    click_link('Verify SAML Configuration')
   end
 
   def stub_saml_config
@@ -142,18 +142,39 @@ describe 'SAML provider settings' do
 
     describe 'test button' do
       let!(:saml_provider) { create(:saml_provider, group: group) }
+      let(:raw_saml_response) do
+        fixture = File.read('ee/spec/fixtures/saml/response.xml')
+        Base64.encode64(fixture)
+      end
 
       before do
         mock_group_saml(uid: '123')
+
+        allow_any_instance_of(Gitlab::Auth::GroupSaml::ResponseStore).to receive(:get_raw).and_return(raw_saml_response)
+
+        allow_any_instance_of(OmniAuth::Strategies::GroupSaml).to receive(:mock_callback_call) do
+          response = Rack::Response.new
+          response.redirect(group_saml_providers_path(group))
+          response.finish
+        end
       end
 
-      it 'POSTs to the SSO path for the group' do
+      it 'displays XML validation errors' do
         visit group_saml_providers_path(group)
 
         test_sso
 
         expect(current_path).to eq group_saml_providers_path(group)
-        expect(page).to have_content("SAML for #{group.name} was added to your connected accounts")
+        expect(page).to have_content("Fingerprint mismatch")
+        expect(page).to have_content("The attributes have expired, based on the SessionNotOnOrAfter")
+      end
+
+      it 'displays SAML Response XML' do
+        visit group_saml_providers_path(group)
+
+        test_sso
+
+        expect(page).to have_content("<saml:Issuer>")
       end
     end
   end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Auth::GroupSaml::ResponseCheck do
+  describe 'validations' do
+    let(:name_id) { '123-456-789' }
+    let(:name_id_format) { 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent' }
+    let(:xml_validation_errors) { [] }
+    let(:xml_response) { double(:xml_response, name_id: name_id, name_id_format: name_id_format, valid?: xml_validation_errors.blank?, errors: xml_validation_errors) }
+
+    subject { described_class.new(xml_response: xml_response) }
+
+    before do
+      subject.valid?
+    end
+
+    context 'with blank NameID' do
+      let(:name_id) { '' }
+
+      it 'adds an error' do
+        expect(subject.errors[:name_id].join).to include('blank')
+      end
+    end
+
+    context "when NameID doesn't match the stored value" do
+      let(:identity) { double(:identity, extern_uid: '987') }
+
+      subject { described_class.new(identity: identity, xml_response: xml_response) }
+
+      it 'warns that NameID has changed and will break sign in' do
+        expect(subject.errors[:name_id].join).to include('must match stored NameID')
+        expect(subject.errors[:name_id].join).to include('unable to sign in')
+      end
+    end
+
+    context 'with non-persistent NameID Format' do
+      let(:name_id_format) { 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' }
+
+      it 'adds a warning' do
+        expect(subject.errors[:name_id_format].join).to include('persistent')
+      end
+    end
+
+    context 'with email for NameID and format' do
+      let(:name_id) { 'user@example.com' }
+      let(:name_id_format) { 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress' }
+
+      it "only warns on the NameID but not the format" do
+        expect(subject.errors[:name_id].join).to include('email')
+        expect(subject.errors[:name_id_format]).to be_blank
+      end
+
+      context 'with a stored NameID' do
+        let(:identity) { double(:identity, extern_uid: 'user@example.com') }
+
+        subject { described_class.new(identity: identity, xml_response: xml_response) }
+
+        it "doesn't warn because making changes will break SSO" do
+          expect(subject.errors).to be_blank
+        end
+      end
+    end
+
+    context 'with an invalid XML response' do
+      let(:xml_validation_errors) { ['Fingerprint mismatch'] }
+
+      it 'reuses the validation errors from ruby-saml' do
+        expect(subject.errors[:xml_response]).to eq xml_validation_errors
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Auth::GroupSaml::ResponseStore do
+  let(:raw_response) { '<xml></xml>' }
+  let(:session_id) { '123-456-789' }
+
+  subject { described_class.new(session_id) }
+
+  describe '#set_raw' do
+    it 'stores values in Redis' do
+      subject.set_raw(raw_response)
+
+      stored_value = Gitlab::Redis::SharedState.with do |redis|
+        redis.get("last_saml_debug_response:#{session_id}")
+      end
+
+      expect(stored_value).to eq raw_response
+    end
+
+    it 'sets a redis expiry time' do
+      Gitlab::Redis::SharedState.with do |redis|
+        expect(redis).to receive(:set).with(anything, anything, ex: 5.minutes)
+      end
+
+      subject.set_raw(raw_response)
+    end
+  end
+
+  describe '#get_raw' do
+    it 'retrives a value set by set_response' do
+      subject.set_raw(raw_response)
+
+      expect(subject.get_raw).to eq raw_response
+    end
+
+    it 'prevents memory bloat by deleting the value' do
+      subject.set_raw(raw_response)
+      subject.get_raw
+
+      expect(subject.get_raw).to be_nil
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Auth::GroupSaml::XmlResponse do
+  let(:saml_provider) { create(:saml_provider) }
+  let(:group) { saml_provider.group }
+  let(:raw_response) do
+    fixture = File.read('ee/spec/fixtures/saml/response.xml')
+    Base64.encode64(fixture)
+  end
+
+  subject { described_class.new(raw_response: raw_response, group: group) }
+
+  it 'configures ruby-saml using configured settings' do
+    expect(subject.saml_response.settings.idp_cert_fingerprint).to eq saml_provider.certificate_fingerprint
+  end
+
+  it 'validates xml according to SAML spec' do
+    expect(subject.errors).to include(/Current time is on or after NotOnOrAfter condition/)
+    expect(subject).not_to be_valid
+  end
+
+  it 'correctly detects fingerprint mismatch' do
+    expect(subject.errors).to include('Fingerprint mismatch')
+  end
+
+  describe 'attributes from encoded XML' do
+    let(:name_id) { '_1f6fcf6be5e13b08b1e3610e7ff59f205fbd814f23' }
+    let(:name_id_format) { 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' }
+
+    it 'retrieves NameID from XML' do
+      expect(subject.name_id).to eq name_id
+    end
+
+    it 'retrieves NameID Format from XML' do
+      expect(subject.name_id_format).to eq name_id_format
+    end
+
+    it 'provides decoded XML' do
+      expect(subject.xml).to start_with('<?xml')
+    end
+  end
+end

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

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe OmniAuth::Strategies::GroupSaml, type: :strategy do
+  include Gitlab::Routing
+
   let(:strategy) { [OmniAuth::Strategies::GroupSaml, {}] }
   let!(:group) { create(:group, name: 'my-group') }
   let(:idp_sso_url) { 'https://saml.example.com/adfs/ls' }
@@ -16,6 +18,14 @@ describe OmniAuth::Strategies::GroupSaml, type: :strategy do
 
   before do
     stub_licensed_features(group_saml: true)
+    fake_actiondispatch_request_session
+  end
+
+  def fake_actiondispatch_request_session
+    session = {}
+    session_id = 123
+    allow(session).to receive(:id).and_return(session_id)
+    env('rack.session', session)
   end
 
   describe 'callback_path option' do
@@ -67,6 +77,22 @@ describe OmniAuth::Strategies::GroupSaml, type: :strategy do
           post "/groups/my-group/-/saml/callback", SAMLResponse: saml_response
         end.to raise_error(ActionController::RoutingError)
       end
+
+      context 'user is testing SAML response' do
+        let(:relay_state) { ::OmniAuth::Strategies::GroupSaml::VERIFY_SAML_RESPONSE }
+
+        it 'stores the saml response for retrieval after redirect' do
+          expect_any_instance_of(::Gitlab::Auth::GroupSaml::ResponseStore).to receive(:set_raw).with(saml_response)
+
+          post "/groups/my-group/-/saml/callback", SAMLResponse: saml_response, RelayState: relay_state
+        end
+
+        it 'redirects back to the settings page' do
+          post "/groups/my-group/-/saml/callback", SAMLResponse: saml_response, RelayState: relay_state
+
+          expect(last_response.location).to eq(group_saml_providers_path(group, anchor: 'response'))
+        end
+      end
     end
 
     context 'with invalid SAMLResponse' do

locale/gitlab.pot

@@ -8637,6 +8637,9 @@ msgstr ""
 msgid "GroupSAML|Configuration"
 msgstr ""
 
+msgid "GroupSAML|Copy SAML Response XML"
+msgstr ""
+
 msgid "GroupSAML|Enable SAML authentication for this group."
 msgstr ""
 
@@ -8670,6 +8673,18 @@ msgstr ""
 msgid "GroupSAML|Members will be forwarded here when signing in to your group. Get this from your identity provider, where it can also be called \"SSO Service Location\", \"SAML Token Issuance Endpoint\", or \"SAML 2.0/W-Federation URL\"."
 msgstr ""
 
+msgid "GroupSAML|NameID"
+msgstr ""
+
+msgid "GroupSAML|NameID Format"
+msgstr ""
+
+msgid "GroupSAML|SAML Response Output"
+msgstr ""
+
+msgid "GroupSAML|SAML Response XML"
+msgstr ""
+
 msgid "GroupSAML|SAML Single Sign On"
 msgstr ""
 
@@ -8697,12 +8712,24 @@ msgstr ""
 msgid "GroupSAML|Toggle SAML authentication"
 msgstr ""
 
+msgid "GroupSAML|Valid SAML Response"
+msgstr ""
+
 msgid "GroupSAML|With group managed accounts enabled, all the users without a group managed account will be excluded from the group."
 msgstr ""
 
 msgid "GroupSAML|Your SCIM token"
 msgstr ""
 
+msgid "GroupSAML|must match stored NameID of \"%{extern_uid}\" as we use this to identify users. If the NameID changes users will be unable to sign in."
+msgstr ""
+
+msgid "GroupSAML|should be \"persistent\""
+msgstr ""
+
+msgid "GroupSAML|should be a random persistent ID, emails are discouraged"
+msgstr ""
+
 msgid "GroupSettings|Auto DevOps pipeline was updated for the group"
 msgstr ""
 
@@ -16925,9 +16952,6 @@ msgstr ""
 msgid "Terms of Service and Privacy Policy"
 msgstr ""
 
-msgid "Test SAML SSO"
-msgstr ""
-
 msgid "Test coverage parsing"
 msgstr ""
 
@@ -19139,6 +19163,9 @@ msgstr ""
 msgid "Verified"
 msgstr ""
 
+msgid "Verify SAML Configuration"
+msgstr ""
+
 msgid "Version"
 msgstr ""
 

qa/qa/specs/features/ee/browser_ui/1_manage/group/group_saml_sso_spec.rb

@@ -46,19 +46,32 @@ module QA
         end
 
         it 'Lets group admin test settings' do
+          incorrect_fingerprint = Digest::SHA1.hexdigest(rand.to_s)
           Page::Group::Menu.perform(&:go_to_saml_sso_group_settings)
 
           EE::Page::Group::Settings::SamlSSO.perform do |saml_sso|
             saml_sso.set_id_provider_sso_url(EE::Runtime::Saml.idp_sso_url)
-            saml_sso.set_cert_fingerprint(EE::Runtime::Saml.idp_certificate_fingerprint)
+            saml_sso.set_cert_fingerprint(incorrect_fingerprint)
             saml_sso.click_save_changes
 
             saml_sso.click_test_button
           end
 
-          login_to_idp_if_required_and_expect_success
+          login_to_idp_if_required
+
+          expect(page).to have_content("Verify SAML Configuration")
+          expect(page).to have_content("Fingerprint mismatch")
+          expect(page).to have_content("<saml:Issuer>#{QA::EE::Runtime::Saml.idp_issuer}</saml:Issuer>")
+
+          EE::Page::Group::Settings::SamlSSO.perform do |saml_sso|
+            saml_sso.set_cert_fingerprint(EE::Runtime::Saml.idp_certificate_fingerprint)
+            saml_sso.click_save_changes
 
-          expect(page).to have_content("Test SAML SSO")
+            saml_sso.click_test_button
+          end
+
+          expect(page).to have_content("Verify SAML Configuration")
+          expect(page).not_to have_content("Fingerprint mismatch")
         end
       end
 
@@ -235,8 +248,12 @@ module QA
       end
     end
 
-    def login_to_idp_if_required_and_expect_success
+    def login_to_idp_if_required
       Vendor::SAMLIdp::Page::Login.perform { |login_page| login_page.login_if_required('user1', 'user1pass') }
+    end
+
+    def login_to_idp_if_required_and_expect_success
+      login_to_idp_if_required
       expect(page).to have_content("SAML for #{Runtime::Env.sandbox_name} was added to your connected accounts")
                         .or have_content("Already signed in with SAML for #{Runtime::Env.sandbox_name}")
     end