Merge branch 'jej/sso-enforcement-redirect' into 'master'

SSO enforement redirects to group sign in

Closes #11558

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

app/controllers/concerns/project_unauthorized.rb

 # frozen_string_literal: true
 
 module ProjectUnauthorized
-  def project_unauthorized_proc
-    lambda do |project|
-      if project
-        label = project.external_authorization_classification_label
+  module ControllerActions
+    def self.on_routable_not_found
+      lambda do |routable|
+        return unless routable.is_a?(Project)
+
+        label = routable.external_authorization_classification_label
         rejection_reason = nil
 
         unless ::Gitlab::ExternalAuthorization.access_allowed?(current_user, label)
@@ -12,9 +14,7 @@ module ProjectUnauthorized
           rejection_reason ||= _('External authorization denied access to this project')
         end
 
-        if rejection_reason
-          access_denied!(rejection_reason)
-        end
+        access_denied!(rejection_reason) if rejection_reason
       end
     end
   end

app/controllers/concerns/routable_actions.rb

@@ -3,15 +3,13 @@
 module RoutableActions
   extend ActiveSupport::Concern
 
-  def find_routable!(routable_klass, requested_full_path, extra_authorization_proc: nil, not_found_or_authorized_proc: nil)
+  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?)
     if routable_authorized?(routable, extra_authorization_proc)
       ensure_canonical_path(routable, requested_full_path)
       routable
     else
-      if not_found_or_authorized_proc
-        not_found_or_authorized_proc.call(routable)
-      end
+      perform_not_found_actions(routable, not_found_actions)
 
       route_not_found unless performed?
 
@@ -19,6 +17,18 @@ module RoutableActions
     end
   end
 
+  def not_found_actions
+    [ProjectUnauthorized::ControllerActions.on_routable_not_found]
+  end
+
+  def perform_not_found_actions(routable, actions)
+    actions.each do |action|
+      break if performed?
+
+      instance_exec(routable, &action)
+    end
+  end
+
   def routable_authorized?(routable, extra_authorization_proc)
     return false unless routable
 
@@ -45,3 +55,5 @@ module RoutableActions
     end
   end
 end
+
+RoutableActions.prepend(EE::RoutableActions)

app/controllers/projects/application_controller.rb

@@ -3,7 +3,6 @@
 class Projects::ApplicationController < ApplicationController
   include CookiesHelper
   include RoutableActions
-  include ProjectUnauthorized
   include ChecksCollaboration
 
   skip_before_action :authenticate_user!
@@ -22,7 +21,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, not_found_or_authorized_proc: project_unauthorized_proc)
+    @project = find_routable!(Project, path, extra_authorization_proc: auth_proc)
   end
 
   def build_canonical_path(project)

app/controllers/projects/clusters/applications_controller.rb

 # frozen_string_literal: true
 
 class Projects::Clusters::ApplicationsController < Clusters::ApplicationsController
-  include ProjectUnauthorized
-
   prepend_before_action :project
 
   private
@@ -12,6 +10,6 @@ class Projects::Clusters::ApplicationsController < Clusters::ApplicationsControl
   end
 
   def project
-    @project ||= find_routable!(Project, File.join(params[:namespace_id], params[:project_id]), not_found_or_authorized_proc: project_unauthorized_proc)
+    @project ||= find_routable!(Project, File.join(params[:namespace_id], params[:project_id]))
   end
 end

app/controllers/projects/clusters_controller.rb

 # frozen_string_literal: true
 
 class Projects::ClustersController < Clusters::ClustersController
-  include ProjectUnauthorized
-
   prepend_before_action :project
   before_action :repository
 
@@ -15,7 +13,7 @@ class Projects::ClustersController < Clusters::ClustersController
   end
 
   def project
-    @project ||= find_routable!(Project, File.join(params[:namespace_id], params[:project_id]), not_found_or_authorized_proc: project_unauthorized_proc)
+    @project ||= find_routable!(Project, File.join(params[:namespace_id], params[:project_id]))
   end
 
   def repository

