Merge branch 'tancnle/219359-record-compliance-mr-approvals-settings' into 'master'

Add compliance merge request approval settings

See merge request gitlab-org/gitlab!33923

db/migrate/20200613104045_add_compliance_frameworks_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddComplianceFrameworksToApplicationSettings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :application_settings, :compliance_frameworks, :integer, limit: 2, array: true, default: [], null: false
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :application_settings, :compliance_frameworks
+    end
+  end
+end

db/structure.sql

@@ -478,6 +478,7 @@ CREATE TABLE public.application_settings (
     repository_storages_weighted jsonb DEFAULT '{}'::jsonb NOT NULL,
     max_import_size integer DEFAULT 50 NOT NULL,
     enforce_pat_expiration boolean DEFAULT true NOT NULL,
+    compliance_frameworks smallint[] DEFAULT '{}'::smallint[] NOT NULL,
     CONSTRAINT check_d03919528d CHECK ((char_length(container_registry_vendor) <= 255)),
     CONSTRAINT check_d820146492 CHECK ((char_length(spam_check_endpoint_url) <= 255)),
     CONSTRAINT check_e5aba18f02 CHECK ((char_length(container_registry_version) <= 255))
@@ -13994,6 +13995,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200609142507
 20200609142508
 20200609212701
+20200613104045
 20200615083635
 20200615121217
 20200615123055

ee/app/controllers/ee/admin/application_settings_controller.rb

@@ -5,12 +5,15 @@ module EE
     module ApplicationSettingsController
       extend ::Gitlab::Utils::Override
 
+      include ::Admin::MergeRequestApprovalSettingsHelper
+
       EE_VALID_SETTING_PANELS = %w(templates).freeze
 
       EE_VALID_SETTING_PANELS.each do |action|
         define_method(action) { perform_update if submitted? }
       end
 
+      # rubocop:disable Metrics/CyclomaticComplexity
       def visible_application_setting_attributes
         attrs = super
 
@@ -54,6 +57,10 @@ module EE
           attrs += EE::ApplicationSettingsHelper.merge_request_appovers_rules_attributes
         end
 
+        if show_compliance_merge_request_approval_settings?
+          attrs << { compliance_frameworks: [] }
+        end
+
         if License.feature_available?(:packages)
           attrs << :npm_package_requests_forwarding
         end
@@ -64,6 +71,7 @@ module EE
 
         attrs
       end
+      # rubocop:enable Metrics/CyclomaticComplexity
 
       def seat_link_payload
         data = ::Gitlab::SeatLinkData.new

ee/app/helpers/admin/merge_request_approval_settings_helper.rb

+# frozen_string_literal: true
+
+module Admin
+  module MergeRequestApprovalSettingsHelper
+    def show_compliance_merge_request_approval_settings?
+      Feature.enabled?(:admin_compliance_merge_request_approval_settings) &&
+        License.feature_available?(:admin_merge_request_approvers_rules)
+    end
+  end
+end

ee/app/helpers/compliance_management/compliance_framework/project_settings_helper.rb

@@ -5,7 +5,13 @@ module ComplianceManagement
     module ProjectSettingsHelper
       def compliance_framework_options
         option_values = compliance_framework_option_values
-        ProjectSettings.frameworks.map { |k, _v| [option_values.fetch(k.to_sym), k] }
+        ::ComplianceManagement::ComplianceFramework::FRAMEWORKS.map { |k, _v| [option_values.fetch(k), k] }
+      end
+
+      def compliance_framework_checkboxes
+        ::ComplianceManagement::ComplianceFramework::FRAMEWORKS.map do |k, v|
+          [v, compliance_framework_title_values.fetch(k)]
+        end
       end
 
       def compliance_framework_description(framework)

ee/app/models/compliance_management/compliance_framework.rb

+# frozen_string_literal: true
+
+module ComplianceManagement
+  module ComplianceFramework
+    FRAMEWORKS = {
+      gdpr: 1,    # General Data Protection Regulation
+      hipaa: 2,   # Health Insurance Portability and Accountability Act
+      pci_dss: 3, # Payment Card Industry-Data Security Standard
+      soc_2: 4,   # Service Organization Control 2
+      sox: 5      # Sarbanes-Oxley
+    }.freeze
+  end
+end

ee/app/models/compliance_management/compliance_framework/project_settings.rb

@@ -8,13 +8,7 @@ module ComplianceManagement
 
       belongs_to :project
 
-      enum framework: {
-        gdpr: 1,    # General Data Protection Regulation
-        hipaa: 2,   # Health Insurance Portability and Accountability Act
-        pci_dss: 3, # Payment Card Industry-Data Security Standard
-        soc_2: 4,   # Service Organization Control 2
-        sox: 5      # Sarbanes-Oxley
-      }
+      enum framework: ::ComplianceManagement::ComplianceFramework::FRAMEWORKS
 
       validates :project, presence: true
       validates :framework, uniqueness: { scope: [:project_id] }

ee/app/models/ee/application_setting.rb

@@ -84,6 +84,8 @@ module EE
                 allow_blank: true,
                 numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 365 }
 
