Enforce active Group SAML SSO session

Prevents access to group resources when user hasn't signed in with SAML
and SSO enforcement is turned on.

Uses the session to track which SAML group the user has signed in with.
This works using global GitLab::Session from the policy via SsoState.

app/controllers/omniauth_callbacks_controller.rb

@@ -86,7 +86,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
       log_audit_event(current_user, with: oauth['provider'])
 
       identity_linker ||= auth_module::IdentityLinker.new(current_user, oauth)
-      identity_linker.link
+
+      link_identity(identity_linker)
 
       if identity_linker.changed?
         redirect_identity_linked
@@ -100,6 +101,10 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     end
   end
 
+  def link_identity(identity_linker)
+    identity_linker.link
+  end
+
   def redirect_identity_exists
     redirect_to after_sign_in_path_for(current_user)
   end

app/policies/group_policy.rb

@@ -129,7 +129,11 @@ class GroupPolicy < BasePolicy
   def access_level
     return GroupMember::NO_ACCESS if @user.nil?
 
-    @access_level ||= @subject.max_member_access_for_user(@user)
+    @access_level ||= lookup_access_level!
+  end
+
+  def lookup_access_level!
+    @subject.max_member_access_for_user(@user)
   end
 end
 

ee/app/controllers/groups/omniauth_callbacks_controller.rb

@@ -18,6 +18,13 @@ class Groups::OmniauthCallbacksController < OmniauthCallbacksController
 
   private
 
+  override :link_identity
+  def link_identity(identity_linker)
+    super.tap do
+      store_active_saml_session unless identity_linker.failed?
+    end
+  end
+
   override :redirect_identity_linked
   def redirect_identity_linked
     flash[:notice] = "SAML for #{@unauthenticated_group.name} was added to your connected accounts"
@@ -46,6 +53,17 @@ class Groups::OmniauthCallbacksController < OmniauthCallbacksController
     super
   end
 
+  override :sign_in
+  def sign_in(resource_or_scope, *args)
+    store_active_saml_session
+
+    super
+  end
+
+  def store_active_saml_session
+    Gitlab::Auth::GroupSaml::SsoEnforcer.new(@saml_provider).update_session
+  end
+
   def redirect_unverified_saml_initiation
     flash[:notice] = "Request to link SAML account must be authorized"
 

ee/app/models/saml_provider.rb

@@ -30,7 +30,7 @@ class SamlProvider < ApplicationRecord
   end
 
   def enforced_sso?
-    enabled? && super
+    enabled? && super && ::Feature.enabled?(:enforced_sso, group)
   end
 
   def enforced_group_managed_accounts?

ee/app/policies/ee/group_policy.rb

@@ -3,6 +3,7 @@
 module EE
   module GroupPolicy
     extend ActiveSupport::Concern
+    extend ::Gitlab::Utils::Override
 
     prepended do
       with_scope :subject
@@ -21,6 +22,10 @@ module EE
         !@subject.feature_available?(:security_dashboard)
       end
 
+      condition(:needs_new_sso_session) do
+        sso_enforcement_prevents_access?
+      end
+
       rule { reporter }.policy do
         enable :admin_list
         enable :admin_board
@@ -71,6 +76,24 @@ module EE
       rule { security_dashboard_feature_disabled }.policy do
         prevent :read_group_security_dashboard
       end
+
+      rule { needs_new_sso_session }.policy do
+        prevent :read_group
+      end
+    end
+
+    override :lookup_access_level!
+    def lookup_access_level!
+      return ::GroupMember::NO_ACCESS if needs_new_sso_session?
+
+      super
+    end
+
+    def sso_enforcement_prevents_access?
+      return false unless subject.persisted?
+      return false if user&.admin?
+
+      ::Gitlab::Auth::GroupSaml::SsoEnforcer.group_access_restricted?(subject)
     end
   end
 end

ee/changelogs/unreleased/jej-group-saml-sso-enforcement.yml

