Merge branch 'jej/group-saml-sso-enforcement' into 'master'

Group Policy can require active SAML SSO session for web access

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

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

@@ -71,6 +71,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 }
@@ -144,7 +178,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