app/controllers/projects/serverless/functions_controller.rb

@@ -3,8 +3,6 @@
 module Projects
   module Serverless
     class FunctionsController < Projects::ApplicationController
-      include ProjectUnauthorized
-
       before_action :authorize_read_cluster!
 
       def index

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

+# frozen_string_literal: true
+
+module EE
+  module RoutableActions
+    extend ::Gitlab::Utils::Override
+
+    override :not_found_actions
+    def not_found_actions
+      super + [SsoEnforcementRedirect::ControllerActions.on_routable_not_found]
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module EE
+  module RoutableActions
+    class SsoEnforcementRedirect
+      include ::Gitlab::Routing
+      include ::Gitlab::Utils::StrongMemoize
+
+      attr_reader :routable
+
+      def initialize(routable)
+        @routable = routable
+      end
+
+      def should_redirect_to_group_saml_sso?(current_user, request)
+        return false unless should_process?
+        return false unless request.get?
+
+        access_restricted_by_sso?(current_user)
+      end
+
+      def sso_redirect_url
+        sso_group_saml_providers_url(root_group, token: root_group.saml_discovery_token)
+      end
+
+      module ControllerActions
+        def self.on_routable_not_found
+          lambda do |routable|
+            redirector = SsoEnforcementRedirect.new(routable)
+
+            if redirector.should_redirect_to_group_saml_sso?(current_user, request)
+              redirect_to redirector.sso_redirect_url
+            end
+          end
+        end
+      end
+
+      private
+
+      def access_restricted_by_sso?(current_user)
+        Ability.policy_for(current_user, routable)&.needs_new_sso_session?
+      end
+
+      def should_process?
+        group.present?
+      end
+
+      def group
+        strong_memoize(:group) do
+          case routable
+          when ::Group
+            routable
+          when ::Project
+            routable.group
+          end
+        end
+      end
+
+      def root_group
+        @root_group ||= group.root_ancestor
+      end
+    end
+  end
+end

ee/changelogs/unreleased/jej-sso-enforcement-redirect.yml

