Merge branch 'sh-fix-saml-sso-redirect-not-working' into 'master'

[RUN AS-IF-FOSS] AS Fix SAML SSO login redirects not working

See merge request gitlab-org/gitlab!66791

app/controllers/admin/users_controller.rb

@@ -304,7 +304,7 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def user
-    @user ||= find_routable!(User, params[:id])
+    @user ||= find_routable!(User, params[:id], request.path_info)
   end
 
   def build_canonical_path(user)

app/controllers/concerns/project_unauthorized.rb

@@ -3,7 +3,7 @@
 module ProjectUnauthorized
   module ControllerActions
     def self.on_routable_not_found
-      lambda do |routable|
+      lambda do |routable, path_info|
         return unless routable.is_a?(Project)
 
         label = routable.external_authorization_classification_label

app/controllers/concerns/routable_actions.rb

@@ -3,13 +3,13 @@
 module RoutableActions
   extend ActiveSupport::Concern
 
-  def find_routable!(routable_klass, requested_full_path, extra_authorization_proc: nil)
-    routable = routable_klass.find_by_full_path(requested_full_path, follow_redirects: request.get?)
+  def find_routable!(routable_klass, routable_full_path, path_info, extra_authorization_proc: nil)
+    routable = routable_klass.find_by_full_path(routable_full_path, follow_redirects: request.get?)
     if routable_authorized?(routable, extra_authorization_proc)
-      ensure_canonical_path(routable, requested_full_path)
+      ensure_canonical_path(routable, routable_full_path)
       routable
     else
-      perform_not_found_actions(routable, not_found_actions)
+      perform_not_found_actions(routable, not_found_actions, path_info)
 
       route_not_found unless performed?
 
@@ -21,11 +21,11 @@ module RoutableActions
     [ProjectUnauthorized::ControllerActions.on_routable_not_found]
   end
 
-  def perform_not_found_actions(routable, actions)
+  def perform_not_found_actions(routable, actions, path_info)
     actions.each do |action|
       break if performed?
 
-      instance_exec(routable, &action)
+      instance_exec(routable, path_info, &action)
     end
   end
 
@@ -42,13 +42,13 @@ module RoutableActions
     end
   end
 
-  def ensure_canonical_path(routable, requested_full_path)
+  def ensure_canonical_path(routable, routable_full_path)
     return unless request.get?
 
     canonical_path = routable.full_path
-    if canonical_path != requested_full_path
-      if !request.xhr? && request.format.html? && canonical_path.casecmp(requested_full_path) != 0
-        flash[:notice] = "#{routable.class.to_s.titleize} '#{requested_full_path}' was moved to '#{canonical_path}'. Please update any links and bookmarks that may still have the old path."
+    if canonical_path != routable_full_path
+      if !request.xhr? && request.format.html? && canonical_path.casecmp(routable_full_path) != 0
+        flash[:notice] = "#{routable.class.to_s.titleize} '#{routable_full_path}' was moved to '#{canonical_path}'. Please update any links and bookmarks that may still have the old path."
       end
 
       redirect_to build_canonical_path(routable), status: :moved_permanently

app/controllers/groups/application_controller.rb

@@ -24,7 +24,7 @@ class Groups::ApplicationController < ApplicationController
   end
 
   def group
-    @group ||= find_routable!(Group, params[:group_id] || params[:id])
+    @group ||= find_routable!(Group, params[:group_id] || params[:id], request.path_info)
   end
 
   def group_projects

app/controllers/groups/clusters/applications_controller.rb

@@ -13,6 +13,6 @@ class Groups::Clusters::ApplicationsController < Clusters::ApplicationsControlle
   end
 
   def group
-    @group ||= find_routable!(Group, params[:group_id] || params[:id])
+    @group ||= find_routable!(Group, params[:group_id] || params[:id], request.path_info)
   end
 end

app/controllers/groups/clusters/integrations_controller.rb

@@ -13,6 +13,6 @@ class Groups::Clusters::IntegrationsController < Clusters::IntegrationsControlle
   end
 
   def group
-    @group ||= find_routable!(Group, params[:group_id] || params[:id])
+    @group ||= find_routable!(Group, params[:group_id] || params[:id], request.path_info)
   end
 end

app/controllers/groups/clusters_controller.rb

@@ -15,7 +15,7 @@ class Groups::ClustersController < Clusters::ClustersController
   end
 
   def group
-    @group ||= find_routable!(Group, params[:group_id] || params[:id])
+    @group ||= find_routable!(Group, params[:group_id] || params[:id], request.path_info)
   end
 
   def metrics_dashboard_params

app/controllers/profiles/groups_controller.rb

@@ -6,7 +6,7 @@ class Profiles::GroupsController < Profiles::ApplicationController
   feature_category :users
 
   def update
-    group = find_routable!(Group, params[:id])
+    group = find_routable!(Group, params[:id], request.path_info)
     notification_setting = current_user.notification_settings_for(group)
 
     if notification_setting.update(update_params)

app/controllers/projects/application_controller.rb

