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 0000000000000000000000000000000000000000..be6f14692e7b911d1ea3c3a2e90dae3cffc7a302 --- /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 7cd30c15c332aae2d95344f5b1c87e92fd99134a..d6fce33622dd8568fecd2f640f30804594874980 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 186ec9445e39e9d81ced27b4e03d84bf16fe6bbf..f7bfafb0f7f9f07b8e197da396dce2344677b63e 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 0000000000000000000000000000000000000000..e3c6fdabd56967c6b7541e28f3a5838466f7da88 --- /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 7089d76ee344e2e6c559971da0979143ff9a79db..7609b6f8407847516079416f472b79a36ff7f483 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 0000000000000000000000000000000000000000..8dff8c9fb757bf0cb9af88daf381bd41b0afece1 --- /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 7599e5b62eba75dbacf7c88000155b23ddbd6da8..5a258276b0c30844e7c72e41899a53119583c32a 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 42295414dbeab945d75e655a2be32e99e508bd13..e20be786e1272689c5a364a07d29b26e8125d872 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 2c4a479b4439c6941c5481e2b097b67f1f58c659..cd4cf132f0d6d19fba77e86664664264ba33c41a 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 0000000000000000000000000000000000000000..7862a35a12ba86fbc28bce17ee6e2f45e402714d --- /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 0000000000000000000000000000000000000000..bd20583f1adf5dafebb7a4405060b8b501f2263b --- /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 1c84b4b0b9f3438b72f84661ded120b1e6652ceb..80917b594c2b33de03ac7c2d16515ace7bce96aa 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 0000000000000000000000000000000000000000..583c4b8e0f653e8ab9eba5ca8a80e0c92dae81ec --- /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 55e9fb5d70a8faa41d0155fd3014ba4ed7fc4a7f..8374c0a3162cd9db38e2f5b6c99e48b214c4dca4 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 8a9cb64a9f4235921968475420bc33c61ec0f12e..b2e3d98db477336a4f0f98e5136a2fe19d58e9f7 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 0000000000000000000000000000000000000000..c94817f2eca41b3f92fe4d2c3a5318d3565489cf --- /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 283e1a41bc8a1f1e14bcb2502025c99f4062ec74..4fa044cb0274cc94d4173179da3e88e3a01e3af4 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 ""