+---
+title: SSO enforement redirects to group sign in when not using SAML
+merge_request: 12246
+author:
+type: added

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::RoutableActions::SsoEnforcementRedirect do
+  let(:saml_provider) { create(:saml_provider, enforced_sso: true) }
+  let(:root_group) { saml_provider.group }
+  let(:nested_group) { create(:group, :private, parent: root_group) }
+  let(:nested_project) { create(:project, :private, group: nested_group) }
+
+  describe '#should_redirect_to_group_saml_sso?' do
+    let(:user) { build_stubbed(:user) }
+    let(:request) { double(:request, get?: true) }
+
+    it 'returns false for User routables' do
+      routable = build_stubbed(:user)
+
+      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)
+
+      expect(subject.should_redirect_to_group_saml_sso?(double, double)).to eq(false)
+    end
+
+    context 'with a project' do
+      subject { described_class.new(nested_project) }
+
+      it 'is false when a new sso session is not needed' do
+        allow_any_instance_of(ProjectPolicy).to receive(:needs_new_sso_session?).and_return(false)
+
+        expect(subject.should_redirect_to_group_saml_sso?(user, request)).to eq false
+      end
+
+      it 'is true when a new sso session is needed' do
+        allow_any_instance_of(ProjectPolicy).to receive(:needs_new_sso_session?).and_return(true)
+
+        expect(subject.should_redirect_to_group_saml_sso?(user, request)).to eq true
+      end
+
+      context 'in a personal namespace' do
+        subject { described_class.new(create(:project)) }
+
+        it 'returns false' do
+          expect(subject.should_redirect_to_group_saml_sso?(user, double)).to eq false
+        end
+      end
+    end
+
+    context 'with a group' do
+      subject { described_class.new(nested_group) }
+
+      it 'is false when a new sso session is not needed' do
+        allow_any_instance_of(GroupPolicy).to receive(:needs_new_sso_session?).and_return(false)
+
+        expect(subject.should_redirect_to_group_saml_sso?(user, request)).to eq false
+      end
+
+      it 'is true when a new sso session is needed' do
+        allow_any_instance_of(GroupPolicy).to receive(:needs_new_sso_session?).and_return(true)
+
+        expect(subject.should_redirect_to_group_saml_sso?(user, request)).to eq true
+      end
+    end
+  end
+
+  describe '#sso_redirect_url' do
+    it 'returns the SSO url for a group' do
+      subject = described_class.new(nested_group)
+
+      expect(subject.sso_redirect_url).to match(/groups\/#{root_group.to_param}\/-\/saml\/sso\?token=/)
+    end
+
+    it "returns the SSO url for a project's root group" do
+      subject = described_class.new(nested_project)
+
+      expect(subject.sso_redirect_url).to match(/groups\/#{root_group.to_param}\/-\/saml\/sso\?token=/)
+    end
+  end
+end

ee/spec/controllers/concerns/routable_actions_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe RoutableActions do
+  controller(::ApplicationController) do
+    include RoutableActions # rubocop:disable RSpec/DescribedClass
+
+    before_action :routable
+
+    def routable
+      @klass = params[:type].constantize
+      @routable = find_routable!(params[:type].constantize, params[:id])
+    end
+
+    def show
+      head :ok
+    end
+
+    def create
+      head :ok
+    end
+  end
+
+  def request_params(routable)
+    { id: routable.full_path, type: routable.class }
+  end
+
+  describe '#find_routable!' do
+    describe 'when SSO enforcement prevents access' do
+      let(:saml_provider) { create(:saml_provider, enforced_sso: true) }
+      let(:identity) { create(:group_saml_identity, saml_provider: saml_provider) }
+      let(:root_group) { saml_provider.group }
+      let(:user) { identity.user }
+
+      before do
+        sign_in(user)
+      end
+
+      shared_examples 'sso redirects' do
+        it 'redirects to group sign in page' do
+          get :show, params: request_params(routable)
+
+          expect(response).to have_gitlab_http_status(302)
+          expect(response.location).to match(/groups\/.*\/-\/saml\/sso\?token=/)
+        end
+
+        it 'does not redirect on POST requests' do
+          post :create, params: request_params(routable)
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+
+      describe 'for a group' do
+        let(:routable) { create(:group, :private, parent: root_group) }
+
+        include_examples 'sso redirects'
+      end
+
+      describe 'for a project' do
+        let(:routable) { create(:project, :private, group: root_group) }
+
+        include_examples 'sso redirects'
+      end
+    end
+  end
+end

ee/spec/controllers/groups/groups_controller_spec.rb

@@ -86,7 +86,8 @@ describe GroupsController do
       it 'prevents access to group resources' do
         get :show, params: { id: group }
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(302)
+        expect(response.location).to match(/groups\/#{group.to_param}\/-\/saml\/sso\?token=/)
       end
     end
 

ee/spec/features/groups/saml_enforcement_spec.rb

@@ -11,14 +11,16 @@ describe 'SAML access enforcement' do
   before do
     group.add_guest(user)
     sign_in(user)
+
+    stub_licensed_features(group_saml: true)
   end
 
   context 'without SAML session' do
-    it 'prevents access to group resources' do
+    it 'prevents access to group resources via SSO redirect' do
       visit group_path(group)
 
-      expect(page).not_to have_content(group.name)
-      expect(page).to have_content('Page Not Found')
+      expect(page).to have_content("SAML SSO Sign in to \"#{group.name}\"")
+      expect(current_url).to match(/groups\/#{group.to_param}\/-\/saml\/sso\?token=/)
     end
   end
 
@@ -32,6 +34,7 @@ describe 'SAML access enforcement' do
       visit group_path(group)
 
       expect(page).not_to have_content('Page Not Found')
+      expect(page).not_to have_content('SAML SSO Sign')
       expect(page).to have_content(group.name)
       expect(current_path).to eq(group_path(group))
     end

spec/controllers/concerns/project_unauthorized_spec.rb

@@ -12,7 +12,7 @@ describe ProjectUnauthorized do
 
   render_views
 
-  describe '#project_unauthorized_proc' do
+  describe '.on_routable_not_found' do
     controller(::Projects::ApplicationController) do
       def show
         head :ok

spec/controllers/concerns/routable_actions_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe RoutableActions do
+  controller(::ApplicationController) do
+    include RoutableActions # rubocop:disable RSpec/DescribedClass
+
+    before_action :routable
+
+    def routable
+      @klass = params[:type].constantize
+      @routable = find_routable!(params[:type].constantize, params[:id])
+    end
+
+    def show
+      head :ok
+    end
+  end
+
+  def get_routable(routable)
+    get :show, params: { id: routable.full_path, type: routable.class }
+  end
+
+  describe '#find_routable!' do
+    context 'when signed in' do
+      let(:user) { create(:user) }
+
+      before do
+        sign_in(user)
+      end
+
+      context 'with a project' do
+        let(:routable) { create(:project) }
+
+        context 'when authorized' do
+          before do
+            routable.add_guest(user)
+          end
+
+          it 'returns the project' do
+            get_routable(routable)
+
+            expect(assigns[:routable]).to be_a(Project)
+          end
+
+          it 'allows access' do
+            get_routable(routable)
+
+            expect(response).to have_gitlab_http_status(200)
+          end
+        end
+
+        it 'prevents access when not authorized' do
+          get_routable(routable)
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+
+      context 'with a group' do
+        let(:routable) { create(:group, :private) }
+
+        context 'when authorized' do
+          before do
+            routable.add_guest(user)
+          end
+
+          it 'returns the group' do
+            get_routable(routable)
+
+            expect(assigns[:routable]).to be_a(Group)
+          end
+
+          it 'allows access' do
+            get_routable(routable)
+
+            expect(response).to have_gitlab_http_status(200)
+          end
+        end
+
+        it 'prevents access when not authorized' do
+          get_routable(routable)
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+
+      context 'with a user' do
+        let(:routable) { user }
+
+        it 'allows access when authorized' do
+          get_routable(routable)
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+
+        it 'prevents access when unauthorized' do
+          allow(subject).to receive(:can?).and_return(false)
+
+          get_routable(user)
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+    end
+
+    context 'when not signed in' do
+      it 'redirects to sign in for private resouces' do
+        routable = create(:project, :private)
+
+        get_routable(routable)
+
+        expect(response).to have_gitlab_http_status(302)
+        expect(response.location).to end_with('/users/sign_in')
+      end
+    end
+  end
+
+  describe '#perform_not_found_actions' do
+    let(:routable) { create(:project) }
+
+    before do
+      sign_in(create(:user))
+    end
+
+    it 'performs multiple checks' do
+      last_check_called = false
+      checks = [proc {}, proc { last_check_called = true }]
+      allow(subject).to receive(:not_found_actions).and_return(checks)
+
+      get_routable(routable)
+
+      expect(last_check_called).to eq(true)
+    end
+
+    it 'performs checks in the context of the controller' do
+      check = lambda { |routable| redirect_to routable }
+      allow(subject).to receive(:not_found_actions).and_return([check])
+
+      get_routable(routable)
+
+      expect(response.location).to end_with(routable.to_param)
+    end
+
+    it 'skips checks once one has resulted in a render/redirect' do
+      first_check = proc { render plain: 'first' }
+      second_check = proc { render plain: 'second' }
+      allow(subject).to receive(:not_found_actions).and_return([first_check, second_check])
+
+      get_routable(routable)
+
+      expect(response.body).to eq('first')
+    end
+  end
+end