+      validate :allowed_frameworks, if: :compliance_frameworks_changed?
+
       after_commit :update_personal_access_tokens_lifetime, if: :saved_change_to_max_personal_access_token_lifetime?
       after_commit :resume_elasticsearch_indexing
     end
@@ -297,6 +299,12 @@ module EE
       max_personal_access_token_lifetime&.days&.from_now
     end
 
+    def compliance_frameworks=(values)
+      cleaned = Array.wrap(values).sort.uniq
+
+      write_attribute(:compliance_frameworks, cleaned)
+    end
+
     private
 
     def elasticsearch_limited_project_exists?(project)
@@ -372,5 +380,11 @@ module EE
     rescue ::Gitlab::UrlBlocker::BlockedUrlError
       errors.add(:elasticsearch_url, "only supports valid HTTP(S) URLs.")
     end
+
+    def allowed_frameworks
+      if Array.wrap(compliance_frameworks).any? { |value| !::ComplianceManagement::ComplianceFramework::FRAMEWORKS.value?(value) }
+        errors.add(:compliance_frameworks, _('must contain only valid frameworks'))
+      end
+    end
   end
 end

ee/app/views/admin/push_rules/_merge_request_approvals.html.haml

@@ -6,26 +6,30 @@
     %button.btn.js-settings-toggle{ type: 'button' }
       = expanded_by_default? ? _('Collapse') : _('Expand')
     %p
-      = _('Settings to prevent self-approval across all projects in the instance. Only an administrator can modify these settings.')
+      - if show_compliance_merge_request_approval_settings?
+        = _('Regulate approvals by authors/committers, based on compliance frameworks. Can be changed only at the instance level.')
+      - else
+        = _('Settings to prevent self-approval across all projects in the instance. Only an administrator can modify these settings.')
+
   .settings-content
     %hr.clearfix.mt-0
 
     = form_for @application_setting, url: general_admin_application_settings_path(anchor: 'merge-request-approval-settings'), html: { class: 'fieldset-form' } do |f|
       = form_errors(@application_setting)
 
-      %fieldset
-        .form-group
-          .form-check
-            = f.check_box :prevent_merge_requests_author_approval, class: 'form-check-input'
-            = f.label :prevent_merge_requests_author_approval, class: 'form-check-label' do
-              = _('Prevent approval of merge requests by merge request author')
-          .form-check
-            = f.check_box :prevent_merge_requests_committers_approval, class: 'form-check-input'
-            = f.label :prevent_merge_requests_committers_approval, class: 'form-check-label' do
-              = _('Prevent approval of merge requests by merge request committers')
-          .form-check
-            = f.check_box :disable_overriding_approvers_per_merge_request , class: 'form-check-input'
-            = f.label :disable_overriding_approvers_per_merge_request , class: 'form-check-label' do
-              = _('Prevent users from modifying merge request approvers list')
+      = render 'merge_request_approvals_fields', f: f
+
+      - if show_compliance_merge_request_approval_settings?
+        .sub-section
+          %h5
+            = _('Compliance frameworks')
+          .form-group
+            .form-text.text-muted.mb-2
+              = _('The above settings apply to all projects with the selected compliance framework(s).')
+
+            = f.collection_check_boxes(:compliance_frameworks, compliance_framework_checkboxes, :first, :last, include_hidden: false) do |b|
+              .form-check
+                = b.check_box(class: "form-check-input")
+                = b.label(class: "form-check-label")
 
       = f.submit _('Save changes'), class: "btn btn-success"

