Merge branch '3285-pgp-gpg-integration-reject-unsigned-commits-with-hooks' into 'master'

Add new push rule to reject unsigned commits

Closes #3285

See merge request gitlab-org/gitlab-ee!2913

app/controllers/admin/push_rules_controller.rb

@@ -26,7 +26,7 @@ class Admin::PushRulesController < Admin::ApplicationController
   def push_rule_params
     params.require(:push_rule).permit(:deny_delete_tag, :delete_branch_regex,
       :commit_message_regex, :branch_name_regex, :force_push_regex, :author_email_regex, :member_check,
-      :file_name_regex, :max_file_size, :prevent_secrets)
+      :file_name_regex, :max_file_size, :prevent_secrets, :reject_unsigned_commits)
   end
 
   def push_rule

app/controllers/projects/push_rules_controller.rb

@@ -25,7 +25,14 @@ class Projects::PushRulesController < Projects::ApplicationController
 
   # Only allow a trusted parameter "white list" through.
   def push_rule_params
-    params.require(:push_rule).permit(:deny_delete_tag, :delete_branch_regex,
-      :commit_message_regex, :branch_name_regex, :force_push_regex, :author_email_regex, :member_check, :file_name_regex, :max_file_size, :prevent_secrets)
+    allowed_fields = %i[deny_delete_tag delete_branch_regex commit_message_regex
+                        branch_name_regex force_push_regex author_email_regex
+                        member_check file_name_regex max_file_size prevent_secrets]
+
+    if can?(current_user, :change_reject_unsigned_commits, project)
+      allowed_fields << :reject_unsigned_commits
+    end
+
+    params.require(:push_rule).permit(allowed_fields)
   end
 end

app/helpers/push_rules_helper.rb

+module PushRulesHelper
+  def reject_unsigned_commits_description(push_rule)
+    message = [s_("ProjectSettings|Only signed commits can be pushed to this repository.")]
+
+    if push_rule.global?
+      message << s_("ProjectSettings|This setting will be applied to all projects unless overridden by an admin.")
+    else
+      if PushRule.global&.reject_unsigned_commits
+        message << if push_rule.reject_unsigned_commits
+                     s_("ProjectSettings|This setting is applied on the server level and can be overridden by an admin.")
+                   else
+                     s_("ProjectSettings|This setting is applied on the server level but has been overridden for this project.")
+                   end
+
+        message << s_("ProjectSettings|Contact an admin to change this setting.") unless current_user.admin?
+      end
+    end
+
+    message.join(' ')
+  end
+end

app/models/push_rule.rb

@@ -6,16 +6,27 @@ class PushRule < ActiveRecord::Base
 
   FILES_BLACKLIST = YAML.load_file(Rails.root.join('lib/gitlab/checks/files_blacklist.yml'))
 
+  def self.global
+    find_by(is_sample: true)
+  end
+
   def commit_validation?
     commit_message_regex.present? ||
       branch_name_regex.present? ||
       author_email_regex.present? ||
+      reject_unsigned_commits ||
       member_check ||
       file_name_regex.present? ||
       max_file_size > 0 ||
       prevent_secrets
   end
 
+  def commit_signature_allowed?(commit)
+    return true unless reject_unsigned_commits
+
+    commit.has_signature?
+  end
+
   def commit_message_allowed?(message)
     data_match?(message, commit_message_regex)
   end
@@ -36,6 +47,33 @@ class PushRule < ActiveRecord::Base
     regex_list.find { |regex| data_match?(file_path, regex) }
   end
 
+  def reject_unsigned_commits=(value)
+    enabled_globally = PushRule.global&.reject_unsigned_commits
+    is_disabled = !Gitlab::Utils.to_boolean(value)
+
+    # If setting is globally disabled and user disable it at project level,
+    # reset the attr so we can use the default global if required later.
+    if !enabled_globally && is_disabled
+      super(nil)
+    else
+      super(value)
+    end
+  end
+
+  def reject_unsigned_commits
+    value = super
+
+    # return if value is true/false or if current object is the global setting
+    return value if global? || !value.nil?
+
+    PushRule.global&.reject_unsigned_commits
+  end
+  alias_method :reject_unsigned_commits?, :reject_unsigned_commits
+
+  def global?
+    is_sample?
+  end
+
   private
 
   def data_match?(data, regex)

