From 40d17ee0154baf071129646bc21f2c7f79d6ef55 Mon Sep 17 00:00:00 2001 From: Tan Le Date: Fri, 5 Jun 2020 09:49:02 +1000 Subject: [PATCH] Record compliance frameworks on MR approval This change allows administrator to scope instance-level MR approval settings to a selected number of compliance frameworks. This field is later used to determine the correct project-level MR approval settings for projects which have compliance label. --- ...ance_frameworks_to_application_settings.rb | 19 ++++++++ db/structure.sql | 2 + .../admin/application_settings_controller.rb | 8 ++++ .../merge_request_approval_settings_helper.rb | 10 +++++ .../project_settings_helper.rb | 8 +++- .../compliance_framework.rb | 13 ++++++ .../compliance_framework/project_settings.rb | 8 +--- ee/app/models/ee/application_setting.rb | 14 ++++++ .../_merge_request_approvals.html.haml | 34 +++++++------- .../_merge_request_approvals_fields.html.haml | 14 ++++++ ...nce-frameworks-to-application-settings.yml | 5 +++ .../application_settings_controller_spec.rb | 27 +++++++++++ ...e_request_approval_settings_helper_spec.rb | 27 +++++++++++ .../project_settings_helper_spec.rb | 22 ++++++--- ee/spec/models/application_setting_spec.rb | 32 +++++++++++++ ..._merge_request_approvals.html.haml_spec.rb | 45 +++++++++++++++++++ locale/gitlab.pot | 12 +++++ 17 files changed, 272 insertions(+), 28 deletions(-) create mode 100644 db/migrate/20200613104045_add_compliance_frameworks_to_application_settings.rb create mode 100644 ee/app/helpers/admin/merge_request_approval_settings_helper.rb create mode 100644 ee/app/models/compliance_management/compliance_framework.rb create mode 100644 ee/app/views/admin/push_rules/_merge_request_approvals_fields.html.haml create mode 100644 ee/changelogs/unreleased/219359-add-compliance-frameworks-to-application-settings.yml create mode 100644 ee/spec/helpers/admin/merge_request_approval_settings_helper_spec.rb create mode 100644 ee/spec/views/admin/push_rules/_merge_request_approvals.html.haml_spec.rb diff --git a/db/migrate/20200613104045_add_compliance_frameworks_to_application_settings.rb b/db/migrate/20200613104045_add_compliance_frameworks_to_application_settings.rb new file mode 100644 index 00000000000000..be6f14692e7b91 --- /dev/null +++ b/db/migrate/20200613104045_add_compliance_frameworks_to_application_settings.rb @@ -0,0 +1,19 @@ +# 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 diff --git a/db/structure.sql b/db/structure.sql index 7cd30c15c332aa..d6fce33622dd85 100644 --- a/db/structure.sql +++ b/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 diff --git a/ee/app/controllers/ee/admin/application_settings_controller.rb b/ee/app/controllers/ee/admin/application_settings_controller.rb index 186ec9445e39e9..f7bfafb0f7f9f0 100644 --- a/ee/app/controllers/ee/admin/application_settings_controller.rb +++ b/ee/app/controllers/ee/admin/application_settings_controller.rb @@ -5,12 +5,15 @@ module Admin 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 @@ def visible_application_setting_attributes 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 @@ def visible_application_setting_attributes attrs end + # rubocop:enable Metrics/CyclomaticComplexity def seat_link_payload data = ::Gitlab::SeatLinkData.new diff --git a/ee/app/helpers/admin/merge_request_approval_settings_helper.rb b/ee/app/helpers/admin/merge_request_approval_settings_helper.rb new file mode 100644 index 00000000000000..e3c6fdabd56967 --- /dev/null +++ b/ee/app/helpers/admin/merge_request_approval_settings_helper.rb @@ -0,0 +1,10 @@ +# 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 diff --git a/ee/app/helpers/compliance_management/compliance_framework/project_settings_helper.rb b/ee/app/helpers/compliance_management/compliance_framework/project_settings_helper.rb index 7089d76ee344e2..7609b6f8407847 100644 --- a/ee/app/helpers/compliance_management/compliance_framework/project_settings_helper.rb +++ b/ee/app/helpers/compliance_management/compliance_framework/project_settings_helper.rb @@ -5,7 +5,13 @@ module ComplianceFramework 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) diff --git a/ee/app/models/compliance_management/compliance_framework.rb b/ee/app/models/compliance_management/compliance_framework.rb new file mode 100644 index 00000000000000..8dff8c9fb757bf --- /dev/null +++ b/ee/app/models/compliance_management/compliance_framework.rb @@ -0,0 +1,13 @@ +# 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 diff --git a/ee/app/models/compliance_management/compliance_framework/project_settings.rb b/ee/app/models/compliance_management/compliance_framework/project_settings.rb index 7599e5b62eba75..5a258276b0c308 100644 --- a/ee/app/models/compliance_management/compliance_framework/project_settings.rb +++ b/ee/app/models/compliance_management/compliance_framework/project_settings.rb @@ -8,13 +8,7 @@ class ProjectSettings < ApplicationRecord 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] } diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index 42295414dbeab9..e20be786e12726 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -84,6 +84,8 @@ module ApplicationSetting 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 @@ def max_personal_access_token_lifetime_from_now 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 @@ def check_elasticsearch_url_scheme 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 diff --git a/ee/app/views/admin/push_rules/_merge_request_approvals.html.haml b/ee/app/views/admin/push_rules/_merge_request_approvals.html.haml index 2c4a479b4439c6..cd4cf132f0d6d1 100644 --- a/ee/app/views/admin/push_rules/_merge_request_approvals.html.haml +++ b/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" diff --git a/ee/app/views/admin/push_rules/_merge_request_approvals_fields.html.haml b/ee/app/views/admin/push_rules/_merge_request_approvals_fields.html.haml new file mode 100644 index 00000000000000..7862a35a12ba86 --- /dev/null +++ b/ee/app/views/admin/push_rules/_merge_request_approvals_fields.html.haml @@ -0,0 +1,14 @@ +%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') diff --git a/ee/changelogs/unreleased/219359-add-compliance-frameworks-to-application-settings.yml b/ee/changelogs/unreleased/219359-add-compliance-frameworks-to-application-settings.yml new file mode 100644 index 00000000000000..bd20583f1adf5d --- /dev/null +++ b/ee/changelogs/unreleased/219359-add-compliance-frameworks-to-application-settings.yml @@ -0,0 +1,5 @@ +--- +title: Add compliance frameworks to application settings table +merge_request: 33923 +author: +type: added diff --git a/ee/spec/controllers/admin/application_settings_controller_spec.rb b/ee/spec/controllers/admin/application_settings_controller_spec.rb index 1c84b4b0b9f343..80917b594c2b33 100644 --- a/ee/spec/controllers/admin/application_settings_controller_spec.rb +++ b/ee/spec/controllers/admin/application_settings_controller_spec.rb @@ -160,6 +160,33 @@ 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 } diff --git a/ee/spec/helpers/admin/merge_request_approval_settings_helper_spec.rb b/ee/spec/helpers/admin/merge_request_approval_settings_helper_spec.rb new file mode 100644 index 00000000000000..583c4b8e0f653e --- /dev/null +++ b/ee/spec/helpers/admin/merge_request_approval_settings_helper_spec.rb @@ -0,0 +1,27 @@ +# 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 diff --git a/ee/spec/helpers/compliance_management/compliance_framework/project_settings_helper_spec.rb b/ee/spec/helpers/compliance_management/compliance_framework/project_settings_helper_spec.rb index 55e9fb5d70a8fa..8374c0a3162cd9 100644 --- a/ee/spec/helpers/compliance_management/compliance_framework/project_settings_helper_spec.rb +++ b/ee/spec/helpers/compliance_management/compliance_framework/project_settings_helper_spec.rb @@ -6,11 +6,23 @@ 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 diff --git a/ee/spec/models/application_setting_spec.rb b/ee/spec/models/application_setting_spec.rb index 8a9cb64a9f4235..b2e3d98db47733 100644 --- a/ee/spec/models/application_setting_spec.rb +++ b/ee/spec/models/application_setting_spec.rb @@ -129,6 +129,24 @@ 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 @@ def expect_is_es_licensed 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 diff --git a/ee/spec/views/admin/push_rules/_merge_request_approvals.html.haml_spec.rb b/ee/spec/views/admin/push_rules/_merge_request_approvals.html.haml_spec.rb new file mode 100644 index 00000000000000..c94817f2eca41b --- /dev/null +++ b/ee/spec/views/admin/push_rules/_merge_request_approvals.html.haml_spec.rb @@ -0,0 +1,45 @@ +# 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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 283e1a41bc8a1f..4fa044cb0274cc 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5841,6 +5841,9 @@ msgstr "" msgid "Compliance framework (optional)" msgstr "" +msgid "Compliance frameworks" +msgstr "" + msgid "ComplianceFramework|GDPR" msgstr "" @@ -18465,6 +18468,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 "" @@ -22211,6 +22217,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 "" @@ -27377,6 +27386,9 @@ msgstr "" msgid "must be greater than start date" msgstr "" +msgid "must contain only valid frameworks" +msgstr "" + msgid "my-awesome-group" msgstr "" -- GitLab