ee/app/views/admin/push_rules/_merge_request_approvals_fields.html.haml

+%fieldset
+  .form-group
+    .form-check
+      = f.check_box :prevent_merge_requests_author_approval, class: 'form-check-input'
+      = f.label :prevent_merge_requests_author_approval, class: 'form-check-label' do
+        = _('Prevent approval of merge requests by merge request author')
+    .form-check
+      = f.check_box :prevent_merge_requests_committers_approval, class: 'form-check-input'
+      = f.label :prevent_merge_requests_committers_approval, class: 'form-check-label' do
+        = _('Prevent approval of merge requests by merge request committers')
+    .form-check
+      = f.check_box :disable_overriding_approvers_per_merge_request , class: 'form-check-input'
+      = f.label :disable_overriding_approvers_per_merge_request , class: 'form-check-label' do
+        = _('Prevent users from modifying merge request approvers list')

ee/changelogs/unreleased/219359-add-compliance-frameworks-to-application-settings.yml

+---
+title: Add compliance frameworks to application settings table
+merge_request: 33923
+author:
+type: added

ee/spec/controllers/admin/application_settings_controller_spec.rb

@@ -160,6 +160,33 @@ RSpec.describe Admin::ApplicationSettingsController do
       it_behaves_like 'settings for licensed features'
     end
 
+    context 'compliance frameworks' do
+      let(:settings) { { compliance_frameworks: [1, 2, 3, 4, 5] } }
+      let(:feature) { :admin_merge_request_approvers_rules }
+
+      context 'when feature flag is enabled' do
+        before do
+          stub_feature_flags(admin_compliance_merge_request_approval_settings: true)
+        end
+
+        it_behaves_like 'settings for licensed features'
+      end
+
+      context 'when feature flag is disabled' do
+        before do
+          stub_licensed_features(feature => true)
+          stub_feature_flags(admin_compliance_merge_request_approval_settings: false)
+        end
+
+        it 'does not update settings' do
+          attribute_names = settings.keys.map(&:to_s)
+
+          expect { put :update, params: { application_setting: settings } }
+            .not_to change { ApplicationSetting.current.reload.attributes.slice(*attribute_names) }
+        end
+      end
+    end
+
     context 'required instance ci template' do
       let(:settings) { { required_instance_ci_template: 'Auto-DevOps' } }
       let(:feature) { :required_ci_templates }

ee/spec/helpers/admin/merge_request_approval_settings_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Admin::MergeRequestApprovalSettingsHelper do
+  describe '#show_compliance_merge_request_approval_settings?' do
+    using RSpec::Parameterized::TableSyntax
+
+    subject { helper.show_compliance_merge_request_approval_settings? }
+
+    where(:feature_flag, :licensed, :result) do
+      true  | true  | true
+      true  | false | false
+      false | true  | false
+      false | false | false
+    end
+
+    with_them do
+      before do
+        stub_feature_flags(admin_compliance_merge_request_approval_settings: feature_flag)
+        stub_licensed_features(admin_merge_request_approvers_rules: licensed)
+      end
+
+      it { is_expected.to eq(result) }
+    end
+  end
+end

ee/spec/helpers/compliance_management/compliance_framework/project_settings_helper_spec.rb

@@ -6,11 +6,23 @@ RSpec.describe ComplianceManagement::ComplianceFramework::ProjectSettingsHelper
   describe '#compliance_framework_options' do
     it 'has all the options' do
       expect(helper.compliance_framework_options).to contain_exactly(
-        ['GDPR - General Data Protection Regulation', 'gdpr'],
-        ['HIPAA - Health Insurance Portability and Accountability Act', 'hipaa'],
-        ['PCI-DSS - Payment Card Industry-Data Security Standard', 'pci_dss'],
-        ['SOC 2 - Service Organization Control 2', 'soc_2'],
-        ['SOX - Sarbanes-Oxley', 'sox']
+        ['GDPR - General Data Protection Regulation', :gdpr],
+        ['HIPAA - Health Insurance Portability and Accountability Act', :hipaa],
+        ['PCI-DSS - Payment Card Industry-Data Security Standard', :pci_dss],
+        ['SOC 2 - Service Organization Control 2', :soc_2],
+        ['SOX - Sarbanes-Oxley', :sox]
+      )
+    end
+  end
+
+  describe '#compliance_framework_checkboxes' do
+    it 'has all the checkboxes' do
+      expect(helper.compliance_framework_checkboxes).to contain_exactly(
+        [1, 'GDPR'],
+        [2, 'HIPAA'],
+        [3, 'PCI-DSS'],
+        [4, 'SOC 2'],
+        [5, 'SOX']
       )
     end
   end

