Merge branch 'tancnle-avoid-leaky-states-in-saml-specs' into 'master'

Remove stubs that leak states across SAML related tests

See merge request gitlab-org/gitlab!64892

ee/spec/features/groups/saml_providers_spec.rb

@@ -3,16 +3,18 @@
 require 'spec_helper'
 
 RSpec.describe 'SAML provider settings' do
-  include CookieHelper
+  let_it_be_with_refind(:user) { create(:user) }
+  let_it_be_with_refind(:group) { create(:group) }
 
-  let(:user) { create(:user) }
-  let(:group) { create(:group) }
   let(:callback_path) { "/groups/#{group.path}/-/saml/callback" }
 
+  before_all do
+    group.add_owner(user)
+  end
+
   before do
     stub_default_url_options(protocol: "https")
     stub_saml_config
-    group.add_owner(user)
   end
 
   around(:all) do |example|
@@ -29,7 +31,7 @@ RSpec.describe 'SAML provider settings' do
 
   def stub_saml_config
     stub_licensed_features(group_saml: true)
-    allow(Devise).to receive(:omniauth_providers).and_return(%i(group_saml))
+    allow(Devise).to receive(:omniauth_providers).and_return(%i[group_saml])
   end
 
   describe 'settings' do
@@ -164,22 +166,22 @@ RSpec.describe 'SAML provider settings' do
 
       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
+        allow_next_instance_of(Gitlab::Auth::GroupSaml::ResponseStore) do |instance|
+          allow(instance).to receive(:get_raw).and_return(raw_saml_response)
         end
+
+        stub_const(
+          '::OmniAuth::Strategies::GroupSaml::VERIFY_SAML_RESPONSE',
+          group_saml_providers_path(group)
+        )
       end
 
-      it 'displays XML validation errors' do
+      it 'displays XML validation errors', :aggregate_failures do
         visit group_saml_providers_path(group)
 
         test_sso
 
-        expect(current_path).to eq group_saml_providers_path(group)
+        expect(page).to have_current_path(group_saml_providers_path(group))
         expect(page).to have_content("Fingerprint mismatch")
         expect(page).to have_content("The attributes have expired, based on the SessionNotOnOrAfter")
       end
@@ -199,7 +201,7 @@ RSpec.describe 'SAML provider settings' do
       it 'acts as if the group was not found' do
         visit sso_group_saml_providers_path(group)
 
-        expect(current_path).to eq(new_user_session_path)
+        expect(page).to have_current_path(new_user_session_path)
       end
 
       context 'as owner' do
@@ -210,7 +212,7 @@ RSpec.describe 'SAML provider settings' do
         it 'redirects to settings page with warning' do
           visit sso_group_saml_providers_path(group)
 
-          expect(current_path).to eq group_saml_providers_path(group)
+          expect(page).to have_current_path(group_saml_providers_path(group))
           expect(page).to have_content 'SAML sign on has not been configured for this group'
         end
       end

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

@@ -18,21 +18,13 @@ RSpec.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
     let(:callback_path) { OmniAuth::Strategies::GroupSaml.default_options[:callback_path] }
 
     def check(path)
-      callback_path.call( "PATH_INFO" => path )
+      callback_path.call("PATH_INFO" => path)
     end
 
     it 'dynamically detects /groups/:group_path/-/saml/callback' do
@@ -82,17 +74,25 @@ RSpec.describe OmniAuth::Strategies::GroupSaml, type: :strategy do
 
       context 'user is testing SAML response' do
         let(:relay_state) { ::OmniAuth::Strategies::GroupSaml::VERIFY_SAML_RESPONSE }
+        let(:mock_session) do
+          rack_session = Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d')
+          instance_spy(ActionDispatch::Request::Session, id: rack_session, '[]': {})
+        end
 
         it 'stores the saml response for retrieval after redirect' do
           expect_next_instance_of(::Gitlab::Auth::GroupSaml::ResponseStore) do |instance|
             allow(instance).to receive(:set_raw).with(saml_response)
           end
 
-          post "/groups/my-group/-/saml/callback", SAMLResponse: saml_response, RelayState: relay_state
+          post "/groups/my-group/-/saml/callback",
+               { SAMLResponse: saml_response, RelayState: relay_state },
+               'rack.session' => mock_session
         end
 
         it 'redirects back to the settings page' do
-          post "/groups/my-group/-/saml/callback", SAMLResponse: saml_response, RelayState: relay_state
+          post "/groups/my-group/-/saml/callback",
+               { SAMLResponse: saml_response, RelayState: relay_state },
+               'rack.session' => mock_session
 
           expect(last_response.location).to eq(group_saml_providers_path(group, anchor: 'response'))
         end
@@ -127,10 +127,11 @@ RSpec.describe OmniAuth::Strategies::GroupSaml, type: :strategy do
   end
 
   describe 'POST /users/auth/group_saml' do
-    it 'redirects to the provider login page' do
+    it 'redirects to the provider login page', :aggregate_failures do
       post '/users/auth/group_saml', group_path: 'my-group'
 
-      expect(last_response).to redirect_to(/#{Regexp.quote(idp_sso_url)}/)
+      expect(last_response.status).to eq(302)
+      expect(last_response.location).to match(/\A#{Regexp.quote(idp_sso_url)}/)
     end
 
     it 'returns 404 for groups without SAML configured' do

spec/initializers/100_patch_omniauth_saml_spec.rb

@@ -7,10 +7,11 @@ RSpec.describe 'OmniAuth::Strategies::SAML', type: :strategy do
   let(:strategy) { [OmniAuth::Strategies::SAML, { idp_sso_target_url: idp_sso_target_url }] }
 
   describe 'POST /users/auth/saml' do
-    it 'redirects to the provider login page' do
+    it 'redirects to the provider login page', :aggregate_failures do
       post '/users/auth/saml'
 
-      expect(last_response).to redirect_to(/\A#{Regexp.quote(idp_sso_target_url)}/)
+      expect(last_response.status).to eq(302)
+      expect(last_response.location).to match(/\A#{Regexp.quote(idp_sso_target_url)}/)
     end
 
     it 'stores request ID during request phase' do

spec/support/omniauth_strategy.rb

@@ -6,12 +6,6 @@ module StrategyHelpers
   include Shoulda::Matchers::ActionController
   include OmniAuth::Test::StrategyTestCase
 
-  def post(*args)
-    super.tap do
-      @response = ActionDispatch::TestResponse.from_response(last_response)
-    end
-  end
-
   def auth_hash
     last_request.env['omniauth.auth']
   end
@@ -21,7 +15,9 @@ module StrategyHelpers
     original_on_failure = OmniAuth.config.on_failure
 
     OmniAuth.config.test_mode = false
-    OmniAuth.config.on_failure = OmniAuth::FailureEndpoint
+    OmniAuth.config.on_failure = proc do |env|
+      OmniAuth::FailureEndpoint.new(env).redirect_to_failure
+    end
 
     yield
   ensure