From d76e176fdcc668ca356eed743b64900a801a19d6 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Thu, 24 Aug 2023 16:32:53 +0200
Subject: [PATCH 01/16] Add SAML Auth for Approvals

- The frontend needs to be aware of whether SAML auth for MR
  approvals is active and if so redirect directly to the SAML IdP for
  reauthentication. That request needs to have a custom callback defined
  after the user re-authenticats that will attempt to approve the MR
  again and then redirect to the MR(/show view).
  Doing it this way keeps some complexity out of the API controllers.

- The return status of the MergeRequestApproval service was changed.
  Using ServiceResponse for consistency.

- Add custom callback controller to handle IdP response

- Cleanup: Remove superfluous represent_blocking_mr from
  MR widget entity. Last calling code was removed in
  0465f0cf8d1061f6012febb1925fbbe82d5e16e6

- Refs:
  https://gitlab.com/groups/gitlab-org/-/epics/11084
  https://gitlab.com/gitlab-org/gitlab/-/issues/421959
  https://gitlab.com/gitlab-org/gitlab/-/issues/421961

Changelog: added
EE: true
---
 .../components/approvals/approvals.vue        | 20 ++++-
 .../stores/mr_widget_store.js                 |  2 +
 .../approve_with_saml_controller.rb           | 54 +++++++++++++
 ee/app/models/ee/project.rb                   | 11 +++
 .../presenters/ee/merge_request_presenter.rb  | 29 +++++++
 .../ee/merge_request_widget_entity.rb         | 10 ++-
 .../ee/merge_requests/approval_service.rb     |  2 -
 .../merge_requests/saml_approval_service.rb   | 43 +++++++++++
 ee/config/routes/merge_requests.rb            |  1 +
 .../resolver.rb                               | 15 +++-
 .../user_approves_with_saml_auth_spec.rb      | 75 +++++++++++++++++++
 ee/spec/frontend/fixtures/merge_requests.rb   |  8 +-
 .../resolver_spec.rb                          | 28 ++++++-
 ee/spec/models/ee/project_spec.rb             | 39 ++++++++++
 .../merge_request_widget_entity_spec.rb       |  8 ++
 .../merge_requests/approval_service_spec.rb   |  1 +
 locale/gitlab.pot                             | 15 ++++
 spec/frontend/fixtures/merge_requests.rb      |  8 +-
 .../components/approvals/approvals_spec.js    | 60 +++++++++++++++
 .../api/merge_request_approvals_spec.rb       |  2 -
 20 files changed, 418 insertions(+), 13 deletions(-)
 create mode 100644 ee/app/controllers/projects/merge_requests/approve_with_saml_controller.rb
 create mode 100644 ee/app/services/ee/merge_requests/saml_approval_service.rb
 create mode 100644 ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue b/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue
index a29393d9f93c44..1bcb7d026b9b4c 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue
@@ -1,6 +1,7 @@
 <script>
 import { GlButton, GlSprintf } from '@gitlab/ui';
 import { createAlert } from '~/alert';
+import { visitUrl } from '~/lib/utils/url_utility';
 import { STATUS_MERGED } from '~/issues/constants';
 import { BV_SHOW_MODAL } from '~/lib/utils/constants';
 import { HTTP_STATUS_UNAUTHORIZED } from '~/lib/utils/http_status';
@@ -114,6 +115,13 @@ export default {
       return this.userHasApproved && !this.userCanApprove && this.mr.state !== STATUS_MERGED;
     },
     approvalText() {
+      // Repeating a text of this to keep i18n easier to do (vs, construcing a compound string)
+      if (this.requireSamlAuthToApprove) {
+        return this.isApproved && this.approvedBy.length > 0
+          ? s__('mrWidget|Approve additionally with SAML')
+          : s__('mrWidget|Approve with SAML');
+      }
+
       return this.isApproved && this.approvedBy.length > 0
         ? s__('mrWidget|Approve additionally')
         : s__('mrWidget|Approve');
@@ -161,6 +169,9 @@ export default {
         .join(', ')
         .concat('.');
     },
+    requireSamlAuthToApprove() {
+      return this.mr.requireSamlAuthToApprove;
+    },
   },
   methods: {
     approve() {
@@ -168,7 +179,10 @@ export default {
         this.$root.$emit(BV_SHOW_MODAL, this.modalId);
         return;
       }
-
+      if (this.requireSamlAuthToApprove) {
+        this.approveWithSamlAuth();
+        return;
+      }
       this.updateApproval(
         () => this.service.approveMergeRequest(),
         () =>
@@ -179,6 +193,10 @@ export default {
           ),
       );
     },
+    approveWithSamlAuth() {
+      // Intentionally direct to SAML Identity Provider for renewed authorization even if SSO session exists
+      visitUrl(this.mr.samlApprovalPath);
+    },
     approveWithAuth(data) {
       this.updateApproval(
         () => this.service.approveMergeRequestWithAuth(data),
diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
index 9dd1f4ad4603fb..1c56ec101c11ee 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js
@@ -29,6 +29,7 @@ export default class MergeRequestStore extends CEMergeRequestStore {
     this.appUrl = gon && gon.gitlab_url;
     this.licenseScanning = data.license_scanning;
     this.requirePasswordToApprove = data.require_password_to_approve;
+    this.requireSamlAuthToApprove = data.require_saml_auth_to_approve;
     this.mergeRequestApproversAvailable = data.merge_request_approvers_available;
     this.aiCommitMessageEnabled = data.aiCommitMessageEnabled;
 
@@ -62,6 +63,7 @@ export default class MergeRequestStore extends CEMergeRequestStore {
 
     this.discoverProjectSecurityPath = data.discover_project_security_path;
     this.apiStatusChecksPath = data.api_status_checks_path;
+    this.samlApprovalPath = data.saml_approval_path;
 
     // Security scan diff paths
     this.containerScanningComparisonPath = data.container_scanning_comparison_path;
diff --git a/ee/app/controllers/projects/merge_requests/approve_with_saml_controller.rb b/ee/app/controllers/projects/merge_requests/approve_with_saml_controller.rb
new file mode 100644
index 00000000000000..19e96644e436ae
--- /dev/null
+++ b/ee/app/controllers/projects/merge_requests/approve_with_saml_controller.rb
@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+module Projects
+  module MergeRequests
+    class ApproveWithSamlController < Projects::ApplicationController
+      feature_category :code_review_workflow
+
+      def approve_with_saml
+        return render_404 unless merge_request
+
+        result = ::EE::MergeRequests::SamlApprovalService
+          .new(project: project, current_user: current_user)
+          .execute(merge_request)
+
+        if result.success?
+          flash[:notice] = _("Approved")
+        else
+          flash[:alert] = _("Approval rejected")
+        end
+
+        redirect_to(
+          namespace_project_merge_request_path(
+            id: merge_request_iid,
+            project_id: merge_request.project,
+            namespace_id: merge_request.project.namespace
+          )
+        )
+      end
+
+      private
+
+      def project_id
+        project.id
+      end
+
+      def group_id
+        project.group.id
+      end
+
+      def merge_request_iid
+        params.permit(:id).fetch(:id)
+      end
+
+      def merge_request
+        @merge_request ||= MergeRequestsFinder.new(
+          current_user,
+          group_id: group_id,
+          project_id: project_id,
+          iids: [merge_request_iid]
+        ).execute&.first
+      end
+    end
+  end
+end
diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb
index 92854dd51d3293..f248adc75c035f 100644
--- a/ee/app/models/ee/project.rb
+++ b/ee/app/models/ee/project.rb
@@ -832,6 +832,17 @@ def require_password_to_approve?
       !!require_password_to_approve
     end
 
+    def require_saml_auth_to_approve
+      ComplianceManagement::MergeRequestApprovalSettings::Resolver
+        .new(group&.root_ancestor, project: self)
+        .require_saml_auth_to_approve
+        .value
+    end
+
+    def require_saml_auth_to_approve?
+      !!require_saml_auth_to_approve
+    end
+
     def find_path_lock(path, exact_match: false, downstream: false)
       path_lock_finder = strong_memoize(:path_lock_finder) do
         ::Gitlab::PathLocksFinder.new(self)
diff --git a/ee/app/presenters/ee/merge_request_presenter.rb b/ee/app/presenters/ee/merge_request_presenter.rb
index 1b86e5fbbf976a..b59498202c7543 100644
--- a/ee/app/presenters/ee/merge_request_presenter.rb
+++ b/ee/app/presenters/ee/merge_request_presenter.rb
@@ -66,8 +66,37 @@ def issue_keys
       ).issue_keys
     end
 
+    def saml_approval_path
+      return unless group.is_a?(Group) # feature does not work for personal namespaces
+
+      return unless group_requires_saml_auth_for_approval?
+
+      expose_path sso_group_saml_providers_path(
+        group,
+        token: group.saml_discovery_token,
+        redirect: saml_approval_redirect_path
+      )
+    end
+
+    def require_saml_auth_to_approve
+      group_requires_saml_auth_for_approval?
+    end
+
     private
 
+    def group
+      target_project.namespace
+    end
+
+    def group_requires_saml_auth_for_approval?
+      !!ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group).require_saml_auth_to_approve.value
+    end
+
+    def saml_approval_redirect_path
+      # Will not work with URL since the SSO controller will sanatize it
+      approve_with_saml_namespace_project_merge_request_path(group, target_project, merge_request.iid)
+    end
+
     def expose_mr_status_checks?
       current_user.present? &&
         project.external_status_checks.applicable_to_branch(merge_request.target_branch).any?
diff --git a/ee/app/serializers/ee/merge_request_widget_entity.rb b/ee/app/serializers/ee/merge_request_widget_entity.rb
index cfb847cb229795..31822e0790f223 100644
--- a/ee/app/serializers/ee/merge_request_widget_entity.rb
+++ b/ee/app/serializers/ee/merge_request_widget_entity.rb
@@ -108,12 +108,14 @@ module MergeRequestWidgetEntity
       expose :merge_immediately_docs_path do |merge_request|
         presenter(merge_request).merge_immediately_docs_path
       end
-    end
 
-    def represent_blocking_mr(blocking_mr)
-      blocking_mr_options = options.merge(from_project: object.target_project)
+      expose :saml_approval_path do |merge_request|
+        presenter(merge_request).saml_approval_path
+      end
 
-      ::BlockingMergeRequestEntity.represent(blocking_mr, blocking_mr_options)
+      expose :require_saml_auth_to_approve do |merge_request|
+        presenter(merge_request).require_saml_auth_to_approve
+      end
     end
   end
 end
diff --git a/ee/app/services/ee/merge_requests/approval_service.rb b/ee/app/services/ee/merge_requests/approval_service.rb
index b28fe283820fdb..5fc2b738005ff8 100644
--- a/ee/app/services/ee/merge_requests/approval_service.rb
+++ b/ee/app/services/ee/merge_requests/approval_service.rb
@@ -5,8 +5,6 @@ module MergeRequests
     module ApprovalService
       extend ::Gitlab::Utils::Override
 
-      IncorrectApprovalPasswordError = Class.new(StandardError)
-
       override :execute
       def execute(merge_request)
         return if incorrect_approval_password?(merge_request)
diff --git a/ee/app/services/ee/merge_requests/saml_approval_service.rb b/ee/app/services/ee/merge_requests/saml_approval_service.rb
new file mode 100644
index 00000000000000..0ed2f4a33ac579
--- /dev/null
+++ b/ee/app/services/ee/merge_requests/saml_approval_service.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+module EE
+  module MergeRequests
+    class SamlApprovalService < ::MergeRequests::ApprovalService
+      SAML_APPROVE_TIMEOUT = 5.seconds
+      SAMLApprovalTimeoutError = Class.new(StandardError)
+
+      def execute(merge_request)
+        if approval_requires_saml_auth?(merge_request) && !saml_approval_in_time?
+          # catch this up the chain an redirect to the SAML IdP with callback to re-approve
+          return ServiceResponse.error(
+            message: _("ComplianceManagment|SAML Auth timeout reached for approval"),
+            reason: 'saml_auth_for_approval_timed_out')
+        end
+
+        # Ancesotr class returns non-standard service response, we are wrapping it here
+        unless super
+          return ServiceResponse.error(
+            message: _("ComplianceManagment|You are not eligble to approve this merge request."),
+            reason: 'approval_service_uneligible'
+          )
+        end
+
+        ServiceResponse.success(payload: merge_request)
+      end
+
+      private
+
+      def approval_requires_saml_auth?(merge_request)
+        merge_request.project.require_saml_auth_to_approve?
+      end
+
+      def saml_provider
+        project.group.root_saml_provider
+      end
+
+      def saml_approval_in_time?
+        ::Gitlab::Auth::GroupSaml::SsoState.new(saml_provider.id).active_since?(SAML_APPROVE_TIMEOUT.ago)
+      end
+    end
+  end
+end
diff --git a/ee/config/routes/merge_requests.rb b/ee/config/routes/merge_requests.rb
index 517e8a8075db26..06b302e5527a1b 100644
--- a/ee/config/routes/merge_requests.rb
+++ b/ee/config/routes/merge_requests.rb
@@ -15,6 +15,7 @@
     get :coverage_fuzzing_reports
     get :api_fuzzing_reports
     get :security_reports
+    get :approve_with_saml, action: :approve_with_saml, controller: 'merge_requests/approve_with_saml'
 
     post :rebase
   end
diff --git a/ee/lib/compliance_management/merge_request_approval_settings/resolver.rb b/ee/lib/compliance_management/merge_request_approval_settings/resolver.rb
index ad704c994f3013..2e87c1f6ec523f 100644
--- a/ee/lib/compliance_management/merge_request_approval_settings/resolver.rb
+++ b/ee/lib/compliance_management/merge_request_approval_settings/resolver.rb
@@ -14,7 +14,8 @@ def execute
           allow_overrides_to_approver_list_per_merge_request: allow_overrides_to_approver_list_per_merge_request,
           retain_approvals_on_push: retain_approvals_on_push,
           selective_code_owner_removals: selective_code_owner_removals,
-          require_password_to_approve: require_password_to_approve
+          require_password_to_approve: require_password_to_approve,
+          require_saml_auth_to_approve: require_saml_auth_to_approve
         }
       end
 
@@ -70,6 +71,16 @@ def require_password_to_approve
         )
       end
 
+      def require_saml_auth_to_approve
+        group_value = group_settings&.require_saml_auth_to_approve
+
+        ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
+          value: [group_value].any?,
+          locked: group_value,
+          inherited_from: :group
+        )
+      end
+
       private
 
       def instance_settings
@@ -77,6 +88,8 @@ def instance_settings
       end
 
       def group_settings
+        return unless @group.is_a? ::Group
+
         @group&.group_merge_request_approval_setting
       end
 
diff --git a/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb b/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
new file mode 100644
index 00000000000000..53e514178e9156
--- /dev/null
+++ b/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
@@ -0,0 +1,75 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Merge request > User approves with SAML auth', :js, feature_category: :code_review_workflow do
+  let_it_be(:user) { create :user }
+  let_it_be(:group) { create :group }
+  let_it_be(:setting) { create :group_merge_request_approval_setting, require_saml_auth_to_approve: true, group: group }
+  let_it_be(:project) do
+    create :project,
+      :public,
+      :repository,
+      group: group,
+      approvals_before_merge: 1,
+      merge_requests_author_approval: true
+  end
+
+  let_it_be(:owner) { project.first_owner }
+  let_it_be(:merge_request) { create :merge_request_with_diffs, source_project: project, reviewers: [user] }
+
+  # This emulates a successful response by the SAML provider
+  around do |example|
+    with_omniauth_full_host { example.run }
+  end
+
+  before_all do
+    group.add_developer(user)
+    project.add_maintainer(user)
+  end
+
+  before do
+    stub_default_url_options(protocol: "https")
+    stub_licensed_features(group_saml: true)
+    create(:saml_provider, group: group, enabled: true)
+    mock_group_saml(uid: '1')
+  end
+
+  def sign_in_and_connect_saml_identity
+    # new users have to link their account to their SAML identiiy provider first
+    sign_in(user)
+    visit sso_group_saml_providers_path group_id: group, token: group.saml_discovery_token
+    wait_for_requests
+    page.within('.login-box') { click_link 'Authorize' }
+    wait_for_requests
+  end
+
+  def approve_with_saml
+    visit project_merge_request_path(project, merge_request)
+    page.within('.js-mr-approvals') { click_button 'Approve with SAML' }
+    wait_for_requests
+  end
+
+  def unapprove
+    click_button 'Revoke approval'
+    wait_for_requests
+  end
+
+  it 'shows user can approve and unapprove' do
+    sign_in_and_connect_saml_identity
+
+    approve_with_saml
+
+    expect(page).to have_text('Approved')
+
+    page.within('.js-mr-approvals') do
+      expect(page).not_to have_button('approve with saml')
+      expect(page).to have_text('Approved by you')
+    end
+
+    unapprove
+
+    expect(page).to have_button('Approve with SAML')
+    expect(page).not_to have_text('Approved by')
+  end
+end
diff --git a/ee/spec/frontend/fixtures/merge_requests.rb b/ee/spec/frontend/fixtures/merge_requests.rb
index cbbe50dc0cbccf..94532d3114d990 100644
--- a/ee/spec/frontend/fixtures/merge_requests.rb
+++ b/ee/spec/frontend/fixtures/merge_requests.rb
@@ -2,7 +2,13 @@
 
 require 'spec_helper'
 