ee/spec/models/application_setting_spec.rb

@@ -129,6 +129,24 @@ RSpec.describe ApplicationSetting do
         end
       end
     end
+
+    context 'when validating compliance_frameworks' do
+      where(:compliance_frameworks, :is_valid) do
+        [1, 2, 3, 4, 5] | true
+        nil             | true
+        1               | true
+        [2, 3, 4, 6]    | false
+        6               | false
+      end
+
+      with_them do
+        specify do
+          setting.compliance_frameworks = compliance_frameworks
+
+          expect(setting.valid?).to eq(is_valid)
+        end
+      end
+    end
   end
 
   describe '#should_check_namespace_plan?' do
@@ -670,4 +688,18 @@ RSpec.describe ApplicationSetting do
       it { is_expected.to eq(result) }
     end
   end
+
+  describe '#compliance_frameworks' do
+    it 'sorts the list' do
+      setting.compliance_frameworks = [5, 4, 1, 3, 2]
+
+      expect(setting.compliance_frameworks).to eq([1, 2, 3, 4, 5])
+    end
+
+    it 'removes duplicates' do
+      setting.compliance_frameworks = [1, 2, 2, 3, 3, 3]
+
+      expect(setting.compliance_frameworks).to eq([1, 2, 3])
+    end
+  end
 end

ee/spec/views/admin/push_rules/_merge_request_approvals.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'admin/push_rules/_merge_request_approvals' do
+  let(:application_setting) { build(:application_setting) }
+
+  before do
+    assign(:application_setting, application_setting)
+
+    stub_licensed_features(admin_merge_request_approvers_rules: true)
+  end
+
+  it 'shows settings form' do
+    render
+
+    expect(rendered).to have_content('Merge requests approvals')
+  end
+
+  context 'when show compliance merge request approval settings' do
+    before do
+      allow(view).to receive(:show_compliance_merge_request_approval_settings?).and_return(true)
+    end
+
+    it 'shows compliance framework content', :aggregate_failures do
+      render
+
+      expect(rendered).to have_content('Regulate approvals by authors/committers')
+      expect(rendered).to have_content('Compliance frameworks')
+    end
+  end
+
+  context 'when not show compliance merge request approval settings' do
+    before do
+      allow(view).to receive(:show_compliance_merge_request_approval_settings?).and_return(false)
+    end
+
+    it 'shows non-compliance framework content', :aggregate_failures do
+      render
+
+      expect(rendered).to have_content('Settings to prevent self-approval across all projects')
+      expect(rendered).not_to have_content('Compliance frameworks')
+    end
+  end
+end

locale/gitlab.pot

@@ -5844,6 +5844,9 @@ msgstr ""
 msgid "Compliance framework (optional)"
 msgstr ""
 
+msgid "Compliance frameworks"
+msgstr ""
+
 msgid "ComplianceFramework|GDPR"
 msgstr ""
 
@@ -18474,6 +18477,9 @@ msgstr ""
 msgid "Registration|Your profile"
 msgstr ""
 
+msgid "Regulate approvals by authors/committers, based on compliance frameworks. Can be changed only at the instance level."
+msgstr ""
+
 msgid "Rejected (closed)"
 msgstr ""
 
@@ -22220,6 +22226,9 @@ msgstr ""
 msgid "The X509 Certificate to use when mutual TLS is required to communicate with the external authorization service. If left blank, the server certificate is still validated when accessing over HTTPS."
 msgstr ""
 
+msgid "The above settings apply to all projects with the selected compliance framework(s)."
+msgstr ""
+
 msgid "The amount of seconds after which a request to get a secondary node status will time out."
 msgstr ""
 
@@ -27386,6 +27395,9 @@ msgstr ""
 msgid "must be greater than start date"
 msgstr ""
 
+msgid "must contain only valid frameworks"
+msgstr ""
+
 msgid "my-awesome-group"
 msgstr ""