app/views/shared/_push_rules_form.html.haml

+.form-group
+  = f.check_box :reject_unsigned_commits, class: "pull-left", disabled: !can_change_reject_unsigned_commits?(f.object)
+  .prepend-left-20
+    = f.label :reject_unsigned_commits, class: "label-light append-bottom-0" do
+      Reject unsigned commits
+    %p.light.append-bottom-0
+      = reject_unsigned_commits_description(f.object)
+
 .form-group
   = f.check_box :deny_delete_tag, class: "pull-left"
   .prepend-left-20

changelogs/unreleased-ee/add-push-rule-to-reject-unsigned-commits.yml

+---
+title: Add new push rule to reject unsigned commits
+merge_request: 2913
+author:

db/migrate/20170914012544_add_reject_unsigned_commits_to_push_rules.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddRejectUnsignedCommitsToPushRules < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def change
+    add_column :push_rules, :reject_unsigned_commits, :boolean
+  end
+end

db/schema.rb

@@ -1616,6 +1616,7 @@ ActiveRecord::Schema.define(version: 20170928100231) do
     t.integer "max_file_size", default: 0, null: false
     t.boolean "prevent_secrets", default: false, null: false
     t.string "branch_name_regex"
+    t.boolean "reject_unsigned_commits"
   end
 
   add_index "push_rules", ["project_id"], name: "index_push_rules_on_project_id", using: :btree

doc/push_rules/push_rules.md

@@ -63,6 +63,7 @@ The following options are available.
 | --------- | :------------: | ----------- |
 | Removal of tags with `git push` | 7.10 | Forbid users to remove git tags with `git push`. Tags will still be able to be deleted through the web UI. |
 | Check whether author is a GitLab user | 7.10 | Restrict commits by author (email) to existing GitLab users. |