-RSpec.describe Projects::MergeRequestsController, '(JavaScript fixtures in EE context)', type: :controller do
+RSpec
+  .describe(
+    Projects::MergeRequestsController,
+    '(JavaScript fixtures in EE context)',
+    type: :controller,
+    feature_category: :code_review_workflow
+  ) do
   include JavaScriptFixturesHelpers
 
   let(:project) { create(:project, :repository, path: 'merge-requests-project') }
diff --git a/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb b/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb
index 2bb96225212d67..5f955b1bf47820 100644
--- a/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb
+++ b/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe ::ComplianceManagement::MergeRequestApprovalSettings::Resolver do
+RSpec.describe ::ComplianceManagement::MergeRequestApprovalSettings::Resolver, feature_category: :compliance_management do
   using RSpec::Parameterized::TableSyntax
 
   let_it_be(:group) { create(:group) }
@@ -224,4 +224,30 @@
       it_behaves_like 'a MR approval setting'
     end
   end
+
+  describe '#require_saml_auth_to_approve' do
+    where(:group_requires_saml_auth, :locked, :value, :inherited_from) do
+      true  | true  | true  | :group
+      false | false | false | :group
+      nil   | false | false | :group
+    end
+
+    with_them do
+      before do
+        group.group_merge_request_approval_setting.update!(require_saml_auth_to_approve: !!group_requires_saml_auth)
+      end
+
+      context "with project" do
+        let(:object) { described_class.new(group, project: project).require_saml_auth_to_approve }
+
+        it_behaves_like 'a MR approval setting'
+      end
+
+      context "without project" do
+        let(:object) { described_class.new(group).require_saml_auth_to_approve }
+
+        it_behaves_like 'a MR approval setting'
+      end
+    end
+  end
 end
diff --git a/ee/spec/models/ee/project_spec.rb b/ee/spec/models/ee/project_spec.rb
index fbf7a5648b1f18..31bee24f46bc95 100644
--- a/ee/spec/models/ee/project_spec.rb
+++ b/ee/spec/models/ee/project_spec.rb
@@ -1199,6 +1199,45 @@
       end
     end
 
+    describe '#require_saml_to_approve?' do
+      let_it_be(:root_ancestor) { create(:group) }
+      let_it_be(:sub_group) { create(:group, parent: root_ancestor) }
+
+      before do
+        allow(project).to receive(:group).and_return(sub_group)
+      end
+
+      it 'returns true when the resolver returns true' do
+        expect(ComplianceManagement::MergeRequestApprovalSettings::Resolver).to receive(:new).with(root_ancestor, project: project)
+
+        allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver|
+          allow(resolver).to receive(:require_saml_auth_to_approve)
+            .and_return(ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
+              value: true,
+              locked: false,
+              inherited_from: nil
+            ))
+        end
+
+        expect(project.require_saml_auth_to_approve).to be true
+      end
+
+      it 'returns false when the resolver returns false' do
+        expect(ComplianceManagement::MergeRequestApprovalSettings::Resolver).to receive(:new).with(root_ancestor, project: project)
+
+        allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver|
+          allow(resolver).to receive(:require_saml_auth_to_approve)
+            .and_return(ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
+              value: false,
+              locked: false,
+              inherited_from: nil
+            ))
+        end
+
+        expect(project.require_saml_auth_to_approve).to be false
+      end
+    end
+
     describe '#merge_requests_author_approval' do
       let(:setting) { :merge_requests_author_approval }
       let(:application_setting) { :prevent_merge_requests_author_approval }
diff --git a/ee/spec/serializers/merge_request_widget_entity_spec.rb b/ee/spec/serializers/merge_request_widget_entity_spec.rb
index a9bc34a8d1939f..9e9f0e41327969 100644
--- a/ee/spec/serializers/merge_request_widget_entity_spec.rb
+++ b/ee/spec/serializers/merge_request_widget_entity_spec.rb
@@ -289,4 +289,12 @@ def create_all_artifacts
 
     expect(subject.as_json).to include(:pipeline_id)
   end
+
+  it 'exposes saml_approval_path', :request_store do
+    expect(subject.as_json).to include(:saml_approval_path)
+  end
+
+  it 'exposes require_saml_auth_to_approve' do
+    expect(subject.as_json).to include(:require_saml_auth_to_approve)
+  end
 end
diff --git a/ee/spec/services/merge_requests/approval_service_spec.rb b/ee/spec/services/merge_requests/approval_service_spec.rb
index ce64c664efae94..7e95626c3edf61 100644
--- a/ee/spec/services/merge_requests/approval_service_spec.rb
+++ b/ee/spec/services/merge_requests/approval_service_spec.rb
@@ -39,6 +39,7 @@
       before do
         project.update!(require_password_to_approve: true)
       end
+
       context 'when password not specified' do
         it 'does not update the approvals' do
           expect { service.execute(merge_request) }.not_to change { merge_request.approvals.size }
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 38cadff29e0119..39dc1b452dd82c 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -6094,6 +6094,9 @@ msgstr ""
 msgid "Approval options"
 msgstr ""
 
+msgid "Approval rejected"
+msgstr ""
+
 msgid "Approval rules"
 msgstr ""
 
@@ -12505,6 +12508,12 @@ msgstr ""
 msgid "ComplianceFramework|No pipeline configuration found"
 msgstr ""
 
+msgid "ComplianceManagment|SAML Auth timeout reached for approval"
+msgstr ""
+
+msgid "ComplianceManagment|You are not eligble to approve this merge request."
+msgstr ""
+
 msgid "ComplianceReport|Add framework"
 msgstr ""
 
@@ -57497,6 +57506,12 @@ msgstr ""
 msgid "mrWidget|Approve additionally"
 msgstr ""
 
+msgid "mrWidget|Approve additionally with SAML"
+msgstr ""
+
+msgid "mrWidget|Approve with SAML"
+msgstr ""
+
 msgid "mrWidget|Approved by"
 msgstr ""
 
diff --git a/spec/frontend/fixtures/merge_requests.rb b/spec/frontend/fixtures/merge_requests.rb
index a1896a6470b752..e8272a1f93a529 100644
--- a/spec/frontend/fixtures/merge_requests.rb
+++ b/spec/frontend/fixtures/merge_requests.rb
@@ -2,7 +2,13 @@
 
 require 'spec_helper'
 
-RSpec.describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :controller do
+RSpec
+  .describe(
+    Projects::MergeRequestsController,
+    '(JavaScript fixtures)',
+    type: :controller,
+    feature_category: :code_review_workflow
+  ) do
   include JavaScriptFixturesHelpers
 
   let(:namespace) { create(:namespace, name: 'frontend-fixtures') }
diff --git a/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js b/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js
index 2aed037be6f0fd..c81f4328d2a14f 100644
--- a/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js
+++ b/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js
@@ -4,6 +4,7 @@ import { GlButton, GlSprintf } from '@gitlab/ui';
 import { shallowMount } from '@vue/test-utils';
 import { createMockSubscription as createMockApolloSubscription } from 'mock-apollo-client';
 import approvedByCurrentUser from 'test_fixtures/graphql/merge_requests/approvals/approvals.query.graphql.json';
+import { visitUrl } from '~/lib/utils/url_utility';
 import createMockApollo from 'helpers/mock_apollo_helper';
 import waitForPromises from 'helpers/wait_for_promises';
 import { getIdFromGraphQLId } from '~/graphql_shared/utils';
@@ -28,6 +29,10 @@ jest.mock('~/alert', () => ({
     dismiss: mockAlertDismiss,
   })),
 }));
+jest.mock('~/lib/utils/url_utility', () => ({
+  ...jest.requireActual('~/lib/utils/url_utility'),
+  visitUrl: jest.fn(),
+}));
 
 const TEST_HELP_PATH = 'help/path';
 const testApprovedBy = () => [1, 7, 10].map((id) => ({ id }));
@@ -113,6 +118,7 @@ describe('MRWidget approvals', () => {
       targetProjectFullPath: 'gitlab-org/gitlab',
       id: 1,
       iid: '1',
+      requireSamlAuthToApprove: false,
     };
 
     jest.spyOn(eventHub, '$emit').mockImplementation(() => {});
@@ -172,6 +178,22 @@ describe('MRWidget approvals', () => {
             category: 'primary',
           });
         });
+
+        describe('with SAML auth requried for approval', () => {
+          beforeEach(async () => {
+            const response = createCanApproveResponse();
+            mr.requireSamlAuthToApprove = true;
+            createComponent({}, { query: response });
+            await waitForPromises();
+          });
+          it('approve action is rendered with correct text', () => {
+            expect(findActionData()).toEqual({
+              variant: 'confirm',
+              text: 'Approve with SAML',
+              category: 'primary',
+            });
+          });
+        });
       });
 
       describe('and MR is approved', () => {
@@ -194,6 +216,25 @@ describe('MRWidget approvals', () => {
           });
         });
 