@@ -26,7 +26,7 @@ class Projects::ApplicationController < ApplicationController
     path = File.join(params[:namespace_id], params[:project_id] || params[:id])
     auth_proc = ->(project) { !project.pending_delete? }
 
-    @project = find_routable!(Project, path, extra_authorization_proc: auth_proc)
+    @project = find_routable!(Project, path, request.path_info, extra_authorization_proc: auth_proc)
   end
 
   def build_canonical_path(project)

app/controllers/projects/clusters/applications_controller.rb

@@ -10,6 +10,6 @@ class Projects::Clusters::ApplicationsController < Clusters::ApplicationsControl
   end
 
   def project
-    @project ||= find_routable!(Project, File.join(params[:namespace_id], params[:project_id]))
+    @project ||= find_routable!(Project, File.join(params[:namespace_id], params[:project_id]), request.path_info)
   end
 end

app/controllers/projects/clusters/integrations_controller.rb

@@ -10,6 +10,6 @@ class Projects::Clusters::IntegrationsController < ::Clusters::IntegrationsContr
   end
 
   def project
-    @project ||= find_routable!(Project, File.join(params[:namespace_id], params[:project_id]))
+    @project ||= find_routable!(Project, File.join(params[:namespace_id], params[:project_id]), request.path_info)
   end
 end

app/controllers/projects/clusters_controller.rb

@@ -17,7 +17,7 @@ class Projects::ClustersController < Clusters::ClustersController
   end
 
   def project
-    @project ||= find_routable!(Project, File.join(params[:namespace_id], params[:project_id]))
+    @project ||= find_routable!(Project, File.join(params[:namespace_id], params[:project_id]), request.path_info)
   end
 
   def repository

app/controllers/users_controller.rb

@@ -172,7 +172,7 @@ class UsersController < ApplicationController
   private
 
   def user
-    @user ||= find_routable!(User, params[:username])
+    @user ||= find_routable!(User, params[:username], request.path_info)
   end
 
   def personal_projects

ee/app/controllers/concerns/ee/routable_actions/sso_enforcement_redirect.rb

@@ -6,10 +6,11 @@ module EE
       include ::Gitlab::Routing
       include ::Gitlab::Utils::StrongMemoize
 
-      attr_reader :routable
+      attr_reader :routable, :path_info
 
-      def initialize(routable)
+      def initialize(routable, path_info)
         @routable = routable
+        @path_info = path_info
       end
 
       def should_redirect_to_group_saml_sso?(current_user, request)
@@ -25,8 +26,8 @@ module EE
 
       module ControllerActions
         def self.on_routable_not_found
-          lambda do |routable|
-            redirector = SsoEnforcementRedirect.new(routable)
+          lambda do |routable, path_info|
+            redirector = SsoEnforcementRedirect.new(routable, path_info)
 
             if redirector.should_redirect_to_group_saml_sso?(current_user, request)
               redirect_to redirector.sso_redirect_url
@@ -63,7 +64,7 @@ module EE
       def url_params
         {
           token: root_group.saml_discovery_token,
-          redirect: "/#{routable.full_path}"
+          redirect: path_info
         }
       end
     end

ee/app/controllers/groups/analytics/application_controller.rb

@@ -23,13 +23,13 @@ class Groups::Analytics::ApplicationController < ApplicationController
   def load_group
     return unless params['group_id']
 
-    @group = find_routable!(Group, params['group_id'])
+    @group = find_routable!(Group, params['group_id'], request.path_info)
   end
 
   def load_project
     return unless @group && params['project_id']
 
-    @project = find_routable!(@group.projects, params['project_id'])
+    @project = find_routable!(@group.projects, params['project_id'], request.path_info)
   end
 
   private_class_method :increment_usage_counter

ee/app/controllers/groups/omniauth_callbacks_controller.rb

@@ -13,6 +13,7 @@ class Groups::OmniauthCallbacksController < OmniauthCallbacksController
 
     identity_linker = Gitlab::Auth::GroupSaml::IdentityLinker.new(current_user, oauth, session, @saml_provider)
 
+    store_location_for(:redirect, saml_redirect_path)
     omniauth_flow(Gitlab::Auth::GroupSaml, identity_linker: identity_linker)
   rescue Gitlab::Auth::Saml::IdentityLinker::UnverifiedRequest
     redirect_unverified_saml_initiation
@@ -131,7 +132,7 @@ class Groups::OmniauthCallbacksController < OmniauthCallbacksController
   end
 
   def saml_redirect_path
-    params['RelayState'].presence if current_user
+    params['RelayState'].presence
   end
 
   override :find_message

ee/app/controllers/subscriptions/groups_controller.rb

@@ -31,7 +31,7 @@ module Subscriptions
     private
 
     def find_group
-      @group ||= find_routable!(Group, params[:id])
+      @group ||= find_routable!(Group, params[:id], request.path_info)
     end
 
     def group_params

ee/spec/controllers/concerns/ee/routable_actions/sso_enforcement_redirect_spec.rb