+| Check whether commit is signed through GPG | 10.1 | Reject commit when it is not signed through GPG. Read [signing commits with GPG][signing-commits]. |
 | Prevent committing secrets to Git | 8.12 | GitLab will reject any files that are likely to contain secrets. Read [what files are forbidden](#prevent-pushing-secrets-to-the-repository). |
 | Restrict by commit message | 7.10 | Only commit messages that match this Ruby regular expression are allowed to be pushed. Leave empty to allow any commit message. |
 | Restrict by branch name | 9.3 | Only branch names that match this Ruby regular expression are allowed to be pushed. Leave empty to allow any branch name. |
@@ -145,6 +146,7 @@ bash_history
 ```
 
 [protected-branches]: ../user/project/protected_branches.md
+[signing-commits]: ../user/project/repository/gpg_signed_commits/index.md
 [ee-385]: https://gitlab.com/gitlab-org/gitlab-ee/issues/385
 [list]: https://gitlab.com/gitlab-org/gitlab-ee/blob/master/lib/gitlab/checks/files_blacklist.yml
 [hooks]: https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks

ee/app/helpers/ee/projects_helper.rb

+module EE
+  module ProjectsHelper
+    def can_change_reject_unsigned_commits?(push_rule)
+      return true if push_rule.global?
+
+      can?(current_user, :change_reject_unsigned_commits, @project)
+    end
+  end
+end

ee/app/policies/ee/project_policy.rb

@@ -15,6 +15,11 @@ module EE
       with_scope :global
       condition(:is_development) { Rails.env.development? }
 
+      with_scope :global
+      condition(:reject_unsigned_commits_disabled_globally) do
+        !PushRule.global&.reject_unsigned_commits
+      end
+
       rule { admin }.enable :change_repository_storage
 
       rule { support_bot }.enable :guest_access
@@ -70,6 +75,8 @@ module EE
       end
 
       rule { ~can?(:push_code) }.prevent :push_code_to_protected_branches
+
+      rule { admin | (reject_unsigned_commits_disabled_globally & can?(:master_access)) }.enable :change_reject_unsigned_commits
     end
   end
 end

lib/gitlab/checks/change_access.rb

@@ -82,7 +82,7 @@ module Gitlab
           raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:non_master_delete_protected_branch]
         end
 
-        unless protocol == 'web'
+        unless updated_from_web?
           raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:non_web_delete_protected_branch]
         end
       end
@@ -122,6 +122,10 @@ module Gitlab
 
       private
 
+      def updated_from_web?
+        protocol == 'web'
+      end
+
       def tag_exists?
         project.repository.tag_exists?(@tag_name)
       end
@@ -187,7 +191,7 @@ module Gitlab
 
       def tag_deletion_denied_by_push_rule?(push_rule)
         push_rule.try(:deny_delete_tag) &&
-          protocol != 'web' &&
+          !updated_from_web? &&
           deletion? &&
           tag_exists?
       end
@@ -208,6 +212,10 @@ module Gitlab
           return "Author's email '#{commit.author_email}' does not follow the pattern '#{push_rule.author_email_regex}'"
         end
 
+        if !updated_from_web? && !push_rule.commit_signature_allowed?(commit)
+          return "Commit must be signed with a GPG key"
+        end
+
         # Check whether author is a GitLab member
         if push_rule.member_check
           unless User.existing_member?(commit.author_email.downcase)

spec/controllers/projects/push_rules_controller_spec.rb

@@ -33,5 +33,55 @@ describe Projects::PushRulesController do
         expect(response).to have_http_status(404)
       end
     end
+
+    context 'Updating reject unsigned commit rule' do
+      context 'as an admin' do
+        let(:user) { create(:admin) }
+
+        it 'updates the setting' do
+          patch :update, namespace_id: project.namespace, project_id: project, id: 1, push_rule: { reject_unsigned_commits: true }
+
+          expect(project.push_rule(true).reject_unsigned_commits).to be_truthy
+        end
+      end
+
+      context 'as a master user' do
+        before do
+          project.add_master(user)
+        end
+
+        context 'when global setting is disabled' do
+          it 'updates the setting' do
+            patch :update, namespace_id: project.namespace, project_id: project, id: 1, push_rule: { reject_unsigned_commits: true }
+
+            expect(project.push_rule(true).reject_unsigned_commits).to be_truthy
+          end
+        end
+
+        context 'when global setting is enabled' do
+          before do
+            create(:push_rule_sample, reject_unsigned_commits: true)
+          end
+
+          it 'does not update the setting' do
+            patch :update, namespace_id: project.namespace, project_id: project, id: 1, push_rule: { reject_unsigned_commits: false }
+
+            expect(project.push_rule(true).reject_unsigned_commits).to be_truthy
+          end
+        end
+      end
+
+      context 'as a developer user' do
+        before do
+          project.add_developer(user)
+        end
+
+        it 'does not update the setting' do
+          patch :update, namespace_id: project.namespace, project_id: project, id: 1, push_rule: { reject_unsigned_commits: true }
+
+          expect(project.push_rule(true).reject_unsigned_commits).to be_falsy
+        end
+      end
+    end
   end
 end

spec/helpers/push_rules_helper_spec.rb

+require 'spec_helper'
+
+describe PushRulesHelper do
+  let(:global_push_rule) { create(:push_rule_sample) }
+  let(:push_rule) { create(:push_rule) }
+  let(:admin) { create(:admin) }
+  let(:project_owner) { push_rule.project.owner }
+  let(:possible_help_texts) do
+    {
+      base_help: /Only signed commits 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/,
+      requires_admin_contact: /Contact an admin to change this setting/
+    }
+  end
+  let(:users) do
+    {
+      admin: admin,
+      owner: project_owner
+    }
+  end
+
+  where(:global_setting, :enabled_globally, :enabled_in_project, :current_user, :help_text, :invalid_text) do
+    [
+      [true,  true,  false, :admin, :default_admin_help,          nil],
+      [true,  false, false, :admin, :default_admin_help,          nil],
+      [true,  true,  true,  :admin, :default_admin_help,          nil],
+      [true,  false, true,  :admin, :default_admin_help,          nil],
+      [false, true,  nil,   :admin, :setting_can_be_overridden,   nil],
+      [false, true,  nil,   :owner, :setting_can_be_overridden,   nil],
+      [false, true,  nil,   :owner, :requires_admin_contact,      nil],
+      [false, true,  false, :admin, :setting_has_been_overridden, nil],
+      [false, true,  false, :owner, :setting_has_been_overridden, nil],
+      [false, true,  false, :owner, :requires_admin_contact,      nil],
+      [false, true,  true,  :owner, :setting_can_be_overridden,   nil],
+      [false, true,  false, :owner, :setting_has_been_overridden, nil],
+      [false, true,  true,  :owner, :requires_admin_contact,      :setting_has_been_overridden],
+      [false, true,  false, :owner, :requires_admin_contact,      :setting_can_be_overridden],
+      [false, false, true,  :admin, :base_help,                   :setting_can_be_overridden],
+      [false, false, true,  :admin, :base_help,                   :setting_has_been_overridden]
+    ]
+  end
+
+  with_them do
+    before do
+      global_push_rule.update_column(:reject_unsigned_commits, enabled_globally)
+      push_rule.update_column(:reject_unsigned_commits, enabled_in_project)
+
+      allow(helper).to receive(:current_user).and_return(users[current_user])
+    end
+
+    it 'has the correct help text' do
+      rule = global_setting ? global_push_rule : push_rule
+
+      expect(helper.reject_unsigned_commits_description(rule)).to match(possible_help_texts[help_text])
+
+      if invalid_text
+        expect(helper.reject_unsigned_commits_description(rule)).not_to match(possible_help_texts[invalid_text])
+      end
+    end
+  end
+end

spec/lib/gitlab/checks/change_access_spec.rb

@@ -360,6 +360,73 @@ describe Gitlab::Checks::ChangeAccess do
           expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "File \"README\" is larger than the allowed size of 1 MB")
         end
       end
+
+      context 'GPG sign rules' do
+        let(:push_rule) { create(:push_rule, reject_unsigned_commits: true) }
+
+        it_behaves_like 'check ignored when push rule unlicensed'
+
+        context 'when it is only enabled in Global settings' do
+          before do
+            project.push_rule.update_column(:reject_unsigned_commits, nil)
+            create(:push_rule_sample, reject_unsigned_commits: true)
+          end
+
+          context 'and commit is not signed' do
+            before do
+              allow_any_instance_of(Commit).to receive(:has_signature?).and_return(false)
+            end
+
+            it 'returns an error' do
+              expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "Commit must be signed with a GPG key")
+            end
+          end
+        end
+
+        context 'when enabled in Project' do
+          context 'and commit is not signed' do
+            before do
+              allow_any_instance_of(Commit).to receive(:has_signature?).and_return(false)
+            end
+
+            it 'returns an error' do
+              expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "Commit must be signed with a GPG key")
+            end
+
+            context 'but the change is made in the web application' do
+              let(:protocol) { 'web' }
+
+              it 'does not return an error' do
+                expect { subject }.not_to raise_error
+              end
+            end
+          end
+
+          context 'and commit is signed' do
+            before do
+              allow_any_instance_of(Commit).to receive(:has_signature?).and_return(true)
+            end
+
+            it 'does not return an error' do
+              expect { subject }.not_to raise_error
+            end
+          end
+        end
+
+        context 'when disabled in Project' do
+          let(:push_rule) { create(:push_rule, reject_unsigned_commits: false) }
+
+          context 'and commit is not signed' do
+            before do
+              allow_any_instance_of(Commit).to receive(:has_signature?).and_return(false)
+            end
+
+            it 'does not return an error' do
+              expect { subject }.not_to raise_error
+            end
+          end
+        end
+      end
     end
 
     context 'file lock rules' do

spec/models/push_rule_spec.rb

 require 'spec_helper'
 
 describe PushRule do
+  let(:global_push_rule) { create(:push_rule_sample) }
+  let(:push_rule) { create(:push_rule) }
+  let(:user) { create(:user) }
+  let(:project) { Projects::CreateService.new(user, { name: 'test', namespace: user.namespace }).execute }
+
   describe "Associations" do
     it { is_expected.to belong_to(:project) }
   end
@@ -9,4 +14,114 @@ describe PushRule do
     it { is_expected.to validate_presence_of(:project) }
     it { is_expected.to validate_numericality_of(:max_file_size).is_greater_than_or_equal_to(0).only_integer }
   end
+
+  describe '#commit_validation?' do
+    let(:settings_with_global_default) { %i(reject_unsigned_commits) }
+
+    settings = {
+      commit_message_regex: 'regex',
+      branch_name_regex: 'regex',
+      author_email_regex: 'regex',
+      file_name_regex: 'regex',
+      reject_unsigned_commits: true,
+      member_check: true,
+      prevent_secrets: true,
+      max_file_size: 1
+    }
+
+    settings.each do |setting, value|
+      context "when #{setting} is enabled at global level" do
+        before do
+          global_push_rule.update_column(setting, value)
+        end
+
+        it "returns true at project level" do
+          rule = project.push_rule
+
+          if settings_with_global_default.include?(setting)
+            rule.update_column(setting, nil)
+          end
+
+          expect(rule.commit_validation?).to eq(true)
+        end
+      end
+    end
+  end
+
+  describe '#commit_signature_allowed?' do
+    let(:signed_commit) { double(has_signature?: true) }
+    let(:unsigned_commit) { double(has_signature?: false) }
+
+    context 'when enabled at a global level' do
+      before do
+        global_push_rule.update_attribute(:reject_unsigned_commits, true)
+      end
+
+      it 'returns false if commit is not signed' do
+        expect(push_rule.commit_signature_allowed?(unsigned_commit)).to eq(false)
+      end
+
+      context 'and disabled at a Project level' do
+        it 'returns true if commit is not signed' do
+          push_rule.update_attribute(:reject_unsigned_commits, false)
+
+          expect(push_rule.commit_signature_allowed?(unsigned_commit)).to eq(true)
+        end
+      end
+
+      context 'and unset at a Project level' do
+        it 'returns false if commit is not signed' do
+          push_rule.update_attribute(:reject_unsigned_commits, nil)
+
+          expect(push_rule.commit_signature_allowed?(unsigned_commit)).to eq(false)
+        end
+      end
+    end
+
+    context 'when disabled at a global level' do
+      before do
+        global_push_rule.update_attribute(:reject_unsigned_commits, false)
+      end
+
+      it 'returns true if commit is not signed' do
+        expect(push_rule.commit_signature_allowed?(unsigned_commit)).to eq(true)
+      end
+
+      context 'but enabled at a Project level' do
+        before do
+          push_rule.update_attribute(:reject_unsigned_commits, true)
+        end
+
+        it 'returns false if commit is not signed' do
+          expect(push_rule.commit_signature_allowed?(unsigned_commit)).to eq(false)
+        end
+
+        it 'returns true if commit is signed' do
+          expect(push_rule.commit_signature_allowed?(signed_commit)).to eq(true)
+        end
+      end
+
+      context 'when user has enabled and disabled it at a project level' do
+        before do
+          # Let's test with the same boolean values that are sent through the form
+          push_rule.update_attribute(:reject_unsigned_commits, '1')
+          push_rule.update_attribute(:reject_unsigned_commits, '0')
+        end
+
+        context 'and it is enabled globally' do
+          before do
+            global_push_rule.update_attribute(:reject_unsigned_commits, true)
+          end
+
+          it 'returns false if commit is not signed' do
+            expect(push_rule.commit_signature_allowed?(unsigned_commit)).to eq(false)
+          end
+
+          it 'returns true if commit is signed' do
+            expect(push_rule.commit_signature_allowed?(signed_commit)).to eq(true)
+          end
+        end
+      end
+    end
+  end
 end