+---
+title: Group SAML enforcement requires active SSO session for group access
+merge_request: 10034
+author:
+type: changed

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    module GroupSaml
+      class SsoEnforcer
+        attr_reader :saml_provider
+
+        def initialize(saml_provider)
+          @saml_provider = saml_provider
+        end
+
+        def update_session
+          SsoState.new(saml_provider.id).update_active(DateTime.now)
+        end
+
+        def active_session?
+          SsoState.new(saml_provider.id).active?
+        end
+
+        def access_restricted?
+          saml_enforced? && !active_session? && ::Feature.enabled?(:enforced_sso_requires_session, group)
+        end
+
+        def self.group_access_restricted?(group)
+          return false unless group
+
+          saml_provider = group&.root_ancestor&.saml_provider
+
+          return false unless saml_provider
+
+          new(saml_provider).access_restricted?
+        end
+
+        private
+
+        def saml_enforced?
+          saml_provider&.enforced_sso?
+        end
+
+        def group
+          saml_provider&.group
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    module GroupSaml
+      class SsoState
+        SESSION_STORE_KEY = :active_group_sso_sign_ins
+
+        attr_reader :saml_provider_id
+
+        def initialize(saml_provider_id)
+          @saml_provider_id = saml_provider_id
+        end
+
+        def active?
+          !session_available? || active_session_data[saml_provider_id]
+        end
+
+        def update_active(value)
+          active_session_data[saml_provider_id] = value
+        end
+
+        private
+
+        def active_session_data
+          Gitlab::NamespacedSessionStore.new(SESSION_STORE_KEY)
+        end
+
+        def session_available?
+          active_session_data.initiated?
+        end
+      end
+    end
+  end
+end

ee/spec/controllers/groups/groups_controller_spec.rb

@@ -59,6 +59,40 @@ describe GroupsController do
     end
   end
 
+  context 'with sso enforcement enabled' do
+    let(:group) { create(:group, :private) }
+    let!(:saml_provider) { create(:saml_provider, group: group, enforced_sso: true) }
+    let(:identity) { create(:group_saml_identity, saml_provider: saml_provider) }
+    let(:guest_user) { identity.user }
+
+    before do
+      group.add_guest(guest_user)
+      sign_in(guest_user)
+    end
+
+    context 'without SAML session' do
+      it 'prevents access to group resources' do
+        get :show, params: { id: group }
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+
+    context 'with active SAML session' do
+      before do
+        Gitlab::Session.with_session(@request.session) do
+          Gitlab::Auth::GroupSaml::SsoEnforcer.new(saml_provider).update_session
+        end
+      end
+
+      it 'allows access to group resources' do
+        get :show, params: { id: group }
+
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
+  end
+
   describe '"group overview content" preference behaviour' do
     describe 'GET #show' do
       subject { get :show, params: { id: group.to_param }, format: format }
@@ -132,7 +166,6 @@ describe GroupsController do
         end
       end
     end
-
     describe 'GET #details' do
       subject { get :details, params: { id: group.to_param } }
 

ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb

@@ -50,15 +50,26 @@ describe Groups::OmniauthCallbacksController do
       Rails.application.env_config['omniauth.auth'] = @original_env_config_omniauth_auth
     end
 
-    shared_examples "and identity already linked" do
-      let!(:user) { create_linked_user }
-
+    shared_examples "SAML session initiated" do
       it "redirects to RelayState" do
         post provider, params: { group_id: group, RelayState: '/explore' }
 
         expect(response).to redirect_to('/explore')
       end
 
+      it 'stores that a SAML session is active' do
+        expect(Gitlab::Auth::GroupSaml::SsoEnforcer).to receive(:new).with(saml_provider).and_call_original
+        expect_any_instance_of(Gitlab::Auth::GroupSaml::SsoEnforcer).to receive(:update_session)
+
+        post provider, params: { group_id: group }
+      end
+    end
+
+    shared_examples "and identity already linked" do
+      let!(:user) { create_linked_user }
+
+      it_behaves_like "SAML session initiated"
+
       it "displays a flash message verifying group sign in" do
         post provider, params: { group_id: group }
 