+        describe('with approvers, with SAML auth requried for approval', () => {
+          beforeEach(async () => {
+            canApproveResponse.data.project.mergeRequest.approvedBy.nodes =
+              approvedByCurrentUser.data.project.mergeRequest.approvedBy.nodes;
+            canApproveResponse.data.project.mergeRequest.approvedBy.nodes[0].id = 69;
+            mr.requireSamlAuthToApprove = true;
+            createComponent({}, { query: canApproveResponse });
+            await waitForPromises();
+          });
+
+          it('approve additionally action is rendered with correct text', () => {
+            expect(findActionData()).toEqual({
+              variant: 'confirm',
+              text: 'Approve additionally with SAML',
+              category: 'secondary',
+            });
+          });
+        });
+
         describe('with approvers', () => {
           beforeEach(async () => {
             canApproveResponse.data.project.mergeRequest.approvedBy.nodes =
@@ -215,6 +256,25 @@ describe('MRWidget approvals', () => {
         });
       });
 
+      describe('when SAML auth is required and user clicks Approve with SAML', () => {
+        const fakeGroupSamlPath = '/example_group_saml';
+
+        beforeEach(async () => {
+          mr.requireSamlAuthToApprove = true;
+          mr.samlApprovalPath = fakeGroupSamlPath;
+
+          createComponent({}, { query: createCanApproveResponse() });
+          await waitForPromises();
+        });
+
+        it('redirects the user to the group SAML path', async () => {
+          const action = findAction();
+          action.vm.$emit('click');
+          await nextTick();
+          expect(visitUrl).toHaveBeenCalledWith(fakeGroupSamlPath);
+        });
+      });
+
       describe('when approve action is clicked', () => {
         beforeEach(async () => {
           createComponent({}, { query: canApproveResponse });
diff --git a/spec/requests/api/merge_request_approvals_spec.rb b/spec/requests/api/merge_request_approvals_spec.rb
index df2b20c62c3dd8..2de597502732bc 100644
--- a/spec/requests/api/merge_request_approvals_spec.rb
+++ b/spec/requests/api/merge_request_approvals_spec.rb
@@ -8,8 +8,6 @@
   let_it_be(:bot) { create(:user, :project_bot) }
   let_it_be(:project) { create(:project, :public, :repository, creator: user, namespace: user.namespace) }
   let_it_be(:approver) { create :user }
-  let_it_be(:group) { create :group }
-
   let(:merge_request) { create(:merge_request, :simple, author: user, source_project: project) }
 
   describe 'GET :id/merge_requests/:merge_request_iid/approvals' do
-- 
GitLab


From 80ac7d625e6ce18c4fc08f511c55ca94a1d8bfe4 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Tue, 7 Nov 2023 17:43:15 +0100
Subject: [PATCH 02/16] Add audit event for require_saml_auth_to_approve

- Log changes to require_saml_auth_to_approve column as audit_event
  on group_merge_request_approval_settings
---
 .../audit_event_streaming/audit_event_types.md    |  1 +
 .../group_merge_request_approval_setting.rb       | 15 +++++++++------
 .../require_saml_auth_to_approve_updated.yml      | 10 ++++++++++
 ...quest_approval_setting_changes_auditor_spec.rb |  1 +
 4 files changed, 21 insertions(+), 6 deletions(-)
 create mode 100644 ee/config/audit_events/types/require_saml_auth_to_approve_updated.yml

diff --git a/doc/administration/audit_event_streaming/audit_event_types.md b/doc/administration/audit_event_streaming/audit_event_types.md
index 88212045d8e1fa..5a280ffd6fb28b 100644
--- a/doc/administration/audit_event_streaming/audit_event_types.md
+++ b/doc/administration/audit_event_streaming/audit_event_types.md
@@ -139,6 +139,7 @@ Audit event types belong to the following product categories.
 | [`remove_gpg_key`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111744) | Event triggered when a GPG Key is destroyed| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/373961) |
 | [`repository_download_operation`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111218) | Event triggered when a Git repository for a project is downloaded| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374108) |
 | [`require_password_to_approve_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/102256) | Event triggered on updating require user password for approvals from group merge request setting| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.6](https://gitlab.com/gitlab-org/gitlab/-/issues/373949) |
+| [`require_saml_auth_to_approve_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130204) | Triggered when a user updates the merge request approval setting for requiring SAML re-authentication to approve.| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.6](https://gitlab.com/gitlab-org/gitlab/-/issues/421959) |
 | [`retain_approvals_on_push_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/102256) | Event triggered on updating require new approvals when new commits are added to an MR from group merge request setting| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.6](https://gitlab.com/gitlab-org/gitlab/-/issues/373949) |
 | [`saml_group_links_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/110525) | Event triggered when a SAML Group Link is created| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/373954) |
 | [`saml_group_links_removed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/110525) | Event triggered when a SAML Group Link is destroyed| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/373954) |
diff --git a/ee/app/models/group_merge_request_approval_setting.rb b/ee/app/models/group_merge_request_approval_setting.rb
index 5685c3a5412f45..80362e020eeaef 100644
--- a/ee/app/models/group_merge_request_approval_setting.rb
+++ b/ee/app/models/group_merge_request_approval_setting.rb
@@ -15,12 +15,15 @@ class GroupMergeRequestApprovalSetting < ApplicationRecord
 
   scope :find_or_initialize_by_group, ->(group) { find_or_initialize_by(group: group) }
 
-  AUDIT_LOG_ALLOWLIST = { allow_author_approval: 'prevent merge request approval from authors',
-                          allow_committer_approval: 'prevent merge request approval from committers',
-                          allow_overrides_to_approver_list_per_merge_request:
-                            'prevent users from modifying MR approval rules in merge requests',
-                          retain_approvals_on_push: 'require new approvals when new commits are added to an MR',
-                          require_password_to_approve: 'require user password for approvals' }.freeze
+  AUDIT_LOG_ALLOWLIST = {
+    allow_author_approval: 'prevent merge request approval from authors',
+    allow_committer_approval: 'prevent merge request approval from committers',
+    allow_overrides_to_approver_list_per_merge_request:
+    'prevent users from modifying MR approval rules in merge requests',
+    retain_approvals_on_push: 'require new approvals when new commits are added to an MR',
+    require_password_to_approve: 'require user password for approvals',
+    require_saml_auth_to_approve: 'requrie user to re-authenticate with SAML to approve'
+  }.freeze
 
   def selective_code_owner_removals
     false
diff --git a/ee/config/audit_events/types/require_saml_auth_to_approve_updated.yml b/ee/config/audit_events/types/require_saml_auth_to_approve_updated.yml
new file mode 100644
index 00000000000000..e466b321c84029
--- /dev/null
+++ b/ee/config/audit_events/types/require_saml_auth_to_approve_updated.yml
@@ -0,0 +1,10 @@
+---
+name: require_saml_auth_to_approve_updated
+description: Triggered when a user updates the merge request approval setting for
+  requiring SAML re-authentication to approve.
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/421959
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130204
+feature_category: compliance_management
+milestone: '16.6'
+saved_to_database: true
+streamed: true
diff --git a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb
index 5bc5e488148cf5..15eb0bcbeea74c 100644
--- a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb
+++ b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb
@@ -39,6 +39,7 @@
              allow_committer_approval: false,
              allow_overrides_to_approver_list_per_merge_request: false,
              retain_approvals_on_push: false,
+             require_saml_auth_to_approve: false,
              require_password_to_approve: false)
     end
 
-- 
GitLab


From 9dc63d4f8b3af6ff95ad7bc079fc10b562bdf80a Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Tue, 7 Nov 2023 18:38:41 +0100
Subject: [PATCH 03/16] Expose require_saml_auth_to_approve to API & frontend

- Export setting saml_provider_enabled through project and group
  helpers for frontend to be aware of SAML provider setup

- Expose require_saml_auth_to_approve to API so the frontend can
  read and update the setting
---
 ee/app/helpers/ee/projects_helper.rb          |  8 +++++
 ee/app/helpers/groups/sso_helper.rb           |  6 ++++
 ..._merge_request_approval_settings.html.haml |  2 +-
 .../merge_request_approval_setting.rb         |  1 +
 ee/lib/api/merge_request_approval_settings.rb |  5 ++-
 .../resolver.rb                               |  3 +-
 ee/lib/ee/api/entities/approval_settings.rb   |  3 ++
 ...group_merge_request_approval_settings.json |  5 +++
 ee/spec/helpers/groups/sso_helper_spec.rb     | 34 ++++++++++++++++++-
 ee/spec/helpers/projects_helper_spec.rb       |  4 ++-
 .../merge_request_approval_setting_spec.rb    |  3 +-
 .../resolver_spec.rb                          |  9 ++---
 .../merge_request_approval_settings_spec.rb   |  1 +
 13 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/ee/app/helpers/ee/projects_helper.rb b/ee/app/helpers/ee/projects_helper.rb
index 9350669b53b691..9d1099f7bd6836 100644
--- a/ee/app/helpers/ee/projects_helper.rb
+++ b/ee/app/helpers/ee/projects_helper.rb
@@ -62,6 +62,7 @@ def approvals_app_data(project = @project)
         can_edit: can_modify_approvers.to_s,
         can_modify_author_settings: can_modify_author_settings.to_s,
         can_modify_commiter_settings: can_modify_commiter_settings.to_s,
+        saml_provider_enabled: saml_provider_enabled_for_project?(project).to_s,
         project_path: expose_path(api_v4_projects_path(id: project.id)),
         approvals_path: expose_path(api_v4_projects_merge_request_approval_setting_path(id: project.id)),
         rules_path: expose_path(api_v4_projects_approval_rules_path(id: project.id)),
@@ -75,6 +76,13 @@ def approvals_app_data(project = @project)
       }
     end
 
+    def saml_provider_enabled_for_project?(project)
+      group = project.root_ancestor
+      return false unless group.is_a? Group
+
+      !!group.saml_provider&.enabled?
+    end
+
     def status_checks_app_data(project)
       {
         data: {
diff --git a/ee/app/helpers/groups/sso_helper.rb b/ee/app/helpers/groups/sso_helper.rb
index d2d0474585a124..fae13eeee3333d 100644
--- a/ee/app/helpers/groups/sso_helper.rb
+++ b/ee/app/helpers/groups/sso_helper.rb
@@ -15,4 +15,10 @@ def authorize_gma_conversion_confirm_modal_data(group_name:, phrase:, remove_for
       phrase: phrase
     }
   end
+
+  def saml_provider_enabled?(group = nil)
+    return false unless group.is_a? Group
+
+    !!group.root_ancestor.saml_provider&.enabled?
+  end
 end
diff --git a/ee/app/views/groups/settings/merge_requests/_merge_request_approval_settings.html.haml b/ee/app/views/groups/settings/merge_requests/_merge_request_approval_settings.html.haml
index c796a72aeb2f23..1f58c84aa9aa3e 100644
--- a/ee/app/views/groups/settings/merge_requests/_merge_request_approval_settings.html.haml
+++ b/ee/app/views/groups/settings/merge_requests/_merge_request_approval_settings.html.haml
@@ -5,4 +5,4 @@
 - return unless show_merge_request_approval_settings?(user, group)
 
 - group_merge_request_approval_setting_api_path = expose_path(api_v4_groups_merge_request_approval_setting_path(id: group.id))
-%section#js-merge-request-approval-settings{ data: { default_expanded: expanded, approval_settings_path: group_merge_request_approval_setting_api_path } }
+%section#js-merge-request-approval-settings{ data: { default_expanded: expanded, approval_settings_path: group_merge_request_approval_setting_api_path, saml_provider_enabled: saml_provider_enabled?(group).to_s }}
diff --git a/ee/lib/api/entities/merge_request_approval_setting.rb b/ee/lib/api/entities/merge_request_approval_setting.rb
index e4c7817b2a6cf7..bc17b8c79dd683 100644
--- a/ee/lib/api/entities/merge_request_approval_setting.rb
+++ b/ee/lib/api/entities/merge_request_approval_setting.rb
@@ -9,6 +9,7 @@ class MergeRequestApprovalSetting < Grape::Entity
       expose :retain_approvals_on_push, documentation: { type: 'boolean', example: true }
       expose :selective_code_owner_removals, documentation: { type: 'boolean', example: true }
       expose :require_password_to_approve, documentation: { type: 'boolean', example: true }
+      expose :require_saml_auth_to_approve, documentation: { type: 'boolean', example: true }
     end
   end
 end
diff --git a/ee/lib/api/merge_request_approval_settings.rb b/ee/lib/api/merge_request_approval_settings.rb
index 80541748fe6167..edce897eb8545e 100644
--- a/ee/lib/api/merge_request_approval_settings.rb
+++ b/ee/lib/api/merge_request_approval_settings.rb
@@ -18,13 +18,16 @@ class MergeRequestApprovalSettings < ::API::Base
         optional :selective_code_owner_removals, type: Boolean, desc: 'Reset approvals from Code Owners if their files changed', allow_blank: false
         optional :require_password_to_approve,
           type: Boolean, desc: 'Require approver to authenticate before approving', allow_blank: false
+        optional :require_saml_auth_to_approve,
+          type: Boolean, desc: 'Require approver to re-authenticate with SAML before approving', allow_blank: false
 
         at_least_one_of :allow_author_approval,
           :allow_committer_approval,
           :allow_overrides_to_approver_list_per_merge_request,
           :retain_approvals_on_push,
           :selective_code_owner_removals,
-          :require_password_to_approve
+          :require_password_to_approve,
+          :require_saml_auth_to_approve
       end
     end
 
diff --git a/ee/lib/compliance_management/merge_request_approval_settings/resolver.rb b/ee/lib/compliance_management/merge_request_approval_settings/resolver.rb
index 2e87c1f6ec523f..55622402495a38 100644
--- a/ee/lib/compliance_management/merge_request_approval_settings/resolver.rb
+++ b/ee/lib/compliance_management/merge_request_approval_settings/resolver.rb
@@ -76,7 +76,8 @@ def require_saml_auth_to_approve
 
         ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
           value: [group_value].any?,
-          locked: group_value,
+          # locked: is always true for projects frontend will manually ovrride it for groups
+          locked: true,
           inherited_from: :group
         )
       end
diff --git a/ee/lib/ee/api/entities/approval_settings.rb b/ee/lib/ee/api/entities/approval_settings.rb
index 7e54d530e31553..715815d44c93c6 100644
--- a/ee/lib/ee/api/entities/approval_settings.rb
+++ b/ee/lib/ee/api/entities/approval_settings.rb
@@ -20,6 +20,9 @@ class ApprovalSettings < Grape::Entity
         expose :merge_requests_disable_committers_approval?,
           as: :merge_requests_disable_committers_approval
 
+        expose :requrie_saml_auth_to_approve?,
+          as: :requrie_saml_auth_to_approve
+
         expose :require_password_to_approve?,
           as: :require_password_to_approve
       end
diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/group_merge_request_approval_settings.json b/ee/spec/fixtures/api/schemas/public_api/v4/group_merge_request_approval_settings.json
index ca30308447fc8b..99379b54344212 100644
--- a/ee/spec/fixtures/api/schemas/public_api/v4/group_merge_request_approval_settings.json
+++ b/ee/spec/fixtures/api/schemas/public_api/v4/group_merge_request_approval_settings.json
@@ -30,6 +30,11 @@
       "value": { "type": "boolean" },
       "locked": { "type": "boolean" },
       "inherited_from": { "type": "string" }
+    },
+    "require_saml_auth_to_approve": {
+      "value": { "type": "boolean" },
+      "locked": { "type": "boolean" },
+      "inherited_from": { "type": "string" }
     }
   },
   "additionalProperties": false
diff --git a/ee/spec/helpers/groups/sso_helper_spec.rb b/ee/spec/helpers/groups/sso_helper_spec.rb
index 4ab0bc472affd9..6f416d74145bb0 100644
--- a/ee/spec/helpers/groups/sso_helper_spec.rb
+++ b/ee/spec/helpers/groups/sso_helper_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Groups::SsoHelper do
+RSpec.describe Groups::SsoHelper, feature_category: :shared do
   describe '#authorize_gma_conversion_confirm_modal_data' do
     let_it_be(:group_name) { 'Foo bar' }
     let_it_be(:phrase) { 'gma_user' }
@@ -20,4 +20,36 @@
       })
     end
   end
+
+  describe '#saml_provider_enabled' do
+    using RSpec::Parameterized::TableSyntax
+    context 'without group' do
+      it 'returns false' do
+        expect(helper.saml_provider_enabled?(nil)).to be false
+        expect(helper.saml_provider_enabled?(build(:user_namespace))).to be false
+        expect(helper.saml_provider_enabled?(build(:project))).to be false
+      end
+    end
+
+    context 'with group' do
+      where(:enabled, :result) do
+        true  | true
+        false | false
+      end
+
+      with_them do
+        it 'returns the expected value' do
+          provider = instance_double('SamlProvider')
+          allow(provider).to receive(:enabled?) { enabled }
+
+          group = instance_double('Group')
+          allow(group).to receive(:is_a?).and_return(true)
+          allow(group).to receive(:root_ancestor) { group }
+          allow(group).to receive(:saml_provider) { provider }
+
+          expect(helper.saml_provider_enabled?(group)).to eq(result)
+        end
+      end
+    end
+  end
 end
diff --git a/ee/spec/helpers/projects_helper_spec.rb b/ee/spec/helpers/projects_helper_spec.rb
index e127ecae248584..6174357110ff96 100644
--- a/ee/spec/helpers/projects_helper_spec.rb
+++ b/ee/spec/helpers/projects_helper_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe ProjectsHelper do
+RSpec.describe ProjectsHelper, feature_category: :shared do
   include ::EE::GeoHelpers
 
   let_it_be_with_refind(:project) { create(:project) }
@@ -548,6 +548,7 @@
     before do
       allow(helper).to receive(:current_user).and_return(user)
       allow(helper).to receive(:can?).and_return(true)
+      allow(helper).to receive(:saml_provider_enabled?).and_return(true)
     end
 
     it 'returns the correct data' do
@@ -556,6 +557,7 @@
         can_edit: 'true',
         can_modify_author_settings: 'true',
         can_modify_commiter_settings: 'true',
+        saml_provider_enabled: 'true',
         approvals_path: expose_path(api_v4_projects_merge_request_approval_setting_path(id: project.id)),
         project_path: expose_path(api_v4_projects_path(id: project.id)),
         rules_path: expose_path(api_v4_projects_approval_rules_path(id: project.id)),
diff --git a/ee/spec/lib/api/entities/merge_request_approval_setting_spec.rb b/ee/spec/lib/api/entities/merge_request_approval_setting_spec.rb
index 76c4e0e3e0ec82..73abac1179d447 100644
--- a/ee/spec/lib/api/entities/merge_request_approval_setting_spec.rb
+++ b/ee/spec/lib/api/entities/merge_request_approval_setting_spec.rb
@@ -15,7 +15,8 @@
         :allow_overrides_to_approver_list_per_merge_request,
         :retain_approvals_on_push,
         :selective_code_owner_removals,
-        :require_password_to_approve
+        :require_password_to_approve,
+        :require_saml_auth_to_approve
       ]
     )
   end
diff --git a/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb b/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb
index 5f955b1bf47820..a0ee82065ad6b8 100644
--- a/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb
+++ b/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb
@@ -226,10 +226,11 @@
   end
 
   describe '#require_saml_auth_to_approve' do
-    where(:group_requires_saml_auth, :locked, :value, :inherited_from) do
-      true  | true  | true  | :group
-      false | false | false | :group
-      nil   | false | false | :group
+    where(:group_requires_saml_auth, :project_requires_saml_auth, :locked, :value, :inherited_from) do
+      true  | true | true | true  | :group
+      false | true | true | false | :group
+      nil   | true | true | false | :group
+      nil   | true | true | false | :group
     end
 
     with_them do
diff --git a/ee/spec/requests/api/merge_request_approval_settings_spec.rb b/ee/spec/requests/api/merge_request_approval_settings_spec.rb
index e2af870fef2297..496c8ccd0f257d 100644
--- a/ee/spec/requests/api/merge_request_approval_settings_spec.rb
+++ b/ee/spec/requests/api/merge_request_approval_settings_spec.rb
@@ -95,6 +95,7 @@
           expect(json_response['retain_approvals_on_push']['value']).to eq(true)
           expect(json_response['selective_code_owner_removals']['value']).to eq(false)
           expect(json_response['require_password_to_approve']['value']).to eq(false)
+          expect(json_response['require_saml_auth_to_approve']['value']).to eq(false)
         end
       end
 
-- 
GitLab


From b6513dc4e5c05b0fe3bc681b2203440bfb7829b5 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Tue, 7 Nov 2023 23:18:55 +0100
Subject: [PATCH 04/16] Fix circumvention of SAML auth via API approval

- A previous refactoring made it so that approvals via the API endpoints
  would bypass the SAML requirement.

- Remove SamlApprovalService again since all code has been folded back
  into EE::MergeRequests::ApprovalService

- Refs:
  https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130204#note_1635049955
  https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130204#note_1635073006
---
 ...roller.rb => saml_approvals_controller.rb} | 10 +--
 .../presenters/ee/merge_request_presenter.rb  |  2 +-
 .../ee/merge_requests/approval_service.rb     | 22 ++++-
 .../merge_requests/saml_approval_service.rb   | 43 ----------
 ..._merge_request_approval_settings.html.haml |  2 +-
 ee/config/routes/merge_requests.rb            |  4 +-
 ...group_merge_request_approval_settings.json | 84 ++++++++++++++-----
 .../merge_requests/approval_service_spec.rb   | 66 ++++++++++++++-
 locale/gitlab.pot                             | 11 +--
 9 files changed, 160 insertions(+), 84 deletions(-)
 rename ee/app/controllers/projects/merge_requests/{approve_with_saml_controller.rb => saml_approvals_controller.rb} (81%)
 delete mode 100644 ee/app/services/ee/merge_requests/saml_approval_service.rb

diff --git a/ee/app/controllers/projects/merge_requests/approve_with_saml_controller.rb b/ee/app/controllers/projects/merge_requests/saml_approvals_controller.rb
similarity index 81%
rename from ee/app/controllers/projects/merge_requests/approve_with_saml_controller.rb
rename to ee/app/controllers/projects/merge_requests/saml_approvals_controller.rb
index 19e96644e436ae..6f727d5445f3fe 100644
--- a/ee/app/controllers/projects/merge_requests/approve_with_saml_controller.rb
+++ b/ee/app/controllers/projects/merge_requests/saml_approvals_controller.rb
@@ -2,20 +2,20 @@
 
 module Projects
   module MergeRequests
-    class ApproveWithSamlController < Projects::ApplicationController
+    class SamlApprovalsController < Projects::ApplicationController
       feature_category :code_review_workflow
 
-      def approve_with_saml
+      def create
         return render_404 unless merge_request
 
-        result = ::EE::MergeRequests::SamlApprovalService
+        result = ::MergeRequests::ApprovalService
           .new(project: project, current_user: current_user)
           .execute(merge_request)
 
-        if result.success?
+        if result
           flash[:notice] = _("Approved")
         else
-          flash[:alert] = _("Approval rejected")
+          flash[:alert] = _("Approval rejected.")
         end
 
         redirect_to(
diff --git a/ee/app/presenters/ee/merge_request_presenter.rb b/ee/app/presenters/ee/merge_request_presenter.rb
index b59498202c7543..9cd0c1788e277f 100644
--- a/ee/app/presenters/ee/merge_request_presenter.rb
+++ b/ee/app/presenters/ee/merge_request_presenter.rb
@@ -94,7 +94,7 @@ def group_requires_saml_auth_for_approval?
 
     def saml_approval_redirect_path
       # Will not work with URL since the SSO controller will sanatize it
-      approve_with_saml_namespace_project_merge_request_path(group, target_project, merge_request.iid)
+      saml_approval_namespace_project_merge_request_path(group, target_project, merge_request.iid)
     end
 
     def expose_mr_status_checks?
diff --git a/ee/app/services/ee/merge_requests/approval_service.rb b/ee/app/services/ee/merge_requests/approval_service.rb
index 5fc2b738005ff8..fa7d9015147e88 100644
--- a/ee/app/services/ee/merge_requests/approval_service.rb
+++ b/ee/app/services/ee/merge_requests/approval_service.rb
@@ -4,10 +4,14 @@ module EE
   module MergeRequests
     module ApprovalService
       extend ::Gitlab::Utils::Override
+      SAML_APPROVE_TIMEOUT = 5.seconds
 
       override :execute
       def execute(merge_request)
-        return if incorrect_approval_password?(merge_request)
+        require_saml_auth = approval_requires_saml_auth?(merge_request)
+
+        return if require_saml_auth && !saml_approval_in_time?
+        return if incorrect_approval_password?(merge_request) && !require_saml_auth
 
         super
       end
@@ -19,6 +23,22 @@ def incorrect_approval_password?(merge_request)
           !::Gitlab::Auth.find_with_user_password(current_user.username, params[:approval_password])
       end
 
+      def approval_requires_saml_auth?(merge_request)
+        merge_request.project.require_saml_auth_to_approve?
+      end
+
+      def saml_provider
+        project.group.root_saml_provider
+      end
+
+      def saml_approval_in_time?
+        return false unless saml_provider
+
+        ::Gitlab::Auth::GroupSaml::SsoState
+          .new(saml_provider.id)
+          .active_since?(SAML_APPROVE_TIMEOUT.ago)
+      end
+
       override :reset_approvals_cache
       def reset_approvals_cache(merge_request)
         merge_request.reset_approval_cache!
diff --git a/ee/app/services/ee/merge_requests/saml_approval_service.rb b/ee/app/services/ee/merge_requests/saml_approval_service.rb
deleted file mode 100644
index 0ed2f4a33ac579..00000000000000
--- a/ee/app/services/ee/merge_requests/saml_approval_service.rb
+++ /dev/null
@@ -1,43 +0,0 @@
-# frozen_string_literal: true
-
-module EE
-  module MergeRequests
-    class SamlApprovalService < ::MergeRequests::ApprovalService
-      SAML_APPROVE_TIMEOUT = 5.seconds
-      SAMLApprovalTimeoutError = Class.new(StandardError)
-
-      def execute(merge_request)
-        if approval_requires_saml_auth?(merge_request) && !saml_approval_in_time?
-          # catch this up the chain an redirect to the SAML IdP with callback to re-approve
-          return ServiceResponse.error(
-            message: _("ComplianceManagment|SAML Auth timeout reached for approval"),
-            reason: 'saml_auth_for_approval_timed_out')
-        end
-
-        # Ancesotr class returns non-standard service response, we are wrapping it here
-        unless super
-          return ServiceResponse.error(
-            message: _("ComplianceManagment|You are not eligble to approve this merge request."),
-            reason: 'approval_service_uneligible'
-          )
-        end
-
-        ServiceResponse.success(payload: merge_request)
-      end
-
-      private
-
-      def approval_requires_saml_auth?(merge_request)
-        merge_request.project.require_saml_auth_to_approve?
-      end
-
-      def saml_provider
-        project.group.root_saml_provider
-      end
-
-      def saml_approval_in_time?
-        ::Gitlab::Auth::GroupSaml::SsoState.new(saml_provider.id).active_since?(SAML_APPROVE_TIMEOUT.ago)
-      end
-    end
-  end
-end
diff --git a/ee/app/views/groups/settings/merge_requests/_merge_request_approval_settings.html.haml b/ee/app/views/groups/settings/merge_requests/_merge_request_approval_settings.html.haml
index 1f58c84aa9aa3e..7eefc3cce53796 100644
--- a/ee/app/views/groups/settings/merge_requests/_merge_request_approval_settings.html.haml
+++ b/ee/app/views/groups/settings/merge_requests/_merge_request_approval_settings.html.haml
@@ -5,4 +5,4 @@
 - return unless show_merge_request_approval_settings?(user, group)
 
 - group_merge_request_approval_setting_api_path = expose_path(api_v4_groups_merge_request_approval_setting_path(id: group.id))
-%section#js-merge-request-approval-settings{ data: { default_expanded: expanded, approval_settings_path: group_merge_request_approval_setting_api_path, saml_provider_enabled: saml_provider_enabled?(group).to_s }}
+%section#js-merge-request-approval-settings{ data: { default_expanded: expanded, approval_settings_path: group_merge_request_approval_setting_api_path, saml_provider_enabled: saml_provider_enabled?(group).to_s } }
diff --git a/ee/config/routes/merge_requests.rb b/ee/config/routes/merge_requests.rb
index 06b302e5527a1b..cff760b13b2362 100644
--- a/ee/config/routes/merge_requests.rb
+++ b/ee/config/routes/merge_requests.rb
@@ -15,7 +15,9 @@
     get :coverage_fuzzing_reports
     get :api_fuzzing_reports
     get :security_reports
-    get :approve_with_saml, action: :approve_with_saml, controller: 'merge_requests/approve_with_saml'
+
+    # We intentionally need get here since this is invoked via a callback from the SAML Identity Provider(OmniAuth)
+    get :saml_approval, action: :create, controller: 'merge_requests/saml_approvals'
 
     post :rebase
   end
diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/group_merge_request_approval_settings.json b/ee/spec/fixtures/api/schemas/public_api/v4/group_merge_request_approval_settings.json
index 99379b54344212..af1b84f1798f6f 100644
--- a/ee/spec/fixtures/api/schemas/public_api/v4/group_merge_request_approval_settings.json
+++ b/ee/spec/fixtures/api/schemas/public_api/v4/group_merge_request_approval_settings.json
@@ -2,39 +2,81 @@
   "type": "object",
   "properties": {
     "allow_author_approval": {
-      "value": { "type": "boolean" },
-      "locked": { "type": "boolean" },
-      "inherited_from": { "type": "string" }
+      "value": {
+        "type": "boolean"
+      },
+      "locked": {
+        "type": "boolean"
+      },
+      "inherited_from": {
+        "type": "string"
+      }
     },
     "allow_committer_approval": {
-      "value": { "type": "boolean" },
-      "locked": { "type": "boolean" },
-      "inherited_from": { "type": "string" }
+      "value": {
+        "type": "boolean"
+      },
+      "locked": {
+        "type": "boolean"
+      },
+      "inherited_from": {
+        "type": "string"
+      }
     },
     "allow_overrides_to_approver_list_per_merge_request": {
-      "value": { "type": "boolean" },
-      "locked": { "type": "boolean" },
-      "inherited_from": { "type": "string" }
+      "value": {
+        "type": "boolean"
+      },
+      "locked": {
+        "type": "boolean"
+      },
+      "inherited_from": {
+        "type": "string"
+      }
     },
     "retain_approvals_on_push": {
-      "value": { "type": "boolean" },
-      "locked": { "type": "boolean" },
-      "inherited_from": { "type": "string" }
+      "value": {
+        "type": "boolean"
+      },
+      "locked": {
+        "type": "boolean"
+      },
+      "inherited_from": {
+        "type": "string"
+      }
     },
     "selective_code_owner_removals": {
-      "value": { "type": "boolean" },
-      "locked": { "type": "boolean" },
-      "inherited_from": { "type": "string" }
+      "value": {
+        "type": "boolean"
+      },
+      "locked": {
+        "type": "boolean"
+      },
+      "inherited_from": {
+        "type": "string"
+      }
     },
     "require_password_to_approve": {
-      "value": { "type": "boolean" },
-      "locked": { "type": "boolean" },
-      "inherited_from": { "type": "string" }
+      "value": {
+        "type": "boolean"
+      },
+      "locked": {
+        "type": "boolean"
+      },
+      "inherited_from": {
+        "type": "string"
+      }
     },
     "require_saml_auth_to_approve": {
-      "value": { "type": "boolean" },
-      "locked": { "type": "boolean" },
-      "inherited_from": { "type": "string" }
+      "value": {
+        "type": "boolean"
+      },
+      "locked": {
+        "type": "boolean"
+      },
+      "inherited_from": {
+        "type": "string"
+      }
     }
   },
   "additionalProperties": false
diff --git a/ee/spec/services/merge_requests/approval_service_spec.rb b/ee/spec/services/merge_requests/approval_service_spec.rb
index 7e95626c3edf61..3c1cf057f0069f 100644
--- a/ee/spec/services/merge_requests/approval_service_spec.rb
+++ b/ee/spec/services/merge_requests/approval_service_spec.rb
@@ -4,17 +4,48 @@
 
 RSpec.describe MergeRequests::ApprovalService, feature_category: :code_review_workflow do
   describe '#execute' do
-    let(:user)          { create(:user) }
-    let(:merge_request) { create(:merge_request) }
-    let(:project)       { merge_request.project }
-    let!(:todo)         { create(:todo, user: user, project: project, target: merge_request) }
+    let_it_be(:user) { create :user }
+    let_it_be(:group) { create :group }
+    let_it_be(:project) do
+      create :project,
+        :public,
+        :repository,
+        group: group,
+        approvals_before_merge: 0,
+        merge_requests_author_approval: true
+    end
+
+    let_it_be(:merge_request) { create :merge_request_with_diffs, source_project: project, reviewers: [user] }
 
     subject(:service) { described_class.new(project: project, current_user: user) }
 
     before do
+      stub_licensed_features(group_saml: true)
+    end
+
+    before_all do
+      group.add_developer(user)
       project.add_developer(user)
     end
 
+    def simulate_require_saml_auth_to_approve_mr_approval_setting
+      create(:saml_provider, group: project.group, enabled: true)
+      allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver|
+        allow(resolver).to receive(:require_saml_auth_to_approve)
+          .and_return(ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
+            value: true,
+            locked: true,
+            inherited_from: :group
+          ))
+      end
+    end
+
+    def simulate_saml_approval_in_time?(in_time:)
+      allow_next_instance_of(MergeRequests::ApprovalService) do |service|
+        allow(service).to receive(:saml_approval_in_time?).and_return(in_time)
+      end
+    end
+
     context 'with invalid approval' do
       before do
         allow(merge_request.approvals).to receive(:new).and_return(double(save: false))
@@ -68,6 +99,33 @@
 
           expect { service_with_params.execute(merge_request) }.to change { merge_request.approvals.size }
         end
+
+        context 'when SAML auth is required' do
+          it 'does not change approval count' do
+            simulate_require_saml_auth_to_approve_mr_approval_setting
+            simulate_saml_approval_in_time?(in_time: false)
+
+            service_with_params = described_class.new(project: project, current_user: user, params: params)
+
+            expect { service_with_params.execute(merge_request) }.not_to change { merge_request.approvals.size }
+          end
+        end
+      end
+    end
+
+    context 'when SAML auth is required' do
+      it 'approves when whithin the timeout' do
+        simulate_require_saml_auth_to_approve_mr_approval_setting
+        simulate_saml_approval_in_time?(in_time: true)
+
+        expect { service.execute(merge_request) }.to change { merge_request.approvals.size }
+      end
+
+      it 'rejects approaval when exceeding the timeout' do
+        simulate_require_saml_auth_to_approve_mr_approval_setting
+        simulate_saml_approval_in_time?(in_time: false)
+
+        expect { service.execute(merge_request) }.not_to change { merge_request.approvals.size }
       end
     end
   end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 39dc1b452dd82c..11497889fcd852 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -6094,7 +6094,7 @@ msgstr ""
 msgid "Approval options"
 msgstr ""
 
-msgid "Approval rejected"
+msgid "Approval rejected."
 msgstr ""
 
 msgid "Approval rules"
@@ -6258,6 +6258,9 @@ msgstr ""
 msgid "ApprovalSettings|Remove approvals by Code Owners if their files changed"
 msgstr ""
 
+msgid "ApprovalSettings|Require SAML re-authentication to approve. Will ovrride password requirement"
+msgstr ""
+
 msgid "ApprovalSettings|Require user password to approve"
 msgstr ""
 
@@ -12508,12 +12511,6 @@ msgstr ""
 msgid "ComplianceFramework|No pipeline configuration found"
 msgstr ""
 
-msgid "ComplianceManagment|SAML Auth timeout reached for approval"
-msgstr ""
-
-msgid "ComplianceManagment|You are not eligble to approve this merge request."
-msgstr ""
-
 msgid "ComplianceReport|Add framework"
 msgstr ""
 
-- 
GitLab


From 5597c4802110ae4330fd99d21a312225d840e353 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Wed, 8 Nov 2023 00:20:45 +0100
Subject: [PATCH 05/16] Add settings for require SAML auth for approval

- WIP: needs more specs

- Refs:
  https://gitlab.com/gitlab-org/gitlab/-/issues/421944
---
 .../approvals/components/approval_settings.vue  | 17 +++++++++++++++++
 .../approvals/components/group_settings/app.vue |  2 ++
 .../assets/javascripts/approvals/constants.js   |  3 +++
 ee/app/assets/javascripts/approvals/mappers.js  |  2 ++
 .../approvals/mount_group_settings.js           | 14 ++++++++++----
 .../approvals/mount_project_settings.js         |  1 +
 .../stores/modules/approval_settings/actions.js |  3 +++
 .../modules/approval_settings/mutation_types.js |  1 +
 .../modules/approval_settings/mutations.js      |  3 +++
 .../components/approval_settings_spec.js        | 13 ++++++++++++-
 ee/spec/frontend/approvals/mappers_spec.js      |  1 +
 ee/spec/frontend/approvals/mocks.js             | 10 ++++++++++
 12 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/ee/app/assets/javascripts/approvals/components/approval_settings.vue b/ee/app/assets/javascripts/approvals/components/approval_settings.vue
index df08bddb817a6a..04eddd21baee70 100644
--- a/ee/app/assets/javascripts/approvals/components/approval_settings.vue
+++ b/ee/app/assets/javascripts/approvals/components/approval_settings.vue
@@ -53,6 +53,11 @@ export default {
       required: false,
       default: true,
     },
+    lockRequireSamlAuth: {
+      type: Boolean,
+      required: false,
+      default: true,
+    },
   },
   computed: {
     ...mapState({
@@ -68,7 +73,9 @@ export default {
       selectiveCodeOwnerRemovals: (state) =>
         state.approvalSettings.settings.selectiveCodeOwnerRemovals,
       requireUserPassword: (state) => state.approvalSettings.settings.requireUserPassword,
+      requireSamlAuth: (state) => state.approvalSettings.settings.requireSamlAuth,
       groupName: (state) => state.settings.groupName,
+      samlProviderEnabled: (state) => state.settings.samlProviderEnabled,
     }),
     ...mapGetters(['settingChanged']),
     hasSettings() {
@@ -105,6 +112,7 @@ export default {
       'setRemoveApprovalsOnPush',
       'setSelectiveCodeOwnerRemovals',
       'setRequireUserPassword',
+      'setRequireSamlAuth',
     ]),
     handleWhenCommitIsAddedRadioGroupInput(value) {
       switch (value) {
@@ -209,6 +217,15 @@ export default {
           data-testid="require-user-password"
           @input="setRequireUserPassword"
         />
+        <approval-settings-checkbox
+          v-if="samlProviderEnabled"
+          :checked="requireSamlAuth.value"
+          :label="settingsLabels.requireSamlAuthLabel"
+          :locked="lockRequireSamlAuth"
+          :locked-text="lockedText(requireSamlAuth)"
+          data-testid="require-saml-auth"
+          @input="setRequireSamlAuth"
+        />
         <div class="gl-mt-5 gl-mb-3">{{ settingsLabels.whenCommitAddedLabel }}</div>
         <gl-form-radio-group
           :checked="whenCommitIsAddedRadioGroupValue"
diff --git a/ee/app/assets/javascripts/approvals/components/group_settings/app.vue b/ee/app/assets/javascripts/approvals/components/group_settings/app.vue
index 66f8d04be83f7a..4e7366e9c325eb 100644
--- a/ee/app/assets/javascripts/approvals/components/group_settings/app.vue
+++ b/ee/app/assets/javascripts/approvals/components/group_settings/app.vue
@@ -70,6 +70,8 @@ export default {
       <approval-settings
         :approval-settings-path="approvalSettingsPath"
         :settings-labels="$options.labels"
+        :lock-require-saml-auth="false"
+        :can-prevent-saml-auth-approval-edit="false"
       />
     </template>
   </settings-block>
diff --git a/ee/app/assets/javascripts/approvals/constants.js b/ee/app/assets/javascripts/approvals/constants.js
index 70022f9be4bc60..358456093cff05 100644
--- a/ee/app/assets/javascripts/approvals/constants.js
+++ b/ee/app/assets/javascripts/approvals/constants.js
@@ -63,6 +63,9 @@ export const PROJECT_APPROVAL_SETTINGS_LABELS_I18N = {
   selectiveCodeOwnerRemovalsLabel: s__(
     'ApprovalSettings|Remove approvals by Code Owners if their files changed',
   ),
+  requireSamlAuthLabel: s__(
+    'ApprovalSettings|Require SAML re-authentication to approve. Will ovrride password requirement',
+  ),
 };
 
 export const GROUP_APPROVAL_SETTINGS_LABELS_I18N = {
diff --git a/ee/app/assets/javascripts/approvals/mappers.js b/ee/app/assets/javascripts/approvals/mappers.js
index de56d713fedfc2..4bc968a7797cac 100644
--- a/ee/app/assets/javascripts/approvals/mappers.js
+++ b/ee/app/assets/javascripts/approvals/mappers.js
@@ -110,6 +110,7 @@ export const mergeRequestApprovalSettingsMappers = {
         preventMrApprovalRuleEdit: invertApprovalSetting(
           data.allow_overrides_to_approver_list_per_merge_request,
         ),
+        requireSamlAuth: data.require_saml_auth_to_approve,
         requireUserPassword: data.require_password_to_approve,
         removeApprovalsOnPush: invertApprovalSetting(data.retain_approvals_on_push),
         preventCommittersApproval: invertApprovalSetting(data.allow_committer_approval),
@@ -120,6 +121,7 @@ export const mergeRequestApprovalSettingsMappers = {
   mapStateToPayload: ({ settings }) => ({
     allow_author_approval: !settings.preventAuthorApproval.value,
     allow_overrides_to_approver_list_per_merge_request: !settings.preventMrApprovalRuleEdit.value,
+    require_saml_auth_to_approve: settings.requireSamlAuth.value,
     require_password_to_approve: settings.requireUserPassword.value,
     retain_approvals_on_push: !settings.removeApprovalsOnPush.value,
     selective_code_owner_removals: settings.selectiveCodeOwnerRemovals.value,
diff --git a/ee/app/assets/javascripts/approvals/mount_group_settings.js b/ee/app/assets/javascripts/approvals/mount_group_settings.js
index 41bdd9a8a94cdf..4ce1708f81406f 100644
--- a/ee/app/assets/javascripts/approvals/mount_group_settings.js
+++ b/ee/app/assets/javascripts/approvals/mount_group_settings.js
@@ -11,10 +11,15 @@ const mountGroupApprovalSettings = (el) => {
     return null;
   }
 
-  const { defaultExpanded, approvalSettingsPath } = el.dataset;
-  const store = createStore({
-    approvalSettings: approvalSettingsModule(mergeRequestApprovalSettingsMappers),
-  });
+  const { defaultExpanded, approvalSettingsPath, samlProviderEnabled } = el.dataset;
+  const store = createStore(
+    {
+      approvalSettings: approvalSettingsModule(mergeRequestApprovalSettingsMappers),
+    },
+    {
+      samlProviderEnabled: parseBoolean(samlProviderEnabled),
+    },
+  );
 
   Vue.use(GlToast);
 
@@ -26,6 +31,7 @@ const mountGroupApprovalSettings = (el) => {
         props: {
           defaultExpanded: parseBoolean(defaultExpanded),
           approvalSettingsPath,
+          samlProviderEnabled: parseBoolean(samlProviderEnabled),
         },
       }),
   });
diff --git a/ee/app/assets/javascripts/approvals/mount_project_settings.js b/ee/app/assets/javascripts/approvals/mount_project_settings.js
index 1269cb4323be22..05f9ed616721fa 100644
--- a/ee/app/assets/javascripts/approvals/mount_project_settings.js
+++ b/ee/app/assets/javascripts/approvals/mount_project_settings.js
@@ -31,6 +31,7 @@ export default function mountProjectSettingsApprovals(el) {
     canEdit: parseBoolean(el.dataset.canEdit),
     canModifyAuthorSettings: parseBoolean(el.dataset.canModifyAuthorSettings),
     canModifyCommiterSettings: parseBoolean(el.dataset.canModifyCommiterSettings),
+    samlProviderEnabled: parseBoolean(el.dataset.samlProviderEnabled),
   });
 
   Vue.use(GlToast);
diff --git a/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/actions.js b/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/actions.js
index 8315d3732c0247..56cc6e2c62275f 100644
--- a/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/actions.js
+++ b/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/actions.js
@@ -65,4 +65,7 @@ export default (mapStateToPayload, updateMethod = 'put') => ({
   setRequireUserPassword({ commit }, value) {
     commit(types.SET_REQUIRE_USER_PASSWORD, value);
   },
+  setRequireSamlAuth({ commit }, value) {
+    commit(types.SET_REQUIRE_SAML_AUTH, value);
+  },
 });
diff --git a/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutation_types.js b/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutation_types.js
index 7e29bcb66e04d4..0b06978ecdad9b 100644
--- a/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutation_types.js
+++ b/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutation_types.js
@@ -15,3 +15,4 @@ export const SET_PREVENT_MR_APPROVAL_RULE_EDIT = 'SET_PREVENT_MR_APPROVAL_RULE_E
 export const SET_REMOVE_APPROVALS_ON_PUSH = 'SET_REMOVE_APPROVALS_ON_PUSH';
 export const SET_SELECTIVE_CODE_OWNER_REMOVALS = 'SET_SELECTIVE_CODE_OWNER_REMOVALS';
 export const SET_REQUIRE_USER_PASSWORD = 'SET_REQUIRE_USER_PASSWORD';
+export const SET_REQUIRE_SAML_AUTH = 'SET_REQUIRE_SAML_AUTH';
diff --git a/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutations.js b/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutations.js
index 344efe316b469d..86859843f28a44 100644
--- a/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutations.js
+++ b/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutations.js
@@ -47,6 +47,9 @@ export default (mapDataToState) => ({
   [types.SET_SELECTIVE_CODE_OWNER_REMOVALS](state, value) {
     state.settings.selectiveCodeOwnerRemovals.value = value;
   },
+  [types.SET_REQUIRE_SAML_AUTH](state, value) {
+    state.settings.requireSamlAuth.value = value;
+  },
   [types.SET_REQUIRE_USER_PASSWORD](state, value) {
     state.settings.requireUserPassword.value = value;
   },
diff --git a/ee/spec/frontend/approvals/components/approval_settings_spec.js b/ee/spec/frontend/approvals/components/approval_settings_spec.js
index dd07d9e6b43822..15004c2c6c5f36 100644
--- a/ee/spec/frontend/approvals/components/approval_settings_spec.js
+++ b/ee/spec/frontend/approvals/components/approval_settings_spec.js
@@ -41,6 +41,7 @@ describe('ApprovalSettings', () => {
 
     store = createStore({ approvalSettings: module, approvals: projectSettingsModule() });
     store.state.settings.groupName = groupName;
+    store.state.settings.samlProviderEnabled = true;
   };
 
   const showToast = jest.fn();
@@ -211,6 +212,7 @@ describe('ApprovalSettings', () => {
       ${'prevent-committers-approval'}   | ${'setPreventCommittersApproval'} | ${'preventCommittersApproval'} | ${'preventCommittersApprovalLabel'}
       ${'prevent-mr-approval-rule-edit'} | ${'setPreventMrApprovalRuleEdit'} | ${'preventMrApprovalRuleEdit'} | ${'preventMrApprovalRuleEditLabel'}
       ${'require-user-password'}         | ${'setRequireUserPassword'}       | ${'requireUserPassword'}       | ${'requireUserPasswordLabel'}
+      ${'require-saml-auth'}             | ${'setRequireSamlAuth'}           | ${'requireSamlAuth'}           | ${'requireSamlAuthLabel'}
     `('with the $testid checkbox', ({ testid, action, setting, labelKey }) => {
       let checkbox = null;
 
@@ -234,7 +236,16 @@ describe('ApprovalSettings', () => {
       });
 
       it('sets the checkbox locked prop', () => {
-        expect(checkbox.props('locked')).toBe(settings[setting].locked);
+        if (setting === 'requireSamlAuth') {
+          // requireSamlAuth defaults to always be locked unless explicitly unlocked (e.g. for grup settings)
+          expect(checkbox.props('locked')).toBe(true);
+
+          createWrapper({ lockRequireSamlAuth: false });
+          const unlockedCheckbox = wrapper.findByTestId(testid);
+          expect(unlockedCheckbox.props('locked')).toBe(false);
+        } else {
+          expect(checkbox.props('locked')).toBe(settings[setting].locked);
+        }
       });
 
       it('sets the checkbox lockedText prop', () => {
diff --git a/ee/spec/frontend/approvals/mappers_spec.js b/ee/spec/frontend/approvals/mappers_spec.js
index 6bb517257c3079..c4faea34b95c01 100644
--- a/ee/spec/frontend/approvals/mappers_spec.js
+++ b/ee/spec/frontend/approvals/mappers_spec.js
@@ -14,6 +14,7 @@ describe('approvals mappers', () => {
       allow_author_approval: true,
       allow_overrides_to_approver_list_per_merge_request: true,
       require_password_to_approve: true,
+      require_saml_auth_to_approve: true,
       retain_approvals_on_push: false,
       selective_code_owner_removals: false,
       allow_committer_approval: true,
diff --git a/ee/spec/frontend/approvals/mocks.js b/ee/spec/frontend/approvals/mocks.js
index 282578abcec6f0..ad07e9dfda397f 100644
--- a/ee/spec/frontend/approvals/mocks.js
+++ b/ee/spec/frontend/approvals/mocks.js
@@ -68,6 +68,11 @@ export const createGroupApprovalsPayload = () => ({
     locked: null,
     inheritedFrom: null,
   },
+  require_saml_auth_to_approve: {
+    value: true,
+    locked: null,
+    inherited_from: null,
+  },
   require_password_to_approve: {
     value: true,
     locked: null,
@@ -102,6 +107,11 @@ export const createGroupApprovalsState = (locked = null) => ({
       locked,
       value: false,
     },
+    requireSamlAuth: {
+      inheritedFrom: null,
+      locked,
+      value: true,
+    },
     requireUserPassword: {
       inheritedFrom: null,
       locked,
-- 
GitLab


From 394082b064fd6ad0d91457aa5f0ea244d74e6ca3 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Wed, 8 Nov 2023 08:02:19 +0100
Subject: [PATCH 06/16] Fix typo in API entity for ApprovalSettings

- Refs:
  https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130204#note_1638452171
  https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130204#note_1638452166
  https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130204#note_1638472988
---
 ee/app/models/group_merge_request_approval_setting.rb       | 3 ++-
 ee/app/services/ee/merge_requests/approval_service.rb       | 2 ++
 ee/lib/ee/api/entities/approval_settings.rb                 | 4 ++--
 ee/spec/models/group_merge_request_approval_setting_spec.rb | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/ee/app/models/group_merge_request_approval_setting.rb b/ee/app/models/group_merge_request_approval_setting.rb
index 80362e020eeaef..1f1b6bb8ea5dac 100644
--- a/ee/app/models/group_merge_request_approval_setting.rb
+++ b/ee/app/models/group_merge_request_approval_setting.rb
@@ -11,6 +11,7 @@ class GroupMergeRequestApprovalSetting < ApplicationRecord
     :allow_overrides_to_approver_list_per_merge_request,
     :retain_approvals_on_push,
     :require_password_to_approve,
+    :require_saml_auth_to_approve,
     inclusion: { in: [true, false], message: N_('must be a boolean value') }
 
   scope :find_or_initialize_by_group, ->(group) { find_or_initialize_by(group: group) }
@@ -22,7 +23,7 @@ class GroupMergeRequestApprovalSetting < ApplicationRecord
     'prevent users from modifying MR approval rules in merge requests',
     retain_approvals_on_push: 'require new approvals when new commits are added to an MR',
     require_password_to_approve: 'require user password for approvals',
-    require_saml_auth_to_approve: 'requrie user to re-authenticate with SAML to approve'
+    require_saml_auth_to_approve: 'require user to re-authenticate with SAML to approve'
   }.freeze
 
   def selective_code_owner_removals
diff --git a/ee/app/services/ee/merge_requests/approval_service.rb b/ee/app/services/ee/merge_requests/approval_service.rb
index fa7d9015147e88..5b3592e681348b 100644
--- a/ee/app/services/ee/merge_requests/approval_service.rb
+++ b/ee/app/services/ee/merge_requests/approval_service.rb
@@ -4,6 +4,8 @@ module EE
   module MergeRequests
     module ApprovalService
       extend ::Gitlab::Utils::Override
+      # 5 seconds is chosen arbitrarily to ensure the user needs to just have re-authenticated to approve
+      # Timeframe gives a short grace period for the callback from the identity provider to have processed.
       SAML_APPROVE_TIMEOUT = 5.seconds
 
       override :execute
diff --git a/ee/lib/ee/api/entities/approval_settings.rb b/ee/lib/ee/api/entities/approval_settings.rb
index 715815d44c93c6..b8d580facc81d4 100644
--- a/ee/lib/ee/api/entities/approval_settings.rb
+++ b/ee/lib/ee/api/entities/approval_settings.rb
@@ -20,8 +20,8 @@ class ApprovalSettings < Grape::Entity
         expose :merge_requests_disable_committers_approval?,
           as: :merge_requests_disable_committers_approval
 
-        expose :requrie_saml_auth_to_approve?,
-          as: :requrie_saml_auth_to_approve
+        expose :require_saml_auth_to_approve?,
+          as: :require_saml_auth_to_approve
 
         expose :require_password_to_approve?,
           as: :require_password_to_approve
diff --git a/ee/spec/models/group_merge_request_approval_setting_spec.rb b/ee/spec/models/group_merge_request_approval_setting_spec.rb
index 97c9fb3f3d3346..21388284cad704 100644
--- a/ee/spec/models/group_merge_request_approval_setting_spec.rb
+++ b/ee/spec/models/group_merge_request_approval_setting_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe GroupMergeRequestApprovalSetting do
+RSpec.describe GroupMergeRequestApprovalSetting, feature_category: :compliance_managment do
   describe 'Associations' do
     it { is_expected.to belong_to :group }
   end
@@ -19,6 +19,7 @@
     it { is_expected.to validate_inclusion_of(:allow_overrides_to_approver_list_per_merge_request).in_array(options) }
     it { is_expected.to validate_inclusion_of(:retain_approvals_on_push).in_array(options) }
     it { is_expected.to validate_inclusion_of(:require_password_to_approve).in_array(options) }
+    it { is_expected.to validate_inclusion_of(:require_saml_auth_to_approve).in_array(options) }
   end
 
   describe '.find_or_initialize_by_group' do
-- 
GitLab


From 58a9a11295856fbfc8a509f64711b12e45f7b049 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Wed, 8 Nov 2023 08:23:42 +0100
Subject: [PATCH 07/16] Fix broken EE projects_helper_spec

- Refs:
  https://gitlab.com/gitlab-org/gitlab/-/jobs/5482854925
---
 ee/spec/helpers/projects_helper_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/spec/helpers/projects_helper_spec.rb b/ee/spec/helpers/projects_helper_spec.rb
index 6174357110ff96..f2b0e5b5e56924 100644
--- a/ee/spec/helpers/projects_helper_spec.rb
+++ b/ee/spec/helpers/projects_helper_spec.rb
@@ -548,7 +548,7 @@
     before do
       allow(helper).to receive(:current_user).and_return(user)
       allow(helper).to receive(:can?).and_return(true)
-      allow(helper).to receive(:saml_provider_enabled?).and_return(true)
+      allow(helper).to receive(:saml_provider_enabled_for_project?).and_return(true)
     end
 
     it 'returns the correct data' do
-- 
GitLab


From 98323faf37ee721d27f4b8325db8373df642f364 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Wed, 8 Nov 2023 09:56:52 +0100
Subject: [PATCH 08/16] Update project_approvers schema

- Include require_saml_auth_to_approve

- Refs:
  https://gitlab.com/gitlab-org/gitlab/-/jobs/5485202304
---
 .../public_api/v4/project_approvers.json      | 52 +++++++++++++++----
 1 file changed, 43 insertions(+), 9 deletions(-)

diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/project_approvers.json b/ee/spec/fixtures/api/schemas/public_api/v4/project_approvers.json
index 3345fbd8906cfa..16a5344e17974b 100644
--- a/ee/spec/fixtures/api/schemas/public_api/v4/project_approvers.json
+++ b/ee/spec/fixtures/api/schemas/public_api/v4/project_approvers.json
@@ -1,25 +1,59 @@
 {
   "type": "object",
   "properties": {
-    "approvals_before_merge": { "type": "integer" },
-    "disable_overriding_approvers_per_merge_request": { "type": ["boolean", "null"] },
-    "reset_approvals_on_push": { "type": "boolean" },
-    "selective_code_owner_removals": { "type": "boolean" },
-    "merge_requests_author_approval": { "type": ["boolean", "null"] },
-    "merge_requests_disable_committers_approval": { "type": ["boolean", "null"] },
-    "require_password_to_approve": { "type": ["boolean", "null"] },
+    "approvals_before_merge": {
+      "type": "integer"
+    },
+    "disable_overriding_approvers_per_merge_request": {
+      "type": [
+        "boolean",
+        "null"
+      ]
+    },
+    "reset_approvals_on_push": {
+      "type": "boolean"
+    },
+    "selective_code_owner_removals": {
+      "type": "boolean"
+    },
+    "merge_requests_author_approval": {
+      "type": [
+        "boolean",
+        "null"
+      ]
+    },
+    "merge_requests_disable_committers_approval": {
+      "type": [
+        "boolean",
+        "null"
+      ]
+    },
+    "require_password_to_approve": {
+      "type": [
+        "boolean",
+        "null"
+      ]
+    },
+    "require_saml_auth_to_approve": {
+      "type": [
+        "boolean",
+        "null"
+      ]
+    },
     "approvers": {
       "type": "array",
       "items": {
         "type": "object",
-        "properties": {}
+        "properties": {
+        }
       }
     },
     "approver_groups": {
       "type": "array",
       "items": {
         "type": "object",
-        "properties": {}
+        "properties": {
+        }
       }
     }
   },
-- 
GitLab


From 6ecbc4310043d182d14f0f8afcccac588ebbd4bb Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Wed, 8 Nov 2023 10:51:13 +0100
Subject: [PATCH 09/16] Give SAML approval precedence over password approvals

---
 .../components/approvals/approvals.vue                    | 8 ++++----
 .../merge_request/user_approves_with_saml_auth_spec.rb    | 8 +++++++-
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue b/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue
index 1bcb7d026b9b4c..524f2c045e6fa7 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals.vue
@@ -175,14 +175,14 @@ export default {
   },
   methods: {
     approve() {
-      if (this.requirePasswordToApprove) {
-        this.$root.$emit(BV_SHOW_MODAL, this.modalId);
-        return;
-      }
       if (this.requireSamlAuthToApprove) {
         this.approveWithSamlAuth();
         return;
       }
+      if (this.requirePasswordToApprove) {
+        this.$root.$emit(BV_SHOW_MODAL, this.modalId);
+        return;
+      }
       this.updateApproval(
         () => this.service.approveMergeRequest(),
         () =>
diff --git a/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb b/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
index 53e514178e9156..3964cbdafa41a4 100644
--- a/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
+++ b/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
@@ -5,7 +5,13 @@
 RSpec.describe 'Merge request > User approves with SAML auth', :js, feature_category: :code_review_workflow do
   let_it_be(:user) { create :user }
   let_it_be(:group) { create :group }
-  let_it_be(:setting) { create :group_merge_request_approval_setting, require_saml_auth_to_approve: true, group: group }
+  let_it_be(:setting) do
+    create :group_merge_request_approval_setting,
+      require_saml_auth_to_approve: true,
+      require_password_to_approve: true, # enable password to spec SAML takes precdence over it
+      group: group
+  end
+
   let_it_be(:project) do
     create :project,
       :public,
-- 
GitLab


From 1255fd0981d27175dd1595a1c484f8c3afe778c8 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Wed, 8 Nov 2023 12:45:00 +0100
Subject: [PATCH 10/16] Add missing frontend specs

---
 ee/app/assets/javascripts/approvals/constants.js                | 2 +-
 .../approvals/stores/modules/approval_settings/actions_spec.js  | 1 +
 .../approvals/stores/modules/approval_settings/getters_spec.js  | 1 +
 .../stores/modules/approval_settings/mutations_spec.js          | 2 ++
 ee/spec/frontend/vue_merge_request_widget/mock_data.js          | 1 +
 .../vue_merge_request_widget/stores/mr_widget_store_spec.js     | 1 +
 locale/gitlab.pot                                               | 2 +-
 7 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/ee/app/assets/javascripts/approvals/constants.js b/ee/app/assets/javascripts/approvals/constants.js
index 358456093cff05..8d3de431210938 100644
--- a/ee/app/assets/javascripts/approvals/constants.js
+++ b/ee/app/assets/javascripts/approvals/constants.js
@@ -64,7 +64,7 @@ export const PROJECT_APPROVAL_SETTINGS_LABELS_I18N = {
     'ApprovalSettings|Remove approvals by Code Owners if their files changed',
   ),
   requireSamlAuthLabel: s__(
-    'ApprovalSettings|Require SAML re-authentication to approve. Will ovrride password requirement',
+    'ApprovalSettings|Require SAML re-authentication to approve. Note: Overrides password requirement.',
   ),
 };
 
diff --git a/ee/spec/frontend/approvals/stores/modules/approval_settings/actions_spec.js b/ee/spec/frontend/approvals/stores/modules/approval_settings/actions_spec.js
index 9d065555c305b8..453400a84ad12a 100644
--- a/ee/spec/frontend/approvals/stores/modules/approval_settings/actions_spec.js
+++ b/ee/spec/frontend/approvals/stores/modules/approval_settings/actions_spec.js
@@ -137,6 +137,7 @@ describe('EE approvals group settings module actions', () => {
     ${'setRemoveApprovalsOnPush'}      | ${types.SET_REMOVE_APPROVALS_ON_PUSH}
     ${'setSelectiveCodeOwnerRemovals'} | ${types.SET_SELECTIVE_CODE_OWNER_REMOVALS}
     ${'setRequireUserPassword'}        | ${types.SET_REQUIRE_USER_PASSWORD}
+    ${'setRequireSamlAuth'}            | ${types.SET_REQUIRE_SAML_AUTH}
   `('$action', ({ action, type }) => {
     it(`commits ${type}`, () => {
       return testAction(actions[action], true, state, [{ type, payload: true }], []);
diff --git a/ee/spec/frontend/approvals/stores/modules/approval_settings/getters_spec.js b/ee/spec/frontend/approvals/stores/modules/approval_settings/getters_spec.js
index f2bfaa10f1718b..85e6ab9b05b465 100644
--- a/ee/spec/frontend/approvals/stores/modules/approval_settings/getters_spec.js
+++ b/ee/spec/frontend/approvals/stores/modules/approval_settings/getters_spec.js
@@ -8,6 +8,7 @@ describe('Group settings store getters', () => {
     preventCommittersApproval: { value: true },
     preventMrApprovalRuleEdit: { value: true },
     requireUserPassword: { value: true },
+    requireSamlAuth: { value: true },
     removeApprovalsOnPush: { value: false },
     selectiveCodeOwnerRemovals: { value: false },
   };
diff --git a/ee/spec/frontend/approvals/stores/modules/approval_settings/mutations_spec.js b/ee/spec/frontend/approvals/stores/modules/approval_settings/mutations_spec.js
index b1f07492b92514..3366ae45c943b2 100644
--- a/ee/spec/frontend/approvals/stores/modules/approval_settings/mutations_spec.js
+++ b/ee/spec/frontend/approvals/stores/modules/approval_settings/mutations_spec.js
@@ -12,6 +12,7 @@ describe('Group settings store mutations', () => {
     preventCommittersApproval: { value: false },
     preventMrApprovalRuleEdit: { value: false },
     requireUserPassword: { value: false },
+    requireSamlAuth: { value: false },
     removeApprovalsOnPush: { value: false },
     selectiveCodeOwnerRemovals: { value: false },
   };
@@ -92,6 +93,7 @@ describe('Group settings store mutations', () => {
     ${'SET_REMOVE_APPROVALS_ON_PUSH'}      | ${'removeApprovalsOnPush'}
     ${'SET_SELECTIVE_CODE_OWNER_REMOVALS'} | ${'selectiveCodeOwnerRemovals'}
     ${'SET_REQUIRE_USER_PASSWORD'}         | ${'requireUserPassword'}
+    ${'SET_REQUIRE_SAML_AUTH'}             | ${'requireSamlAuth'}
   `('$mutation', ({ mutation, prop }) => {
     beforeEach(() => {
       mutations.RECEIVE_SETTINGS_SUCCESS(state, settings);
diff --git a/ee/spec/frontend/vue_merge_request_widget/mock_data.js b/ee/spec/frontend/vue_merge_request_widget/mock_data.js
index bfcced11415d11..92679118002374 100644
--- a/ee/spec/frontend/vue_merge_request_widget/mock_data.js
+++ b/ee/spec/frontend/vue_merge_request_widget/mock_data.js
@@ -18,6 +18,7 @@ export default {
   dast_comparison_path: '/dast_comparison_path',
   coverage_fuzzing_comparison_path: '/coverage_fuzzing_comparison_path',
   api_fuzzing_comparison_path: '/api_fuzzing_comparison_path',
+  saml_approval_path: '/group/saml_sso_path',
 };
 
 // Browser Performance Testing
diff --git a/ee/spec/frontend/vue_merge_request_widget/stores/mr_widget_store_spec.js b/ee/spec/frontend/vue_merge_request_widget/stores/mr_widget_store_spec.js
index ae3bdb7c95f309..bdcef9b77b6d9c 100644
--- a/ee/spec/frontend/vue_merge_request_widget/stores/mr_widget_store_spec.js
+++ b/ee/spec/frontend/vue_merge_request_widget/stores/mr_widget_store_spec.js
@@ -83,6 +83,7 @@ describe('MergeRequestStore', () => {
       'secret_detection_comparison_path',
       'api_fuzzing_comparison_path',
       'coverage_fuzzing_comparison_path',
+      'saml_approval_path',
     ])('should set %s path', (property) => {
       // Ensure something is set in the mock data
       expect(property in mockData).toBe(true);
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 11497889fcd852..ee78bbf0e5c7d3 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -6258,7 +6258,7 @@ msgstr ""
 msgid "ApprovalSettings|Remove approvals by Code Owners if their files changed"
 msgstr ""
 
-msgid "ApprovalSettings|Require SAML re-authentication to approve. Will ovrride password requirement"
+msgid "ApprovalSettings|Require SAML re-authentication to approve. Note: Overrides password requirement."
 msgstr ""
 
 msgid "ApprovalSettings|Require user password to approve"
-- 
GitLab


From a634e93a1ea96099aa4cc763c2fa81f5e6a31611 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Thu, 9 Nov 2023 10:26:51 +0100
Subject: [PATCH 11/16] Remove audit event for saml auth approval

---
 .../audit_event_streaming/audit_event_types.md         |  1 -
 ee/app/models/group_merge_request_approval_setting.rb  |  3 +--
 .../types/require_saml_auth_to_approve_updated.yml     | 10 ----------
 .../merge_request/user_approves_with_saml_auth_spec.rb |  2 +-
 .../group_merge_request_approval_setting_spec.rb       |  1 -
 5 files changed, 2 insertions(+), 15 deletions(-)
 delete mode 100644 ee/config/audit_events/types/require_saml_auth_to_approve_updated.yml

diff --git a/doc/administration/audit_event_streaming/audit_event_types.md b/doc/administration/audit_event_streaming/audit_event_types.md
index 5a280ffd6fb28b..88212045d8e1fa 100644
--- a/doc/administration/audit_event_streaming/audit_event_types.md
+++ b/doc/administration/audit_event_streaming/audit_event_types.md
@@ -139,7 +139,6 @@ Audit event types belong to the following product categories.
 | [`remove_gpg_key`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111744) | Event triggered when a GPG Key is destroyed| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/373961) |
 | [`repository_download_operation`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111218) | Event triggered when a Git repository for a project is downloaded| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374108) |
 | [`require_password_to_approve_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/102256) | Event triggered on updating require user password for approvals from group merge request setting| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.6](https://gitlab.com/gitlab-org/gitlab/-/issues/373949) |
-| [`require_saml_auth_to_approve_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130204) | Triggered when a user updates the merge request approval setting for requiring SAML re-authentication to approve.| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.6](https://gitlab.com/gitlab-org/gitlab/-/issues/421959) |
 | [`retain_approvals_on_push_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/102256) | Event triggered on updating require new approvals when new commits are added to an MR from group merge request setting| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.6](https://gitlab.com/gitlab-org/gitlab/-/issues/373949) |
 | [`saml_group_links_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/110525) | Event triggered when a SAML Group Link is created| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/373954) |
 | [`saml_group_links_removed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/110525) | Event triggered when a SAML Group Link is destroyed| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/373954) |
diff --git a/ee/app/models/group_merge_request_approval_setting.rb b/ee/app/models/group_merge_request_approval_setting.rb
index 1f1b6bb8ea5dac..a2d8d8cbc2f06c 100644
--- a/ee/app/models/group_merge_request_approval_setting.rb
+++ b/ee/app/models/group_merge_request_approval_setting.rb
@@ -22,8 +22,7 @@ class GroupMergeRequestApprovalSetting < ApplicationRecord
     allow_overrides_to_approver_list_per_merge_request:
     'prevent users from modifying MR approval rules in merge requests',
     retain_approvals_on_push: 'require new approvals when new commits are added to an MR',
-    require_password_to_approve: 'require user password for approvals',
-    require_saml_auth_to_approve: 'require user to re-authenticate with SAML to approve'
+    require_password_to_approve: 'require user password for approvals'
   }.freeze
 
   def selective_code_owner_removals
diff --git a/ee/config/audit_events/types/require_saml_auth_to_approve_updated.yml b/ee/config/audit_events/types/require_saml_auth_to_approve_updated.yml
deleted file mode 100644
index e466b321c84029..00000000000000
--- a/ee/config/audit_events/types/require_saml_auth_to_approve_updated.yml
+++ /dev/null
@@ -1,10 +0,0 @@
----
-name: require_saml_auth_to_approve_updated
-description: Triggered when a user updates the merge request approval setting for
-  requiring SAML re-authentication to approve.
-introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/421959
-introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130204
-feature_category: compliance_management
-milestone: '16.6'
-saved_to_database: true
-streamed: true
diff --git a/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb b/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
index 3964cbdafa41a4..98ea621d6408ac 100644
--- a/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
+++ b/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
@@ -69,7 +69,7 @@ def unapprove
     expect(page).to have_text('Approved')
 
     page.within('.js-mr-approvals') do
-      expect(page).not_to have_button('approve with saml')
+      expect(page).not_to have_button('Approve with SAML')
       expect(page).to have_text('Approved by you')
     end
 
diff --git a/ee/spec/models/group_merge_request_approval_setting_spec.rb b/ee/spec/models/group_merge_request_approval_setting_spec.rb
index 21388284cad704..bfe3edc514084b 100644
--- a/ee/spec/models/group_merge_request_approval_setting_spec.rb
+++ b/ee/spec/models/group_merge_request_approval_setting_spec.rb
@@ -19,7 +19,6 @@
     it { is_expected.to validate_inclusion_of(:allow_overrides_to_approver_list_per_merge_request).in_array(options) }
     it { is_expected.to validate_inclusion_of(:retain_approvals_on_push).in_array(options) }
     it { is_expected.to validate_inclusion_of(:require_password_to_approve).in_array(options) }
-    it { is_expected.to validate_inclusion_of(:require_saml_auth_to_approve).in_array(options) }
   end
 
   describe '.find_or_initialize_by_group' do
-- 
GitLab


From 9f3df06ff35a3f1b0f09c72be2301fa428a21b97 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Thu, 9 Nov 2023 11:35:39 +0100
Subject: [PATCH 12/16] Remove SAML auth mr approval setting

- From frontend
---
 .../approvals/components/approval_settings.vue  | 17 -----------------
 .../approvals/components/group_settings/app.vue |  2 --
 .../assets/javascripts/approvals/constants.js   |  7 +++----
 ee/app/assets/javascripts/approvals/mappers.js  |  2 --
 .../approvals/mount_group_settings.js           | 14 ++++----------
 .../approvals/mount_project_settings.js         |  1 -
 .../stores/modules/approval_settings/actions.js |  3 ---
 .../modules/approval_settings/mutation_types.js |  1 -
 .../modules/approval_settings/mutations.js      |  3 ---
 .../components/approval_settings_spec.js        | 12 +-----------
 ee/spec/frontend/approvals/mappers_spec.js      |  1 -
 ee/spec/frontend/approvals/mocks.js             | 10 ----------
 .../modules/approval_settings/actions_spec.js   |  1 -
 .../modules/approval_settings/mutations_spec.js |  2 --
 14 files changed, 8 insertions(+), 68 deletions(-)

diff --git a/ee/app/assets/javascripts/approvals/components/approval_settings.vue b/ee/app/assets/javascripts/approvals/components/approval_settings.vue
index 04eddd21baee70..df08bddb817a6a 100644
--- a/ee/app/assets/javascripts/approvals/components/approval_settings.vue
+++ b/ee/app/assets/javascripts/approvals/components/approval_settings.vue
@@ -53,11 +53,6 @@ export default {
       required: false,
       default: true,
     },
-    lockRequireSamlAuth: {
-      type: Boolean,
-      required: false,
-      default: true,
-    },
   },
   computed: {
     ...mapState({
@@ -73,9 +68,7 @@ export default {
       selectiveCodeOwnerRemovals: (state) =>
         state.approvalSettings.settings.selectiveCodeOwnerRemovals,
       requireUserPassword: (state) => state.approvalSettings.settings.requireUserPassword,
-      requireSamlAuth: (state) => state.approvalSettings.settings.requireSamlAuth,
       groupName: (state) => state.settings.groupName,
-      samlProviderEnabled: (state) => state.settings.samlProviderEnabled,
     }),
     ...mapGetters(['settingChanged']),
     hasSettings() {
@@ -112,7 +105,6 @@ export default {
       'setRemoveApprovalsOnPush',
       'setSelectiveCodeOwnerRemovals',
       'setRequireUserPassword',
-      'setRequireSamlAuth',
     ]),
     handleWhenCommitIsAddedRadioGroupInput(value) {
       switch (value) {
@@ -217,15 +209,6 @@ export default {
           data-testid="require-user-password"
           @input="setRequireUserPassword"
         />
-        <approval-settings-checkbox
-          v-if="samlProviderEnabled"
-          :checked="requireSamlAuth.value"
-          :label="settingsLabels.requireSamlAuthLabel"
-          :locked="lockRequireSamlAuth"
-          :locked-text="lockedText(requireSamlAuth)"
-          data-testid="require-saml-auth"
-          @input="setRequireSamlAuth"
-        />
         <div class="gl-mt-5 gl-mb-3">{{ settingsLabels.whenCommitAddedLabel }}</div>
         <gl-form-radio-group
           :checked="whenCommitIsAddedRadioGroupValue"
diff --git a/ee/app/assets/javascripts/approvals/components/group_settings/app.vue b/ee/app/assets/javascripts/approvals/components/group_settings/app.vue
index 4e7366e9c325eb..66f8d04be83f7a 100644
--- a/ee/app/assets/javascripts/approvals/components/group_settings/app.vue
+++ b/ee/app/assets/javascripts/approvals/components/group_settings/app.vue
@@ -70,8 +70,6 @@ export default {
       <approval-settings
         :approval-settings-path="approvalSettingsPath"
         :settings-labels="$options.labels"
-        :lock-require-saml-auth="false"
-        :can-prevent-saml-auth-approval-edit="false"
       />
     </template>
   </settings-block>
diff --git a/ee/app/assets/javascripts/approvals/constants.js b/ee/app/assets/javascripts/approvals/constants.js
index 8d3de431210938..adf35119a59e45 100644
--- a/ee/app/assets/javascripts/approvals/constants.js
+++ b/ee/app/assets/javascripts/approvals/constants.js
@@ -56,16 +56,15 @@ export const PROJECT_APPROVAL_SETTINGS_LABELS_I18N = {
   preventCommittersApprovalLabel: s__(
     'ApprovalSettings|Prevent approvals by users who add commits',
   ),
-  requireUserPasswordLabel: s__('ApprovalSettings|Require user password to approve'),
+  requireUserPasswordLabel: s__(
+    'ApprovalSettings|Require user re-authentication (passoword or SAML) to approve',
+  ),
   whenCommitAddedLabel: s__('ApprovalSettings|When a commit is added:'),
   keepApprovalsLabel: s__('ApprovalSettings|Keep approvals'),
   removeApprovalsOnPushLabel: s__('ApprovalSettings|Remove all approvals'),
   selectiveCodeOwnerRemovalsLabel: s__(
     'ApprovalSettings|Remove approvals by Code Owners if their files changed',
   ),
-  requireSamlAuthLabel: s__(
-    'ApprovalSettings|Require SAML re-authentication to approve. Note: Overrides password requirement.',
-  ),
 };
 
 export const GROUP_APPROVAL_SETTINGS_LABELS_I18N = {
diff --git a/ee/app/assets/javascripts/approvals/mappers.js b/ee/app/assets/javascripts/approvals/mappers.js
index 4bc968a7797cac..de56d713fedfc2 100644
--- a/ee/app/assets/javascripts/approvals/mappers.js
+++ b/ee/app/assets/javascripts/approvals/mappers.js
@@ -110,7 +110,6 @@ export const mergeRequestApprovalSettingsMappers = {
         preventMrApprovalRuleEdit: invertApprovalSetting(
           data.allow_overrides_to_approver_list_per_merge_request,
         ),
-        requireSamlAuth: data.require_saml_auth_to_approve,
         requireUserPassword: data.require_password_to_approve,
         removeApprovalsOnPush: invertApprovalSetting(data.retain_approvals_on_push),
         preventCommittersApproval: invertApprovalSetting(data.allow_committer_approval),
@@ -121,7 +120,6 @@ export const mergeRequestApprovalSettingsMappers = {
   mapStateToPayload: ({ settings }) => ({
     allow_author_approval: !settings.preventAuthorApproval.value,
     allow_overrides_to_approver_list_per_merge_request: !settings.preventMrApprovalRuleEdit.value,
-    require_saml_auth_to_approve: settings.requireSamlAuth.value,
     require_password_to_approve: settings.requireUserPassword.value,
     retain_approvals_on_push: !settings.removeApprovalsOnPush.value,
     selective_code_owner_removals: settings.selectiveCodeOwnerRemovals.value,
diff --git a/ee/app/assets/javascripts/approvals/mount_group_settings.js b/ee/app/assets/javascripts/approvals/mount_group_settings.js
index 4ce1708f81406f..41bdd9a8a94cdf 100644
--- a/ee/app/assets/javascripts/approvals/mount_group_settings.js
+++ b/ee/app/assets/javascripts/approvals/mount_group_settings.js
@@ -11,15 +11,10 @@ const mountGroupApprovalSettings = (el) => {
     return null;
   }
 
-  const { defaultExpanded, approvalSettingsPath, samlProviderEnabled } = el.dataset;
-  const store = createStore(
-    {
-      approvalSettings: approvalSettingsModule(mergeRequestApprovalSettingsMappers),
-    },
-    {
-      samlProviderEnabled: parseBoolean(samlProviderEnabled),
-    },
-  );
+  const { defaultExpanded, approvalSettingsPath } = el.dataset;
+  const store = createStore({
+    approvalSettings: approvalSettingsModule(mergeRequestApprovalSettingsMappers),
+  });
 
   Vue.use(GlToast);
 
@@ -31,7 +26,6 @@ const mountGroupApprovalSettings = (el) => {
         props: {
           defaultExpanded: parseBoolean(defaultExpanded),
           approvalSettingsPath,
-          samlProviderEnabled: parseBoolean(samlProviderEnabled),
         },
       }),
   });
diff --git a/ee/app/assets/javascripts/approvals/mount_project_settings.js b/ee/app/assets/javascripts/approvals/mount_project_settings.js
index 05f9ed616721fa..1269cb4323be22 100644
--- a/ee/app/assets/javascripts/approvals/mount_project_settings.js
+++ b/ee/app/assets/javascripts/approvals/mount_project_settings.js
@@ -31,7 +31,6 @@ export default function mountProjectSettingsApprovals(el) {
     canEdit: parseBoolean(el.dataset.canEdit),
     canModifyAuthorSettings: parseBoolean(el.dataset.canModifyAuthorSettings),
     canModifyCommiterSettings: parseBoolean(el.dataset.canModifyCommiterSettings),
-    samlProviderEnabled: parseBoolean(el.dataset.samlProviderEnabled),
   });
 
   Vue.use(GlToast);
diff --git a/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/actions.js b/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/actions.js
index 56cc6e2c62275f..8315d3732c0247 100644
--- a/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/actions.js
+++ b/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/actions.js
@@ -65,7 +65,4 @@ export default (mapStateToPayload, updateMethod = 'put') => ({
   setRequireUserPassword({ commit }, value) {
     commit(types.SET_REQUIRE_USER_PASSWORD, value);
   },
-  setRequireSamlAuth({ commit }, value) {
-    commit(types.SET_REQUIRE_SAML_AUTH, value);
-  },
 });
diff --git a/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutation_types.js b/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutation_types.js
index 0b06978ecdad9b..7e29bcb66e04d4 100644
--- a/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutation_types.js
+++ b/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutation_types.js
@@ -15,4 +15,3 @@ export const SET_PREVENT_MR_APPROVAL_RULE_EDIT = 'SET_PREVENT_MR_APPROVAL_RULE_E
 export const SET_REMOVE_APPROVALS_ON_PUSH = 'SET_REMOVE_APPROVALS_ON_PUSH';
 export const SET_SELECTIVE_CODE_OWNER_REMOVALS = 'SET_SELECTIVE_CODE_OWNER_REMOVALS';
 export const SET_REQUIRE_USER_PASSWORD = 'SET_REQUIRE_USER_PASSWORD';
-export const SET_REQUIRE_SAML_AUTH = 'SET_REQUIRE_SAML_AUTH';
diff --git a/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutations.js b/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutations.js
index 86859843f28a44..344efe316b469d 100644
--- a/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutations.js
+++ b/ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutations.js
@@ -47,9 +47,6 @@ export default (mapDataToState) => ({
   [types.SET_SELECTIVE_CODE_OWNER_REMOVALS](state, value) {
     state.settings.selectiveCodeOwnerRemovals.value = value;
   },
-  [types.SET_REQUIRE_SAML_AUTH](state, value) {
-    state.settings.requireSamlAuth.value = value;
-  },
   [types.SET_REQUIRE_USER_PASSWORD](state, value) {
     state.settings.requireUserPassword.value = value;
   },
diff --git a/ee/spec/frontend/approvals/components/approval_settings_spec.js b/ee/spec/frontend/approvals/components/approval_settings_spec.js
index 15004c2c6c5f36..efb4ff706061cb 100644
--- a/ee/spec/frontend/approvals/components/approval_settings_spec.js
+++ b/ee/spec/frontend/approvals/components/approval_settings_spec.js
@@ -212,7 +212,6 @@ describe('ApprovalSettings', () => {
       ${'prevent-committers-approval'}   | ${'setPreventCommittersApproval'} | ${'preventCommittersApproval'} | ${'preventCommittersApprovalLabel'}
       ${'prevent-mr-approval-rule-edit'} | ${'setPreventMrApprovalRuleEdit'} | ${'preventMrApprovalRuleEdit'} | ${'preventMrApprovalRuleEditLabel'}
       ${'require-user-password'}         | ${'setRequireUserPassword'}       | ${'requireUserPassword'}       | ${'requireUserPasswordLabel'}
-      ${'require-saml-auth'}             | ${'setRequireSamlAuth'}           | ${'requireSamlAuth'}           | ${'requireSamlAuthLabel'}
     `('with the $testid checkbox', ({ testid, action, setting, labelKey }) => {
       let checkbox = null;
 
@@ -236,16 +235,7 @@ describe('ApprovalSettings', () => {
       });
 
       it('sets the checkbox locked prop', () => {
-        if (setting === 'requireSamlAuth') {
-          // requireSamlAuth defaults to always be locked unless explicitly unlocked (e.g. for grup settings)
-          expect(checkbox.props('locked')).toBe(true);
-
-          createWrapper({ lockRequireSamlAuth: false });
-          const unlockedCheckbox = wrapper.findByTestId(testid);
-          expect(unlockedCheckbox.props('locked')).toBe(false);
-        } else {
-          expect(checkbox.props('locked')).toBe(settings[setting].locked);
-        }
+        expect(checkbox.props('locked')).toBe(settings[setting].locked);
       });
 
       it('sets the checkbox lockedText prop', () => {
diff --git a/ee/spec/frontend/approvals/mappers_spec.js b/ee/spec/frontend/approvals/mappers_spec.js
index c4faea34b95c01..6bb517257c3079 100644
--- a/ee/spec/frontend/approvals/mappers_spec.js
+++ b/ee/spec/frontend/approvals/mappers_spec.js
@@ -14,7 +14,6 @@ describe('approvals mappers', () => {
       allow_author_approval: true,
       allow_overrides_to_approver_list_per_merge_request: true,
       require_password_to_approve: true,
-      require_saml_auth_to_approve: true,
       retain_approvals_on_push: false,
       selective_code_owner_removals: false,
       allow_committer_approval: true,
diff --git a/ee/spec/frontend/approvals/mocks.js b/ee/spec/frontend/approvals/mocks.js
index ad07e9dfda397f..282578abcec6f0 100644
--- a/ee/spec/frontend/approvals/mocks.js
+++ b/ee/spec/frontend/approvals/mocks.js
@@ -68,11 +68,6 @@ export const createGroupApprovalsPayload = () => ({
     locked: null,
     inheritedFrom: null,
   },
-  require_saml_auth_to_approve: {
-    value: true,
-    locked: null,
-    inherited_from: null,
-  },
   require_password_to_approve: {
     value: true,
     locked: null,
@@ -107,11 +102,6 @@ export const createGroupApprovalsState = (locked = null) => ({
       locked,
       value: false,
     },
-    requireSamlAuth: {
-      inheritedFrom: null,
-      locked,
-      value: true,
-    },
     requireUserPassword: {
       inheritedFrom: null,
       locked,
diff --git a/ee/spec/frontend/approvals/stores/modules/approval_settings/actions_spec.js b/ee/spec/frontend/approvals/stores/modules/approval_settings/actions_spec.js
index 453400a84ad12a..9d065555c305b8 100644
--- a/ee/spec/frontend/approvals/stores/modules/approval_settings/actions_spec.js
+++ b/ee/spec/frontend/approvals/stores/modules/approval_settings/actions_spec.js
@@ -137,7 +137,6 @@ describe('EE approvals group settings module actions', () => {
     ${'setRemoveApprovalsOnPush'}      | ${types.SET_REMOVE_APPROVALS_ON_PUSH}
     ${'setSelectiveCodeOwnerRemovals'} | ${types.SET_SELECTIVE_CODE_OWNER_REMOVALS}
     ${'setRequireUserPassword'}        | ${types.SET_REQUIRE_USER_PASSWORD}
-    ${'setRequireSamlAuth'}            | ${types.SET_REQUIRE_SAML_AUTH}
   `('$action', ({ action, type }) => {
     it(`commits ${type}`, () => {
       return testAction(actions[action], true, state, [{ type, payload: true }], []);
diff --git a/ee/spec/frontend/approvals/stores/modules/approval_settings/mutations_spec.js b/ee/spec/frontend/approvals/stores/modules/approval_settings/mutations_spec.js
index 3366ae45c943b2..b1f07492b92514 100644
--- a/ee/spec/frontend/approvals/stores/modules/approval_settings/mutations_spec.js
+++ b/ee/spec/frontend/approvals/stores/modules/approval_settings/mutations_spec.js
@@ -12,7 +12,6 @@ describe('Group settings store mutations', () => {
     preventCommittersApproval: { value: false },
     preventMrApprovalRuleEdit: { value: false },
     requireUserPassword: { value: false },
-    requireSamlAuth: { value: false },
     removeApprovalsOnPush: { value: false },
     selectiveCodeOwnerRemovals: { value: false },
   };
@@ -93,7 +92,6 @@ describe('Group settings store mutations', () => {
     ${'SET_REMOVE_APPROVALS_ON_PUSH'}      | ${'removeApprovalsOnPush'}
     ${'SET_SELECTIVE_CODE_OWNER_REMOVALS'} | ${'selectiveCodeOwnerRemovals'}
     ${'SET_REQUIRE_USER_PASSWORD'}         | ${'requireUserPassword'}
-    ${'SET_REQUIRE_SAML_AUTH'}             | ${'requireSamlAuth'}
   `('$mutation', ({ mutation, prop }) => {
     beforeEach(() => {
       mutations.RECEIVE_SETTINGS_SUCCESS(state, settings);
-- 
GitLab


From 6258abb5d1b1b5d3d8a391aa71cf6dc0955f44e3 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Thu, 9 Nov 2023 14:14:08 +0100
Subject: [PATCH 13/16] Use SsoEnforcer to determine SAML status

---
 ee/app/models/ee/project.rb                   | 11 -----
 .../presenters/ee/merge_request_presenter.rb  | 12 ++++-
 .../ee/merge_requests/approval_service.rb     | 10 +++-
 ee/lib/gitlab/auth/group_saml/sso_enforcer.rb | 11 +++--
 .../user_approves_with_saml_auth_spec.rb      |  1 -
 .../resolver_spec.rb                          | 27 -----------
 .../auth/group_saml/sso_enforcer_spec.rb      | 46 +++++++++++++++++--
 .../merge_requests/approval_service_spec.rb   | 38 ++++-----------
 locale/gitlab.pot                             |  5 +-
 9 files changed, 77 insertions(+), 84 deletions(-)

diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb
index f248adc75c035f..92854dd51d3293 100644
--- a/ee/app/models/ee/project.rb
+++ b/ee/app/models/ee/project.rb
@@ -832,17 +832,6 @@ def require_password_to_approve?
       !!require_password_to_approve
     end
 
-    def require_saml_auth_to_approve
-      ComplianceManagement::MergeRequestApprovalSettings::Resolver
-        .new(group&.root_ancestor, project: self)
-        .require_saml_auth_to_approve
-        .value
-    end
-
-    def require_saml_auth_to_approve?
-      !!require_saml_auth_to_approve
-    end
-
     def find_path_lock(path, exact_match: false, downstream: false)
       path_lock_finder = strong_memoize(:path_lock_finder) do
         ::Gitlab::PathLocksFinder.new(self)
diff --git a/ee/app/presenters/ee/merge_request_presenter.rb b/ee/app/presenters/ee/merge_request_presenter.rb
index 9cd0c1788e277f..fabd76dc5cf023 100644
--- a/ee/app/presenters/ee/merge_request_presenter.rb
+++ b/ee/app/presenters/ee/merge_request_presenter.rb
@@ -89,7 +89,17 @@ def group
     end
 
     def group_requires_saml_auth_for_approval?
-      !!ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group).require_saml_auth_to_approve.value
+      return false unless mr_approval_setting_password_required?
+
+      ::Gitlab::Auth::GroupSaml::SsoEnforcer.access_restricted?(
+        user: current_user,
+        resource: merge_request.project,
+        session_timeout: 0.seconds
+      )
+    end
+
+    def mr_approval_setting_password_required?
+      merge_request.require_password_to_approve?
     end
 
     def saml_approval_redirect_path
diff --git a/ee/app/services/ee/merge_requests/approval_service.rb b/ee/app/services/ee/merge_requests/approval_service.rb
index 5b3592e681348b..97e8c66e3b2277 100644
--- a/ee/app/services/ee/merge_requests/approval_service.rb
+++ b/ee/app/services/ee/merge_requests/approval_service.rb
@@ -10,6 +10,10 @@ module ApprovalService
 
       override :execute
       def execute(merge_request)
+        # TODO: rename merge request approval setting to require_reauthentication_to_approve
+        # Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/431346
+        return super unless merge_request.require_password_to_approve?
+
         require_saml_auth = approval_requires_saml_auth?(merge_request)
 
         return if require_saml_auth && !saml_approval_in_time?
@@ -26,7 +30,11 @@ def incorrect_approval_password?(merge_request)
       end
 
       def approval_requires_saml_auth?(merge_request)
-        merge_request.project.require_saml_auth_to_approve?
+        ::Gitlab::Auth::GroupSaml::SsoEnforcer.access_restricted?(
+          user: current_user,
+          resource: merge_request.project,
+          session_timeout: 0.seconds
+        )
       end
 
       def saml_provider
diff --git a/ee/lib/gitlab/auth/group_saml/sso_enforcer.rb b/ee/lib/gitlab/auth/group_saml/sso_enforcer.rb
index 6c6faa54cb2649..4480588a7b0541 100644
--- a/ee/lib/gitlab/auth/group_saml/sso_enforcer.rb
+++ b/ee/lib/gitlab/auth/group_saml/sso_enforcer.rb
@@ -7,7 +7,7 @@ class SsoEnforcer
         DEFAULT_SESSION_TIMEOUT = 1.day
 
         class << self
-          def access_restricted?(user:, resource:)
+          def access_restricted?(user:, resource:, session_timeout: DEFAULT_SESSION_TIMEOUT)
             group = resource.is_a?(::Group) ? resource : resource.group
 
             return false unless group
@@ -17,7 +17,7 @@ def access_restricted?(user:, resource:)
             return false unless saml_provider
             return false if user_authorized?(user, resource)
 
-            new(saml_provider, user: user).access_restricted?
+            new(saml_provider, user: user, session_timeout: session_timeout).access_restricted?
           end
 
           # Given an array of groups or subgroups, return an array
@@ -47,11 +47,12 @@ def resource_member?(resource, user)
           end
         end
 
-        attr_reader :saml_provider, :user
+        attr_reader :saml_provider, :user, :session_timeout
 
-        def initialize(saml_provider, user: nil)
+        def initialize(saml_provider, user: nil, session_timeout: DEFAULT_SESSION_TIMEOUT)
           @saml_provider = saml_provider
           @user = user
+          @session_timeout = session_timeout
         end
 
         def update_session
@@ -59,7 +60,7 @@ def update_session
         end
 
         def active_session?
-          SsoState.new(saml_provider.id).active_since?(DEFAULT_SESSION_TIMEOUT.ago)
+          SsoState.new(saml_provider.id).active_since?(session_timeout.ago)
         end
 
         def access_restricted?
diff --git a/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb b/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
index 98ea621d6408ac..69deda7a594bd4 100644
--- a/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
+++ b/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
@@ -53,7 +53,6 @@ def sign_in_and_connect_saml_identity
   def approve_with_saml
     visit project_merge_request_path(project, merge_request)
     page.within('.js-mr-approvals') { click_button 'Approve with SAML' }
-    wait_for_requests
   end
 
   def unapprove
diff --git a/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb b/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb
index a0ee82065ad6b8..725317af0a89b3 100644
--- a/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb
+++ b/ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb
@@ -224,31 +224,4 @@
       it_behaves_like 'a MR approval setting'
     end
   end
-
-  describe '#require_saml_auth_to_approve' do
-    where(:group_requires_saml_auth, :project_requires_saml_auth, :locked, :value, :inherited_from) do
-      true  | true | true | true  | :group
-      false | true | true | false | :group
-      nil   | true | true | false | :group
-      nil   | true | true | false | :group
-    end
-
-    with_them do
-      before do
-        group.group_merge_request_approval_setting.update!(require_saml_auth_to_approve: !!group_requires_saml_auth)
-      end
-
-      context "with project" do
-        let(:object) { described_class.new(group, project: project).require_saml_auth_to_approve }
-
-        it_behaves_like 'a MR approval setting'
-      end
-
-      context "without project" do
-        let(:object) { described_class.new(group).require_saml_auth_to_approve }
-
-        it_behaves_like 'a MR approval setting'
-      end
-    end
-  end
 end
diff --git a/ee/spec/lib/gitlab/auth/group_saml/sso_enforcer_spec.rb b/ee/spec/lib/gitlab/auth/group_saml/sso_enforcer_spec.rb
index f8b2a018307a7d..984598649bfe21 100644
--- a/ee/spec/lib/gitlab/auth/group_saml/sso_enforcer_spec.rb
+++ b/ee/spec/lib/gitlab/auth/group_saml/sso_enforcer_spec.rb
@@ -50,6 +50,24 @@
         expect(subject).not_to be_active_session
       end
     end
+
+    context 'when a session timeout is specified' do
+      subject(:enforcer) { described_class.new(saml_provider, user: user, session_timeout: 1.hour) }
+
+      it 'returns true within timeout' do
+        enforcer.update_session
+
+        expect(enforcer).to be_active_session
+      end
+
+      it 'returns false after timeout elapses' do
+        enforcer.update_session
+
+        travel_to(2.hours.from_now) do
+          expect(enforcer).not_to be_active_session
+        end
+      end
+    end
   end
 
   describe '#access_restricted?' do
@@ -61,12 +79,32 @@
       end
 
       context 'when there is active saml session' do
-        before do
-          subject.update_session
+        context 'when the session timeout is the default' do
+          before do
+            subject.update_session
+          end
+
+          it 'returns false' do
+            expect(subject).not_to be_access_restricted
+          end
         end
 
-        it 'returns false' do
-          expect(subject).not_to be_access_restricted
+        context 'when a session timeout is specified' do
+          subject(:enforcer) { described_class.new(saml_provider, user: user, session_timeout: 1.hour) }
+
+          it 'returns true within timeout' do
+            enforcer.update_session
+
+            expect(enforcer).not_to be_access_restricted
+          end
+
+          it 'returns false after timeout elapses' do
+            enforcer.update_session
+
+            travel_to(2.hours.from_now) do
+              expect(enforcer).to be_access_restricted
+            end
+          end
         end
       end
 
diff --git a/ee/spec/services/merge_requests/approval_service_spec.rb b/ee/spec/services/merge_requests/approval_service_spec.rb
index 3c1cf057f0069f..352385ae5a4d2b 100644
--- a/ee/spec/services/merge_requests/approval_service_spec.rb
+++ b/ee/spec/services/merge_requests/approval_service_spec.rb
@@ -16,33 +16,26 @@
     end
 
     let_it_be(:merge_request) { create :merge_request_with_diffs, source_project: project, reviewers: [user] }
+    let(:enforced_sso) { false }
 
     subject(:service) { described_class.new(project: project, current_user: user) }
 
     before do
       stub_licensed_features(group_saml: true)
+      create(:saml_provider, group: project.group, enforced_sso: enforced_sso, enabled: true)
     end
 
     before_all do
-      group.add_developer(user)
       project.add_developer(user)
     end
 
-    def simulate_require_saml_auth_to_approve_mr_approval_setting
-      create(:saml_provider, group: project.group, enabled: true)
-      allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver|
-        allow(resolver).to receive(:require_saml_auth_to_approve)
-          .and_return(ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
-            value: true,
-            locked: true,
-            inherited_from: :group
-          ))
-      end
+    def simulate_require_saml_auth_to_approve_mr_approval_setting(restricted: true)
+      allow(::Gitlab::Auth::GroupSaml::SsoEnforcer).to(receive(:access_restricted?).and_return(restricted))
     end
 
     def simulate_saml_approval_in_time?(in_time:)
-      allow_next_instance_of(MergeRequests::ApprovalService) do |service|
-        allow(service).to receive(:saml_approval_in_time?).and_return(in_time)
+      allow_next_instance_of(::Gitlab::Auth::GroupSaml::SsoState) do |state|
+        allow(state).to receive(:active_since?).and_return(in_time)
       end
     end
 
@@ -101,9 +94,10 @@ def simulate_saml_approval_in_time?(in_time:)
         end
 
         context 'when SAML auth is required' do
+          let(:enforced_sso) { true }
+
           it 'does not change approval count' do
             simulate_require_saml_auth_to_approve_mr_approval_setting
-            simulate_saml_approval_in_time?(in_time: false)
 
             service_with_params = described_class.new(project: project, current_user: user, params: params)
 
@@ -112,21 +106,5 @@ def simulate_saml_approval_in_time?(in_time:)
         end
       end
     end
-
-    context 'when SAML auth is required' do
-      it 'approves when whithin the timeout' do
-        simulate_require_saml_auth_to_approve_mr_approval_setting
-        simulate_saml_approval_in_time?(in_time: true)
-
-        expect { service.execute(merge_request) }.to change { merge_request.approvals.size }
-      end
-
-      it 'rejects approaval when exceeding the timeout' do
-        simulate_require_saml_auth_to_approve_mr_approval_setting
-        simulate_saml_approval_in_time?(in_time: false)
-
-        expect { service.execute(merge_request) }.not_to change { merge_request.approvals.size }
-      end
-    end
   end
 end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index ee78bbf0e5c7d3..7cc71aedce1170 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -6258,10 +6258,7 @@ msgstr ""
 msgid "ApprovalSettings|Remove approvals by Code Owners if their files changed"
 msgstr ""
 
-msgid "ApprovalSettings|Require SAML re-authentication to approve. Note: Overrides password requirement."
-msgstr ""
-
-msgid "ApprovalSettings|Require user password to approve"
+msgid "ApprovalSettings|Require user re-authentication (passoword or SAML) to approve"
 msgstr ""
 
 msgid "ApprovalSettings|There was an error loading merge request approval settings."
-- 
GitLab


From 4288dcc70ee10760aca0ab8817e6a5c31eb9e310 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Fri, 10 Nov 2023 12:26:54 +0100
Subject: [PATCH 14/16] Spec failed approval in feature

- Refs:
  https://gitlab.com/gitlab-org/gitlab/-/jobs/5503376343
---
 .../user_approves_with_saml_auth_spec.rb          | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb b/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
index 69deda7a594bd4..b04e18a0b07ab1 100644
--- a/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
+++ b/ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb
@@ -55,7 +55,7 @@ def approve_with_saml
     page.within('.js-mr-approvals') { click_button 'Approve with SAML' }
   end
 
-  def unapprove
+  def revoke_approval
     click_button 'Revoke approval'
     wait_for_requests
   end
@@ -63,6 +63,7 @@ def unapprove
   it 'shows user can approve and unapprove' do
     sign_in_and_connect_saml_identity
 
+    # approval
     approve_with_saml
 
     expect(page).to have_text('Approved')
@@ -72,9 +73,19 @@ def unapprove
       expect(page).to have_text('Approved by you')
     end
 
-    unapprove
+    # revoke approval
+    revoke_approval
 
     expect(page).to have_button('Approve with SAML')
     expect(page).not_to have_text('Approved by')
+
+    # flash message with error when approval fails
+    allow_next_instance_of(::MergeRequests::ApprovalService) do |approval_service|
+      allow(approval_service).to receive(:execute).and_return(nil)
+    end
+    visit saml_approval_namespace_project_merge_request_path(group, project, merge_request)
+
+    approve_with_saml
+    expect(page).to have_text('Approval rejected')
   end
 end
-- 
GitLab


From 38695b982a18b9bdc275e6140d4fcdcd28904d08 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Fri, 10 Nov 2023 12:35:33 +0100
Subject: [PATCH 15/16] Fix spelling mistakes

- Original fix in: 7fea9b499eb16b8df6c9387094d0db4577c8a93b
  Rebasing on master
  With force push
---
 ee/app/assets/javascripts/approvals/constants.js | 2 +-
 locale/gitlab.pot                                | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ee/app/assets/javascripts/approvals/constants.js b/ee/app/assets/javascripts/approvals/constants.js
index adf35119a59e45..963511ad306d19 100644
--- a/ee/app/assets/javascripts/approvals/constants.js
+++ b/ee/app/assets/javascripts/approvals/constants.js
@@ -57,7 +57,7 @@ export const PROJECT_APPROVAL_SETTINGS_LABELS_I18N = {
     'ApprovalSettings|Prevent approvals by users who add commits',
   ),
   requireUserPasswordLabel: s__(
-    'ApprovalSettings|Require user re-authentication (passoword or SAML) to approve',
+    'ApprovalSettings|Require user re-authentication (password or SAML) to approve',
   ),
   whenCommitAddedLabel: s__('ApprovalSettings|When a commit is added:'),
   keepApprovalsLabel: s__('ApprovalSettings|Keep approvals'),
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 7cc71aedce1170..0abe6002bf5c4d 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -6258,7 +6258,7 @@ msgstr ""
 msgid "ApprovalSettings|Remove approvals by Code Owners if their files changed"
 msgstr ""
 
-msgid "ApprovalSettings|Require user re-authentication (passoword or SAML) to approve"
+msgid "ApprovalSettings|Require user re-authentication (password or SAML) to approve"
 msgstr ""
 
 msgid "ApprovalSettings|There was an error loading merge request approval settings."
-- 
GitLab


From a6690c916e38c923eeb217db62b7e20c0b750692 Mon Sep 17 00:00:00 2001
From: Sam Figueroa <sfigueroa@gitlab.com>
Date: Fri, 10 Nov 2023 14:27:11 +0100
Subject: [PATCH 16/16] Remove superfluous require_saml_to_approve references

- Refs:
  https://gitlab.com/gitlab-org/gitlab/-/jobs/5508352296
  https://gitlab.com/gitlab-org/gitlab/-/jobs/5508352293
  https://gitlab.com/gitlab-org/gitlab/-/jobs/5508292581
---
 ee/lib/ee/api/entities/approval_settings.rb   |  3 --
 .../public_api/v4/project_approvers.json      |  6 ---
 ee/spec/models/ee/project_spec.rb             | 39 -------------------
 3 files changed, 48 deletions(-)

diff --git a/ee/lib/ee/api/entities/approval_settings.rb b/ee/lib/ee/api/entities/approval_settings.rb
index b8d580facc81d4..7e54d530e31553 100644
--- a/ee/lib/ee/api/entities/approval_settings.rb
+++ b/ee/lib/ee/api/entities/approval_settings.rb
@@ -20,9 +20,6 @@ class ApprovalSettings < Grape::Entity
         expose :merge_requests_disable_committers_approval?,
           as: :merge_requests_disable_committers_approval
 
-        expose :require_saml_auth_to_approve?,
-          as: :require_saml_auth_to_approve
-
         expose :require_password_to_approve?,
           as: :require_password_to_approve
       end
diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/project_approvers.json b/ee/spec/fixtures/api/schemas/public_api/v4/project_approvers.json
index 16a5344e17974b..83813baf1a681d 100644
--- a/ee/spec/fixtures/api/schemas/public_api/v4/project_approvers.json
+++ b/ee/spec/fixtures/api/schemas/public_api/v4/project_approvers.json
@@ -34,12 +34,6 @@
         "null"
       ]
     },
-    "require_saml_auth_to_approve": {
-      "type": [
-        "boolean",
-        "null"
-      ]
-    },
     "approvers": {
       "type": "array",
       "items": {
diff --git a/ee/spec/models/ee/project_spec.rb b/ee/spec/models/ee/project_spec.rb
index 31bee24f46bc95..fbf7a5648b1f18 100644
--- a/ee/spec/models/ee/project_spec.rb
+++ b/ee/spec/models/ee/project_spec.rb
@@ -1199,45 +1199,6 @@
       end
     end
 
-    describe '#require_saml_to_approve?' do
-      let_it_be(:root_ancestor) { create(:group) }
-      let_it_be(:sub_group) { create(:group, parent: root_ancestor) }
-
-      before do
-        allow(project).to receive(:group).and_return(sub_group)
-      end
-
-      it 'returns true when the resolver returns true' do
-        expect(ComplianceManagement::MergeRequestApprovalSettings::Resolver).to receive(:new).with(root_ancestor, project: project)
-
-        allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver|
-          allow(resolver).to receive(:require_saml_auth_to_approve)
-            .and_return(ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
-              value: true,
-              locked: false,
-              inherited_from: nil
-            ))
-        end
-
-        expect(project.require_saml_auth_to_approve).to be true
-      end
-
-      it 'returns false when the resolver returns false' do
-        expect(ComplianceManagement::MergeRequestApprovalSettings::Resolver).to receive(:new).with(root_ancestor, project: project)
-
-        allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver|
-          allow(resolver).to receive(:require_saml_auth_to_approve)
-            .and_return(ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
-              value: false,
-              locked: false,
-              inherited_from: nil
-            ))
-        end
-
-        expect(project.require_saml_auth_to_approve).to be false
-      end
-    end
-
     describe '#merge_requests_author_approval' do
       let(:setting) { :merge_requests_author_approval }
       let(:application_setting) { :prevent_merge_requests_author_approval }
-- 
GitLab