Skip to content
Snippets Groups Projects
Commit 9de4dca8 authored by Drew Blessing's avatar Drew Blessing :three:
Browse files

Merge branch 'sf/feature/GL421959-saml-auth-flow-for-mr-approval' into 'master'

Add optional SAML Auth requirement for MR approval

See merge request !130204



Merged-by: default avatarDrew Blessing <drew@gitlab.com>
Approved-by: default avatarIllya Klymov <xanf@xanf.me>
Approved-by: default avatarDrew Blessing <drew@gitlab.com>
Reviewed-by: default avatarDrew Blessing <drew@gitlab.com>
Reviewed-by: Jessie Young's avatarJessie Young <jessieyoung@gitlab.com>
Co-authored-by: default avatarSam Figueroa <sfigueroa@gitlab.com>
parents c933ecfa 3fc89ba6
No related branches found
No related tags found
1 merge request!130204Add optional SAML Auth requirement for MR approval
Pipeline #1069618481 passed
Pipeline: E2E Omnibus GitLab EE

#1069654026

    Pipeline: GitLab

    #1069624370

      Pipeline: E2E GDK

      #1069624288

        +22
        Showing
        with 401 additions and 50 deletions
        <script> <script>
        import { GlButton, GlSprintf } from '@gitlab/ui'; import { GlButton, GlSprintf } from '@gitlab/ui';
        import { createAlert } from '~/alert'; import { createAlert } from '~/alert';
        import { visitUrl } from '~/lib/utils/url_utility';
        import { STATUS_MERGED } from '~/issues/constants'; import { STATUS_MERGED } from '~/issues/constants';
        import { BV_SHOW_MODAL } from '~/lib/utils/constants'; import { BV_SHOW_MODAL } from '~/lib/utils/constants';
        import { HTTP_STATUS_UNAUTHORIZED } from '~/lib/utils/http_status'; import { HTTP_STATUS_UNAUTHORIZED } from '~/lib/utils/http_status';
        ...@@ -114,6 +115,13 @@ export default { ...@@ -114,6 +115,13 @@ export default {
        return this.userHasApproved && !this.userCanApprove && this.mr.state !== STATUS_MERGED; return this.userHasApproved && !this.userCanApprove && this.mr.state !== STATUS_MERGED;
        }, },
        approvalText() { 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 return this.isApproved && this.approvedBy.length > 0
        ? s__('mrWidget|Approve additionally') ? s__('mrWidget|Approve additionally')
        : s__('mrWidget|Approve'); : s__('mrWidget|Approve');
        ...@@ -161,14 +169,20 @@ export default { ...@@ -161,14 +169,20 @@ export default {
        .join(', ') .join(', ')
        .concat('.'); .concat('.');
        }, },
        requireSamlAuthToApprove() {
        return this.mr.requireSamlAuthToApprove;
        },
        }, },
        methods: { methods: {
        approve() { approve() {
        if (this.requireSamlAuthToApprove) {
        this.approveWithSamlAuth();
        return;
        }
        if (this.requirePasswordToApprove) { if (this.requirePasswordToApprove) {
        this.$root.$emit(BV_SHOW_MODAL, this.modalId); this.$root.$emit(BV_SHOW_MODAL, this.modalId);
        return; return;
        } }
        this.updateApproval( this.updateApproval(
        () => this.service.approveMergeRequest(), () => this.service.approveMergeRequest(),
        () => () =>
        ...@@ -179,6 +193,10 @@ export default { ...@@ -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) { approveWithAuth(data) {
        this.updateApproval( this.updateApproval(
        () => this.service.approveMergeRequestWithAuth(data), () => this.service.approveMergeRequestWithAuth(data),
        ......
        ...@@ -56,7 +56,9 @@ export const PROJECT_APPROVAL_SETTINGS_LABELS_I18N = { ...@@ -56,7 +56,9 @@ export const PROJECT_APPROVAL_SETTINGS_LABELS_I18N = {
        preventCommittersApprovalLabel: s__( preventCommittersApprovalLabel: s__(
        'ApprovalSettings|Prevent approvals by users who add commits', 'ApprovalSettings|Prevent approvals by users who add commits',
        ), ),
        requireUserPasswordLabel: s__('ApprovalSettings|Require user password to approve'), requireUserPasswordLabel: s__(
        'ApprovalSettings|Require user re-authentication (password or SAML) to approve',
        ),
        whenCommitAddedLabel: s__('ApprovalSettings|When a commit is added:'), whenCommitAddedLabel: s__('ApprovalSettings|When a commit is added:'),
        keepApprovalsLabel: s__('ApprovalSettings|Keep approvals'), keepApprovalsLabel: s__('ApprovalSettings|Keep approvals'),
        removeApprovalsOnPushLabel: s__('ApprovalSettings|Remove all approvals'), removeApprovalsOnPushLabel: s__('ApprovalSettings|Remove all approvals'),
        ......
        ...@@ -29,6 +29,7 @@ export default class MergeRequestStore extends CEMergeRequestStore { ...@@ -29,6 +29,7 @@ export default class MergeRequestStore extends CEMergeRequestStore {
        this.appUrl = gon && gon.gitlab_url; this.appUrl = gon && gon.gitlab_url;
        this.licenseScanning = data.license_scanning; this.licenseScanning = data.license_scanning;
        this.requirePasswordToApprove = data.require_password_to_approve; this.requirePasswordToApprove = data.require_password_to_approve;
        this.requireSamlAuthToApprove = data.require_saml_auth_to_approve;
        this.mergeRequestApproversAvailable = data.merge_request_approvers_available; this.mergeRequestApproversAvailable = data.merge_request_approvers_available;
        this.aiCommitMessageEnabled = data.aiCommitMessageEnabled; this.aiCommitMessageEnabled = data.aiCommitMessageEnabled;
        ...@@ -62,6 +63,7 @@ export default class MergeRequestStore extends CEMergeRequestStore { ...@@ -62,6 +63,7 @@ export default class MergeRequestStore extends CEMergeRequestStore {
        this.discoverProjectSecurityPath = data.discover_project_security_path; this.discoverProjectSecurityPath = data.discover_project_security_path;
        this.apiStatusChecksPath = data.api_status_checks_path; this.apiStatusChecksPath = data.api_status_checks_path;
        this.samlApprovalPath = data.saml_approval_path;
        // Security scan diff paths // Security scan diff paths
        this.containerScanningComparisonPath = data.container_scanning_comparison_path; this.containerScanningComparisonPath = data.container_scanning_comparison_path;
        ......
        # frozen_string_literal: true
        module Projects
        module MergeRequests
        class SamlApprovalsController < Projects::ApplicationController
        feature_category :code_review_workflow
        def create
        return render_404 unless merge_request
        result = ::MergeRequests::ApprovalService
        .new(project: project, current_user: current_user)
        .execute(merge_request)
        if result
        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
        ...@@ -62,6 +62,7 @@ def approvals_app_data(project = @project) ...@@ -62,6 +62,7 @@ def approvals_app_data(project = @project)
        can_edit: can_modify_approvers.to_s, can_edit: can_modify_approvers.to_s,
        can_modify_author_settings: can_modify_author_settings.to_s, can_modify_author_settings: can_modify_author_settings.to_s,
        can_modify_commiter_settings: can_modify_commiter_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)), 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)), 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)), rules_path: expose_path(api_v4_projects_approval_rules_path(id: project.id)),
        ...@@ -75,6 +76,13 @@ def approvals_app_data(project = @project) ...@@ -75,6 +76,13 @@ def approvals_app_data(project = @project)
        } }
        end 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) def status_checks_app_data(project)
        { {
        data: { data: {
        ......
        ...@@ -15,4 +15,10 @@ def authorize_gma_conversion_confirm_modal_data(group_name:, phrase:, remove_for ...@@ -15,4 +15,10 @@ def authorize_gma_conversion_confirm_modal_data(group_name:, phrase:, remove_for
        phrase: phrase phrase: phrase
        } }
        end end
        def saml_provider_enabled?(group = nil)
        return false unless group.is_a? Group
        !!group.root_ancestor.saml_provider&.enabled?
        end
        end end
        ...@@ -11,16 +11,19 @@ class GroupMergeRequestApprovalSetting < ApplicationRecord ...@@ -11,16 +11,19 @@ class GroupMergeRequestApprovalSetting < ApplicationRecord
        :allow_overrides_to_approver_list_per_merge_request, :allow_overrides_to_approver_list_per_merge_request,
        :retain_approvals_on_push, :retain_approvals_on_push,
        :require_password_to_approve, :require_password_to_approve,
        :require_saml_auth_to_approve,
        inclusion: { in: [true, false], message: N_('must be a boolean value') } inclusion: { in: [true, false], message: N_('must be a boolean value') }
        scope :find_or_initialize_by_group, ->(group) { find_or_initialize_by(group: group) } 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', AUDIT_LOG_ALLOWLIST = {
        allow_committer_approval: 'prevent merge request approval from committers', allow_author_approval: 'prevent merge request approval from authors',
        allow_overrides_to_approver_list_per_merge_request: allow_committer_approval: 'prevent merge request approval from committers',
        'prevent users from modifying MR approval rules in merge requests', allow_overrides_to_approver_list_per_merge_request:
        retain_approvals_on_push: 'require new approvals when new commits are added to an MR', 'prevent users from modifying MR approval rules in merge requests',
        require_password_to_approve: 'require user password for approvals' }.freeze 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
        def selective_code_owner_removals def selective_code_owner_removals
        false false
        ......
        ...@@ -66,8 +66,47 @@ def issue_keys ...@@ -66,8 +66,47 @@ def issue_keys
        ).issue_keys ).issue_keys
        end 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 private
        def group
        target_project.namespace
        end
        def group_requires_saml_auth_for_approval?
        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
        # Will not work with URL since the SSO controller will sanatize it
        saml_approval_namespace_project_merge_request_path(group, target_project, merge_request.iid)
        end
        def expose_mr_status_checks? def expose_mr_status_checks?
        current_user.present? && current_user.present? &&
        project.external_status_checks.applicable_to_branch(merge_request.target_branch).any? project.external_status_checks.applicable_to_branch(merge_request.target_branch).any?
        ......
        ...@@ -108,12 +108,14 @@ module MergeRequestWidgetEntity ...@@ -108,12 +108,14 @@ module MergeRequestWidgetEntity
        expose :merge_immediately_docs_path do |merge_request| expose :merge_immediately_docs_path do |merge_request|
        presenter(merge_request).merge_immediately_docs_path presenter(merge_request).merge_immediately_docs_path
        end end
        end
        def represent_blocking_mr(blocking_mr) expose :saml_approval_path do |merge_request|
        blocking_mr_options = options.merge(from_project: object.target_project) 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 end
        end end
        ...@@ -4,12 +4,20 @@ module EE ...@@ -4,12 +4,20 @@ module EE
        module MergeRequests module MergeRequests
        module ApprovalService module ApprovalService
        extend ::Gitlab::Utils::Override extend ::Gitlab::Utils::Override
        # 5 seconds is chosen arbitrarily to ensure the user needs to just have re-authenticated to approve
        IncorrectApprovalPasswordError = Class.new(StandardError) # Timeframe gives a short grace period for the callback from the identity provider to have processed.
        SAML_APPROVE_TIMEOUT = 5.seconds
        override :execute override :execute
        def execute(merge_request) def execute(merge_request)
        return if incorrect_approval_password?(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?
        return if incorrect_approval_password?(merge_request) && !require_saml_auth
        super super
        end end
        ...@@ -21,6 +29,26 @@ def incorrect_approval_password?(merge_request) ...@@ -21,6 +29,26 @@ def incorrect_approval_password?(merge_request)
        !::Gitlab::Auth.find_with_user_password(current_user.username, params[:approval_password]) !::Gitlab::Auth.find_with_user_password(current_user.username, params[:approval_password])
        end end
        def approval_requires_saml_auth?(merge_request)
        ::Gitlab::Auth::GroupSaml::SsoEnforcer.access_restricted?(
        user: current_user,
        resource: merge_request.project,
        session_timeout: 0.seconds
        )
        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 override :reset_approvals_cache
        def reset_approvals_cache(merge_request) def reset_approvals_cache(merge_request)
        merge_request.reset_approval_cache! merge_request.reset_approval_cache!
        ......
        ...@@ -5,4 +5,4 @@ ...@@ -5,4 +5,4 @@
        - return unless show_merge_request_approval_settings?(user, group) - 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)) - 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 } }
        ...@@ -16,6 +16,9 @@ ...@@ -16,6 +16,9 @@
        get :api_fuzzing_reports get :api_fuzzing_reports
        get :security_reports get :security_reports
        # 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 post :rebase
        end end
        ......
        ...@@ -9,6 +9,7 @@ class MergeRequestApprovalSetting < Grape::Entity ...@@ -9,6 +9,7 @@ class MergeRequestApprovalSetting < Grape::Entity
        expose :retain_approvals_on_push, documentation: { type: 'boolean', example: true } expose :retain_approvals_on_push, documentation: { type: 'boolean', example: true }
        expose :selective_code_owner_removals, 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_password_to_approve, documentation: { type: 'boolean', example: true }
        expose :require_saml_auth_to_approve, documentation: { type: 'boolean', example: true }
        end end
        end end
        end end
        ...@@ -18,13 +18,16 @@ class MergeRequestApprovalSettings < ::API::Base ...@@ -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 :selective_code_owner_removals, type: Boolean, desc: 'Reset approvals from Code Owners if their files changed', allow_blank: false
        optional :require_password_to_approve, optional :require_password_to_approve,
        type: Boolean, desc: 'Require approver to authenticate before approving', allow_blank: false 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, at_least_one_of :allow_author_approval,
        :allow_committer_approval, :allow_committer_approval,
        :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_saml_auth_to_approve
        end end
        end end
        ......
        ...@@ -14,7 +14,8 @@ def execute ...@@ -14,7 +14,8 @@ def execute
        allow_overrides_to_approver_list_per_merge_request: allow_overrides_to_approver_list_per_merge_request, allow_overrides_to_approver_list_per_merge_request: allow_overrides_to_approver_list_per_merge_request,
        retain_approvals_on_push: retain_approvals_on_push, retain_approvals_on_push: retain_approvals_on_push,
        selective_code_owner_removals: selective_code_owner_removals, 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 end
        ...@@ -70,6 +71,17 @@ def require_password_to_approve ...@@ -70,6 +71,17 @@ def require_password_to_approve
        ) )
        end 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: is always true for projects frontend will manually ovrride it for groups
        locked: true,
        inherited_from: :group
        )
        end
        private private
        def instance_settings def instance_settings
        ...@@ -77,6 +89,8 @@ def instance_settings ...@@ -77,6 +89,8 @@ def instance_settings
        end end
        def group_settings def group_settings
        return unless @group.is_a? ::Group
        @group&.group_merge_request_approval_setting @group&.group_merge_request_approval_setting
        end end
        ......
        ...@@ -7,7 +7,7 @@ class SsoEnforcer ...@@ -7,7 +7,7 @@ class SsoEnforcer
        DEFAULT_SESSION_TIMEOUT = 1.day DEFAULT_SESSION_TIMEOUT = 1.day
        class << self 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 group = resource.is_a?(::Group) ? resource : resource.group
        return false unless group return false unless group
        ...@@ -17,7 +17,7 @@ def access_restricted?(user:, resource:) ...@@ -17,7 +17,7 @@ def access_restricted?(user:, resource:)
        return false unless saml_provider return false unless saml_provider
        return false if user_authorized?(user, resource) 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 end
        # Given an array of groups or subgroups, return an array # Given an array of groups or subgroups, return an array
        ...@@ -47,11 +47,12 @@ def resource_member?(resource, user) ...@@ -47,11 +47,12 @@ def resource_member?(resource, user)
        end end
        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 @saml_provider = saml_provider
        @user = user @user = user
        @session_timeout = session_timeout
        end end
        def update_session def update_session
        ...@@ -59,7 +60,7 @@ def update_session ...@@ -59,7 +60,7 @@ def update_session
        end end
        def active_session? 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 end
        def access_restricted? def access_restricted?
        ......
        # 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) 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,
        :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' }
        end
        def revoke_approval
        click_button 'Revoke approval'
        wait_for_requests
        end
        it 'shows user can approve and unapprove' do
        sign_in_and_connect_saml_identity
        # approval
        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
        # 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
        ...@@ -2,34 +2,81 @@ ...@@ -2,34 +2,81 @@
        "type": "object", "type": "object",
        "properties": { "properties": {
        "allow_author_approval": { "allow_author_approval": {
        "value": { "type": "boolean" }, "value": {
        "locked": { "type": "boolean" }, "type": "boolean"
        "inherited_from": { "type": "string" } },
        "locked": {
        "type": "boolean"
        },
        "inherited_from": {
        "type": "string"
        }
        }, },
        "allow_committer_approval": { "allow_committer_approval": {
        "value": { "type": "boolean" }, "value": {
        "locked": { "type": "boolean" }, "type": "boolean"
        "inherited_from": { "type": "string" } },
        "locked": {
        "type": "boolean"
        },
        "inherited_from": {
        "type": "string"
        }
        }, },
        "allow_overrides_to_approver_list_per_merge_request": { "allow_overrides_to_approver_list_per_merge_request": {
        "value": { "type": "boolean" }, "value": {
        "locked": { "type": "boolean" }, "type": "boolean"
        "inherited_from": { "type": "string" } },
        "locked": {
        "type": "boolean"
        },
        "inherited_from": {
        "type": "string"
        }
        }, },
        "retain_approvals_on_push": { "retain_approvals_on_push": {
        "value": { "type": "boolean" }, "value": {
        "locked": { "type": "boolean" }, "type": "boolean"
        "inherited_from": { "type": "string" } },
        "locked": {
        "type": "boolean"
        },
        "inherited_from": {
        "type": "string"
        }
        }, },
        "selective_code_owner_removals": { "selective_code_owner_removals": {
        "value": { "type": "boolean" }, "value": {
        "locked": { "type": "boolean" }, "type": "boolean"
        "inherited_from": { "type": "string" } },
        "locked": {
        "type": "boolean"
        },
        "inherited_from": {
        "type": "string"
        }
        }, },
        "require_password_to_approve": { "require_password_to_approve": {
        "value": { "type": "boolean" }, "value": {
        "locked": { "type": "boolean" }, "type": "boolean"
        "inherited_from": { "type": "string" } },
        "locked": {
        "type": "boolean"
        },
        "inherited_from": {
        "type": "string"
        }
        },
        "require_saml_auth_to_approve": {
        "value": {
        "type": "boolean"
        },
        "locked": {
        "type": "boolean"
        },
        "inherited_from": {
        "type": "string"
        }
        } }
        }, },
        "additionalProperties": false "additionalProperties": false
        ......
        { {
        "type": "object", "type": "object",
        "properties": { "properties": {
        "approvals_before_merge": { "type": "integer" }, "approvals_before_merge": {
        "disable_overriding_approvers_per_merge_request": { "type": ["boolean", "null"] }, "type": "integer"
        "reset_approvals_on_push": { "type": "boolean" }, },
        "selective_code_owner_removals": { "type": "boolean" }, "disable_overriding_approvers_per_merge_request": {
        "merge_requests_author_approval": { "type": ["boolean", "null"] }, "type": [
        "merge_requests_disable_committers_approval": { "type": ["boolean", "null"] }, "boolean",
        "require_password_to_approve": { "type": ["boolean", "null"] }, "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"
        ]
        },
        "approvers": { "approvers": {
        "type": "array", "type": "array",
        "items": { "items": {
        "type": "object", "type": "object",
        "properties": {} "properties": {
        }
        } }
        }, },
        "approver_groups": { "approver_groups": {
        "type": "array", "type": "array",
        "items": { "items": {
        "type": "object", "type": "object",
        "properties": {} "properties": {
        }
        } }
        } }
        }, },
        ......
        ...@@ -41,6 +41,7 @@ describe('ApprovalSettings', () => { ...@@ -41,6 +41,7 @@ describe('ApprovalSettings', () => {
        store = createStore({ approvalSettings: module, approvals: projectSettingsModule() }); store = createStore({ approvalSettings: module, approvals: projectSettingsModule() });
        store.state.settings.groupName = groupName; store.state.settings.groupName = groupName;
        store.state.settings.samlProviderEnabled = true;
        }; };
        const showToast = jest.fn(); const showToast = jest.fn();
        ......
        0% Loading or .
        You are about to add 0 people to the discussion. Proceed with caution.
        Finish editing this message first!
        Please register or to comment