@@ -86,6 +97,18 @@ describe Groups::OmniauthCallbacksController do
 
       it_behaves_like "and identity already linked"
 
+      context 'oauth linked with different NameID' do
+        before do
+          create(:identity, user: user, extern_uid: 'some-other-name-id', provider: provider, saml_provider: saml_provider)
+        end
+
+        it 'displays warning to user' do
+          post provider, params: { group_id: group }
+
+          expect(flash[:notice]).to match(/has already been taken*/)
+        end
+      end
+
       context 'oauth already linked to another account' do
         before do
           create_linked_user
@@ -105,11 +128,7 @@ describe Groups::OmniauthCallbacksController do
           expect(group).to be_member(user)
         end
 
-        it "redirects to RelayState" do
-          post provider, params: { group_id: group, RelayState: '/explore' }
-
-          expect(response).to redirect_to('/explore')
-        end
+        it_behaves_like "SAML session initiated"
 
         it "displays a flash indicating the account has been linked" do
           post provider, params: { group_id: group }

ee/spec/features/groups/saml_enforcement_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'SAML access enforcement' do
+  let(:group) { create(:group, :private, name: 'The Group Name') }
+  let(:saml_provider) { create(:saml_provider, group: group, enforced_sso: true) }
+  let(:identity) { create(:group_saml_identity, saml_provider: saml_provider) }
+  let(:user) { identity.user }
+
+  before do
+    group.add_guest(user)
+    sign_in(user)
+  end
+
+  context 'without SAML session' do
+    it 'prevents access to group resources' do
+      visit group_path(group)
+
+      expect(page).not_to have_content(group.name)
+      expect(page).to have_content('Page Not Found')
+    end
+  end
+
+  context 'with active SAML login from session' do
+    before do
+      dummy_session = { active_group_sso_sign_ins: { saml_provider.id => DateTime.now } }
+      allow(Gitlab::Session).to receive(:current).and_return(dummy_session)
+    end
+
+    it 'allows access to group resources' do
+      visit group_path(group)
+
+      expect(page).not_to have_content('Page Not Found')
+      expect(page).to have_content(group.name)
+      expect(current_path).to eq(group_path(group))
+    end
+  end
+end

ee/spec/finders/epics_finder_spec.rb

@@ -55,10 +55,8 @@ describe EpicsFinder do
           expect(epics).to contain_exactly(epic1, epic2, epic3)
         end
 
-        it 'does not execute more than 7 SQL queries' do
-          amount = ActiveRecord::QueryRecorder.new { epics.to_a }.count
-
-          expect(amount).to be <= 7
+        it 'does not execute more than 8 SQL queries' do
+          expect { epics.to_a }.not_to exceed_all_query_limit(8)
         end
 
         context 'sorting' do
@@ -122,22 +120,18 @@ describe EpicsFinder do
             expect(epics).to contain_exactly(epic1, epic2, epic3, subepic1, subepic2)
           end
 
-          it 'does not execute more than 9 SQL queries' do
-            amount = ActiveRecord::QueryRecorder.new { epics.to_a }.count
-
-            expect(amount).to be <= 9
+          it 'does not execute more than 14 SQL queries' do
+            expect { epics.to_a }.not_to exceed_all_query_limit(14)
           end
 
-          it 'does not execute more than 11 SQL queries when checking namespace plans' do
+          it 'does not execute more than 15 SQL queries when checking namespace plans' do
             allow(Gitlab::CurrentSettings)
               .to receive(:should_check_namespace_plan?)
               .and_return(true)
 
             create(:gitlab_subscription, :gold, namespace: group)
 
