From b22068b7a8dcdd39b4a13eed395abff0d2746749 Mon Sep 17 00:00:00 2001 From: Raimund Hook Date: Fri, 23 Sep 2022 16:24:23 +0100 Subject: [PATCH 1/9] Add Push Rule for DCO Signoff Adds Push rule to enforce DCO signoff at project level. Resolves https://gitlab.com/gitlab-org/gitlab/-/issues/368894 Signed-off-by: Raimund Hook Changelog: added EE: true --- doc/user/project/repository/push_rules.md | 3 +++ .../admin/push_rules_controller.rb | 5 ++++ .../groups/push_rules_controller.rb | 4 +++ .../projects/push_rules_controller.rb | 4 +++ ee/app/helpers/push_rules_helper.rb | 8 +++++- .../models/gitlab_subscriptions/features.rb | 1 + ee/app/models/push_rule.rb | 21 +++++++++++++++ ee/app/policies/ee/group_policy.rb | 10 +++++++ ee/app/policies/ee/project_policy.rb | 11 ++++++++ .../views/shared/push_rules/_form.html.haml | 1 + ...reject_commits_not_dco_signed_setting.haml | 11 ++++++++ .../gitlab/checks/push_rules/commit_check.rb | 4 +++ .../checks/push_rules/commit_check_spec.rb | 26 +++++++++++++++++++ 13 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 ee/app/views/shared/push_rules/_reject_commits_not_dco_signed_setting.haml diff --git a/doc/user/project/repository/push_rules.md b/doc/user/project/repository/push_rules.md index 244cb1245f0318..f5e22b37e7b12c 100644 --- a/doc/user/project/repository/push_rules.md +++ b/doc/user/project/repository/push_rules.md @@ -77,6 +77,9 @@ Use these rules for your commit messages. - **Reject expression in commit messages**: Commit messages must not match the expression. To allow any commit message, leave empty. Uses multiline mode, which can be disabled by using `(?-m)`. +- **Reject commits that aren't DCO certified**: Commit messages must contain + a `Signed-off-by:` trailer certifying the contributor's acceptance of the + [Developer Certificate of Origin (DCO)](https://developercertificate.org/). ## Validate branch names diff --git a/ee/app/controllers/admin/push_rules_controller.rb b/ee/app/controllers/admin/push_rules_controller.rb index e2efd0042fc70e..e110d59f65f934 100644 --- a/ee/app/controllers/admin/push_rules_controller.rb +++ b/ee/app/controllers/admin/push_rules_controller.rb @@ -42,6 +42,10 @@ def push_rule_params allowed_fields << :commit_committer_check end + if @push_rule.available?(:reject_non_dco_commits) + allowed_fields << :reject_non_dco_commits + end + params.require(:push_rule).permit(allowed_fields) end @@ -56,6 +60,7 @@ def set_application_setting end def link_push_rule_to_application_settings + binding.pry_shell return if @application_setting.push_rule_id @application_setting.update(push_rule_id: @push_rule.id) diff --git a/ee/app/controllers/groups/push_rules_controller.rb b/ee/app/controllers/groups/push_rules_controller.rb index ce005af7f9108a..98f222c2cca955 100644 --- a/ee/app/controllers/groups/push_rules_controller.rb +++ b/ee/app/controllers/groups/push_rules_controller.rb @@ -42,6 +42,10 @@ def push_rule_params allowed_fields << :commit_committer_check end + if can?(current_user, :change_reject_non_dco_commits, group) + allowed_fields << :reject_non_dco_commits + end + params.require(:push_rule).permit(allowed_fields) end diff --git a/ee/app/controllers/projects/push_rules_controller.rb b/ee/app/controllers/projects/push_rules_controller.rb index 900dab706a0776..22768716c240a3 100644 --- a/ee/app/controllers/projects/push_rules_controller.rb +++ b/ee/app/controllers/projects/push_rules_controller.rb @@ -45,6 +45,10 @@ def push_rule_params allowed_fields << :commit_committer_check end + if can?(current_user, :change_reject_non_dco_commits, project) + allowed_fields << :reject_non_dco_commits + end + params.require(:push_rule).permit(allowed_fields) end end diff --git a/ee/app/helpers/push_rules_helper.rb b/ee/app/helpers/push_rules_helper.rb index b645d93e200779..d5bc939f636f5e 100644 --- a/ee/app/helpers/push_rules_helper.rb +++ b/ee/app/helpers/push_rules_helper.rb @@ -11,6 +11,12 @@ def reject_unsigned_commits_description(push_rule) push_rule_update_description(message, push_rule, :reject_unsigned_commits) end + def reject_non_dco_commits_description(push_rule) + message = s_("ProjectSettings|Only commits that include a %{code_block_start}Signed-off-by:%{code_block_end} element can be pushed to this repository.").html_safe % { code_block_start: ''.html_safe, code_block_end: ''.html_safe } + + push_rule_update_description(message, push_rule, :reject_non_dco_commits) + end + def commit_committer_check_description(push_rule) message = s_("ProjectSettings|Users can only push commits to this repository "\ "if the committer email is one of their own verified emails.") @@ -39,6 +45,6 @@ def push_rule_update_description(message, push_rule, rule) end end - messages.join(' ') + messages.join(' ').html_safe end end diff --git a/ee/app/models/gitlab_subscriptions/features.rb b/ee/app/models/gitlab_subscriptions/features.rb index 5fb6f3c15a6ad9..9cb4c90e21d11c 100644 --- a/ee/app/models/gitlab_subscriptions/features.rb +++ b/ee/app/models/gitlab_subscriptions/features.rb @@ -143,6 +143,7 @@ class Features productivity_analytics project_aliases protected_environments + reject_non_dco_commits reject_unsigned_commits saml_group_sync scoped_labels diff --git a/ee/app/models/push_rule.rb b/ee/app/models/push_rule.rb index ff3b8758738fc8..9b0873fbd1c2b6 100644 --- a/ee/app/models/push_rule.rb +++ b/ee/app/models/push_rule.rb @@ -22,6 +22,7 @@ class PushRule < ApplicationRecord AUDIT_LOG_ALLOWLIST = { commit_committer_check: 'reject unverified users', reject_unsigned_commits: 'reject unsigned commits', + reject_non_dco_commits: 'reject non-dco commits', deny_delete_tag: 'do not allow users to remove Git tags with git push', member_check: 'check whether the commit author is a GitLab user', prevent_secrets: 'prevent pushing secret files', @@ -45,7 +46,10 @@ class PushRule < ApplicationRecord SETTINGS_WITH_GLOBAL_DEFAULT = %i[ reject_unsigned_commits commit_committer_check + reject_non_dco_commits ].freeze + + DCO_COMMIT_REGEX = 'Signed-off-by:.+<.+@.+>'.freeze def self.global find_by(is_sample: true) @@ -57,6 +61,7 @@ def commit_validation? branch_name_regex.present? || author_email_regex.present? || reject_unsigned_commits || + reject_non_dco_commits || commit_committer_check || member_check || file_name_regex.present? || @@ -77,6 +82,13 @@ def committer_allowed?(committer_email, current_user) current_user.verified_email?(committer_email) end + def non_dco_commit_allowed?(commit) + return true unless available?(:reject_non_dco_commits) + return true unless reject_non_dco_commits + + data_match?(commit.safe_message, DCO_COMMIT_REGEX, multiline: true) + end + def commit_message_allowed?(message) data_match?(message, commit_message_regex, multiline: true) end @@ -132,6 +144,15 @@ def commit_committer_check=(value) write_setting_with_global_default(:commit_committer_check, value) end + def reject_non_dco_commits + read_setting_with_global_default(:reject_non_dco_commits) + end + alias_method :reject_non_dco_commits?, :reject_non_dco_commits + + def reject_non_dco_commits=(value) + write_setting_with_global_default(:reject_non_dco_commits, value) + end + private def data_match?(data, regex, multiline: false) diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index 99cd934d4d62bc..38bae7d9c3535f 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -132,6 +132,10 @@ module GroupPolicy @subject.feature_available?(:reject_unsigned_commits) end + condition(:reject_non_dco_commits_available, scope: :subject) do + @subject.feature_available?(:reject_non_dco_commits) + end + condition(:push_rules_available, scope: :subject) do @subject.feature_available?(:push_rules) end @@ -448,6 +452,12 @@ module GroupPolicy rule { ~reject_unsigned_commits_available }.prevent :change_reject_unsigned_commits + rule { admin | maintainer }.enable :change_reject_non_dco_commits + + rule { reject_non_dco_commits_available }.enable :read_reject_non_dco_commits + + rule { ~reject_non_dco_commits_available }.prevent :change_reject_non_dco_commits + rule { can?(:maintainer_access) & push_rules_available }.enable :change_push_rules rule { admin & is_gitlab_com }.enable :update_subscription_limit diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 098a1a523f56f7..5607224348290b 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -77,6 +77,11 @@ module ProjectPolicy @subject.feature_available?(:reject_unsigned_commits) end + with_scope :subject + condition(:reject_non_dco_commits_available) do + @subject.feature_available?(:reject_non_dco_commits) + end + with_scope :subject condition(:security_orchestration_policies_enabled) do @subject.feature_available?(:security_orchestration_policies) @@ -370,6 +375,12 @@ module ProjectPolicy rule { ~commit_committer_check_available }.prevent :change_commit_committer_check + rule { admin | maintainer }.enable :change_reject_non_dco_commits + + rule { reject_non_dco_commits_available }.enable :read_reject_non_dco_commits + + rule { ~reject_non_dco_commits_available }.prevent :change_reject_non_dco_commits + rule { owner | reporter | internal_access | public_project }.enable :build_read_project rule { ~admin & owner & owner_cannot_destroy_project }.prevent :remove_project diff --git a/ee/app/views/shared/push_rules/_form.html.haml b/ee/app/views/shared/push_rules/_form.html.haml index b5cdebdf3a3a84..fc61833a2b73c4 100644 --- a/ee/app/views/shared/push_rules/_form.html.haml +++ b/ee/app/views/shared/push_rules/_form.html.haml @@ -1,5 +1,6 @@ = render 'shared/push_rules/commit_committer_check_setting', form: f, push_rule: f.object, context: context = render 'shared/push_rules/reject_unsigned_commits_setting', form: f, push_rule: f.object, context: context += render 'shared/push_rules/reject_commits_not_dco_signed_setting', form: f, push_rule: f.object, context: context - wiki_syntax_link_url = 'https://github.com/google/re2/wiki/Syntax' - wiki_syntax_link_start = ''.html_safe % { url: wiki_syntax_link_url } diff --git a/ee/app/views/shared/push_rules/_reject_commits_not_dco_signed_setting.haml b/ee/app/views/shared/push_rules/_reject_commits_not_dco_signed_setting.haml new file mode 100644 index 00000000000000..cb7595dd74727c --- /dev/null +++ b/ee/app/views/shared/push_rules/_reject_commits_not_dco_signed_setting.haml @@ -0,0 +1,11 @@ +- context = local_assigns.fetch(:context) + +- return unless push_rule.available?(:reject_non_dco_commits, object: context) + +- form = local_assigns.fetch(:form) +- push_rule = local_assigns.fetch(:push_rule) + += form.gitlab_ui_checkbox_component :reject_non_dco_commits, + s_("PushRules|Reject commits that aren't DCO certified"), + checkbox_options: { data: { qa_selector: 'reject_non_dco_commits_checkbox' }, disabled: !can_change_push_rule?(form.object, :reject_non_dco_commits, context) }, + help_text: reject_non_dco_commits_description(push_rule) diff --git a/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb b/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb index ccfad101662741..070f68849e6ebe 100644 --- a/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb @@ -60,6 +60,10 @@ def check_commit(commit) committer_error_message = committer_check(commit) return committer_error_message if committer_error_message + if !push_rule.non_dco_commit_allowed?(commit) + return "Commit must contain DCO Sign-off" + end + if !updated_from_web? && !push_rule.commit_signature_allowed?(commit) return "Commit must be signed with a GPG key" end diff --git a/ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb b/ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb index 564c9e6f931a3c..956bd386c8abd0 100644 --- a/ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb +++ b/ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb @@ -38,6 +38,32 @@ end end + context 'DCO check rules' do + let(:push_rule) { create(:push_rule, reject_non_dco_commits: true) } + + before do + stub_licensed_features(reject_non_dco_commits: true) + end + + it_behaves_like 'check ignored when push rule unlicensed' + + context 'when enabled in Project and commit is not DCO signed' do + it 'returns an error' do + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "Commit must contain DCO Sign-off") + end + end + + context 'when enabled in Project and the commit is DCO signed' do + it 'does not return an error' do + commit_message = "DCO Signed Commit\n\nSigned-off-by: Test user " + + allow_any_instance_of(Commit).to receive(:safe_message).and_return(commit_message) + + expect { subject.validate! }.not_to raise_error + end + end + end + context 'author email rules' do let!(:push_rule) { create(:push_rule, author_email_regex: '.*@valid.com') } -- GitLab From 9f3251265d55e8d1a4186337daf0c58e8162b9a5 Mon Sep 17 00:00:00 2001 From: Raimund Hook Date: Fri, 23 Sep 2022 19:45:54 +0100 Subject: [PATCH 2/9] Fix Linting issues Signed-off-by: Raimund Hook --- doc/user/project/repository/push_rules.md | 2 +- ee/app/helpers/push_rules_helper.rb | 2 +- ee/app/models/push_rule.rb | 4 ++-- ee/lib/ee/gitlab/checks/push_rules/commit_check.rb | 6 ++---- .../lib/ee/gitlab/checks/push_rules/commit_check_spec.rb | 4 ++-- locale/gitlab.pot | 6 ++++++ 6 files changed, 14 insertions(+), 10 deletions(-) diff --git a/doc/user/project/repository/push_rules.md b/doc/user/project/repository/push_rules.md index f5e22b37e7b12c..0218aed4a7192e 100644 --- a/doc/user/project/repository/push_rules.md +++ b/doc/user/project/repository/push_rules.md @@ -78,7 +78,7 @@ Use these rules for your commit messages. the expression. To allow any commit message, leave empty. Uses multiline mode, which can be disabled by using `(?-m)`. - **Reject commits that aren't DCO certified**: Commit messages must contain - a `Signed-off-by:` trailer certifying the contributor's acceptance of the + a `Signed-off-by:` trailer certifying the contributor's acceptance of the [Developer Certificate of Origin (DCO)](https://developercertificate.org/). ## Validate branch names diff --git a/ee/app/helpers/push_rules_helper.rb b/ee/app/helpers/push_rules_helper.rb index d5bc939f636f5e..4c3723ed8a1ddb 100644 --- a/ee/app/helpers/push_rules_helper.rb +++ b/ee/app/helpers/push_rules_helper.rb @@ -12,7 +12,7 @@ def reject_unsigned_commits_description(push_rule) end def reject_non_dco_commits_description(push_rule) - message = s_("ProjectSettings|Only commits that include a %{code_block_start}Signed-off-by:%{code_block_end} element can be pushed to this repository.").html_safe % { code_block_start: ''.html_safe, code_block_end: ''.html_safe } + message = format(s_("ProjectSettings|Only commits that include a %{code_block_start}Signed-off-by:%{code_block_end} element can be pushed to this repository.").html_safe, code_block_start: ''.html_safe, code_block_end: ''.html_safe) push_rule_update_description(message, push_rule, :reject_non_dco_commits) end diff --git a/ee/app/models/push_rule.rb b/ee/app/models/push_rule.rb index 9b0873fbd1c2b6..dfc4e678aeb652 100644 --- a/ee/app/models/push_rule.rb +++ b/ee/app/models/push_rule.rb @@ -48,8 +48,8 @@ class PushRule < ApplicationRecord commit_committer_check reject_non_dco_commits ].freeze - - DCO_COMMIT_REGEX = 'Signed-off-by:.+<.+@.+>'.freeze + + DCO_COMMIT_REGEX = 'Signed-off-by:.+<.+@.+>' def self.global find_by(is_sample: true) diff --git a/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb b/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb index 070f68849e6ebe..2a4666c734329d 100644 --- a/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb @@ -60,10 +60,8 @@ def check_commit(commit) committer_error_message = committer_check(commit) return committer_error_message if committer_error_message - if !push_rule.non_dco_commit_allowed?(commit) - return "Commit must contain DCO Sign-off" - end - + return "Commit must contain DCO Sign-off" unless push_rule.non_dco_commit_allowed?(commit) + if !updated_from_web? && !push_rule.commit_signature_allowed?(commit) return "Commit must be signed with a GPG key" end diff --git a/ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb b/ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb index 956bd386c8abd0..5bca809b28b819 100644 --- a/ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb +++ b/ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb @@ -44,11 +44,11 @@ before do stub_licensed_features(reject_non_dco_commits: true) end - + it_behaves_like 'check ignored when push rule unlicensed' context 'when enabled in Project and commit is not DCO signed' do - it 'returns an error' do + it 'returns an error' do expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "Commit must contain DCO Sign-off") end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d912e4536b5b92..e2b940939646ed 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -31677,6 +31677,9 @@ msgstr "" msgid "ProjectSettings|Note: The container registry is always visible when a project is public and the container registry is set to '%{access_level_description}'" msgstr "" +msgid "ProjectSettings|Only commits that include a %{code_block_start}Signed-off-by:%{code_block_end} element can be pushed to this repository." +msgstr "" + msgid "ProjectSettings|Only signed commits can be pushed to this repository." msgstr "" @@ -32808,6 +32811,9 @@ msgstr "" msgid "PushRules|Reject any files likely to contain secrets. %{secret_files_link_start}What secret files are rejected?%{secret_files_link_end}" msgstr "" +msgid "PushRules|Reject commits that aren't DCO certified" +msgstr "" + msgid "PushRules|Reject expression in commit messages" msgstr "" -- GitLab From f03047aabecc8ce0f3df2d0ece7d79c669853e3f Mon Sep 17 00:00:00 2001 From: Raimund Hook Date: Thu, 29 Sep 2022 17:09:05 +0100 Subject: [PATCH 3/9] Handle new push rule inside merge request commit Signed-off-by: Raimund Hook --- ee/app/controllers/admin/push_rules_controller.rb | 1 - ee/app/models/push_rule.rb | 4 ++-- ee/app/services/ee/merge_requests/merge_base_service.rb | 2 ++ ee/lib/ee/gitlab/checks/push_rules/commit_check.rb | 2 +- ee/spec/helpers/push_rules_helper_spec.rb | 1 + 5 files changed, 6 insertions(+), 4 deletions(-) diff --git a/ee/app/controllers/admin/push_rules_controller.rb b/ee/app/controllers/admin/push_rules_controller.rb index e110d59f65f934..03c8e3084f6b72 100644 --- a/ee/app/controllers/admin/push_rules_controller.rb +++ b/ee/app/controllers/admin/push_rules_controller.rb @@ -60,7 +60,6 @@ def set_application_setting end def link_push_rule_to_application_settings - binding.pry_shell return if @application_setting.push_rule_id @application_setting.update(push_rule_id: @push_rule.id) diff --git a/ee/app/models/push_rule.rb b/ee/app/models/push_rule.rb index dfc4e678aeb652..281b70f79b54a3 100644 --- a/ee/app/models/push_rule.rb +++ b/ee/app/models/push_rule.rb @@ -82,11 +82,11 @@ def committer_allowed?(committer_email, current_user) current_user.verified_email?(committer_email) end - def non_dco_commit_allowed?(commit) + def non_dco_commit_allowed?(message) return true unless available?(:reject_non_dco_commits) return true unless reject_non_dco_commits - data_match?(commit.safe_message, DCO_COMMIT_REGEX, multiline: true) + data_match?(message, DCO_COMMIT_REGEX, multiline: true) end def commit_message_allowed?(message) diff --git a/ee/app/services/ee/merge_requests/merge_base_service.rb b/ee/app/services/ee/merge_requests/merge_base_service.rb index 96921c5aeffaf0..1675d5988f6580 100644 --- a/ee/app/services/ee/merge_requests/merge_base_service.rb +++ b/ee/app/services/ee/merge_requests/merge_base_service.rb @@ -48,6 +48,8 @@ def hooks_validation_error(merge_request, validate_squash_message: false) "Author's commit email '#{current_user.commit_email_or_default}' does not follow the pattern '#{push_rule.author_email_regex}'" elsif validate_squash_message squash_message_validation_error + elsif !push_rule.non_dco_commit_allowed?(params[:commit_message]) + "Commit message must contain a DCO signoff" end end diff --git a/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb b/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb index 2a4666c734329d..de79a7863da578 100644 --- a/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb @@ -60,7 +60,7 @@ def check_commit(commit) committer_error_message = committer_check(commit) return committer_error_message if committer_error_message - return "Commit must contain DCO Sign-off" unless push_rule.non_dco_commit_allowed?(commit) + return "Commit must contain DCO Sign-off" unless push_rule.non_dco_commit_allowed?(commit.safe_message) if !updated_from_web? && !push_rule.commit_signature_allowed?(commit) return "Commit must be signed with a GPG key" diff --git a/ee/spec/helpers/push_rules_helper_spec.rb b/ee/spec/helpers/push_rules_helper_spec.rb index 5991a531e992be..bfb60445400699 100644 --- a/ee/spec/helpers/push_rules_helper_spec.rb +++ b/ee/spec/helpers/push_rules_helper_spec.rb @@ -11,6 +11,7 @@ { commit_committer_check_base_help: /Users can only push commits to this repository if the committer email is one of their own verified emails/, reject_unsigned_commits_base_help: /Only signed commits can be pushed to this repository/, + reject_non_dco_commits_base_help: %r{Only commits that include a Signed-off-by: element can be pushed to this repository}, default_admin_help: /This setting will be applied to all projects unless overridden by an admin/, setting_can_be_overridden: /This setting is applied on the server level and can be overridden by an admin/, setting_has_been_overridden: /This setting is applied on the server level but has been overridden for this project/, -- GitLab From 2bd9d86524743a3ef5ede8dcb5201518b8aa7a5f Mon Sep 17 00:00:00 2001 From: Raimund Hook Date: Mon, 3 Oct 2022 15:31:54 +0100 Subject: [PATCH 4/9] Adding tests for undercoverage Signed-off-by: Raimund Hook --- .../admin/push_rules_controller_spec.rb | 4 ++- .../merge_merge_requests_shared_examples.rb | 32 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/ee/spec/controllers/admin/push_rules_controller_spec.rb b/ee/spec/controllers/admin/push_rules_controller_spec.rb index 72155f5910da93..6ba6c3e28e53dc 100644 --- a/ee/spec/controllers/admin/push_rules_controller_spec.rb +++ b/ee/spec/controllers/admin/push_rules_controller_spec.rb @@ -16,12 +16,14 @@ { deny_delete_tag: "true", delete_branch_regex: "any", commit_message_regex: "any", branch_name_regex: "any", force_push_regex: "any", author_email_regex: "any", member_check: "true", file_name_regex: "any", - max_file_size: "0", prevent_secrets: "true" + max_file_size: "0", prevent_secrets: "true", commit_committer_check: "true", reject_unsigned_commits: "true", + reject_non_dco_commits: "true" } end before do stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + stub_licensed_features(commit_committer_check: true, reject_unsigned_commits: true, reject_non_dco_commits: true) end it 'updates sample push rule' do diff --git a/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb b/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb index 8fd331ec83f760..7902c10ef90db5 100644 --- a/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb +++ b/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb @@ -87,6 +87,38 @@ def hooks_pass?(squashing: false) end end + context 'DCO signoff validation' do + it_behaves_like 'hook validations are skipped when push rules unlicensed' + + before do + stub_licensed_features(reject_non_dco_commits: true) + allow(project).to receive(:push_rule) { build(:push_rule, reject_non_dco_commits: true) } + end + + context 'when a non DCO commit message is used' do + it 'returns false and saves error when invalid' do + expect(hooks_pass?).to be(false) + expect(hooks_error).not_to be_empty + + if args[:persisted] + expect(merge_request.merge_error).to eq(hooks_error) + else + expect(merge_request.merge_error).to be_nil + end + end + end + + context 'when a DCO compliant commit message is used' do + let(:dco_commit_message) { 'DCO Signed Commit\n\nSigned-off-by: Test user ' } + let(:params) { super().merge(commit_message: dco_commit_message) } + + it 'accepts the commit message' do + expect(hooks_pass?).to be(true) + expect(hooks_error).to be_nil + end + end + end + context 'fast forward merge request' do it 'returns true when fast forward is enabled' do allow(project).to receive(:merge_requests_ff_only_enabled) { true } -- GitLab From 3ced33cbd368444a7bb0c6db364bfcf410319f05 Mon Sep 17 00:00:00 2001 From: Raimund Hook Date: Mon, 3 Oct 2022 14:44:05 +0000 Subject: [PATCH 5/9] Standardise the error messages Signed-off-by: Raimund Hook --- ee/lib/ee/gitlab/checks/push_rules/commit_check.rb | 4 +++- ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb b/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb index de79a7863da578..5edc6afdad1f9a 100644 --- a/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb @@ -60,7 +60,9 @@ def check_commit(commit) committer_error_message = committer_check(commit) return committer_error_message if committer_error_message - return "Commit must contain DCO Sign-off" unless push_rule.non_dco_commit_allowed?(commit.safe_message) + unless push_rule.non_dco_commit_allowed?(commit.safe_message) + return "Commit message must contain a DCO signoff" + end if !updated_from_web? && !push_rule.commit_signature_allowed?(commit) return "Commit must be signed with a GPG key" diff --git a/ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb b/ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb index 5bca809b28b819..16428e8407802a 100644 --- a/ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb +++ b/ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb @@ -49,7 +49,7 @@ context 'when enabled in Project and commit is not DCO signed' do it 'returns an error' do - expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "Commit must contain DCO Sign-off") + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "Commit message must contain a DCO signoff") end end -- GitLab From fd578ba715cbacc54daf61a88ba6d0cc61f18cfe Mon Sep 17 00:00:00 2001 From: Raimund Hook Date: Tue, 4 Oct 2022 14:40:30 +0100 Subject: [PATCH 6/9] Updated doc entry Signed-off-by: Raimund Hook --- doc/user/project/repository/push_rules.md | 11 ++++++++--- .../services/merge_merge_requests_shared_examples.rb | 8 ++++---- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/doc/user/project/repository/push_rules.md b/doc/user/project/repository/push_rules.md index 0218aed4a7192e..3c664d7ee5fded 100644 --- a/doc/user/project/repository/push_rules.md +++ b/doc/user/project/repository/push_rules.md @@ -77,9 +77,14 @@ Use these rules for your commit messages. - **Reject expression in commit messages**: Commit messages must not match the expression. To allow any commit message, leave empty. Uses multiline mode, which can be disabled by using `(?-m)`. -- **Reject commits that aren't DCO certified**: Commit messages must contain - a `Signed-off-by:` trailer certifying the contributor's acceptance of the - [Developer Certificate of Origin (DCO)](https://developercertificate.org/). + +## Reject commits that aren't DCO certified + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98810) in GitLab 15.5 + +Some projects require the committer to attest to the [Developer Certificate of Origin (DCO)](https://developercertificate.org/) +when committing. This is typically done by adding a `Signed-off-by:` trailer to the commit message certifying the committer's +acceptance. Enabling this rule will reject any commits that do not contain a `Signed-off-by:` trailer in a commit message. ## Validate branch names diff --git a/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb b/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb index 7902c10ef90db5..c3638402f92933 100644 --- a/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb +++ b/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb @@ -88,15 +88,15 @@ def hooks_pass?(squashing: false) end context 'DCO signoff validation' do - it_behaves_like 'hook validations are skipped when push rules unlicensed' - before do stub_licensed_features(reject_non_dco_commits: true) allow(project).to receive(:push_rule) { build(:push_rule, reject_non_dco_commits: true) } end - + + it_behaves_like 'hook validations are skipped when push rules unlicensed' + context 'when a non DCO commit message is used' do - it 'returns false and saves error when invalid' do + it 'returns false and saves error when invalid' do expect(hooks_pass?).to be(false) expect(hooks_error).not_to be_empty -- GitLab From b6b6aabdc09ef65a56702f51f1f9d1ad419b4453 Mon Sep 17 00:00:00 2001 From: Amy Qualls Date: Wed, 5 Oct 2022 14:00:56 +0000 Subject: [PATCH 7/9] Apply 1 suggestion(s) to 1 file(s) --- doc/user/project/repository/push_rules.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/project/repository/push_rules.md b/doc/user/project/repository/push_rules.md index 3c664d7ee5fded..9a4bd9ab291581 100644 --- a/doc/user/project/repository/push_rules.md +++ b/doc/user/project/repository/push_rules.md @@ -80,7 +80,7 @@ Use these rules for your commit messages. ## Reject commits that aren't DCO certified -> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98810) in GitLab 15.5 +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98810) in GitLab 15.5. Some projects require the committer to attest to the [Developer Certificate of Origin (DCO)](https://developercertificate.org/) when committing. This is typically done by adding a `Signed-off-by:` trailer to the commit message certifying the committer's -- GitLab From 80dc23026583279439cee428beafc0a9503eabf7 Mon Sep 17 00:00:00 2001 From: Amy Qualls Date: Thu, 6 Oct 2022 10:08:44 +0000 Subject: [PATCH 8/9] Apply 1 suggestion(s) to 1 file(s) --- doc/user/project/repository/push_rules.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/user/project/repository/push_rules.md b/doc/user/project/repository/push_rules.md index 9a4bd9ab291581..9e4fa7abdea06b 100644 --- a/doc/user/project/repository/push_rules.md +++ b/doc/user/project/repository/push_rules.md @@ -82,9 +82,10 @@ Use these rules for your commit messages. > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98810) in GitLab 15.5. -Some projects require the committer to attest to the [Developer Certificate of Origin (DCO)](https://developercertificate.org/) -when committing. This is typically done by adding a `Signed-off-by:` trailer to the commit message certifying the committer's -acceptance. Enabling this rule will reject any commits that do not contain a `Signed-off-by:` trailer in a commit message. +Commits signed with the [Developer Certificate of Origin](https://developercertificate.org/) (DCO) +certify the contributor wrote, or has the right to submit, the code contributed in that commit. +You can require all commits to your project to comply with the DCO. This push rule requires a +`Signed-off-by:` trailer in every commit message, and rejects any commits that lack it. ## Validate branch names -- GitLab From e64e63d16e434b0d2bd3633d16d7ad3bc7727843 Mon Sep 17 00:00:00 2001 From: Raimund Hook Date: Thu, 6 Oct 2022 17:05:54 +0100 Subject: [PATCH 9/9] Removed multiline from data_match Signed-off-by: Raimund Hook --- ee/app/models/push_rule.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/push_rule.rb b/ee/app/models/push_rule.rb index 281b70f79b54a3..718cb5749c9379 100644 --- a/ee/app/models/push_rule.rb +++ b/ee/app/models/push_rule.rb @@ -86,7 +86,7 @@ def non_dco_commit_allowed?(message) return true unless available?(:reject_non_dco_commits) return true unless reject_non_dco_commits - data_match?(message, DCO_COMMIT_REGEX, multiline: true) + data_match?(message, DCO_COMMIT_REGEX) end def commit_message_allowed?(message) -- GitLab