Create session enforcer class for group saml to add to git check

Refactor naming into more readable

Add new database column

Add specs and model method

Add specs for group saml session enforcer class

WIP on git access specs

Refactor git access specs

Add specs to git access

Add new setting to view and form

Add new setting to controller

Add changelog entry

Add translations file

Refactor git access class

Add method to find group

Fix container method

Add cr remarks

Simplify spec example

Change copy in saml sso form

Add documentation regarding checking sso session on git activity

Add cr remarks

Reorganize code to make sso check more effective

Add remarks to git check migration

Add remarks about enforcing sso on Git activity docs

Move check_additional_conditions method

changelogs/unreleased/9152-check-sso-status-on-git-activity-and-direct-user-to-sso.yml

+---
+title: Group SAML - Check SSO status on Git activity
+merge_request: 56867
+author:
+type: added

db/migrate/20210312174321_add_enforced_git_check_to_saml_provider.rb

+# frozen_string_literal: true
+
+class AddEnforcedGitCheckToSamlProvider < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    add_column :saml_providers, :git_check_enforced, :boolean, default: false, null: false
+  end
+
+  def down
+    remove_column :saml_providers, :git_check_enforced
+  end
+end

db/schema_migrations/20210312174321

+4fa88193ae328f04465980210d9a43ce8cad978c157bda5e8ae9951538209268
\ No newline at end of file

db/structure.sql

@@ -17270,7 +17270,8 @@ CREATE TABLE saml_providers (
     enforced_sso boolean DEFAULT false NOT NULL,
     enforced_group_managed_accounts boolean DEFAULT false NOT NULL,
     prohibited_outer_forks boolean DEFAULT true NOT NULL,
-    default_membership_role smallint DEFAULT 10 NOT NULL
+    default_membership_role smallint DEFAULT 10 NOT NULL,
+    git_check_enforced boolean DEFAULT false NOT NULL
 );
 
 CREATE SEQUENCE saml_providers_id_seq

doc/user/group/saml_sso/index.md

