Merge branch '9152-check-sso-status-on-git-activity-and-direct-user-to-sso' into 'master'

Resolve "Group SAML - Check SSO status on Git activity"

See merge request gitlab-org/gitlab!56867

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

@@ -17323,7 +17323,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 if they wish to access group resources through the UI. Users can't be manually added as members.
 
@@ -104,9 +105,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

@@ -14957,9 +14957,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 ""
 
@@ -14975,7 +14981,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."
@@ -15071,9 +15080,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 ""