@@ -16,13 +16,13 @@ RSpec.describe EE::RoutableActions::SsoEnforcementRedirect do
     it 'returns false for User routables' do
       routable = build_stubbed(:user)
 
-      subject = described_class.new(routable)
+      subject = described_class.new(routable, '/')
 
       expect(subject.should_redirect_to_group_saml_sso?(double, double)).to eq(false)
     end
 
     it 'returns false when routable is nil' do
-      subject = described_class.new(nil)
+      subject = described_class.new(nil, '/')
 
       expect(subject.should_redirect_to_group_saml_sso?(double, double)).to eq(false)
     end
@@ -46,19 +46,19 @@ RSpec.describe EE::RoutableActions::SsoEnforcementRedirect do
     end
 
     context 'with a project' do
-      subject { described_class.new(project) }
+      subject { described_class.new(project, '/') }
 
       it_behaves_like 'a routable with SSO enforcement redirect'
     end
 
     context 'with a nested project' do
-      subject { described_class.new(nested_project) }
+      subject { described_class.new(nested_project, '/') }
 
       it_behaves_like 'a routable with SSO enforcement redirect'
     end
 
     context 'with a project in a personal namespace' do
-      subject { described_class.new(create(:project)) }
+      subject { described_class.new(create(:project), '/') }
 
       it 'returns false' do
         expect(subject.should_redirect_to_group_saml_sso?(user, double)).to eq false
@@ -66,13 +66,13 @@ RSpec.describe EE::RoutableActions::SsoEnforcementRedirect do
     end
 
     context 'with a group' do
-      subject { described_class.new(root_group) }
+      subject { described_class.new(root_group, '/') }
 
       it_behaves_like 'a routable with SSO enforcement redirect'
     end
 
     context 'with a nested group' do
-      subject { described_class.new(nested_group) }
+      subject { described_class.new(nested_group, '/') }
 
       it_behaves_like 'a routable with SSO enforcement redirect'
     end
@@ -88,25 +88,25 @@ RSpec.describe EE::RoutableActions::SsoEnforcementRedirect do
     end
 
     context 'with a group' do
-      subject { described_class.new(root_group) }
+      subject { described_class.new(root_group, "/#{root_group.full_path}") }
 
       it_behaves_like 'a routable SSO url'
     end
 
     context 'with a nested group' do
-      subject { described_class.new(nested_group) }
+      subject { described_class.new(nested_group, "/#{nested_group.full_path}") }
 
       it_behaves_like 'a routable SSO url'
     end
 
     context 'with a project' do
-      subject { described_class.new(project) }
+      subject { described_class.new(project, "/#{project.full_path}") }
 
       it_behaves_like 'a routable SSO url'
     end
 
     context 'with a nested project' do
-      subject { described_class.new(nested_project) }
+      subject { described_class.new(nested_project, "/#{nested_project.full_path}") }
 
       it_behaves_like 'a routable SSO url'
     end

ee/spec/controllers/concerns/routable_actions_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe RoutableActions do
 
     def routable
       @klass = params[:type].constantize
-      @routable = find_routable!(params[:type].constantize, params[:id])
+      @routable = find_routable!(params[:type].constantize, params[:id], '/')
     end
 
     def show

ee/spec/features/groups/saml_enforcement_spec.rb

@@ -119,5 +119,20 @@ RSpec.describe 'SAML access enforcement' do
         expect(page).to have_selector('#js-auto-redirect-to-provider', visible: false)
       end
     end
+
+    context 'with a merge request' do
+      let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+      let(:resource_path) { project_merge_request_path(project, merge_request) }
+
+      it 'redirects to the SSO page and then merge request page after login' do
+        visit resource_path
+
+        expect(current_url).to include("redirect=#{CGI.escape(resource_path)}")
+
+        click_link 'Sign in with Single Sign-On'
+
+        expect(current_path).to eq(resource_path)
+      end
+    end
   end
 end

ee/spec/features/users/login_spec.rb

@@ -229,7 +229,7 @@ RSpec.describe 'Login' do
 
         fake_successful_u2f_authentication
 
-        expect(current_path).to eq root_path
+        expect(current_path).to eq group_path(group)
       end
     end
 
@@ -263,7 +263,7 @@ RSpec.describe 'Login' do
 
         fake_successful_webauthn_authentication
 
-        expect(current_path).to eq root_path
+        expect(current_path).to eq group_path(group)
       end
     end
   end

spec/controllers/concerns/routable_actions_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe RoutableActions do
 
     def routable
       @klass = params[:type].constantize
-      @routable = find_routable!(params[:type].constantize, params[:id])
+      @routable = find_routable!(params[:type].constantize, params[:id], '/')
     end
 
     def show
@@ -135,7 +135,7 @@ RSpec.describe RoutableActions do
     end
 
     it 'performs checks in the context of the controller' do
-      check = lambda { |routable| redirect_to routable }
+      check = lambda { |routable, path_info| redirect_to routable }
       allow(subject).to receive(:not_found_actions).and_return([check])
 
       get_routable(routable)