@@ -95,6 +95,7 @@ Please note that the certificate [fingerprint algorithm](../../../integration/sa
 - [Improved](https://gitlab.com/gitlab-org/gitlab/-/issues/9255) in GitLab 11.11 with ongoing enforcement in the GitLab UI.
 - [Improved](https://gitlab.com/gitlab-org/gitlab/-/issues/292811) in GitLab 13.8, with an updated timeout experience.
 - [Improved](https://gitlab.com/gitlab-org/gitlab/-/issues/211962) in GitLab 13.8 with allowing group owners to not go through SSO.
+- [Improved](https://gitlab.com/gitlab-org/gitlab/-/issues/9152) in GitLab 13.11 with enforcing open SSO session to use Git if this setting is switched on.
 
 With this option enabled, users must go through your group's GitLab single sign-on URL. They may also be added via SCIM, if configured. Users can't be added manually, and may only access project/group resources via the UI by signing in through the SSO URL.
 
@@ -102,9 +103,15 @@ However, users are not prompted to sign in through SSO on each visit. GitLab che
 has authenticated through SSO. If it's been more than 1 day since the last sign-in, GitLab
 prompts the user to sign in again through SSO.
 
-We intend to add a similar SSO requirement for [Git and API activity](https://gitlab.com/gitlab-org/gitlab/-/issues/9152).
+We intend to add a similar SSO requirement for [API activity](https://gitlab.com/gitlab-org/gitlab/-/issues/9152).
 
-When SSO enforcement is enabled for a group, users can't share a project in the group outside the top-level group, even if the project is forked.
+SSO has the following effects when enabled:
+
+- For groups, users can't share a project in the group outside the top-level group,
+  even if the project is forked.
+- For a Git activity, users must be signed-in through SSO before they can push to or
+  pull from a GitLab repository.
+<!-- Add bullet for API activity when https://gitlab.com/gitlab-org/gitlab/-/issues/9152 is complete -->
 
 ## Providers
 

ee/app/assets/javascripts/saml_providers/saml_settings_form.js

@@ -39,6 +39,11 @@ export default class SamlSettingsForm {
         el: this.form.querySelector('.js-group-saml-enforced-group-managed-accounts-toggle-area'),
         dependsOn: 'enforced-sso',
       },
+      {
+        name: 'enforced-git-activity-check',
+        el: this.form.querySelector('.js-group-saml-enforced-git-check-toggle-area'),
+        dependsOn: 'enforced-sso',
+      },
       {
         name: 'prohibited-outer-forks',
         el: this.form.querySelector('.js-group-saml-prohibited-outer-forks-toggle-area'),

ee/app/controllers/groups/saml_providers_controller.rb

@@ -47,7 +47,7 @@ class Groups::SamlProvidersController < Groups::ApplicationController
   end
 
   def saml_provider_params
-    allowed_params = %i[sso_url certificate_fingerprint enabled enforced_sso default_membership_role]
+    allowed_params = %i[sso_url certificate_fingerprint enabled enforced_sso default_membership_role git_check_enforced]
 
     if Feature.enabled?(:group_managed_accounts, group)
       allowed_params += [:enforced_group_managed_accounts, :prohibited_outer_forks]

ee/app/models/saml_provider.rb

@@ -10,6 +10,7 @@ class SamlProvider < ApplicationRecord
   validates :sso_url, presence: true, addressable_url: { schemes: %w(https), ascii_only: true }
   validates :certificate_fingerprint, presence: true, certificate_fingerprint: true
   validates :default_membership_role, presence: true
+  validate :git_check_enforced_allowed
   validate :access_level_inclusion
 
   after_initialize :set_defaults, if: :new_record?
@@ -35,6 +36,10 @@ class SamlProvider < ApplicationRecord
     enabled? && super && group.feature_available?(:group_saml)
   end
 
+  def git_check_enforced?
+    super && enforced_sso?
+  end
+
   def enforced_group_managed_accounts?
     super && enforced_sso? && Feature.enabled?(:group_managed_accounts, group)
   end
@@ -92,6 +97,13 @@ class SamlProvider < ApplicationRecord
     errors.add(:default_membership_role, "is not included in the list")
   end
 
+  def git_check_enforced_allowed
+    return unless git_check_enforced
+    return if enforced_sso?
+
+    errors.add(:git_check_enforced, "is not allowed when SSO is not enforced.")
+  end
+
   def set_defaults
     self.enabled = true
   end

ee/app/views/groups/saml_providers/_form.html.haml

@@ -10,10 +10,18 @@
       %label.gl-mt-2.mb-0.js-group-saml-enforced-sso-toggle-area
         = render "shared/buttons/project_feature_toggle", is_checked: saml_provider.enforced_sso, disabled: !saml_provider.enabled?, label: s_("GroupSAML|Enforced SSO"), class_list: "js-project-feature-toggle js-group-saml-enforced-sso-toggle project-feature-toggle d-inline", data: { qa_selector: 'enforced_sso_toggle_button' } do
           = f.hidden_field :enforced_sso, { class: 'js-group-saml-enforced-sso-input js-project-feature-toggle-input'}
-        %span.form-text.d-inline.font-weight-normal.align-text-bottom.ml-3= s_('GroupSAML|Enforce SSO-only authentication for this group.')
+        %span.form-text.d-inline.font-weight-normal.align-text-bottom.ml-3= s_('GroupSAML|Enforce SSO-only authentication for web activity for this group.')
       .form-text.text-muted.js-helper-text{ style: "display: #{'none' if saml_provider.enabled?} #{'block' unless saml_provider.enabled?}" }
         %span
-          = s_('GroupSAML|To be able to enable enforced SSO, you first need to enable SAML authentication.')
+          = s_('GroupSAML|Before enforcing SSO, enable SAML authentication.')
+    .form-group
+      %label.gl-mt-2.mb-0.js-group-saml-enforced-git-check-toggle-area
+        = render "shared/buttons/project_feature_toggle", is_checked: saml_provider.git_check_enforced, disabled: !saml_provider.enabled?, label: s_("GroupSAML|Check SSO on git activity"), class_list: "js-project-feature-toggle js-group-saml-enforced-git-check-toggle-area project-feature-toggle d-inline", data: { qa_selector: 'git_activity_check_toggle_button' } do
+          = f.hidden_field :git_check_enforced, { class: 'js-group-enforced-git-check-input js-project-feature-toggle-input'}
+        %span.form-text.d-inline.font-weight-normal.align-text-bottom.ml-3= s_('GroupSAML|Enforce SSO-access for Git in this group.')
+      .form-text.text-muted.js-helper-text{ style: "display: #{saml_provider.enabled? ? 'none' : 'block'}" }
+        %span
+          = s_('GroupSAML|Before enforcing SSO, enable SAML authentication.')
     - if Feature.enabled?(:group_managed_accounts, group)
       .form-group
         %label.gl-mt-2.mb-0.js-group-saml-enforced-group-managed-accounts-toggle-area

ee/lib/ee/gitlab/git_access.rb

@@ -83,6 +83,13 @@ module EE
         super
       end
 
+      override :check_additional_conditions!
+      def check_additional_conditions!
+        check_sso_session!
+
+        super
+      end
+
       def check_geo_license!
         if ::Gitlab::Geo.secondary? && !::Gitlab::Geo.license_allows?
           raise ::Gitlab::GitAccess::ForbiddenError, 'Your current license does not have GitLab Geo add-on enabled.'
@@ -104,12 +111,21 @@ module EE
 
         if ::Gitlab::Auth::Otp::SessionEnforcer.new(actor).access_restricted?
           message = "OTP verification is required to access the repository.\n\n"\
-                  "   Use: #{build_ssh_otp_verify_command}"
+          "   Use: #{build_ssh_otp_verify_command}"
 
           raise ::Gitlab::GitAccess::ForbiddenError, message
         end
       end
 
+      def check_sso_session!
+        return true unless user && container
+
+        return unless ::Gitlab::Auth::GroupSaml::SessionEnforcer.new(user, containing_group).access_restricted?
+
+        group_saml_url = Rails.application.routes.url_helpers.sso_group_saml_providers_url(containing_group, token: containing_group.saml_discovery_token)
+        raise ::Gitlab::GitAccess::ForbiddenError, "Cannot find valid SSO session. Please login via your group's SSO at #{group_saml_url}"
+      end
+
       def build_ssh_otp_verify_command
         user = "#{::Gitlab.config.gitlab_shell.ssh_user}@" unless ::Gitlab.config.gitlab_shell.ssh_user.empty?
         user_host = "#{user}#{::Gitlab.config.gitlab_shell.ssh_host}"
@@ -155,6 +171,11 @@ module EE
           size_checker.enabled? && super
         end
       end
+
+      def containing_group
+        return group if group?
+        return project.group if project?
+      end
     end
   end
 end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    module GroupSaml
+      class SessionEnforcer
+        SESSION_STORE_KEY = :active_group_sso_sign_ins
+
+        def initialize(user, group)
+          @user = user
+          @group = group
+        end
+
+        def access_restricted?
+          return false if skip_check?
+
+          latest_sign_in = find_session
+
+          return true unless latest_sign_in
+          return SsoEnforcer::DEFAULT_SESSION_TIMEOUT.ago > latest_sign_in if ::Feature.enabled?(:enforced_sso_expiry, group)
+
+          false
+        end
+
+        private
+
+        attr_reader :user, :group
+
+        def skip_check?
+          return true if no_group_or_provider?
+          return true if user_allowed?
+          return true unless git_check_enforced?
+
+          false
+        end
+
+        def no_group_or_provider?
+          return true unless group
+          return true unless group.root_ancestor
+          return true unless saml_provider
+
+          false
+        end
+
+        def saml_provider
+          @saml_provider ||= group.root_ancestor.saml_provider
+        end
+
+        def git_check_enforced?
+          saml_provider.git_check_enforced?
+        end
+
+        def user_allowed?
+          return true if user.auditor? || user.can_read_all_resources?
+          return true if group.owned_by?(user)
+
+          false
+        end
+
+        def find_session
+          sessions = ActiveSession.list_sessions(user)
+          sessions.filter_map do |session|
+            Gitlab::NamespacedSessionStore.new(SESSION_STORE_KEY, session.with_indifferent_access)[saml_provider.id]
+          end.last
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Auth::GroupSaml::SessionEnforcer do
+  describe '#access_restricted' do
+    let_it_be(:saml_provider) { create(:saml_provider, enforced_sso: true) }
+    let_it_be(:user) { create(:user) }
+    let_it_be(:identity) { create(:group_saml_identity, saml_provider: saml_provider, user: user) }
+
+    let(:root_group) { saml_provider.group }
+
+    subject { described_class.new(user, root_group).access_restricted? }
+
+    before do
+      stub_licensed_features(group_saml: true)
+    end
+
+    context 'when git check is enforced' do
+      before do
+        allow(saml_provider).to receive(:git_check_enforced?).and_return(true)
+      end
+
+      context 'with an active session', :clean_gitlab_redis_shared_state do
+        let(:session_id) { '42' }
+        let(:session_time) { 5.minutes.ago }
+        let(:stored_session) do
+          { 'active_group_sso_sign_ins' => { saml_provider.id => session_time } }
+        end
+
+        before do
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.set("session:gitlab:#{session_id}", Marshal.dump(stored_session))
+            redis.sadd("session:lookup:user:gitlab:#{user.id}", [session_id])
+          end
+        end
+
+        it { is_expected.to be_falsey }
+
+        context 'with sub-group' do
+          let(:group) { create(:group, parent: root_group) }
+
+          subject { described_class.new(user, group).access_restricted? }
+
+          it { is_expected.to be_falsey }
+        end
+
+        context 'with expired session' do
+          let(:session_time) { 2.days.ago }
+
+          it { is_expected.to be_truthy }
+        end
+
+        context 'with two active sessions', :clean_gitlab_redis_shared_state do
+          let(:second_session_id) { '52' }
+          let(:second_stored_session) do
+            { 'active_group_sso_sign_ins' => { create(:saml_provider, enforced_sso: true).id => session_time } }
+          end
+
+          before do
+            Gitlab::Redis::SharedState.with do |redis|
+              redis.set("session:gitlab:#{second_session_id}", Marshal.dump(second_stored_session))
+              redis.sadd("session:lookup:user:gitlab:#{user.id}", [session_id, second_session_id])
+            end
+          end
+
+          it { is_expected.to be_falsey }
+        end
+
+        context 'without enforced_sso_expiry feature flag' do
+          let(:session_time) { 2.days.ago }
+
+          before do
+            stub_feature_flags(enforced_sso_expiry: false)
+          end
+
+          it { is_expected.to be_falsey }
+        end
+
+        context 'without group' do
+          let(:root_group) { nil }
+
+          it { is_expected.to be_falsey }
+        end
+
+        context 'without saml_provider' do
+          let(:root_group) { create(:group) }
+
+          it { is_expected.to be_falsey }
+        end
+
+        context 'with admin', :enable_admin_mode do
+          let(:user) { create(:user, :admin) }
+
+          it { is_expected.to be_falsey }
+        end
+
+        context 'with auditor' do
+          let(:user) { create(:user, :auditor) }
+
+          it { is_expected.to be_falsey }
+        end
+
+        context 'with group owner' do
+          before do
+            root_group.add_owner(user)
+          end
+
+          it { is_expected.to be_falsey }
+        end
+      end
+
+      context 'without any session' do
+        it { is_expected.to be_truthy }
+
+        context 'with admin', :enable_admin_mode do
+          let(:user) { create(:user, :admin) }
+
+          it { is_expected.to be_falsey }
+        end
+
+        context 'with auditor' do
+          let(:user) { create(:user, :auditor) }
+
+          it { is_expected.to be_falsey }
+        end
+
+        context 'with group owner' do
+          before do
+            root_group.add_owner(user)
+          end
+
+          it { is_expected.to be_falsey }
+        end
+      end
+    end
+
+    context 'when git check is not enforced' do
+      before do
+        allow(saml_provider).to receive(:git_check_enforced?).and_return(false)
+      end
+
+      context 'with an active session', :clean_gitlab_redis_shared_state do
+        let(:session_id) { '42' }
+        let(:stored_session) do
+          { 'active_group_sso_sign_ins' => { saml_provider.id => 5.minutes.ago } }
+        end
+
+        before do
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.set("session:gitlab:#{session_id}", Marshal.dump(stored_session))
+            redis.sadd("session:lookup:user:gitlab:#{user.id}", [session_id])
+          end
+        end
+
+        it { is_expected.to be_falsey }
+      end
+
+      context 'without any session' do
+        it { is_expected.to be_falsey }
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/git_access_spec.rb

@@ -915,6 +915,55 @@ RSpec.describe Gitlab::GitAccess do
     end
   end
 
+  describe '#check_sso_session!' do
+    before do
+      project.add_developer(user)
+    end
+
+    context 'with project without group' do
+      it 'allows pull and push changes' do
+        expect(Gitlab::Auth::GroupSaml::SessionEnforcer).to receive(:new).with(user, nil).and_return(double(access_restricted?: false))
+        pull_changes
+      end
+    end
+
+    context 'with project with group' do
+      let_it_be(:group) { create(:group) }
+
+      before do
+        project.update!(namespace: group)
+      end
+
+      context 'user with a sso session' do
+        let(:access_restricted?) { false }
+
+        it 'allows pull and push changes' do
+          expect(Gitlab::Auth::GroupSaml::SessionEnforcer).to receive(:new).with(user, group).twice.and_return(double(access_restricted?: access_restricted?))
+
+          expect { pull_changes }.not_to raise_error
+          expect { push_changes }.not_to raise_error
+        end
+      end
+
+      context 'user without a sso session' do
+        let(:access_restricted?) { true }
+
+        before do
+          expect(Gitlab::Auth::GroupSaml::SessionEnforcer).to receive(:new).with(user, group).twice.and_return(double(access_restricted?: access_restricted?))
+        end
+
+        it 'does not allow pull or push changes with proper url in the message' do
+          aggregate_failures do
+            address = "http://localhost/groups/#{group.name}/-/saml/sso"
+
+            expect { pull_changes }.to raise_error(Gitlab::GitAccess::ForbiddenError, /#{Regexp.quote(address)}/)
+            expect { push_changes }.to raise_error(Gitlab::GitAccess::ForbiddenError, /#{Regexp.quote(address)}/)
+          end
+        end
+      end
+    end
+  end
+
   describe '#check_maintenance_mode!' do
     let(:changes) { Gitlab::GitAccess::ANY }
 

ee/spec/models/saml_provider_spec.rb

@@ -98,6 +98,27 @@ RSpec.describe SamlProvider do
         end
       end
     end
+
+    describe 'git_check_enforced' do
+      let_it_be(:group) { create(:group) }
+
+      context 'sso is enforced' do
+        it 'git_check_enforced is valid' do
+          expect(build(:saml_provider, group: group, enabled: true, enforced_sso: true, git_check_enforced: true)).to be_valid
+          expect(build(:saml_provider, group: group, enabled: true, enforced_sso: true, git_check_enforced: false)).to be_valid
+        end
+      end
+
+      context 'sso is not enforced' do
+        it 'git_check_enforced is invalid when set to true' do
+          expect(build(:saml_provider, group: group, enabled: true, enforced_sso: false, git_check_enforced: true)).to be_invalid
+        end
+
+        it 'git_check_enforced is valid when set to false' do
+          expect(build(:saml_provider, group: group, enabled: true, enforced_sso: false, git_check_enforced: false)).to be_valid
+        end
+      end
+    end
   end
 
   describe 'Default values' do
@@ -216,6 +237,34 @@ RSpec.describe SamlProvider do
     end
   end
 
+  describe '#git_check_enforced?' do
+    context 'without enforced sso' do
+      before do
+        allow(subject).to receive(:enforced_sso?).and_return(false)
+      end
+
+      it 'does not enforce git activity check' do
+        subject.git_check_enforced = true
+        expect(subject).not_to be_git_check_enforced
+        subject.git_check_enforced = false
+        expect(subject).not_to be_git_check_enforced
+      end
+    end
+
+    context 'with enforced sso' do
+      before do
+        allow(subject).to receive(:enforced_sso?).and_return(true)
+      end
+
+      it 'enforces git activity check when attribute is set to true' do
+        subject.git_check_enforced = true
+        expect(subject).to be_git_check_enforced
+        subject.git_check_enforced = false
+        expect(subject).not_to be_git_check_enforced
+      end
+    end
+  end
+
   describe '#prohibited_outer_forks?' do
     context 'without enforced GMA' do
       it 'is false when prohibited_outer_forks flag value is true' do

lib/gitlab/git_access.rb

@@ -91,6 +91,7 @@ module Gitlab
       when *PUSH_COMMANDS
         check_push_access!
       end
+      check_additional_conditions!
 
       success_result
     end
@@ -530,6 +531,10 @@ module Gitlab
     def size_checker
       container.repository_size_checker
     end
+
+    # overriden in EE
+    def check_additional_conditions!
+    end
   end
 end
 

locale/gitlab.pot

@@ -14851,9 +14851,15 @@ msgstr ""
 msgid "GroupSAML|Are you sure you want to remove the SAML group link?"
 msgstr ""
 
+msgid "GroupSAML|Before enforcing SSO, enable SAML authentication."
+msgstr ""
+
 msgid "GroupSAML|Certificate fingerprint"
 msgstr ""
 
+msgid "GroupSAML|Check SSO on git activity"
+msgstr ""
+
 msgid "GroupSAML|Configuration"
 msgstr ""
 
@@ -14869,7 +14875,10 @@ msgstr ""
 msgid "GroupSAML|Enable SAML authentication for this group."
 msgstr ""
 
-msgid "GroupSAML|Enforce SSO-only authentication for this group."
+msgid "GroupSAML|Enforce SSO-access for Git in this group."
+msgstr ""
+
+msgid "GroupSAML|Enforce SSO-only authentication for web activity for this group."
 msgstr ""
 
 msgid "GroupSAML|Enforce users to have dedicated group managed accounts for this group."
@@ -14965,9 +14974,6 @@ msgstr ""
 msgid "GroupSAML|This will be set as the access level of users added to the group."
 msgstr ""
 
-msgid "GroupSAML|To be able to enable enforced SSO, you first need to enable SAML authentication."
-msgstr ""
-
 msgid "GroupSAML|To be able to enable group managed accounts, you first need to enable enforced SSO."
 msgstr ""