From 48b9380149567be5cfcd3ff62798cba0d19a2915 Mon Sep 17 00:00:00 2001 From: Zehua Zhang <zhzhang@jihulab.com> Date: Mon, 24 Oct 2022 18:35:43 +0800 Subject: [PATCH] Add committer name check to push_rules Add a column named `commit_committer_name_check` to push_rules. If this option is enabled, we will check whether the git user name of the user is consistent with the gitlab user name when the user pushes the code. If not, the push will be rejected. Changelog: added EE: true --- .../admin/push_rules_controller.rb | 4 + .../groups/push_rules_controller.rb | 5 + .../projects/push_rules_controller.rb | 5 + ee/app/helpers/push_rules_helper.rb | 7 ++ .../models/gitlab_subscriptions/features.rb | 1 + ee/app/models/push_rule.rb | 13 +++ ee/app/policies/ee/group_policy.rb | 14 +++ ee/app/policies/ee/project_policy.rb | 11 ++ ...mit_committer_name_check_setting.html.haml | 13 +++ .../views/shared/push_rules/_form.html.haml | 1 + .../commit_committer_name_check_ff.yml | 8 ++ .../gitlab/checks/push_rules/commit_check.rb | 8 +- .../admin/push_rules_controller_spec.rb | 8 +- .../groups/push_rules_controller_spec.rb | 108 ++++++++++++++++++ .../projects/push_rules_controller_spec.rb | 90 +++++++++++++++ ee/spec/helpers/push_rules_helper_spec.rb | 9 ++ .../checks/push_rules/commit_check_spec.rb | 32 ++++++ ee/spec/models/push_rule_spec.rb | 19 +-- locale/gitlab.pot | 6 + 19 files changed, 349 insertions(+), 13 deletions(-) create mode 100644 ee/app/views/shared/push_rules/_commit_committer_name_check_setting.html.haml create mode 100644 ee/config/feature_flags/development/commit_committer_name_check_ff.yml diff --git a/ee/app/controllers/admin/push_rules_controller.rb b/ee/app/controllers/admin/push_rules_controller.rb index 03c8e3084f6b72ec..6ce7ac3e5602b204 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?(:commit_committer_name_check) && Feature.enabled?(:commit_committer_name_check_ff) + allowed_fields << :commit_committer_name_check + end + if @push_rule.available?(:reject_non_dco_commits) allowed_fields << :reject_non_dco_commits end diff --git a/ee/app/controllers/groups/push_rules_controller.rb b/ee/app/controllers/groups/push_rules_controller.rb index 98f222c2cca955af..d1d732eace4f5114 100644 --- a/ee/app/controllers/groups/push_rules_controller.rb +++ b/ee/app/controllers/groups/push_rules_controller.rb @@ -42,6 +42,11 @@ def push_rule_params allowed_fields << :commit_committer_check end + if can?(current_user, :change_commit_committer_name_check, group) && + Feature.enabled?(:commit_committer_name_check_ff) + allowed_fields << :commit_committer_name_check + end + if can?(current_user, :change_reject_non_dco_commits, group) allowed_fields << :reject_non_dco_commits end diff --git a/ee/app/controllers/projects/push_rules_controller.rb b/ee/app/controllers/projects/push_rules_controller.rb index 22768716c240a39f..54c4b742519e5949 100644 --- a/ee/app/controllers/projects/push_rules_controller.rb +++ b/ee/app/controllers/projects/push_rules_controller.rb @@ -45,6 +45,11 @@ def push_rule_params allowed_fields << :commit_committer_check end + if can?(current_user, :change_commit_committer_name_check, project) && + Feature.enabled?(:commit_committer_name_check_ff) + allowed_fields << :commit_committer_name_check + end + if can?(current_user, :change_reject_non_dco_commits, project) allowed_fields << :reject_non_dco_commits end diff --git a/ee/app/helpers/push_rules_helper.rb b/ee/app/helpers/push_rules_helper.rb index 4c3723ed8a1ddbc9..4809912abfeafd14 100644 --- a/ee/app/helpers/push_rules_helper.rb +++ b/ee/app/helpers/push_rules_helper.rb @@ -24,6 +24,13 @@ def commit_committer_check_description(push_rule) push_rule_update_description(message, push_rule, :commit_committer_check) end + def commit_committer_name_check_description(push_rule) + message = s_("ProjectSettings|Users can only push commits to this repository "\ + "if the committer name is consistent with their git config username.") + + push_rule_update_description(message, push_rule, :commit_committer_name_check) + end + private def push_rule_update_description(message, push_rule, rule) diff --git a/ee/app/models/gitlab_subscriptions/features.rb b/ee/app/models/gitlab_subscriptions/features.rb index 87a53a2eda4849bd..cb97587e26d1c64a 100644 --- a/ee/app/models/gitlab_subscriptions/features.rb +++ b/ee/app/models/gitlab_subscriptions/features.rb @@ -86,6 +86,7 @@ class Features cluster_deployments code_owner_approval_required commit_committer_check + commit_committer_name_check compliance_framework custom_compliance_frameworks cross_project_pipelines diff --git a/ee/app/models/push_rule.rb b/ee/app/models/push_rule.rb index 0385ab44eb7553a2..6322c8f5fc19dec0 100644 --- a/ee/app/models/push_rule.rb +++ b/ee/app/models/push_rule.rb @@ -63,6 +63,7 @@ def commit_validation? reject_unsigned_commits || reject_non_dco_commits || commit_committer_check || + commit_committer_name_check || member_check || file_name_regex.present? || prevent_secrets @@ -82,6 +83,13 @@ def committer_allowed?(committer_email, current_user) current_user.verified_email?(committer_email) end + def committer_name_allowed?(committer_name, current_user) + return true unless available?(:commit_committer_name_check) + return true unless commit_committer_name_check + + current_user.name == committer_name + end + def non_dco_commit_allowed?(message) return true unless available?(:reject_non_dco_commits) return true unless reject_non_dco_commits @@ -145,6 +153,11 @@ def commit_committer_check=(value) write_setting_with_global_default(:commit_committer_check, value) end + def commit_committer_name_check + read_setting_with_global_default(:commit_committer_name_check) + end + alias_method :commit_committer_name_check?, :commit_committer_name_check + def reject_non_dco_commits read_setting_with_global_default(:reject_non_dco_commits) end diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index ffc0c4adde45b341..048ff0b7522669ba 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -128,6 +128,10 @@ module GroupPolicy @subject.feature_available?(:commit_committer_check) end + condition(:commit_committer_name_check_available, scope: :subject) do + @subject.feature_available?(:commit_committer_name_check) + end + condition(:reject_unsigned_commits_available, scope: :subject) do @subject.feature_available?(:reject_unsigned_commits) end @@ -443,6 +447,8 @@ module GroupPolicy rule { admin | maintainer }.enable :change_commit_committer_check + rule { admin | maintainer }.enable :change_commit_committer_name_check + rule { commit_committer_check_available }.policy do enable :read_commit_committer_check end @@ -451,6 +457,14 @@ module GroupPolicy prevent :change_commit_committer_check end + rule { commit_committer_name_check_available }.policy do + enable :read_commit_committer_name_check + end + + rule { ~commit_committer_name_check_available }.policy do + prevent :change_commit_committer_name_check + end + rule { admin | maintainer }.enable :change_reject_unsigned_commits rule { reject_unsigned_commits_available }.enable :read_reject_unsigned_commits diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 290c84a732e1cd0e..fff10808c29c4713 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -72,6 +72,11 @@ module ProjectPolicy @subject.feature_available?(:commit_committer_check) end + with_scope :subject + condition(:commit_committer_name_check_available) do + @subject.feature_available?(:commit_committer_name_check) + end + with_scope :subject condition(:reject_unsigned_commits_available) do @subject.feature_available?(:reject_unsigned_commits) @@ -379,6 +384,12 @@ module ProjectPolicy rule { ~commit_committer_check_available }.prevent :change_commit_committer_check + rule { admin | maintainer }.enable :change_commit_committer_name_check + + rule { commit_committer_name_check_available }.enable :read_commit_committer_name_check + + rule { ~commit_committer_name_check_available }.prevent :change_commit_committer_name_check + rule { admin | maintainer }.enable :change_reject_non_dco_commits rule { reject_non_dco_commits_available }.enable :read_reject_non_dco_commits diff --git a/ee/app/views/shared/push_rules/_commit_committer_name_check_setting.html.haml b/ee/app/views/shared/push_rules/_commit_committer_name_check_setting.html.haml new file mode 100644 index 0000000000000000..7ed9430e63c0cd72 --- /dev/null +++ b/ee/app/views/shared/push_rules/_commit_committer_name_check_setting.html.haml @@ -0,0 +1,13 @@ +- return unless Feature.enabled?(:commit_committer_name_check_ff) + +- context = local_assigns.fetch(:context) + +- return unless push_rule.available?(:commit_committer_name_check, object: context) + +- form = local_assigns.fetch(:form) +- push_rule = local_assigns.fetch(:push_rule) + += form.gitlab_ui_checkbox_component :commit_committer_name_check, + s_("PushRule|Reject inconsistent user name"), + checkbox_options: { data: { qa_selector: 'committer_name_restriction_checkbox' }, disabled: !can_change_push_rule?(form.object, :commit_committer_name_check, context) }, + help_text: commit_committer_name_check_description(push_rule) diff --git a/ee/app/views/shared/push_rules/_form.html.haml b/ee/app/views/shared/push_rules/_form.html.haml index fc61833a2b73c46b..a85423ee3d0a65ef 100644 --- a/ee/app/views/shared/push_rules/_form.html.haml +++ b/ee/app/views/shared/push_rules/_form.html.haml @@ -1,4 +1,5 @@ = render 'shared/push_rules/commit_committer_check_setting', form: f, push_rule: f.object, context: context += render 'shared/push_rules/commit_committer_name_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' diff --git a/ee/config/feature_flags/development/commit_committer_name_check_ff.yml b/ee/config/feature_flags/development/commit_committer_name_check_ff.yml new file mode 100644 index 0000000000000000..ff11bedbb5918df1 --- /dev/null +++ b/ee/config/feature_flags/development/commit_committer_name_check_ff.yml @@ -0,0 +1,8 @@ +--- +name: commit_committer_name_check_ff +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/101909 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/381694 +milestone: '15.6' +type: development +group: group::source code +default_enabled: false 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 5edc6afdad1f9a92..542250fb9d473561 100644 --- a/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rules/commit_check.rb @@ -93,11 +93,15 @@ def committer_check(commit) committer_is_current_user = committer == user_access.user if committer_is_current_user && !committer.verified_email?(commit.committer_email) - ERROR_MESSAGES[:committer_not_verified] % { committer_email: commit.committer_email } + return ERROR_MESSAGES[:committer_not_verified] % { committer_email: commit.committer_email } else - ERROR_MESSAGES[:committer_not_allowed] % { committer_email: commit.committer_email } + return ERROR_MESSAGES[:committer_not_allowed] % { committer_email: commit.committer_email } end end + + unless push_rule.committer_name_allowed?(commit.committer_name, user_access.user) + "Your git username is inconsistent with GitLab account name" + end end end end diff --git a/ee/spec/controllers/admin/push_rules_controller_spec.rb b/ee/spec/controllers/admin/push_rules_controller_spec.rb index 6ba6c3e28e53dcde..65855dc4d512876c 100644 --- a/ee/spec/controllers/admin/push_rules_controller_spec.rb +++ b/ee/spec/controllers/admin/push_rules_controller_spec.rb @@ -17,13 +17,17 @@ 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", commit_committer_check: "true", reject_unsigned_commits: "true", - reject_non_dco_commits: "true" + reject_non_dco_commits: "true", commit_committer_name_check: "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) + stub_licensed_features( + commit_committer_check: true, + reject_unsigned_commits: true, + reject_non_dco_commits: true, + commit_committer_name_check: true) end it 'updates sample push rule' do diff --git a/ee/spec/controllers/groups/push_rules_controller_spec.rb b/ee/spec/controllers/groups/push_rules_controller_spec.rb index dc4bb9fb6fb1dbde..bc704f32ce3fe8d5 100644 --- a/ee/spec/controllers/groups/push_rules_controller_spec.rb +++ b/ee/spec/controllers/groups/push_rules_controller_spec.rb @@ -147,6 +147,114 @@ def do_update end end end + + shared_examples 'updates push rule commit_committer_name_check of group' do |result| + it 'matches the given result' do + patch :update, params: { group_id: group, push_rule: { 'commit_committer_name_check' => true } } + + expect(group.reload.push_rule.commit_committer_name_check).to eq(result) + end + end + + context "Updating commit_committer_name_check rule" do + let(:push_rule_for_group) { create(:push_rule) } + + before do + group.update!(push_rule_id: push_rule_for_group.id) + end + + context 'when commit_committer_name_check is disabled' do + before do + stub_licensed_features(commit_committer_name_check: false) + end + + context 'as an admin' do + let(:user) { create(:admin) } + + context 'when admin mode enabled', :enable_admin_mode do + it_behaves_like 'updates push rule commit_committer_name_check of group', false + end + end + + context 'as a maintainer user' do + before do + group.add_maintainer(user) + end + + it_behaves_like 'updates push rule commit_committer_name_check of group', false + end + + context 'as a developer user' do + before do + group.add_developer(user) + end + + it_behaves_like 'updates push rule commit_committer_name_check of group', false + end + end + + context 'when commit_committer_name_check is enabled' do + before do + stub_licensed_features(commit_committer_name_check: true) + end + + context 'when commit_committer_name_check_ff is enabled' do + context 'as an admin' do + let(:user) { create(:admin) } + + context 'when admin mode enabled', :enable_admin_mode do + it_behaves_like 'updates push rule commit_committer_name_check of group', true + end + end + + context 'as a maintainer user' do + before do + group.add_maintainer(user) + end + + it_behaves_like 'updates push rule commit_committer_name_check of group', true + end + + context 'as a developer user' do + before do + group.add_developer(user) + end + + it_behaves_like 'updates push rule commit_committer_name_check of group', false + end + end + + context 'when commit_committer_name_check_ff is disabled' do + before do + stub_feature_flags(commit_committer_name_check_ff: false) + end + + context 'as an admin' do + let(:user) { create(:admin) } + + context 'when admin mode enabled', :enable_admin_mode do + it_behaves_like 'updates push rule commit_committer_name_check of group', false + end + end + + context 'as a maintainer user' do + before do + group.add_maintainer(user) + end + + it_behaves_like 'updates push rule commit_committer_name_check of group', false + end + + context 'as a developer user' do + before do + group.add_developer(user) + end + + it_behaves_like 'updates push rule commit_committer_name_check of group', false + end + end + end + end end context 'when user role is lower than maintainer' do diff --git a/ee/spec/controllers/projects/push_rules_controller_spec.rb b/ee/spec/controllers/projects/push_rules_controller_spec.rb index 899d86c4ee10fc63..e454000b1c345210 100644 --- a/ee/spec/controllers/projects/push_rules_controller_spec.rb +++ b/ee/spec/controllers/projects/push_rules_controller_spec.rb @@ -92,5 +92,95 @@ def do_update end end end + + shared_examples 'updates push rule commit_committer_name_check of project' do |result| + it 'matches the given result' do + patch :update, params: { namespace_id: project.namespace, project_id: project, id: 1, push_rule: { commit_committer_name_check: true } } + + expect(project.reload_push_rule.commit_committer_name_check).to eq(result) + end + end + + context "Updating commit_committer_name_check rule" do + context 'when commit_committer_name_check is disabled' do + before do + stub_licensed_features(commit_committer_name_check: false) + end + + context 'as an admin' do + let(:user) { create(:admin) } + + it_behaves_like 'updates push rule commit_committer_name_check of project', false + end + + context 'as a maintainer user' do + before do + project.add_maintainer(user) + end + it_behaves_like 'updates push rule commit_committer_name_check of project', false + end + + context 'as a developer user' do + before do + project.add_developer(user) + end + it_behaves_like 'updates push rule commit_committer_name_check of project', false + end + end + + context 'when commit_committer_name_check is enabled' do + before do + stub_licensed_features(commit_committer_name_check: true) + end + + context 'when commit_committer_name_check_ff enabled' do + context 'as an admin' do + let(:user) { create(:admin) } + + it_behaves_like 'updates push rule commit_committer_name_check of project', true + end + + context 'as a maintainer user' do + before do + project.add_maintainer(user) + end + it_behaves_like 'updates push rule commit_committer_name_check of project', true + end + + context 'as a developer user' do + before do + project.add_developer(user) + end + it_behaves_like 'updates push rule commit_committer_name_check of project', false + end + end + + context 'when commit_committer_name_check_ff disabled' do + before do + stub_feature_flags(commit_committer_name_check_ff: false) + end + + context 'as an admin' do + let(:user) { create(:admin) } + + it_behaves_like 'updates push rule commit_committer_name_check of project', false + end + + context 'as a maintainer user' do + before do + project.add_maintainer(user) + end + it_behaves_like 'updates push rule commit_committer_name_check of project', false + end + + context 'as a developer user' do + before do + project.add_developer(user) + end + it_behaves_like 'updates push rule commit_committer_name_check of project', false + end + end + end + end end end diff --git a/ee/spec/helpers/push_rules_helper_spec.rb b/ee/spec/helpers/push_rules_helper_spec.rb index bfb604454006997a..7075bb90e4cf4e06 100644 --- a/ee/spec/helpers/push_rules_helper_spec.rb +++ b/ee/spec/helpers/push_rules_helper_spec.rb @@ -68,4 +68,13 @@ end end end + + describe '#commit_committer_name_check_description' do + it 'returns the right description' do + expect( + helper.commit_committer_name_check_description(push_rule) + ).to eq(s_("ProjectSettings|Users can only push commits to this repository "\ + "if the committer name is consistent with their git config username.")) + end + end 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 4a1faaacf57c2bd8..f6fd2f1d6e61edad 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 @@ -331,5 +331,37 @@ end end end + + context 'Check commit author name rules' do + let(:push_rule) { create(:push_rule, commit_committer_name_check: true) } + + before do + stub_licensed_features(commit_committer_name_check: true) + end + + context 'with consistent user name' do + before do + allow_any_instance_of(Commit).to receive(:committer_name).and_return(user.name) + end + + it 'not raise error' do + expect { subject.validate! }.not_to raise_error + end + end + + context 'with inconsistent user name' do + let(:user) { create(:user, name: 'Test') } + + before do + allow_any_instance_of(Commit).to receive(:committer_name).and_return('Test1') + end + + it 'raises error' do + expect { subject.validate! } + .to raise_error(Gitlab::GitAccess::ForbiddenError, + "Your git username is inconsistent with GitLab account name") + end + end + end end end diff --git a/ee/spec/models/push_rule_spec.rb b/ee/spec/models/push_rule_spec.rb index 5fb14ed383b731c8..ccc0d63b7f47ed9b 100644 --- a/ee/spec/models/push_rule_spec.rb +++ b/ee/spec/models/push_rule_spec.rb @@ -131,15 +131,16 @@ let(:settings_with_global_default) { %i(reject_unsigned_commits) } where(:setting, :value, :result) do - :commit_message_regex | 'regex' | true - :branch_name_regex | 'regex' | true - :author_email_regex | 'regex' | true - :file_name_regex | 'regex' | true - :reject_unsigned_commits | true | true - :commit_committer_check | true | true - :member_check | true | true - :prevent_secrets | true | true - :max_file_size | 1 | false + :commit_message_regex | 'regex' | true + :branch_name_regex | 'regex' | true + :author_email_regex | 'regex' | true + :file_name_regex | 'regex' | true + :reject_unsigned_commits | true | true + :commit_committer_check | true | true + :commit_committer_name_check | true | true + :member_check | true | true + :prevent_secrets | true | true + :max_file_size | 1 | false end with_them do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index aba42326d98cb79a..cbcaa6019722197d 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -32150,6 +32150,9 @@ msgstr "" msgid "ProjectSettings|Users can only push commits to this repository if the committer email is one of their own verified emails." msgstr "" +msgid "ProjectSettings|Users can only push commits to this repository if the committer name is consistent with their git config username." +msgstr "" + msgid "ProjectSettings|Users can request access" msgstr "" @@ -33188,6 +33191,9 @@ msgstr "" msgid "PushRule|Push rules" msgstr "" +msgid "PushRule|Reject inconsistent user name" +msgstr "" + msgid "PushRule|Reject unverified users" msgstr "" -- GitLab