-            amount = ActiveRecord::QueryRecorder.new { epics.to_a }.count
-
-            expect(amount).to be <= 10
+            expect { epics.to_a }.not_to exceed_all_query_limit(15)
           end
         end
 

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Auth::GroupSaml::SsoEnforcer do
+  let(:saml_provider) { build_stubbed(:saml_provider, enforced_sso: true) }
+  let(:session) { {} }
+
+  around do |example|
+    Gitlab::Session.with_session(session) do
+      example.run
+    end
+  end
+
+  subject { described_class.new(saml_provider) }
+
+  describe '#update_session' do
+    it 'stores that a session is active for the given provider' do
+      expect { subject.update_session }.to change { session[:active_group_sso_sign_ins] }
+    end
+
+    it 'stores the current time for later comparison' do
+      Timecop.freeze do
+        subject.update_session
+
+        expect(session[:active_group_sso_sign_ins][saml_provider.id]).to eq DateTime.now
+      end
+    end
+  end
+
+  describe '#active_session?' do
+    it 'returns false if nothing has been stored' do
+      expect(subject).not_to be_active_session
+    end
+
+    it 'returns true if a sign in has been recorded' do
+      subject.update_session
+
+      expect(subject).to be_active_session
+    end
+  end
+
+  describe '#allows_access?' do
+    it 'allows access when saml_provider is nil' do
+      subject = described_class.new(nil)
+
+      expect(subject).not_to be_access_restricted
+    end
+
+    context 'when sso enforcement is disabled' do
+      let(:saml_provider) { build_stubbed(:saml_provider, enforced_sso: false) }
+
+      it 'allows access when sso enforcement is disabled' do
+        expect(subject).not_to be_access_restricted
+      end
+    end
+
+    context 'when saml_provider is disabled' do
+      let(:saml_provider) { build_stubbed(:saml_provider, enforced_sso: true, enabled: false) }
+
+      it 'allows access when saml_provider is disabled' do
+        expect(subject).not_to be_access_restricted
+      end
+    end
+
+    it 'allows access when the sso enforcement feature is disabled' do
+      stub_feature_flags(enforced_sso: { enabled: false, thing: saml_provider.group })
+
+      expect(subject).not_to be_access_restricted
+    end
+
+    it 'prevents access when sso enforcement active but there is no session' do
+      expect(subject).to be_access_restricted
+    end
+
+    it 'allows access when sso is enforced but a saml session is active' do
+      subject.update_session
+
+      expect(subject).not_to be_access_restricted
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Auth::GroupSaml::SsoState do
+  let(:saml_provider_id) { 10 }
+
+  subject { described_class.new(saml_provider_id) }
+
+  describe '#update_active' do
+    it 'updates the current sign in state' do
+      Gitlab::Session.with_session({}) do
+        new_state = double
+        subject.update_active(new_state)
+
+        expect(Gitlab::Session.current[:active_group_sso_sign_ins]).to eq({ saml_provider_id => new_state })
+      end
+    end
+  end
+
+  describe '#active?' do
+    it 'gets the current sign in state' do
+      current_state = double
+
+      Gitlab::Session.with_session(active_group_sso_sign_ins: { saml_provider_id => current_state }) do
+        expect(subject.active?).to eq current_state
+      end
+    end
+  end
+end

ee/spec/policies/group_policy_spec.rb

@@ -55,6 +55,36 @@ describe GroupPolicy do
 
       it { is_expected.to be_allowed(:admin_group_saml) }
     end
+
+    context 'with sso enforcement enabled' do
+      let(:current_user) { guest }
+      let(:group) { create(:group, :private) }
+      let!(:saml_provider) { create(:saml_provider, group: group, enforced_sso: true) }
+
+      context 'when the session has been set globally' do
+        around do |example|
+          Gitlab::Session.with_session({}) do
+            example.run
+          end
+        end
+
+        it 'prevents access without a SAML session' do
+          is_expected.not_to be_allowed(:read_group)
+        end
+
+        it 'allows access with a SAML session' do
+          Gitlab::Auth::GroupSaml::SsoEnforcer.new(saml_provider).update_session
+
+          is_expected.to be_allowed(:read_group)
+        end
+      end
+
+      context 'when there is no global session or sso state' do
+        it "allows access because we haven't yet restricted all use cases" do
+          is_expected.to be_allowed(:read_group)
+        end
+      end
+    end
   end
 
   context 'when LDAP sync is not enabled' do